[win32] Create image handles on demand for width/height based constructors #1884
Conversation
akoch-yatta
force-pushed
the
refactor-image-branch
branch
from
d943889 to
5eeb513
Compare
akoch-yatta
force-pushed
the
refactor-image-branch
branch
from
5eeb513 to
5eba21f
Compare
akoch-yatta
force-pushed
the
refactor-image-branch
branch
2 times, most recently
from
f8377ad to
eae19e4
Compare
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Show resolved
Hide resolved
akoch-yatta
force-pushed
the
refactor-image-branch
branch
from
eae19e4 to
dd9b823
Compare
| @@ -330,8 +327,7 @@ public Image(Device device, Rectangle bounds) { | |||
| super(device); | |||
| if (bounds == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); | |||
| initialNativeZoom = DPIUtil.getNativeDeviceZoom(); | |||
| bounds = DPIUtil.scaleUp (bounds, getZoom()); | |||
| init(bounds.width, bounds.height); | |||
| this.imageProvider = new PlainImageProviderWrapper(bounds.width, bounds.height); | |||
There was a problem hiding this comment.
There is a (very low) risk that this leads to a change in behavior for consumers: when passing bounds with non-zero x/y, the DPIUtil.scaleUp for the bounds may produce a different result regarding width/height than when executing it with x=y=0. I guess we can accept that but given the fact that some of the bounds' values are not used anyway, this constructor is quite misleading. Should we directly deprecate it (and replace its known usages), either as preparatory PR or directly after?
There was a problem hiding this comment.
Yes, I agree. But we have that already on the roadmap vi-eclipse/Eclipse-Platform#203
| if (memGC != null) { | ||
| return getImageDataAtCurrentZoom(); | ||
| } |
There was a problem hiding this comment.
Isn't this wrong now? The image handle used by the GC is not created for the Image's initialNativeZoom anymore (for which this method call returns the according image data), but for the zoom used for the GC. Thus we have to use that specific image data, don't we? In addition, can't we keep the memGC handling at a central place? This is now duplicated code and comment, once in the getImageData(int) implementation of Image and once here. Same applies to the rest of the method code. Shouldn't that be put at some central place for all "static images" until all of them are refactored to use an ImageProvider implementation?
There was a problem hiding this comment.
Isn't it the same behavior as before? getImageDataAtCurrentZoom calls getImageMetadata(getZoom()).getImageData() which uses initialNativeZoom
I agree to the duplication. Then I'll create a temporary method in Image that is used from both places
This commit refactors the Image constructor using width and height to create a bitmap to create handles on demand instead of creating a first handle in the constructor.
akoch-yatta
force-pushed
the
refactor-image-branch
branch
from
dd9b823 to
95cec51
Compare
This PR refactors the Image constructor using width and height to create a bitmap to create the handles always on demand instead of creating its first handle in the constructor.