Revert "RS-20085: Only set rhtmlwidget-status to ready once images have loaded (#133)"

This reverts commit 91e17e7.

Author: JustinCCYap

DESCRIPTION

@@ -1,7 +1,7 @@
 Package: rhtmlPictographs
 Type: Package
 Title: Create Pictograph html widgets
-Version: 1.0.9
+Version: 1.0.8
 Author: Displayr <opensource@displayr.com>
 Maintainer: Displayr <opensource@displayr.com>
 Description: An R HTMLWidget that can generate single image graphics, mutli

inst/htmlwidgets/rhtmlPictographs.js

@@ -128,9 +128,8 @@ class BaseCell {
   }
 
   draw () {
-    return Promise.resolve()
-      .then(() => this._draw())
-      .then(() => this._generateDynamicCss())
+    this._draw()
+    this._generateDynamicCss()
   }
 
   setCss (cssLocation, cssAttr, cssValue) {

inst/htmlwidgets/rhtmlPictographs.js.map

@@ -538,31 +538,38 @@ class GraphicCell extends BaseCell {
       throw error // NB This is thrown because displayr wants the error too
     }
 
-    const imageRenderPromises = []
+    let baseImageCompletePromise = Promise.resolve()
     if (this.config.baseImage != null) {
       const baseImageConfig = this.config.baseImage
       const imageFactory = this.imageFactory
+      const baseImageRenderPromises = []
       enteringLeafNodes.each(function (dataAttributes) {
         const d3Node = d3.select(this)
-        imageRenderPromises.push(
+        baseImageRenderPromises.push(
           imageFactory.addBaseImageTo(d3Node, baseImageConfig, imageWidth, imageHeight, dataAttributes)
         )
       })
+      baseImageCompletePromise = Promise.all(baseImageRenderPromises).catch(imageErrorHandler)
     }
 
+    // TODO this should be tied to base image promise unconditioanlly (ie. swap lines below)
+    // let variableImageCompletePromise = baseImageCompletePromise
+    let variableImageCompletePromise = Promise.resolve()
     if (this.config.variableImage != null) {
       const variableImageConfig = this.config.variableImage
       const imageFactory = this.imageFactory
-      enteringLeafNodes.each(function (dataAttributes) {
-        const d3Node = d3.select(this)
-        imageRenderPromises.push(
-          imageFactory.addVarImageTo(d3Node, variableImageConfig, imageWidth, imageHeight, dataAttributes)
-        )
+      variableImageCompletePromise = baseImageCompletePromise.then(function () {
+        const variableImageRenderPromises = []
+        enteringLeafNodes.each(function (dataAttributes) {
+          const d3Node = d3.select(this)
+          variableImageRenderPromises.push(
+            imageFactory.addVarImageTo(d3Node, variableImageConfig, imageWidth, imageHeight, dataAttributes)
+          )
+        })
+        return Promise.all(variableImageRenderPromises).catch(imageErrorHandler)
       })
     }
 
-    const variableImageCompletePromise = Promise.all(imageRenderPromises).catch(imageErrorHandler)
-
     return variableImageCompletePromise.then(() => {
       if (this.config.tooltip) {
         enteringLeafNodes.append('svg:title')

theSrc/scripts/BaseCell.js

@@ -220,7 +220,12 @@ class ImageFactory {
           return null
         }
       })
-      .then((clipId) => imageInstance.appendToSvg(clipId))
+      .then((clipId) => {
+        const imageHandle = imageInstance.appendToSvg()
+        if (clipId) {
+          imageHandle.attr('clip-path', `url(#${clipId})`)
+        }
+      })
   }
 
   calculateAspectRatio (config) {

theSrc/scripts/GraphicCell.js

@@ -14,6 +14,7 @@ class Pictograph {
   constructor (el) {
     this.rootElement = _.has(el, 'length') ? el[0] : el
     d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'loading')
+    d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'loading') // to be removed once regression testing code checks for rhtmlwidget-status
     const actualDimensions = this.getContainerDimensions()
     log.info('Pictograph() called. Container dimensions:', actualDimensions, 'element:', el)
 
@@ -41,6 +42,7 @@ class Pictograph {
         if (error.type === InsufficientContainerSizeError.type) {
           console.log(error.message)
           d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready')
+          d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
         } else {
           console.error(`error in pictograph draw: ${error.message}`)
           console.error(error.stack)
@@ -77,6 +79,7 @@ class Pictograph {
         if (error.type === InsufficientContainerSizeError.type) {
           console.log(error.message)
           d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready')
+          d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
         } else {
           console.error(`error in pictograph resize: ${error.message}`)
           console.error(error.stack)
@@ -494,8 +497,6 @@ class Pictograph {
       .attr('class', 'table-cell')
       .attr('transform', d => `translate(${d.x},${d.y})`)
 
-    const cellDrawingPromises = []
-
     enteringCells.each(function (d) {
       const instance = d.instance
       log.debug(`rendering entering cell`, instance)
@@ -507,11 +508,11 @@ class Pictograph {
       instance.setWidth(d.width)
       instance.setHeight(d.height)
       instance.setDynamicMargins(d.dynamicMargins)
-
-      cellDrawingPromises.push(instance.draw())
+      instance.draw()
     })
 
-    Promise.all(cellDrawingPromises).then(() => d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready'))
+    d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready')
+    d3.select(this.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
   }
 
   // TODO Duplicated code from graphicCell

theSrc/scripts/ImageFactory.js

@@ -64,12 +64,6 @@ class BaseImageType {
   get baseShapeHiding () {
     return (this.config.baseShapeScale != null) ? this.config.baseShapeScale : 1
   }
-
-  addClipId (clipId) {
-    if (clipId) {
-      this.imageHandle.attr('clip-path', `url(#${clipId})`)
-    }
-  }
 }
 
 module.exports = BaseImageType

theSrc/scripts/Pictograph.js

@@ -16,7 +16,7 @@ class CircleType extends BaseImageType {
     return Promise.resolve(1)
   }
 
-  appendToSvg (clipId) {
+  appendToSvg () {
     this.imageHandle = this.d3Node.append('svg:circle')
       .classed('circle', true)
       .attr('cx', this.containerWidth / 2)
@@ -25,9 +25,7 @@ class CircleType extends BaseImageType {
       .style('fill', this.color)
       .style('opacity', this.opacity)
 
-    this.addClipId(clipId)
-
-    return Promise.resolve()
+    return this.imageHandle
   }
 }
 

theSrc/scripts/imageTypes/base.imagetype.js

@@ -9,7 +9,7 @@ class EllipseType extends BaseImageType {
     return Promise.resolve(null)
   }
 
-  appendToSvg (clipId) {
+  appendToSvg () {
     this.imageHandle = this.d3Node.append('svg:ellipse')
       .classed('ellipse', true)
       .attr('cx', this.containerWidth / 2)
@@ -19,9 +19,7 @@ class EllipseType extends BaseImageType {
       .style('fill', this.color)
       .style('opacity', this.opacity)
 
-    this.addClipId(clipId)
-
-    return Promise.resolve()
+    return this.imageHandle
   }
 }