[win32] Wrapper for existing image handles #2027
Conversation
There was a problem hiding this comment.
In general, the change looks sound to me. I was wondering if we can avoid creating a new type of provider by just using the image data of the given handle, but if I am not mistaken that's not possible (or not a good idea at least), because the current implementation allows to share handles between Image instances, which is also why disposed resource tracking is not instantiated for an Image created via an existing handle. Is that correct?
This kind of change seems to allow for more cleanups. The follow methods have implementations for imageProvider == null, which should become obsolete by this change and can be removed, can't they?
equals/hashCodegetBoundsgetImageDatagetImageMetaData(see detailled comment)
| } else { | ||
| ImageData resizedData = getImageData(zoom); | ||
| ImageData newData = adaptImageDataIfDisabledOrGray(resizedData); | ||
| init(newData, zoom); |
There was a problem hiding this comment.
In the Image#getImageMetadata(int) method, there is a special initialization handling for the case that the image is an icon. What if the image created with this new provider is such an icon-type image, won't the initialization potentially lead to wrong results?
Or even more: the imageProvider == null case in the Image#getImageMetadata(int) method should become completely obsolete by this change, so it should be removed and there and completely be moved into this method, shouldn't it?
There was a problem hiding this comment.
Hmm, regarding the first point. probably yes, but isn't this an issue with e.g. MaskedImageDataProviderWrapper as well?
Second point. Yes, question to me is, when and how do we do the cleanup.
There was a problem hiding this comment.
Hmm, regarding the first point. probably yes, but isn't this an issue with e.g. MaskedImageDataProviderWrapper as well?
Probably, yes. We should check again the original issue that was fixed by that special handling for icon to see if we can reproduce the issue now.
Second point. Yes, question to me is, when and how do we do the cleanup.
Okay, let's do that as a follow-up. But I would do that now (and before #1996) if possible.
Yes, we definitely need to use the passed OS handle, because it is created outside of the control of the Image. Therefor I did not see another approach as creating a separate wrapper for it
Yes, but I wanted to do that cleanup separately. |
akoch-yatta
force-pushed
the
win32-wrapper-with-existing-image-handle
branch
from
38b8c42 to
ab19dd0
Compare
akoch-yatta
force-pushed
the
win32-wrapper-with-existing-image-handle
branch
from
ab19dd0 to
3dad941
Compare
This commit adds a wrapper for images created with an existing OS handle to ensure all existing image constructors are tied to a final wrapper instance.
HeikoKlare
force-pushed
the
win32-wrapper-with-existing-image-handle
branch
from
3dad941 to
478c83f
Compare
|
Merging this despite Jenkins failure caused by https://gitlab.eclipse.org/eclipsefdn/helpdesk/-/issues/5892 Since master builds seem to still work on Jenkins, the subsequent master build should hopefully work, thus I will check results of that build to retrospectively validate this PR. |
This PR adds a wrapper for images created with an existing OS handle to ensure all existing image constructors are tied to a final wrapper instance.