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

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

This reverts commit 4091c89.

* Fix

* inst changes

* Render variable image after base image

* Fix

* Add logging

* Diagnostic

* Fix

Author: JustinCCYap

DESCRIPTION

@@ -1,7 +1,7 @@
 Package: rhtmlPictographs
 Type: Package
 Title: Create Pictograph html widgets
-Version: 1.0.8
+Version: 1.0.9
 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,8 +128,9 @@ class BaseCell {
   }
 
   draw () {
-    this._draw()
-    this._generateDynamicCss()
+    return Promise.resolve()
+      .then(() => this._draw())
+      .then(() => this._generateDynamicCss())
   }
 
   setCss (cssLocation, cssAttr, cssValue) {

inst/htmlwidgets/rhtmlPictographs.js.map

@@ -552,9 +552,7 @@ class GraphicCell extends BaseCell {
       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()
+    let variableImageCompletePromise = baseImageCompletePromise
     if (this.config.variableImage != null) {
       const variableImageConfig = this.config.variableImage
       const imageFactory = this.imageFactory

theSrc/scripts/BaseCell.js

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

theSrc/scripts/GraphicCell.js

@@ -14,7 +14,6 @@ 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)
 
@@ -42,7 +41,6 @@ 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)
@@ -79,7 +77,6 @@ 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)
@@ -497,6 +494,8 @@ 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)
@@ -508,11 +507,11 @@ class Pictograph {
       instance.setWidth(d.width)
       instance.setHeight(d.height)
       instance.setDynamicMargins(d.dynamicMargins)
-      instance.draw()
+
+      cellDrawingPromises.push(instance.draw())
     })
 
-    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
+    Promise.all(cellDrawingPromises).then(() => d3.select(this.rootElement).attr(`rhtmlwidget-status`, 'ready'))
   }
 
   // TODO Duplicated code from graphicCell

theSrc/scripts/ImageFactory.js

@@ -64,6 +64,12 @@ 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 () {
+  appendToSvg (clipId) {
     this.imageHandle = this.d3Node.append('svg:circle')
       .classed('circle', true)
       .attr('cx', this.containerWidth / 2)
@@ -25,7 +25,9 @@ class CircleType extends BaseImageType {
       .style('fill', this.color)
       .style('opacity', this.opacity)
 
-    return this.imageHandle
+    this.addClipId(clipId)
+
+    return Promise.resolve()
   }
 }
 

theSrc/scripts/imageTypes/base.imagetype.js

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