Skip to content

Conversation

@HeikoKlare
Copy link
Contributor

This replaces the fix for black icon background because of faulty alpha values contributed via #999 with the solution proposed in #998. As documented in #1130, #999 produced a regression as some program icons (such as for .exe files) are not loaded anymore.

Under certain conditions, program icons loaded on Windows via GDI+ have empty mask data, even though the original icon has proper mask data. As a result, these icons are printed with a black instead of a transparent background. Still these icons can contain valid alpha data in their usual 32-bit data.

With this change, alpha data is extracted for icons which are loaded
without proper mask data to ensure that they have proper transparency
information.

Fixes #715
Fixes #1130

@HeikoKlare
Copy link
Contributor Author

Proposed change has already been validated during the initial attempt to fix #715. In particular, see #715 (comment).

@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2024

Test Results

   299 files  ±0     299 suites  ±0   7m 9s ⏱️ - 1m 7s
 4 100 tests ±0   4 092 ✅ ±0   8 💤 ±0  0 ❌ ±0 
12 212 runs  ±0  12 137 ✅ ±0  75 💤 ±0  0 ❌ ±0 

Results for commit 78573aa. ± Comparison against base commit 847a78b.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare marked this pull request as ready for review March 24, 2024 17:04
@deepika-u
Copy link
Contributor

When applied this patch i am seeing like this on 100%, 125%, 150%, 175%, 200% resolutions respectively.

1138_fix

Sorry if you expected an early reply from me.
100% images are very appropriate. 125% and 150% images slowly got cutoff like. 175% and 200% images became difficult to map against right entry.

@HeikoKlare
Copy link
Contributor Author

Thanks for the verification @deepika-u! We were not waiting for any feedback so far. This was supposed to demonstrate an alternative to #1131.

Unfortunately the fix was missing the usage of given zoom for retrieving the proper image data. I have corrected that and now the results look as follows for 100%, 125%, 150%, 175% and 200%:
after1138

This is how it looks before this PR:
after999

@deepika-u
Copy link
Contributor

@HeikoKlare @tmssngr
Today i have tested latest 1138 and 1131 pr. I am not seeing black background with #1138 way of fixing the issue.

With out any pr ->
1138_before

With 1131 pr ->
1131_fix

With 1138 pr ->
1138_fix

From a user perspective i would prefer to go with 1138 way of fixing because it looks better in my opinion.

@tmssngr
Copy link
Contributor

tmssngr commented Apr 1, 2024

@deepika-u Thanks for the screenshots. I'd also say the last result (1138) is the best.

@HeikoKlare HeikoKlare force-pushed the issue-1130 branch 2 times, most recently from 171acf7 to 1133702 Compare April 2, 2024 07:36
@HeikoKlare
Copy link
Contributor Author

Thanks for testing again, @deepika-u, and for providing all the screenshots!

The screenshots show one final issue that was not visible in my screenshots because we use different settings (swt.autoScale=quarter in my screenshots and swt.autoScale=integer200 in yours). The images with swt.autoScale=integer200 are cut off, which happens because I missed to apply the zoom factor "normalization" w.r.t. the primary monitor also to the image data retrieval using the re-added API method call. I have corrected this in 1d2578a and now we get the following proper results:

With swt.autoScale=quarter:
image

