#1130: [Win32]: Program.getImageData is broken for ".exe" extension

[tmssngr]

Use previous code to retrieve icon with SHGetFileInfo if ExtractIconEx did not return one

[github-actions]

Test Results

   298 files   - 1     298 suites   - 1   8m 7s ⏱️ + 1m 29s
 4 099 tests ±0   4 090 ✅  - 1   8 💤 ±0  1 ❌ +1 
12 209 runs  ±0  12 133 ✅  - 1  75 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 5500a22. ± Comparison against base commit 103b10e.

♻️ This comment has been updated with latest results.

[iloveeclipse]

@tmssngr : please consider to change commit message to properly refer to the original issue.

Adding a line Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/715 would automatically link this PR to the bug, close the bug if PR is merged and help later to navigate directly to the issue from IDE

[tmssngr]

@iloveeclipse Better now?

[HeikoKlare]

The missing .exe icon is obviously a regression. But please note that with this change, program icons can have faulty alpha channel data again, which has been fixed with #999. This can be seen when using them in an IDE product:

[iloveeclipse]

@iloveeclipse Better now?

Sure, but I mean literally: could you please add an extra line to the commit message

Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/715

because this would automatically link this PR to the bug, close the bug if PR is merged and help later to navigate directly to the issue from IDE

[Phillipus]

With the latest commit for this PR there is a black background in Eclipse on Windows hi-dpi 200% scaling:

[tmssngr]

The missing .exe icon is obviously a regression. But please note that with this change, program icons can have faulty alpha channel data again, which has been fixed with #999. This can be seen when using them in an IDE product:

If you know how to get the missing icon (or maybe there are more - I just noticed it for .exe files) without the black background, you are welcome to send a pull request. I prefer to have an icon with black background instead of no icon at all.

[HeikoKlare]

I prefer to have an icon with black background instead of no icon at all.

I have never questioned that. I just wanted to point out that this will replace one issue with another (no matter whether one is more severe than the other). The situation produced by this PR would, in my opinion, be worse than it was before #999, as then you will have some icons with proper alpha values and some with broken alpha values, making the appearance even more inconsistent and making it even more difficult to analyze the issue.

Furthermore, you have already pointed to the original issue #715, in which I had proposed an alternative solution. We just preferred the solution in #999 because it was the cleaner one and we did not find the regression addressed here when validating that approach. So I would prefer to analyse whether we should not better revert #999 and implement the alternative solution, which was shown in #998, instead.

To this end, I did some rundimentary analyses on the performance of the different solutions. I created an image out the program's image data 10k times, once for an image that can be retrieve via ExtractIconEx (represented by .pptx) and once for an image that can only be retrieved via SHGetFileInfo (represented by .exe):

final Display display = new Display();
final Program program = Program.findProgram(".pptx");
long startTime = System.currentTimeMillis();
for (int i = 0; i < 10000; i++)  {
	ImageData data = program.getImageData();
	Image image = new Image(display, data);
	image.dispose();
}
long endTime = System.currentTimeMillis();
System.out.println("Took: " + (endTime-startTime));

Results (average of 5 runs):

• Original (before [Win32] Remove buggy icon retrieval from file extensions #715 #999):
  • .pptx: 2100ms
  • .exe: 2400ms
• With [Win32] Remove buggy icon retrieval from file extensions #715 #999 (current)
  • .pptx: 2300ms
• With #1130: [Win32]: Program.getImageData is broken for ".exe" extension #1131 (this PR)
  • .pptx: 3000ms
  • .exe: 4500ms
• With [Win32] Extract alpha values from icons if available #715 #998:
  • .pptx: 1500ms
  • .exe: 1700ms

So in terms of performance, completeness and cleanness of the solution, I would propose to revert #999 and try #998 instead.
Any objections?

[Phillipus]

Any objections?

I'm somewhat wary of changes to the Image class. A change that appears to work today might cause a regression only discovered later on. Just expressing my concerns. :-)

[tmssngr]

So in terms of performance, completeness and cleanness of the solution, I would propose to revert #999 and try #998 instead.
Any objections?

No objections. For me the black background did not matter much, but the missing icon did. So my patch fixed that. If you have a solution that is better than mine, also addressing the black background, feel free to share.

[HeikoKlare]

I'm somewhat wary of changes to the Image class. A change that appears to work today might cause a regression only discovered later on. Just expressing my concerns. :-)

I understand the concern, as SWT code is badly tested and so there is a higher risk that regressions will show up later (see the issue addressed in this PR, the GC fix issue you found just before 2024-03 release etc.). But if we took that as an argument against making changes, SWT would completely get stuck in the current state.
I am always kind of "afraid" to make changes to SWT because of the expressed reasons. But without trying to move forward, we will not see any real progress anymore.

I have created PR #1138 that replaces #999 with #998, as an alternative to this PR with the reasons given above.

[tmssngr]

What kind of unit tests we could make for this code? Check that we receive an image at all. Would it make sense to verify the size of the image or its content? The content might change (seldom) depending on the Windows version or installed software. Could the size depend on the screen configuration/zoom factors?

[deepika-u]

Please see my comment regarding this pr in
-> #1138 (comment)

[Phillipus]

Can this be closed now as we have #1138?