Fix blurry shell icon #62 #1850
Conversation
fedejeanne
force-pushed
the
fix_blurry_shell_icon_easy
branch
from
b09e26a to
65e3fbf
Compare
Test Results 505 files - 1 505 suites - 1 10m 6s ⏱️ + 1m 47s For more details on these failures, see this check. Results for commit 67b2422. ± Comparison against base commit ccc69c2. This pull request removes 37 tests.♻️ This comment has been updated with latest results. |
| datas = new ImageData [images.length]; | ||
| for (int i=0; i<datas.length; i++) { | ||
| datas [i] = images [i].getImageData (getZoom()); | ||
| datas [i] = images [i].getImageData (); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I do see the effect tho, but I dont understand why data has to do anything with it
There was a problem hiding this comment.
Hm, odd. I do see a difference. I am printing the results by adding these lines
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Apart from the depth discussion, here's the flow of the whole thing:
- setImages takes images [] array - usually 2 images in it - one small image, one large image
- If you call getImageData (with or without zoom), you can see the size difference as shown in Federico's logs.
- by default small icon has a size 16 px and large icon 32 px with better resolution - which means large icon looks more detailed.
- when you call getImageData(getZoom()) for both the image - let's say at 200%, it becomes 32 px and 64 px each.
- 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.
- getSystemMetrics (OS.SM_CXSMICON) is used to evaluate the small icon and getSystemMetrics (OS.SM_CXICON) is used to evaluate the large icon.
- 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.
There was a problem hiding this comment.
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...
...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 ;-)
There was a problem hiding this comment.
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())?
|
I added some details about this PR fixing a regression and how I would like to proceed. I still want to ask for PMC approval and merge this PR for RC2, accepting the fact that it technically doesn't restore the previous (blurry) icon but instead it fixes the original problem i.e. the fact that a small icon (16x16) is used for all zoom levels. |
|
@fedejeanne A procedural comment here - Please ask reviews from @eclipse-platform/eclipse-platform-project-leads and PMC . That way the former group will get notification thus better chances for review. |
|
cc PMC @HannesWell |
We do not use |
To quote you:
|
|
How is that quote related to this change? It refers to a PR that changes code which is only called when an experimental feature is active. This PR affects default behavior of SWT, as the code is executed in every SWT-based application. |
That is the part I was missing, thank you. So the difference is about risk: I'm fine with pushing this to M1 then 👍 |
Windows only: use the original width and height of the images set to the shell when searching for the best fit instead of adapting them to the zoom, otherwise the smallest image will always be picked and it will result in a blurry shell icon when scaling up for monitors with a higher zoom levels. contributes to eclipse-platform#62 and eclipse-platform#127
fedejeanne
force-pushed
the
fix_blurry_shell_icon_easy
branch
from
65e3fbf to
67b2422
Compare
|
Test failure is unrelated #1843. |
4 participants
Windows only: wse the original width and height of the images set to the shell when searching for the best fit instead of adapting them to the zoom, otherwise the smallest image will always be picked and it will result in a blurry shell icon when scaling up for monitors with a higher zoom levels.
contributes to
#62 and #127
How to test
Before this PR
After this PR
Is this a regression?
YesSort of (one needs to activate the experimental feature Monitor-specific UI scaling in order to experience this behavior), introduced when we started using theSMOOTHscaling method (instead ofNEAREST) on Windows (df0ae71 / ffa1084).As demonstration, here's how the shell icon looks like without this PR and using the
NEARESTscaling method:And this is how it looks like in 4.34 (identical):
Since reverting back to the
NEARESTscaling method has other implications (and overall the scaling methodSMOOTHis better), I still propose to accept this PR as a regression fix even though it technically doesn't restore the previous behavior but it makes it better.