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

[Win32] Enable monitor-specific scaling by default #2868

Conversation

HeikoKlare
Copy link
Contributor

The monitor-specific UI scaling on Windows is disabled by default and can be activated via a preference.

This change inverts the default enablement: it enables the feature by default and allows to disable it via preference. The existing experimental preference is renamed to reset the configuration for every consumer. All information about the feature being experimental is removed.

Copy link
Contributor

github-actions bot commented Mar 28, 2025

Test Results

 1 824 files  ±0   1 824 suites  ±0   1h 45m 35s ⏱️ + 11m 10s
 7 918 tests ±0   7 688 ✅  - 2  228 💤 ±0  2 ❌ +2 
23 841 runs  ±0  23 091 ✅  - 2  748 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit b45c4a0. ± Comparison against base commit 88e83a8.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the monitor-specific-scaling-default branch from e1e079e to 296fa08 Compare March 28, 2025 18:18
@HeikoKlare HeikoKlare marked this pull request as ready for review March 31, 2025 07:39
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and I'd like to propose some minor changes.

Also, shouldn't the option be then enabled when starting on a new workspace? I started a brand new Eclipse Application with a clean WS and it wasn't

image

@HeikoKlare
Copy link
Contributor Author

Also, shouldn't the option be then enabled when starting on a new workspace? I started a brand new Eclipse Application with a clean WS and it wasn't

@fedejeanne The preference is not stored in the workspace but in the configuration. Did you clean that or create a new one as well? At least I cannot reproduce this issue.

@HeikoKlare HeikoKlare force-pushed the monitor-specific-scaling-default branch from 296fa08 to 19c7446 Compare March 31, 2025 09:50
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't the option be then enabled when starting on a new workspace? I started a brand new Eclipse Application with a clean WS and it wasn't

@fedejeanne The preference is not stored in the workspace but in the configuration. Did you clean that or create a new one as well? At least I cannot reproduce this issue.

Thank you for the pointer. I must have screwed up somewhere else when checking out your PR because I tried it again and I cannot reproduce it either. One pointer (for my future self too) is that when creating a new Run Configuration, this option is enabled by default:

image

This makes testing this kind of PRs impossible: I was getting always the default value (true) for the property and it was therefore always enabled after a restart. I disabled that option and was able to test this PR just fine.

I have one final remark: the initial value of true should also be applied in line 375 of the ViewsPreferencePage, otherwise changing the preference from true to false for the very 1st time doesn't not ask for a workbench restart (subsequent changes will ask for a restart though)

@fedejeanne fedejeanne force-pushed the monitor-specific-scaling-default branch 2 times, most recently from 056cc2f to 1ee97cc Compare March 31, 2025 12:23
@merks
Copy link
Contributor

merks commented Mar 31, 2025

There is a checkbox in each launch configuration whether to clear the configuration before launching.

@fedejeanne
Copy link
Contributor

There is a checkbox in each launch configuration whether to clear the configuration before launching.

Do you mean this one? #2868 (review)

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view, this PR is now good to go ✔️

@HeikoKlare fyi I made 2 changes (2 commits), they are already squashed.

The monitor-specific UI scaling on Windows is disabled by default and
can be activated via a preference.

This change inverts the default enablement: it enables the feature by
default and allows to disable it via preference. The existing
experimental preference is renamed to reset the configuration for every
consumer. All information about the feature being experimental is
removed.
@fedejeanne fedejeanne force-pushed the monitor-specific-scaling-default branch from 1ee97cc to b45c4a0 Compare March 31, 2025 13:38
@merks
Copy link
Contributor

merks commented Mar 31, 2025

There is a checkbox in each launch configuration whether to clear the configuration before launching.

Do you mean this one? #2868 (review)

Yes. Reading on phone so harder to see all details. Sorry.

@fedejeanne
Copy link
Contributor

There is a checkbox in each launch configuration whether to clear the configuration before launching.

Do you mean this one? #2868 (review)

Yes. Reading on phone so harder to see all details. Sorry.

No worries, I was just making sure I got it right :-) . Thank you, Ed!

@fedejeanne
Copy link
Contributor

fedejeanne commented Mar 31, 2025

Test failures are unrelated:

@HeikoKlare
Copy link
Contributor Author

Thank you! Changes look good to me.

fwiw: when introducing the preference, we had to move it to the configuration area because it has to be read before the workspace is available.

@fedejeanne fedejeanne merged commit 0bc52a9 into eclipse-platform:master Mar 31, 2025
15 of 18 checks passed
@fedejeanne fedejeanne deleted the monitor-specific-scaling-default branch April 1, 2025 11:37
@iloveeclipse
Copy link
Member

We have a regression: eclipse-platform/eclipse.platform.swt#2003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide Productive Preference for Activating Multi-Monitor HiDPI Support
5 participants