Skip to content

Fix blurry shell icon #62 #1850

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 6, 2025
Merged

Fix blurry shell icon #62 #1850

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Decorations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ void setImages (Image image, Image [] images) {
System.arraycopy (images, 0, bestImages, 0, images.length);
datas = new ImageData [images.length];
for (int i=0; i<datas.length; i++) {
datas [i] = images [i].getImageData (getZoom());
datas [i] = images [i].getImageData ();
Copy link
Contributor

@amartya4256 amartya4256 Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the getZoom call from here just calls getImageData(100), which doesn't change the ultimate effect of this. For example, even if you use getZoom() (e.g. 200) in this block and in the setImages you pass a small and a largeImage. If they have original size of 16 px and 32 px, it will still select smallImage of 16 px (32 px on 200) and largeImage of 32 px (64 px on 200). On debugging, I see it still allocates the image with the smallest size to smallIcon and the largest to largeIcon. I reckon the previous implementation is fine here.

I think the problem is rather in how you obtain the handle.

if (smallIcon != null) {
		switch (smallIcon.type) {
			case SWT.BITMAP:
				smallImage = Display.createIcon (smallIcon, getZoom());
				hSmallIcon = Image.win32_getHandle(smallImage, getZoom());
				break;
			case SWT.ICON:
				hSmallIcon = Image.win32_getHandle(smallIcon, getZoom());
				break;
		}
	}
	OS.SendMessage (handle, OS.WM_SETICON, OS.ICON_SMALL, hSmallIcon);
	if (largeIcon != null) {
		switch (largeIcon.type) {
			case SWT.BITMAP:
				largeImage = Display.createIcon (largeIcon, getZoom());
				hLargeIcon = Image.win32_getHandle(largeImage, getZoom());
				break;
			case SWT.ICON:
				hLargeIcon = Image.win32_getHandle(largeIcon, getZoom());
				break;
		}
	}
	OS.SendMessage (handle, OS.WM_SETICON, OS.ICON_BIG, hLargeIcon);

The call to Image::win32_getHandle will scale it down to the current zoom. Rather, we should use the correct zoom value for which the icon is supposed to be used. But the question is how the default image sizes which are passed in the method differ.

Copy link
Contributor

@amartya4256 amartya4256 Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see the effect tho, but I dont understand why data has to do anything with it

Copy link
Member Author

@fedejeanne fedejeanne Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, odd. I do see a difference. I am printing the results by adding these lines

image

These are the outputs:
Before this PR

Zoom: 100
smallIcon bounds=Rectangle {0, 0, 16, 16}
largeIcon bounds=Rectangle {0, 0, 32, 32}
Zoom: 125
smallIcon bounds=Rectangle {0, 0, 16, 16}
largeIcon bounds=Rectangle {0, 0, 32, 32}
Zoom: 200
smallIcon bounds=Rectangle {0, 0, 16, 16}
largeIcon bounds=Rectangle {0, 0, 32, 32}

After this PR

Zoom: 100
smallIcon bounds=Rectangle {0, 0, 16, 16}
largeIcon bounds=Rectangle {0, 0, 32, 32}
Zoom: 125
smallIcon bounds=Rectangle {0, 0, 16, 16}
largeIcon bounds=Rectangle {0, 0, 48, 48}
Zoom: 200
smallIcon bounds=Rectangle {0, 0, 32, 32}
largeIcon bounds=Rectangle {0, 0, 48, 48}

As you can see, without this PR there is absolutely no difference in the output and with this PR, bigger icons are selected for higher zoom levels.

Copy link
Member Author

@fedejeanne fedejeanne Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see the effect tho, but I dont understand why data has to do anything with it

My intention when using getImageMetadata() (without zoom) was to retrieve the "original" metadata i.e. the size for the 100% image. Doesn't the method retrieve that?

If I do this:

System.out.println("--- no zoom ---");
for (int i=0; i<images.length; i++) {
	ImageData imageData = images[i].getImageData();
	System.out.println("width=" + imageData.width + ", height=" + imageData.height + ", depth=" + imageData.depth);
}
System.out.println("--- getZoom() is " + getZoom() + " ---");
for (int i=0; i<images.length; i++) {
	ImageData imageData = images[i].getImageData(getZoom());
	System.out.println("width=" + imageData.width + ", height=" + imageData.height + ", depth=" + imageData.depth);
}

I see this:

--- no zoom ---
width=16, height=16, depth=24
width=32, height=32, depth=24
width=48, height=48, depth=24
--- getZoom() is 150 ---
width=24, height=24, depth=32
width=48, height=48, depth=32
width=72, height=72, depth=32

As you can see, the second set of sizes (and depth???) are scaled to 150%, which doesn't fit the requirements.

Am I seeing this wrong, @amartya4256 ?

Copy link
Contributor

@HeikoKlare HeikoKlare Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't an icon that is 150% the height and width exactly what is expected at 150% zoom?
And the depth is not scaled to 150% (neither mathematically nor technically). It rather seems as if auto-scaling changes the image data type from some RGB type (24 bit) to some ARGB type (32 bit).

Copy link
Member Author

@fedejeanne fedejeanne Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is what I expect but not what I need: I need to find the image whose original intended size (without rescaling) best matches the desired size in pixels. For example, if I have 2 images of sizes 16x16 and 32x32 and I want to create a shell icon of size 64x64 then I want to use the image that was created with a size of 32x32 (and scale it later).

Copy link
Contributor

@HeikoKlare HeikoKlare Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by "original intended size". You always retrieve image data for a specific zoom, so the size of an image will depend on what zoom you pass to that method. If you want to know the logical size of an image (according to the auto-scaling capabilities), you may use image.getBounds().

Copy link
Contributor

@amartya4256 amartya4256 Mar 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the depth discussion, here's the flow of the whole thing:

  1. setImages takes images [] array - usually 2 images in it - one small image, one large image
  2. If you call getImageData (with or without zoom), you can see the size difference as shown in Federico's logs.
  3. by default small icon has a size 16 px and large icon 32 px with better resolution - which means large icon looks more detailed.
  4. when you call getImageData(getZoom()) for both the image - let's say at 200%, it becomes 32 px and 64 px each.
  5. Compare method is called with the desired size information using getSystemMetrics (OS.SM_CXSMICON) and getSystemMetrics (OS.SM_CXICON), in a usual case it should be 16 and 32 px.
  6. getSystemMetrics (OS.SM_CXSMICON) is used to evaluate the small icon and getSystemMetrics (OS.SM_CXICON) is used to evaluate the large icon.
  7. If we pass the already scaled image data to sort method with getSystemMetrics (OS.SM_CXICON) to evaluate the large icon, it looks for the image with closest size to 32. Since small image's imaeg data is already 32, it doesn't choose the 64 (large) one but the small one's scaled imageData and it ends up looking pixelated at a higher zoom level.

For now, this fix is a good hack but since we are working with per dpi awareness, we should use the DPI aware system metrics (GetSystemMetricsForDPI) and find out a way to still obtain the large image instance for largeIcon. Anyway, the whole implementation of sort and compare method which uses loops to find out which one is big and which one is small looks quite old school and we can make it better design wise.

Copy link
Member Author

@fedejeanne fedejeanne Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to know the logical size of an image (according to the auto-scaling capabilities), you may use image.getBounds()

Now you hit the nail in the head! Thank you for hint, I'll look into it.

For now, this fix is a good hack...

I don't know why you guys keep calling it a hack, the method says that it returns...
image

...which is still a valid use case and a valid usage of the official API.

... we should use the DPI aware system metrics (GetSystemMetricsForDPI)...

How do you mean? getSystemMetrics(int) ends up calling OS::GetSystemMetricsForDpi

Anyway, the whole implementation of sort and compare method which uses loops to find out which one is big and which one is small looks quite old school and we can make it better design wise.

That's what I wanted to address in #1879 ;-)

Copy link
Contributor

@HeikoKlare HeikoKlare Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a deeper look into #1878 to understand what really happens here and left some comments there. From my current understanding, it would make sense to use getImageData(100) here, as the values are compared against 100% icon sizes, so why not explicitly retrieve the 100% data (and really doing that by passing 100 to avoid that the reader needs to understand the contract of parameter-less getImageData())?

}
images = bestImages;
sort (images, datas, getSystemMetrics (OS.SM_CXSMICON), getSystemMetrics (OS.SM_CYSMICON), depth);
Expand Down
Loading