[win32] Move of unified code from Wrapper to Image #2069
Conversation
akoch-yatta
force-pushed
the
win32-move-image-getimagedata
branch
from
aad5ca0 to
31730f4
Compare
| @@ -562,7 +567,7 @@ public Image(Device device, ImageFileNameProvider imageFileNameProvider) { | |||
| super(device); | |||
| this.imageProvider = new ImageFileNameProviderWrapper(imageFileNameProvider); | |||
| initialNativeZoom = DPIUtil.getNativeDeviceZoom(); | |||
| if (imageProvider.getImageData(100) == null) { | |||
| if (imageProvider.newImageData(100) == null) { | |||
There was a problem hiding this comment.
I was wondering if we could avoid/improve this, as it's quite some potentially unnecessary effort to load image data at 100% if they are never used. Then I saw that the documentation says:
ERROR_INVALID_ARGUMENT - if the fileName provided by ImageFileNameProvider is null at 100% zoom
So the current implementation is even wrong: it should not be tested if image data at 100% can be loaded but if the passed imageFileNameProvider returns a non-null value at 100%.
There was a problem hiding this comment.
The problem are the other exceptions mentioned:
* <li>ERROR_INVALID_IMAGE - if the image file contains invalid data </li>
* <li>ERROR_UNSUPPORTED_DEPTH - if the image file describes an image with an unsupported depth</li>
* <li>ERROR_UNSUPPORTED_FORMAT - if the image file contains an unrecognized format</li>
Those will only be detected if the ImageData is loaded once. If we remove that call here it would fail later, which would change the behavior
There was a problem hiding this comment.
I see. Since we are now caching the already loaded image data, it might make sense to then not use zoom 100 here but instead retrieve some data that we will likely use?
Still, in my opinion we throw the mentioned exception in the wrong case: it should not be thrown when no image data can be generated but when the fileName provided by the file name provider at 100% is null. But maybe that just happens in the same situations.
There was a problem hiding this comment.
Ah, now I fully get, what you mean. I just took a short look into the history. Before we only used getImageMetadata that seemed to trigger the exceptions, we could probably do a newImageHandle + immediatly destroy if afterwards. I will try to have a look tomorrow.
| @@ -521,7 +526,7 @@ public Image (Device device, String filename) { | |||
| } | |||
| return null; | |||
| }); | |||
| if (imageProvider.getImageData(100) == null) { | |||
| if (imageProvider.newImageData(100) == null) { | |||
There was a problem hiding this comment.
I just saw that there is no documentation about this kind of check and it's also not present in the other OS implementations. So probably we should just remove it?
There was a problem hiding this comment.
As above implicitly checking other exception mentioned in the javadoc, e.g.
* <li>ERROR_IO - if an IO error occurs while reading from the file</li>
There was a problem hiding this comment.
But like above: the exception we throw does not seem to be documented. So I guess it would be more correct to just call imageProvider.newImageData() without throwing a exception if the result is null?
This commit moves the code that is equal to all AbstractImageProviderWrapper implementations back to Image.
akoch-yatta
force-pushed
the
win32-move-image-getimagedata
branch
from
31730f4 to
7d05313
Compare
|
Everything related to the above discussion has been extracted into #2070, so this is fine now. Failing version increment check is due to infrastructure issues and no version increment is required here. |
This PR is a simple refactoring and just moves the code that is equal to all AbstractImageProviderWrapper implementations back to Image.