-
Notifications
You must be signed in to change notification settings - Fork 184
[Win32] Fix transparency when scaling images with transparentPixel #2615
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
base: master
Are you sure you want to change the base?
Conversation
efc46c5
to
587627a
Compare
Yeah! 🥳 |
4340fa2
to
08fa953
Compare
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.
As we already discussed, moving transparentPixel/transparentColor to the individual handle makes sense. However, there now seem to be some flaws on how to retrieve the transparentPixel
at Image level, where arbitrary handles or image data are used for that. Would it maybe make sense to additionally store whether the image was (originally) based on a transparent pixel to have this information available at image level, no matter whether some rescaling operation produced other handles that do not use the transparent pixel anymore? Or maybe store a reference to the according image handle, such that getBackgroundColor()
can use the correct handle that is based on the transparent color?
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
Show resolved
Hide resolved
Previously, the transparency pixel was managed at the Image level, which caused issues when an Image had multiple handles at different zoom levels or color models (e.g., indexed vs. direct/ARGB). For example, a GIF image at 100% zoom may use a transparency pixel, but when scaled, SWT converts it to a direct image with alpha data, making the transparency pixel concept invalid for the scaled handle. This change moves the transparency pixel field from the Image class to the ImageHandle class. Now, each handle manages its own transparency information, allowing a single Image to have multiple handles, each with the correct transparency type (pixel or alpha) according to its color model. This ensures correct transparency handling for all handles, regardless of how they were created or scaled.
08fa953
to
df77fa7
Compare
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.
I think that we agree that the
transparentPixelbelongs to the handle and not to the image, so this is generally the right thing to do. @ShahzaibIbrahim can you please extract the preparatory refactoring to change the return type of
getHandle()and use that one instead of
win32_getHandle()` into a separate preparatory PR, such that this one becomes easier to review?
I left some additional comments regarding the functional changes in this PR.
OS.SelectObject(hdcDest, hOldDest); | ||
OS.DeleteDC(hdcSource); | ||
OS.DeleteDC(hdcDest); | ||
imageMetadata.transparentPixel = imageHandle.transparentPixel; |
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.
Why not pass this to the constructor call to ImageHandle above?
*/ | ||
int transparentPixel = -1, transparentColor = -1; | ||
|
||
public ImageHandle(long handle, int 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.
Can't we remove this constructor to put the responsibility for thinking about whether a transparentPixel needs to be specified or not to the consumer? Otherwise we risk that we or someone unexperienced just calls this constructor without thinking about the transparent pixel, but would actually need to take into account that it must be set.
if (transparentPixel == -1) return; | ||
transparentColor = -1; | ||
backgroundColor = color.getRGB(); | ||
zoomLevelToImageHandle.values().forEach(imageHandle -> imageHandle.setBackground(backgroundColor)); |
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.
Why do we completely remove this? In case there are already existing handles, shouldn't we ...
- apply the color if possible?
- print a warning that it cannot be applied if the existing handles are scaled versions (maybe encapsulated in a strict check to avoid performance impacts)?
Description
Move Transparent Pixel Handling from Image to ImageHandle
Previously, the transparency pixel was managed at the Image level, which caused issues when an Image had multiple handles at different zoom levels or color models (e.g., indexed vs. direct/ARGB). For example, a GIF image at 100% zoom may use a transparency pixel, but when scaled, SWT converts it to a direct image with alpha data, making the transparency pixel concept invalid for the scaled handle.
This change moves the transparency pixel field from the Image class to the ImageHandle class. Now, each handle manages its own transparency information, allowing a single Image to have multiple handles, each with the correct transparency type (pixel or alpha) according to its color model. This ensures correct transparency handling for all handles, regardless of how they were created or scaled.
How to test
Result
Here's the preview how it looked before and after this PR change. 100 -> 150 -> 100
Before:

After

Fixes: #2494