[Win32] Correct zoom level used for image of decorations/shell #2037

[HeikoKlare]

When setting the images for a decorations instance (in particular for a shell) or when updating them upon a zoom change, the two images best fitting to sizes required by the OS in their 100% version are calculated. Then, however, a zoomed version of the image is used based on the auto-scaled zoom of the decorations object. This zoom is, however, completely unrelated to what is required, as the auto-scaled zoom was not taken into account for identifying the best fitting image before.

In the best case, a zoomed version of the image exactly fitting to the size required by the OS would be generated. This would be possible via auto-scaling the image (or in case of SVGs rendering it at the desired size), but this would also lead to bad results if a simple auto-scaling mode was used. Thus, the best possible solution is to use the 100% version of the image, as its size does best fit to the required sizes as conforming to the calculation done before.

Contributes to #2037

[github-actions]

Test Results

   546 files  ±0     546 suites  ±0   34m 19s ⏱️ + 4m 3s
 4 426 tests ±0   4 409 ✅ ±0   17 💤 ±0  0 ❌ ±0 
16 750 runs  ±0  16 623 ✅ ±0  127 💤 ±0  0 ❌ ±0 

Results for commit 2b601ed. ± Comparison against base commit 6b5b54a.

[ShahzaibIbrahim]

The changes looks reasonable as creating 100% zoom image initially is what we usually do with Images as well and it resolves the double scaling problem i.e. If you pre-scale an image with the shell’s auto-zoom before SWT picks one, you’re effectively giving SWT an already scaled image. Then SWT may again resize it to fit the OS size request → leading to blurry, distorted, or oversized icons.

[fedejeanne]

LGTM. Merging.

Just one final question, for my understanding: I noticed you only adapted the usages in Decorations and not in other places (for example, Display.createIcon(...) is also called in TrayItem and TaskItem). Why aren't the same changes needed in those consumers too?

[HeikoKlare]

Just one final question, for my understanding: I noticed you only adapted the usages in Decorations and not in other places (for example, Display.createIcon(...) is also called in TrayItem and TaskItem). Why aren't the same changes needed in those consumers too?

This change is not about the usage of Display.createIcon(...) but about how the zoom to create the icon at is calculated. As described in commit and PR message, in this case we calculate the icon best fitting for the OS metrics based on the 100% of the icons, but we then use a version that is zoomed with an arbitrary value (arbitrary in the sense that it is completely unrelated to what is requested).
@fedejeanne So what do you think needs to be changed at those places?

[fedejeanne]

Thank you for the clarification.

@fedejeanne So what do you think needs to be changed at those places?

If I understand the issue correctly, it should also manifest in the tray area of the OS, which means that the usages in TrayItem::setImage(Image) would also require the change. Does the issue also happen in there?

I'm not sure about TaskItem though, I'm not familiar with the code.

[HeikoKlare]

If I understand the issue correctly, it should also manifest in the tray area of the OS, which means that the usages in TrayItem::setImage(Image) would also require the change. Does the issue also happen in there?

Again: why should that be the case? There is no calculation in TrayItem based on system metrics, matching a specific zoom of multiple images to such metrics etc. (actually there is only a single image in that case), like it's on Decorations. Why should any other zoom fit better than the one returned of getZoom() and how should that be calculated?

[fedejeanne]

I just assumed that the code being so similar in both classes, the issue might manifest itself in the tray. But I read between the lines that this is not the case :-)

Thank you.

[HeikoKlare]

fwiw: there is nothing in between the lines. I just don't understand where the code is similar in both cases, as it's actually completely different. Maybe I miss something.
As I said, in the TrayItem there is just one image retrieved at the appropriate size for the context zoom. In Decorations there are multiple images provided and the best zoomed version of the best fitting image is retrieved by comparing against the size that is required according to system metrics. That's also what the commit and PR description explains. So those calculations are completely different and that's why I wanted to understand why there may be a need for adaptation. You seem to have something specific in mind when you say that the code is similiar and may be updated equally.

[fedejeanne]

I just don't understand where the code is similar in both cases

I meant this pattern of checking for a type (a switch) and, based on it, either create an icon (Display.createIcon(x, zoom)) and get a handle (Image.win32_getHandle(x, zoom)) or just get the handle.

I saw the pattern in 3 places:

TrayItem

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/TrayItem.java

Lines 458 to 466 in 1094ebd

	switch (icon.type) {
	case SWT.BITMAP:
	image2 = Display.createIcon (image, getZoom());
	hIcon = Image.win32_getHandle(image2, getZoom());
	break;
	case SWT.ICON:
	hIcon = Image.win32_getHandle(icon, getZoom());
	break;
	}

TaskItem

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/TaskItem.java

Lines 474 to 482 in 1094ebd

	switch (overlayImage.type) {
	case SWT.BITMAP:
	image2 = Display.createIcon (overlayImage, getZoom());
	hIcon = Image.win32_getHandle(image2, getZoom());
	break;
	case SWT.ICON:
	hIcon = Image.win32_getHandle(overlayImage, getZoom());
	break;
	}

Decorations (adapted in this PR)

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Decorations.java

Lines 889 to 897 in 1094ebd

	switch (smallIcon.type) {
	case SWT.BITMAP:
	smallImage = Display.createIcon (smallIcon, 100);
	hSmallIcon = Image.win32_getHandle(smallImage, 100);
	break;
	case SWT.ICON:
	hSmallIcon = Image.win32_getHandle(smallIcon, 100);
	break;
	}

---

eclipse.platform.swt/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Decorations.java

Lines 901 to 909 in 1094ebd

	switch (largeIcon.type) {
	case SWT.BITMAP:
	largeImage = Display.createIcon (largeIcon, 100);
	hLargeIcon = Image.win32_getHandle(largeImage, 100);
	break;
	case SWT.ICON:
	hLargeIcon = Image.win32_getHandle(largeIcon, 100);
	break;
	}

[HeikoKlare]

So you just refer to code duplication and propose to extract that pattern to a common utilty method? But what's the relation to the change in this PR then?

[fedejeanne]

Nope, I just wanted to raise the question to see if the problem arises in those other 2 places too.

[HeikoKlare]

Sorry, I still don't know which problem you refer you. What's the change you may expect at those other places?

[HeikoKlare]

I guess we can close this discussion as there does not seem to be a concrete issue description or fix proposal?