Skip to content
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.

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

Incorrect scaling of program icons #186

Closed
HeikoKlare opened this issue Jan 17, 2025 · 15 comments · Fixed by eclipse-platform/eclipse.platform.swt#1804
Closed

Incorrect scaling of program icons #186

HeikoKlare opened this issue Jan 17, 2025 · 15 comments · Fixed by eclipse-platform/eclipse.platform.swt#1804
Assignees
Labels
Bug A Derivation of Expected Behavior HiDPI A HiDPI-Related Issue or Feature
Milestone

Comments

@HeikoKlare
Copy link
Contributor

Under specific conditions, program icons are not scaled correct.
E.g., do the following:

  • In a workspace, create a file using a program icon within an project.
  • Set your monitor scaling to 100%
  • Restart the application with -Dswt.autoScale.updateOnRuntime=true
  • Do not expand the project with the file using the program icon (i.e., until now, that program icon should not have been loaded)
  • Change the monitor scaling to 150%
  • Expand the project with the file using a program icon

You will see that the program icons are scaled according to the original zoom rather than the current one:

Image

@HeikoKlare HeikoKlare added this to HiDPI Jan 17, 2025
@HeikoKlare HeikoKlare converted this from a draft issue Jan 17, 2025
@HeikoKlare HeikoKlare added Bug A Derivation of Expected Behavior HiDPI A HiDPI-Related Issue or Feature labels Jan 17, 2025
@HeikoKlare HeikoKlare added this to the 4.35 M3 milestone Jan 17, 2025
@amartya4256
Copy link

Can you tell me what is the zoom of your primary monitor? Since the retrieved size of the program icons before scaling is related to primary monitor zoom, it might be important to know that too.

@HeikoKlare
Copy link
Contributor Author

Primary monitor zoom is according to the described workflow. The workflow just assumes/requires a single monitor.

@amartya4256 amartya4256 self-assigned this Jan 26, 2025
@amartya4256 amartya4256 moved this from 🔖 Ready: Atomic to 🏗 In Work: Long in HiDPI Jan 26, 2025
@amartya4256 amartya4256 moved this from 🏗 In Work: Long to 🏗 In Work: Short in HiDPI Jan 26, 2025
@HeikoKlare HeikoKlare moved this from 🏗 In Work: Short to 🔖 Ready: Atomic in HiDPI Feb 3, 2025
@amartya4256
Copy link

On debugging Program::getImageData, I found that OS.SHGetFileInfo gets the size 16 x 16 for 150% , which is 12 x 12 for 100% which is not the standard icon size here, hence where it is expected to haev 24 x 24 size on 150%, it is scaled to 16 x 16, leading to a smaller appearance. Since, OS.SHGetFileInfo gets the image and its size based on the zoom of primary monitor, it might be an issue related to the call itself. I also tried to perform the same on non-primary monitor; there it seems to work fine.

@HeikoKlare
Copy link
Contributor Author

I found that OS.SHGetFileInfo gets the size 16 x 16 for 150%

Isn't this actually about OS.SHGetFileInfo returning the size according to the primary monitor zoom at application startup? So if you start the application at 100%, it will always return 16x16 for an icon, no matter how the different zooms of the different monitors (including the primary monitor) change. That would call for a replacement of using OS.SHGetFileInfo or maybe the usage of Device#getDPI().

@amartya4256 amartya4256 moved this from 🔖 Ready: Atomic to 🏗 In Work: Short in HiDPI Feb 4, 2025
@amartya4256 amartya4256 self-assigned this Feb 4, 2025
@amartya4256
Copy link

amartya4256 commented Feb 4, 2025

I found out that SHGetFileInfo is a System DPI aware call. Which means it only returns the icon for the DPI at the application startup. There's no alterantive available for per monitor DPI aware functions at the moment like GetSystemMetricsForDpi for GetSystemMetrics, etc. However, we can either

  • maybe use SHGetImageList to collect the list of icon with all sizes and retreive the image with nearest size.
  • or, we can obtain the DPI at the application startup and use the zoom relevant to that for creating the image as its initialNativeZoom.

I am going to explore both the possibilities to analyze which could be an optimal solution in our case.
reference: https://stackoverflow.com/questions/43230907/per-monitor-dpi-aware-windows-system-image-list, https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-shgetfileinfow, https://learn.microsoft.com/en-us/windows/win32/hidpi/high-dpi-desktop-application-development-on-windows#system-dpi-awareness

@HeikoKlare
Copy link
Contributor Author

Might we make a two-step process?

  1. Use Device#getDPI() instead of primary monitor zoom as a quick fix
  2. Replace SHGetFileInfo with something allowing us to always retrieve a propery scaled icon (not even depending on primary monitor zoom but properly fitting for the current monitor)

@amartya4256
Copy link

I agree with you. I have a problem with varying value of getDPI here. Here's the outcome of the investigation using the first approach:

If I start my IDE at 200% monitor with a primary monitor (PM) at 100% zoom. Then the DPI of the PM is 96 which maps to 100% but meanwhile if I change the PM to 150% then call getDPI again, the dpi is 64 which is 67% zoom which should not be the case. Does getDPI behave in a system DPI aware manner?
If I start the application directly at 150% zoom of PM then the DPI is 144 which maps correctly to 150% zoom

@amartya4256
Copy link

amartya4256 commented Feb 4, 2025

I see the problem with getDPI now:

public Point getDPI () {
	checkDevice ();
	long hDC = internal_new_GC (null);
	int dpiX = OS.GetDeviceCaps (hDC, OS.LOGPIXELSX);
	int dpiY = OS.GetDeviceCaps (hDC, OS.LOGPIXELSY);
	internal_dispose_GC (hDC, null);
	return DPIUtil.scaleDown(new Point (dpiX, dpiY), DPIUtil.getZoomForAutoscaleProperty(getDeviceZoom()));
}

it returns a scaled down value. Thus in the consumer, I need to scale it up with getDeviceZoom which I think is a bad idea. The question is, why are we scaling it down with getDeviceZoom in getDpi in the first place.
@akoch-yatta do you have any insight into this?

I tested out using just dpiX value, it is always concrete and doesnt change with the monitor zoom change.

@HeikoKlare
Copy link
Contributor Author

Exactly. We do the scaling as it has always been done like this and consumers rely on it (remember the regression we had in the 2024-12 release). _getDPIx() should actually return the value that is static throughout application lifecycle.

Scaling up the value returned by getDPI() is definitely not a good solution, but maybe something to improve the current situation? Anyway, we should completely replace the program icon retrieval logic depending on the original primary monitor zoom...

@amartya4256
Copy link

Yeah i completely didnt see _getDPIX(). However, it is package protected. So we need a way to access the unscaled DPI value.

@amartya4256
Copy link

Yes the logic would be now based on this system-wide DPI value since there's a direct relation between OS.SHGetFileInfo. My proposal is that after a quick fix, we can look out for a better way of retreiving the program icons.

@amartya4256
Copy link

One way could be to Do it like this:

        Display display = Display.getCurrent();
	long hDC = display.internal_new_GC (null);
	int dpi = OS.GetDeviceCaps (hDC, OS.LOGPIXELSX);
	display.internal_dispose_GC (hDC, null);
	int initialNativeZoom = DPIUtil.mapDPIToZoom(dpi);

But it adds a boilerplate to the existing _getDPIX logic

@HeikoKlare
Copy link
Contributor Author

You mean to add that code to the Program class?

@amartya4256
Copy link

Yes i simplified it a bit. Now we don't need the display instance at all. This would also fix: eclipse-platform/eclipse.platform.swt#1759

@amartya4256 amartya4256 moved this from 🏗 In Work: Short to 👀 In Review in HiDPI Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A Derivation of Expected Behavior HiDPI A HiDPI-Related Issue or Feature
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants