Fixed Program Icon Scaling for Multi Zoom #1709
Conversation
There was a problem hiding this comment.
The code looks good to me. If I understand everything correctly, the change to Program makes total sense and the implementaton is far more clean and reasonable with the change than it was before.
I currently only have one comment regarding a change to Image, as from my understanding that change indicates an existing bug, independent from the changes to program icons, so I would propose to extract that one into a dedicated PR for that specific issue.
I have not tested the changes so far, but will do that as soon as possible.
| if (isDisposed()) SWT.error(SWT.ERROR_GRAPHIC_DISPOSED); | ||
| int currentZoom = getZoom(); | ||
| if (zoom == currentZoom) { | ||
| return getImageDataAtCurrentZoom(); | ||
| } else if (imageProvider != null) { | ||
| if (zoomLevelToImageHandle.get(zoom) != null) { | ||
| return zoomLevelToImageHandle.get(zoom).getImageData(); | ||
| } |
There was a problem hiding this comment.
Isn't this something was wrong wrong anyway, independent from Program icons? Querying the image data for specific zoom value should not consider any autoscaling-value-based correction of that zoom value anyway, should it? So I think this change should be extracted into a separate PR with an according regression test, e.g., for autoscaling integer200 retrieving image data for zoom 125 and 150 one after another and assuming that they do not deliver the same image data.
There was a problem hiding this comment.
I have adjusted the PR as per requested. Here's the PR for the Image::getImageData fix: #1713
amartya4256
force-pushed
the
program_icon_scaling
branch
3 times, most recently
from
8fecf0b to
2b5e970
Compare
There was a problem hiding this comment.
I've now also tested the change and have some findings. Some are expected and unavoidable with the current type of implementation whereas at least seems to be a regression.
Expected Limitations
Since the used Windows API methods return icons scaled according to the primary monitor, you will often have two scalings applied: first by Windows scaling one of the original 16x16, 32x32, ... icons to what is required for the zoom level of the primary monitor, and second, by the application to use it in the scaling required for the monitor the window is currently placed on.
This means for example means if your primary monitor has a rather high resolution (e.g., scaled at 200% or more), the icons for other, lower resolution monitors (e.g., at 100%) are scaled down dramatically, leading to information loss. E.g., at 200% primary zoom the large icon is loaded and scaled down to 100% for a 100% secondary monitor:
It even looks bad if you have an "ordinary" 100% monitor but the primary monitor has, e.g., 150% scaling on the 100%, for which you would expect the original 100% icons to be loaded:
Regression
Environment:
- Single monitor sufficient
- Primary monitor scaling 150%
- Auto scale mode
quarter
Original appearance:
Appearance with this PR:
You can see the information, in particular in the icons for PowerPoint and Excel documents. They become hardly comprehensible.
The reason is that the package explorer loads 200% images, thus making the new implementation retrieve the large icon, which is delivered at 300% (two times the native zoom) and then for final rendering scaled down to 150%.
Note that this effect was also covered in #1014. The following code has been used there to determine when to use large icons (note that comment and implementation are not aligned):
// Use small icon if expected icon size is less than 150% of delivered icon size
// and use large icon if it is more than 150% of the delivered icon size
boolean useLargeIcon = 100 * zoom / primaryMonitorZoom > 200;Maybe that's something to reuse.
amartya4256
force-pushed
the
program_icon_scaling
branch
2 times, most recently
from
9746820 to
5a8a39a
Compare
|
HeikoKlare
force-pushed
the
program_icon_scaling
branch
from
5a8a39a to
2f67eb2
Compare
There was a problem hiding this comment.
The change now looks sound to me and I did not find any issues while testing.
Could you please have a second look regarding the changes in the Image class, whether passing zoom to the win32_new method is also fine from your point of view, @akoch-yatta?
Yes, this is the necessary here and probably even the better solution overall. Only thing to mention here, is that the passed some is different in some usages. The used zoom look correct, but we should be aware that there could be some regressions, if the zoom is wrong -> regression would "only" mean blurry scaling |
Yes, important point. I was also thinking about whether the actual passed zoom values are correct, but found them all to be reasonable. If I am not mistaken, only risk for regressions is in Program icons (which are actually to be improved by this PR) and in TaskBar. At all other places the zoom is retrieved via Another change affecting the zoom is that the ImageHandle is now created with |
HeikoKlare
force-pushed
the
program_icon_scaling
branch
from
2f67eb2 to
fd88c37
Compare
This contribution reimplements the logic for scaling the Program Icons while getting rid of the DPIUtil::getDeviceZoom usage and enhancing the image quality of icons on scaling up. contributes to eclipse-platform#62 and eclipse-platform#127
HeikoKlare
force-pushed
the
program_icon_scaling
branch
from
fd88c37 to
bacb660
Compare
In the previous implementation, according to the documentation, SHGetFileInfo retrieves a system icon at the primary monitor native zoom. It means if the primary monitor is 175%, the dimension of the icon will be 56 x 56 (considering the standard dimension of 16 x 16 at 100%). Previously, the Image::win32_new was used to create an image object by passing the handle. Since the support of multi zoom, we know that now an image can have multiple handles, each for a zoom level. Image::win32_new used to assign the handle to the image at DPIUtil::getDeviceZoom regarless of what zoom level the handle was created for (in our case primary monitor native zoom, e.g. 175%). And it used to do some calcultation what proportion of primary monitor zoom would be this assigned zoom of the image. In other words, if you wanna obtain an image for 100% zoom, it will be 57% of 175%. Since, it used to call image.getImageData(57), which kinda seems like a hack to obtain the icon with the right dimension.
Another problem that was quite visible was that if the monitor to draw the icon has a higher zoom level (say 300%) than the primary monitor zoom, the icon will be pixelated because the retrieved icon from SHGetFileInfo is 175% (primary montior native zoom)
This contribution reimplements the logic for scaling the Program Icons while getting rid of the DPIUtil::getDeviceZoom usage and enhancing the image quality of icons on scaling up using the Small Icon in case we have to scale down and using the Large Icon in case we have to scale up.
contributes to #62 and #127