With swt.autoScale=integer200 (or not having it specified, as that's the default):
image

Also thanks to @tmssngr for sharing your opinion on the results.
I consider the comments of the two of you as approvals of this PR's results, so I would propose to merge it soon.

@tmssngr
Copy link
Contributor

tmssngr commented Apr 2, 2024

@HeikoKlare: Out of curiosity. Could you please highlight in deepika-u's screenshots where the remaining glitch was noticable? I have not spotted it yet.

@HeikoKlare
Copy link
Contributor Author

Sure. The icons of external programs at 125% and 150% were affected. You find them highlighted in the following screenshot:
image

Actually the ones at 175% were also affected (slightly too small), but that's not really noticable.

@deepika-u
Copy link
Contributor

deepika-u commented Apr 2, 2024

Hi @HeikoKlare
Thanks for the fix again. I have actually observed that cutoff in 125% and 150% but felt that is still OK than we were earlier.

I have tested your latest fix again and see they are not cutoff anymore now. I say its really a "WOW" for your consistent efforts for driving this fix.

Without 1138 pr
->
1138_before

With latest 1138 pr(after 2 new commits added)
->
1138_new

I am good to go with this fix. Thanks alot once again.

I still see another problem which we may want to fix later(may be in another issue as an improvement on top of this).
I see the overall icon size from 100% to 125% decreases. It further decreases in 150%. But from 150% to 175% it increases drastically. Then increases 175% to 200% consistently. So basically similar to the cutoff, icon sizes deviate alot in 125% and 150% dominantly.
Whereas the font corresponding to the icons always increase from 100% to 125% to 150% to 175% to 200% consistently.
But yeah sorry to add another issue here though we have improved alot.
I have highlighted one for ease of understanding here
->
1138_new_highlighted

@HeikoKlare
Copy link
Contributor Author

I still see another problem which we may want to fix later(may be in another issue as an improvement on top of this).
I see the overall icon size from 100% to 125% decreases. It further decreases in 150%. But from 150% to 175% it increases drastically. Then increases 175% to 200% consistently. So basically similar to the cutoff, icon sizes deviate alot in 125% and 150% dominantly.
Whereas the font corresponding to the icons always increase from 100% to 125% to 150% to 175% to 200% consistently.
But yeah sorry to add another issue here though we have improved alot.

That's a correct observation and even applies to all icons, not only program icons. The reason is the usage of swt.autoScale=integer200, as this makes the DPIUtil class always return a device zoom that is a multiple of 100. So for monitor scalings of 125% and 150%, it always returns 100 and for 175% it returns 200. This value is only used at few faces, one of them being icons, which makes them look scaled incorrectly. In my opinion, it does not really make sense to use swt.autoScale=integer200 as a default value on Windows, as Windows usually uses 25% steps of scaling, so swt.autoScale=quarter would be appropriate. I have used that option in my screenshots, in which you see that the icons are sized properly.
Probably this scaling option did not work properly some time ago, but by now it seems to work fine and should probably be the default on Windows. On Linux/GTK and MacOS/Cocoa the integer200 mode makes sense due to the behavior of their graphics libraries.

@deepika-u
Copy link
Contributor

deepika-u commented Apr 2, 2024

That's a correct observation and even applies to all icons, not only program icons. The reason is the usage of swt.autoScale=integer200, as this makes the DPIUtil class always return a device zoom that is a multiple of 100. So for monitor scalings of 125% and 150%, it always returns 100 and for 175% it returns 200. This value is only used at few faces, one of them being icons, which makes them look scaled incorrectly. In my opinion, it does not really make sense to use swt.autoScale=integer200 as a default value on Windows, as Windows usually uses 25% steps of scaling, so swt.autoScale=quarter would be appropriate. I have used that option in my screenshots, in which you see that the icons are sized properly.

Do you also want to add that part too(change the default setting to make it neat and complete) and give it a try in this fix itself? Or to make it simpler it might be recommended to do in a different pr altogether? Whats your opinion on it?
Where should i be changing if i want to see that change locally?

@HeikoKlare
Copy link
Contributor Author

I would do rather such a change in a separate PR as it is unrelated to what we do here and there may be more opinions and discussion on it (and also potential reasons not to do the change).
You can test the behavior by adding -Dswt.autoScale=quarter as a VM argument to your run configuration.

…pse-platform#715"

This reverts commit 97ca656 because it
resulted in specific program icons (such as the one for .exe files) not
being loaded anymore.

Contributes to
eclipse-platform#1130
…#715 eclipse-platform#1130

Under certain conditions, program icons loaded on Windows via GDI+ have
empty mask data, even though the original icon has proper mask data. As
a result, these icons are printed with a black instead of a transparent
background. Still these icons can contain valid alpha data in their
usual 32-bit data.

With this change, alpha data is extracted for icons which are loaded
without proper mask data to ensure that they have proper transparency
information.

Fixes eclipse-platform#715
Fixes
eclipse-platform#1130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Win32]: Program.getImageData is broken for ".exe" extension [Windows Hi-DPI] Program icons have black background

3 participants