-
Notifications
You must be signed in to change notification settings - Fork 187
Unify instantiation of image handles in ImageFileNameProvider #1877
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.
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
Conversation
8d3985b to
c51c9c9
Compare
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a failing test for image initialization on Windows, so the change does to not seem to be correct as is.
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
832980d to
3c4f49a
Compare
|
Tests that are failing will be resolved once #1898 is merged. So it makes this PR dependent. |
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR now basically reverts the commit made to Image recently pushed on master. Please adapt to current behavior of the class first.
7dad43f to
52e4a8a
Compare
HeikoKlare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the change looks good. I only have a remark regarding adaptation of gray/disabled images.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
Show resolved
Hide resolved
Initializing image handle only using initNative method for both the cases where zoom is equal to the filezoom and when its not equal to the filezoom. The handle in case of not equal is created temporarily and destroyed later before calling init.
| imageDataAtZoom = ImageDataLoader.load(fileForZoom.element(), fileForZoom.zoom(), zoom); | ||
| } else { | ||
| imageDataAtZoom = new ElementAtZoom<>(nativeInitializedImage.getImageData(), fileForZoom.zoom()); | ||
| destroyHandleForZoom(zoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to destroy the handle if it was retireved from the zoomLevelToImageHandle map? I guess we should only do that if it was not in that map before (i.e., if it was temporarily created for retrieving the image data), shouldn't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is confusing, yes, but logically okay. zoom != fileForZoom.zoom(). It is again a place, where we will need to do a second round, when we refactored all the places to be able to better create temporary handles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see, it is because the calling method Image#getImageData(int) ensures that. I didn't see that. Yes, that's definitely confusing and we should clean up in a second round.
Initializing image handle only using initNative method for both the cases where zoom is equal to the filezoom and when its not equal to the filezoom. The handle in case of not equal is created temporarily and destroyed later before calling init.
Dependent on: