-
Notifications
You must be signed in to change notification settings - Fork 161
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
Improve Program Icon Scaling #1831
Improve Program Icon Scaling #1831
Conversation
Test Results 509 files - 1 509 suites - 1 9m 31s ⏱️ + 1m 58s For more details on these failures, see this check. Results for commit 81af4de. ± Comparison against base commit 20e1eaa. This pull request removes 37 tests.
♻️ This comment has been updated with latest results. |
1e7c92e
to
eea657c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before taking a deeper look into the changes, I have some questions regarding the overall change and in particular the used API, as the current proposal only seems to partially resolve the underlying issue and I am wondering if we cannot find a solution that completely resolves the issue with scaled program icons.
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Outdated
Show resolved
Hide resolved
ee7e0e7
to
155edf9
Compare
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested the code and it works fine for me. Once we get rid of the zoom limitations in CompositeImageDescriptor, we will even see properly scaled program icons in the package explorer.
I was just wondering whether preferring loading the icon from the icon path instead of the extension may have any drawback or can produce regression (like the ones we faced after completely removing that case in #999), but I was not able to produce any problematic scenario so far.
So with the few requested changes, this is fine from my side and can be merged.
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT Program/win32/org/eclipse/swt/program/Program.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT PI/win32/org/eclipse/swt/internal/win32/OS.java
Outdated
Show resolved
Hide resolved
155edf9
to
dba3c50
Compare
This commit implements scaling of program icons by retrieving it from SHDefExtractIcon, which also provides the system default icons for the file type followed by a fallback if the pszFile for the icon is not valid. The fallback uses the extension to retrieve the icon using SHGetFileInfo in small and large size and scales it accordingly with the required zoom level. contributes to eclipse-platform#62 and eclipse-platform#127
dba3c50
to
81af4de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the adaptations and of course for finally having a solution that seems to work fine for most if not all of the use cases. The change itself looks sound to me.
I have tested it in all different relevant scenarios I can imagine and did not found any issue but, on the opposite, found the results to be better in several of the situations as expected. At least, I have tested the following for diffent program icons retrieved from icon paths (based on an actual file for a program and the path to the program itself) and from the extension (for .exe files):
- Moving a shell showing such program icons between monitors of different zoom (100%, 150%, 175%)
- Changing the zoom of the current and, in particular, primary monitor when showing such program icons
- Changing primary monitor zoom after the application started but before the program icons were loaded for the first time (we had issues with that scenario because of using the wrong zoom then in previous issues).
The failing test is unrelated and documented: #1843
FTR: binaries successfully built and pushed with commit a92d9db |
This PR contributes to the improvement in the scaling of program icons. The implementation uses a windows API SHGetImageList to obtain imagelists at small, large and extra large sizes. Then the icon nearest to the required size is retrieved and its image data is scaled up returned.
Typically the small, large and extra large icons are supposed to have 16, 32 and 48 px of sizes respectively. But apparently, the API SHGetImageList turns out to be System-DPI aware. It means if the primary monitor zoom at app startup was 100% then those scaled values are valid but for instance if it was 200% then the sizes from small, large and extra large would be 32, 64 and 128 px. Hence, with this approach we do get a room to scale up with better image quality since, the retrieved images are usually, 1x, 2x and 3x the size of the primary monitor zoom at startup. However, if there exists a monitor with significantly lower zoom than the primary monitor zoom at startup, then the 1x zoom (small) icon will be used, which will end up in compressed images.
This approach still opens headspace in one direction (scale up), hence, the quality is indeed better while scaling up. However, while scaling down below the primary monitor zoom at app startup, it will compress the icon.
contributes to #62 and #127