-
Notifications
You must be signed in to change notification settings - Fork 187
Improve scaling of SystemImage #62 #1821
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 scaling of SystemImage #62 #1821
Conversation
Test Results 505 files - 1 505 suites - 1 7m 56s ⏱️ -5s For more details on these failures, see this check. Results for commit f6f11bf. ± Comparison against base commit 9b1bcf4. This pull request removes 37 tests.♻️ This comment has been updated with latest results. |
HeikoKlare
left a comment
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.
At a first glance, the change looks sound and moving to up-to-date system icons that can be retrieved in proper scaling is great.
For now, I have one question regarding the used scale value.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Show resolved
Hide resolved
bcb618c to
f5d50c9
Compare
f5d50c9 to
69695da
Compare
HeikoKlare
left a comment
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.
In general, the change looks good. I've tested it and it also works quite fine. Only the sizes of the retrieved icons do not make sense to me.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Show resolved
Hide resolved
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
24e6eaf to
782efc3
Compare
HeikoKlare
left a comment
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.
Looks good now. When taking another look, I only came across a chance for a little code cleanup. Maybe you can update the PR to incorporate that.
bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
Outdated
Show resolved
Hide resolved
782efc3 to
a326140
Compare
|
Please revert the formatting changes reintroduced with the recent change. |
a326140 to
a0e3e16
Compare
c0478f1 to
f720b02
Compare
This commit contributes to better scaling of Images obtained from Display::getSystemImage using better windows API LoadIconWithScaleDown for scaling. The new API also changes the old system icons to the new Windows System Icons. Contributes to eclipse-platform#62 and eclipse-platform#127
f720b02 to
f6f11bf
Compare
|
Failing test is unrelated and documented: #1843 |
|
FTR: binaries successfully built and pushsed on 2nd attempt: d25bdc3 |
This PR contributes to better scaling of Images obtained from Display::getSystemImage using better windows API LoadIconWithScaleDown for scaling. The new API also changes the old system icons to the modern Windows System Icons.
The idea is to have an ImageDataProvider for such Images which retrieve the image from the OS for the specified dimensions relevant to the zoom level and obtain its imageData instead of scaling the ImageData obtained from the handle of initialNativeZoom.
Standard Icons loaded using LoadImage (using user32.dll):

Standard Icons loaded using LoadIconWithScaleDown (using imageres.dll):

Contributes to
#62 and #127