Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JustinCCYap merged 8 commits into from
Nov 10, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d226f7
Reapply "RS-20085: Only set rhtmlwidget-status to ready once images h…
JustinCCYap Nov 10, 2025
c95641a
Fix
JustinCCYap Nov 10, 2025
567c2c5
inst changes
JustinCCYap Nov 10, 2025
27e6551
Render variable image after base image
JustinCCYap Nov 10, 2025
282f169
Fix
JustinCCYap Nov 10, 2025
fe88542
Add logging
JustinCCYap Nov 10, 2025
fcbb378
Diagnostic
JustinCCYap Nov 10, 2025
fa4cf24
Fix
JustinCCYap Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion inst/htmlwidgets/rhtmlPictographs.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion inst/htmlwidgets/rhtmlPictographs.js.map
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions theSrc/scripts/BaseCell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ class BaseCell {
}

draw () {
this._draw()
this._generateDynamicCss()
return Promise.resolve()
.then(() => this._draw())
.then(() => this._generateDynamicCss())
}

setCss (cssLocation, cssAttr, cssValue) {
Expand Down
4 changes: 1 addition & 3 deletions theSrc/scripts/GraphicCell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@JustinCCYap JustinCCYap Nov 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why a TODO was left here if the fix was already mentioned in the comment. Without this change, we wouldn't be waiting for baseImageCompletePromise if this.config.variableImage is null.

@JustinCCYap JustinCCYap Nov 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps they mean moving the logic in https://github.com/Displayr/rhtmlPictographs/blob/RS-20085-2/theSrc/scripts/GraphicCell.js#L559-L568 out of the then, which is what I attempted. It turned out the old logic was necessary (after my changes) in order to get the base image drawn before the variable image.

if (this.config.variableImage != null) {
const variableImageConfig = this.config.variableImage
const imageFactory = this.imageFactory
Expand Down
7 changes: 1 addition & 6 deletions theSrc/scripts/ImageFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 5 additions & 6 deletions theSrc/scripts/Pictograph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions theSrc/scripts/imageTypes/base.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 4 additions & 2 deletions theSrc/scripts/imageTypes/circle.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
}
}

Expand Down
6 changes: 4 additions & 2 deletions theSrc/scripts/imageTypes/ellipse.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
}
}

Expand Down
7 changes: 5 additions & 2 deletions theSrc/scripts/imageTypes/recoloredExternalSvg.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,12 +95,15 @@ class RecoloredExternalSvg extends BaseImageType {
return null
}

appendToSvg () {
appendToSvg (clipId) {
const cleanedSvgString = this.getRecoloredString()
const cacheKey = this.getRecoloredStringCacheKey()
const definitionId = this.definitionManager.addDefinition(cacheKey, cleanedSvgString)
this.imageHandle = this.d3Node.append('use').attr('xlink:href', `#${definitionId}`)
return this.imageHandle

this.addClipId(clipId)

return Promise.resolve()

@JustinCCYap JustinCCYap Nov 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used to be return new Promise((resolve) => { this.imageHandle.on('load', () => resolve(), { once: true }) }) in my previous PR which was reverted.

There was no such event for this element (so it was never resolved), and waiting for load wasn't needed because we already wait for the svg to download (recolored external svgs) in

rhtmlPictographs/theSrc/scripts/imageTypes/recoloredExternalSvg.imagetype.js

Lines 11 to 31 in 567c2c5

calculateDesiredAspectRatio () {
return new Promise((resolve, reject) => {
const onDownloadSuccess = (xmlString) => {
const data = $.parseXML(xmlString)
this.svg = $(data).find('svg')
let imageAspectRatio = this._extractAspectRatioFromSvg()
if (!imageAspectRatio) {
console.error(`WARN: recolor SVG from ${this.config.url} : Cannot compute aspect ratio : unexpected SVG format (no viewbox , no width & height).`)
return resolve(null)
}
return resolve(imageAspectRatio)
}
const onDownloadFailure = () => reject(new Error(`Downloading svg failed: ${this.config.url}`))
return this.getOrDownload(this.config.url)
.done(onDownloadSuccess)
.fail(onDownloadFailure)
})
}

}

getRecolorArgs () {
Expand Down
6 changes: 4 additions & 2 deletions theSrc/scripts/imageTypes/rectangle.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class RectangleType extends BaseImageType {
return Promise.resolve(null)
}

appendToSvg () {
appendToSvg (clipId) {
this.imageHandle = this.d3Node.append('svg:rect')
.classed('rect', true)
.attr('x', (this.containerWidth * this.baseShapeHiding * (1 - this.ratio)) / 2)
Expand All @@ -19,7 +19,9 @@ class RectangleType extends BaseImageType {
.style('fill', this.color)
.style('opacity', this.opacity)

return this.imageHandle
this.addClipId(clipId)

return Promise.resolve()
}
}

Expand Down
6 changes: 4 additions & 2 deletions theSrc/scripts/imageTypes/square.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class SquareType extends BaseImageType {
return this.imageDimensions
}

appendToSvg () {
appendToSvg (clipId) {
const length = this.imageDimensions.length
this.imageHandle = this.d3Node.append('svg:rect')
.classed('square', true)
Expand All @@ -27,7 +27,9 @@ class SquareType extends BaseImageType {
.style('fill', this.color)
.style('opacity', this.opacity)

return this.imageHandle
this.addClipId(clipId)

return Promise.resolve()
}
}

Expand Down
7 changes: 5 additions & 2 deletions theSrc/scripts/imageTypes/url.imagetype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class UrlType extends BaseImageType {
})
}

appendToSvg () {
appendToSvg (clipId) {
this.imageHandle = this.d3Node.append('svg:image')
.attr('x', (this.containerWidth * (1 - this.ratio)) / 2)
.attr('y', (this.containerHeight * (1 - this.ratio)) / 2)
Expand All @@ -93,7 +93,10 @@ class UrlType extends BaseImageType {
if (_.has(this.config, 'preserveAspectRatio')) {
this.imageHandle.attr('preserveAspectRatio', this.config.preserveAspectRatio)
}
return this.imageHandle

this.addClipId(clipId)

return new Promise((resolve) => this.imageHandle.on('load', () => resolve(), { once: true }))
}
}

Expand Down
1 change: 0 additions & 1 deletion theSrc/scripts/rhtmlPictographs.factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ module.exports = function (element, width, height, stateChangedCallback) {
if (err.type === InsufficientContainerSizeError.type) {
console.log(err.message)
d3.select(instance.rootElement).attr(`rhtmlwidget-status`, 'ready')
d3.select(instance.rootElement).attr(`rhtmlPictographs-status`, 'ready') // to be removed once regression testing code checks for rhtmlwidget-status
} else {
_showError(err, element)
}
Expand Down