[win32] Use DPI dependent OS API calls #1506
Conversation
akoch-yatta
force-pushed
the
theme-dpi-dependent
branch
from
c1b770b to
5de13f2
Compare
|
is it possible to add a junit test, that shows/tests the changed behaviour? |
akoch-yatta
force-pushed
the
theme-dpi-dependent
branch
2 times, most recently
from
d3cd69e to
cb31fbe
Compare
Hmm, depends a bit on what the test should achieve. A test for a realistic scenario would need two monitors with different zoom, which is not possible in the acccessible test environment. And even then, the usage of the replaced calls is only in drawing callback, so currently I don't see any helpful automated test here, we could add. But I am of course open for ideas. |
|
May be some test that checks drawing for single monitor with different dpi settings works as expected. Would that make sense, or are there already such tests? |
fedejeanne
left a comment
fedejeanne
left a comment
There was a problem hiding this comment.
Changes look good and the fix work.
I only have a minor comment regarding making the replaced methods private.
Other than that, this PR is good to go on my account.
| return OS.OpenThemeDataForDpi(hwnd, themeName, dpi); | ||
| } else { | ||
| return OS.OpenThemeData(hwnd, themeName); |
There was a problem hiding this comment.
Since these 2 methods are now replaced by your new method, it would be worth declaring them private.
I mean of course after Jörg's comments have been addressed. |
This commit replaces the OS calls for OpenThemeData with calls to the dpi dependent equivalent OpenThemeDataForDpi. Therefor the handling of loading/unloading of theme in Display is refactored to be able to manage multiple DPI dependent variants of a theme in multi zoom environments Contributes to eclipse-platform#62 nd eclipse-platform#131
akoch-yatta
force-pushed
the
theme-dpi-dependent
branch
from
cb31fbe to
6bda7b0
Compare
I added Snippet383 to test that behavior for Button, Tree & Table. An automated test for that would be quite difficult, because the changes affect internal custom drawing behavior which is hard to test. I would only see something like comparing screenshots for that use case. |
|
i confirm the snippet looks better with the Patch, even though it looks a bit different for me: |
fedejeanne
left a comment
fedejeanne
left a comment
There was a problem hiding this comment.
Ignoring random failing tests: #1523
3 participants
This commit replaces the OS calls for OpenThemeData with calls to the dpi dependent equivalent OpenThemeDataForDpi. Therefor the handling of loading/unloading of theme in Display is refactored to be able to manage multiple DPI dependent variants of a theme in multi zoom environments. Without this change some widget can look inconsistent in a multi-zoom scenario on the non-primary monitor.
One example that is affected by that change can be found in the Manifest editor
Without PR
With PR