[WIP] Fixing "imageSmoothingEnabled" resetting to 'True' (should fix #1593) #1595
Conversation
…led is still initalized on resize
|
OK, I think I figured it out, let me know what you think! |
src/drawer.js
Outdated
| @@ -247,13 +247,15 @@ $.Drawer.prototype = { | |||
| var viewportSize = this._calculateCanvasSize(); | |||
| if( this.canvas.width != viewportSize.x || | |||
| this.canvas.height != viewportSize.y ) { | |||
| var imageSmoothingEnabled = this.canvas.getContext('2d').imageSmoothingEnabled; | |||
There was a problem hiding this comment.
I think what I would do would be to keep our own _imageSmoothingEnabled property that gets set in setImageSmoothingEnabled and then used here. That way you are relying on an explicit variable and not something in the Canvas that might get changed out of our control.
src/drawer.js
Outdated
| this.canvas.width = viewportSize.x; | ||
| this.canvas.height = viewportSize.y; | ||
| if ( this.sketchCanvas !== null ) { | ||
| var sketchCanvasSize = this._calculateSketchCanvasSize(); | ||
| this.sketchCanvas.width = sketchCanvasSize.x; | ||
| this.sketchCanvas.height = sketchCanvasSize.y; | ||
| } | ||
| this.setImageSmoothingEnabled(imageSmoothingEnabled); |
There was a problem hiding this comment.
It might be overkill, but I think I might actually split setImageSmoothingEnabled into two functions; a "private" one that just sets the various Canvas flags for different browsers, and the "public" one that sets _imageSmoothingEnabled, calls the "private" one, and tells the viewer to redraw. That way you can just call the "private" one from here, since you don't need the other stuff in this case. It could be called _setImageSmoothingEnabled or something.
|
We're pretty conservative about keeping around old code here in OSD... I think we still be mostly IE8 compatible... for that reason I'm in favor of leaving |
|
Something like this? |
src/drawer.js
Outdated
| @@ -609,6 +610,9 @@ $.Drawer.prototype = { | |||
| }, | |||
|
|
|||
|
|
|||
| // private | |||
| _imageSmoothingEnabled: true, | |||
There was a problem hiding this comment.
Here's a subtle JavaScript gotcha: by putting this property directly on the prototype like this, it means if any instance changes it, it gets changed for all instances. The fix is to make it part of the object directly by doing this._imageSmoothingEnabled = true in the constructor instead of having it here.
There was a problem hiding this comment.
OK, I'm realizing this gotcha doesn't apply to simple variables like booleans (it only applies to things like objects and arrays). That said, it would still be good to do the other way to keep everything together.
src/drawer.js
Outdated
| this.viewer.forceRedraw(); | ||
| } | ||
| }, | ||
|
|
||
| // private | ||
| _setImageSmoothingEnabled: function(){ |
There was a problem hiding this comment.
I'm thinking _updateImageSmoothingEnabled might be a better name for this, since it's just updating the canvas to match the value that's already been set elsewhere. That'll help with any confusion between this function and setImageSmoothingEnabled as well. I hope you don't mind the new suggestion (I realize the other name was my suggestion as well).
There was a problem hiding this comment.
Not at all, I agree and I'm happy to learn...
|
Beautiful! Thank you for finding and fixing this! |
As explained in #1593, the main issue is that the property imageSmoothingEnabled of a canvas is being reset to "True" each time the canvas is resized (probably by design), while we only set it once on initialization.
This revealed another issue - when window.devicePixelRatio is > 1 and not an integer (this depends on the display type and browser's zoom and not IE vs Chrome as I thought before), _calculateCanvasSize often returns non-integer values.
As canvas width/height are always integers, this if in "clear" function will be evaluated to true on some window sizes:
( this.canvas.width != viewportSize.x || this.canvas.height != viewportSize.y )
which will "resize" the canvas to its current size, and will reset imageSmoothingEnabled as explained before.
In this commit I "floored" _calculateCanvasSize return values.
Regarding the main issue, we should set imageSmoothingEnabled after every resize. I will appreciate some help figuring out the best place to call setImageSmoothingEnabled().
It should be deep enough so it will happen after every resize, but we still need access to the user-defined value.
BTW, from what I see (here for instance), BackingStorePixelRatio is deprecated and always returns 1, so maybe we can get rid of it?
(Sorry for the long msg, and I hope this pull request was done correctly, it's my first one!)