Image type SWT.ICON loses its type during re-scale

[ShahzaibIbrahim]

Image initialized as SWT.ICON in org.eclipse.swt.graphics.Image.Image(Device, ImageData, ImageData) loses its "Icon" properties, i.e. type, mask etc., when handle for different zoom level is requested. This change adapts the initialization of the image when the handle is requested for image type SWT.ICON to preserve the properties. The unit test is added that was failing before the fix. You can also see it in action in the snippet provided below:

Display display = new Display();
Shell shell = new Shell(display);

InputStream stream = ShellImageExample.class.getResourceAsStream("yourImage.png");
ImageData imageData = new ImageData(stream);
ImageData mask = imageData.getTransparencyMask();
Image icon = new Image(display, imageData, mask);
System.out.println("Icon Type = " + icon.type);
Image.win32_getHandle(icon, 200);
System.out.println("Icon Type After = " + icon.type);

Fixes #1805

[github-actions]

Test Results

   502 files  +8     502 suites  +8   11m 16s ⏱️ + 1m 15s
 4 334 tests +1   4 320 ✅ +1   14 💤 ±0  0 ❌ ±0 
16 575 runs  +1  16 466 ✅ +1  109 💤 ±0  0 ❌ ±0 

Results for commit 2630103. ± Comparison against base commit e2ae17b.

♻️ This comment has been updated with latest results.

[HeikoKlare]

I do not completely understand the change. The assumption that an image with type SWT.ICON loses it's type when rescaling does not seem to be always true. The following snippet succeeds, even without this change:

		Image image = Display.getCurrent().getSystemImage(SWT.ICON_ERROR);
		assertEquals("Image type should stay to SWT.ICON", SWT.ICON, image.type);
		Image.win32_getHandle(image, 200);
		assertEquals("Image type should stay to SWT.ICON", SWT.ICON, image.type);

So the issue rather seems to be about specific ways in which images of type Icon are created. Is it safe to apply this new behavior for all of them, even for those that properly worked before with the existing implementation?

[ShahzaibIbrahim]

So the issue rather seems to be about specific ways in which images of type Icon are created. Is it safe to apply this new behavior for all of them, even for those that properly worked before with the existing implementation?

The test were passing without the new changes because they were running without quarter scaling. I had my configuration done with quarter scaling on. Now I have move my test to different package as other "Resources Test" with correct settings.

[HeikoKlare]

The test were passing without the new changes because they were running without quarter scaling. I had my configuration done with quarter scaling on. Now I have move my test to different package as other "Resources Test" with correct settings.

Can you explain why running the test with quarter scaling is necessary and why moving the test to a different bundle is necessary for that?

For me, it does not make any difference whether I run the test with quarter scaling or not. I always succeeds, with the change in this PR and without.

[ShahzaibIbrahim]

For me, it does not make any difference whether I run the test with quarter scaling or not. I always succeeds, with the change in this PR and without.

I am not sure why, but when we start working on it the issue was only in quarter. I can still see the test failing without quarter scaling (also updateOnRunTime should not be true)

[HeikoKlare]

Can you please post the exact configuration to execute the test without the fix so that it fails?
I am not able to reproduce it. I used different monitor scalings, I have removed the extension for monitor-specific zoom from the test, reverted the change in Image and executed the test with and without quarter. Everytime it succeeds:

[akoch-yatta]

Can you please post the exact configuration to execute the test without the fix so that it fails? I am not able to reproduce it. I used different monitor scalings, I have removed the extension for monitor-specific zoom from the test, reverted the change in Image and executed the test with and without quarter. Everytime it succeeds

Is your primary monitor 200%? we probably should extend the test to explicitly request multiple zoom setting like:

	Image.win32_getHandle(icon, 200);
	assertEquals("Image type should stay to SWT.ICON", SWT.ICON, icon.type);
	Image.win32_getHandle(icon, 150);
	assertEquals("Image type should stay to SWT.ICON", SWT.ICON, icon.type);
	Image.win32_getHandle(icon, 100);
	assertEquals("Image type should stay to SWT.ICON", SWT.ICON, icon.type);

That should definitely fail

[HeikoKlare]

I tested with primary monitor at 100%, 125% and 200%. All succeed.

Even with the previous extension, it succeeds for me.

Seems like I am missing some configuration, if it fails for all of you. But actually, I run the test with default configuration and on 100% (which is the configuration that should by used by any CI environment as well).

[akoch-yatta]

Hmm, very confusing. I'm pretty sure your test is not executed in monitor-specific-scaling setting.
I didn't configure anything, just executed the test via context menu.

I just tried reverting the image change and let the Github runner execute the build and I get the expected result:

[HeikoKlare]

Change has the expected effect. I tested different documented scenarios and both linked issues are resolved by the change.
We will address an underlying issue (losing properties of the image such as transparency mask when autpscaling image data with "SMOOTH" mode in DPIUtil) as a follow-up

[fedejeanne]

Regarding the test, Shahzahib and I debugged and found the problem: he uses 150% zoom which ends up using the auto scale method SMOOTH and his test follows a different path than mine (and probably @HeikoKlare's).

So the solution would be to either change the monitor zoom in the test (probably not possible in the CI) or setting -Dswt.autoScale.method=smooth early in the test, before Display is initialized. We're looking into that.

[HeikoKlare]

Indeed, see: vi-eclipse/Eclipse-Platform#228

That will be resolved via: vi-eclipse/Eclipse-Platform#227

And in Tycho runs, smooth scaling seems to be activated anyway, as there are tests previously executed setting the device zoom and thus enabling smooth scaling as a "side effect" (again, see vi-eclipse/Eclipse-Platform#227).

[fedejeanne]

How about setting it explicitly? That way we don't need to rely on the environment and one could run the test locally too. See #1807

[HeikoKlare]

The test either wants to test monitor-specific scaling (like suggested right now), then we need to fix how monitor-specific enables smooth scaling (see vi-eclipse/Eclipse-Platform#227) or it is supposed to test smooth scaling behavior, then the test needs to be rephrased and the existing extension regarding monitor-specific scaling needs to be removed.