Skip to content

Conversation

@ShahzaibIbrahim
Copy link
Contributor

@ShahzaibIbrahim ShahzaibIbrahim commented Oct 30, 2025

Workbench needs few methods from DPIUtil that were only present for Win32. Keeping the functionality for Win32DPIUtil as is, just making them available for DPIUtil.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025

Test Results

  118 files  + 3    118 suites  +3   17m 44s ⏱️ +41s
4 652 tests +37  4 634 ✅ +36  18 💤 +2  0 ❌  - 1 
  338 runs  +19    334 ✅ +18   4 💤 +1  0 ❌ ±0 

Results for commit 3ad69c8. ± Comparison against base commit 0ee1589.

This pull request skips 1 and un-skips 1 tests.
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser ‑ test_setUrl_remote_with_post
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser_IE ‑ test_setUrl_remote_with_post

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

I would be in favor of doing such a change more incrementally. Obviously, there is code in Platform UI depending on (internal) methods of DPIUtil. Moving them to another class means that Platform UI code becomes incompatible once we merge the SWT PR and if we merge the Platform UI PR at the same time (without knowing if it works as the CI will fail) will make Platform UI builds fail until the next I-Build.

Thus, in such a case a multi-step transition should be made by refactoring and maybe adding methods while still keeping the old ones (as delegates), then adapt the consumers (such as Platform UI) and once that is done remove the delegates that became obsolete.

It also seems like the AutoScaling class and DPIUtil would be highly mutually dependent. This could be an indicator that splitting them up is not a good idea. In any case, AutoScaling seems like it disposes much new public API that we will tie ourselves to. I know that I proposed to make some its functionality API, but seeing it I am not so sure anymore whether we should really do it or better keep it internal (like with DPIUtil) and access the internal API from Platform UI for now. At least, whatever we make public should be as lean as possible and in my opinion we should definitely not make something like an autoscale mode publicly accessible.

@ShahzaibIbrahim ShahzaibIbrahim changed the title [Refactor] Move autoscaling methods in AutoScaling class [Refactor] Moving methods from Win32DPIUtil to DPIUtil for autoscaling Oct 31, 2025
@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-399 branch 2 times, most recently from 7df9a79 to 08dcd65 Compare October 31, 2025 13:01
@ShahzaibIbrahim
Copy link
Contributor Author

As suggested, I have kept the methods in DPIUtil class. This PR now only moves few of the method in DPIUtil as these will be required to me in the workbench for subsequent PR: eclipse-platform/eclipse.platform.ui#3475.

The idea is to limit monitor specific for both SWT and eclipse products with compatible autoscaling methods.

@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-399 branch 4 times, most recently from 50b09f6 to bed297b Compare October 31, 2025 13:46
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft October 31, 2025 13:51
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review October 31, 2025 14:55
@HeikoKlare
Copy link
Contributor

@akurtakov this PR is not yet ready to be merged, but at least your specific request for changes was resolved, wasn't it?

@akurtakov akurtakov dismissed their stale review November 11, 2025 17:24

My concern has been adressed.

@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as draft November 12, 2025 12:01
@ShahzaibIbrahim ShahzaibIbrahim force-pushed the master-399 branch 2 times, most recently from 030948f to 93d5d62 Compare November 12, 2025 13:36
@ShahzaibIbrahim ShahzaibIbrahim marked this pull request as ready for review November 12, 2025 13:43
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

If I see correctly, with one of the recent changes the isSetupCompatibleToMonitorSpecificScaling() method was removed from DPIUtil:
https://github.com/eclipse-platform/eclipse.platform.swt/compare/59dcd79dbbac1b225456de45d862112be58ad0fe..030948f5d401d2640cd0ca0ec8bf6d53ff039dec

However, if I understand this change correctly, it prepares for:

But that PR requires exactly that method.

So shouldn't the method be provided with this PR? Otherwise I don't the point of it.

@ShahzaibIbrahim
Copy link
Contributor Author

I have made 3 PRs in total.

  1. This is the first PR (just the refactoring)
  2. The method you mention is now in Limit autoscale mode to quarter and exact only #2771 (Preparatory PR for the one in Platform
  3. The last PR (Platform)

@HeikoKlare
Copy link
Contributor

I have made 3 PRs in total.

  1. This is the first PR (just the refactoring)
  2. The method you mention is now in Limit autoscale mode to quarter and exact only #2771 (Preparatory PR for the one in Platform
  3. The last PR (Platform)

But that does not fit to the order we need:

  1. Adapt Win32DPIUtil/DPIUtil (without any behavioral changes), so that we can process Limit monitor-specific scaling to supported autoscale modes eclipse.platform.ui#3475
  2. Process Limit monitor-specific scaling to supported autoscale modes eclipse.platform.ui#3475
  3. Limit autoscale modes to quarter/exact for monitor-specific scaling (do be done for next release)

@ShahzaibIbrahim
Copy link
Contributor Author

@HeikoKlare Updated again.

  1. This PR Adapt Win32DPIUtil/DPIUtil to process Limit monitor-specific scaling to supported autoscale modes eclipse.platform.ui#3475 without any behavior change (we still allow false value and also concrete values as before)
  2. Limit monitor-specific scaling to supported autoscale modes eclipse.platform.ui#3475 is then ready to be processed.
  3. At the very end we process Limit autoscale mode to quarter and exact only #2771 to limit autoscale values to quarter and exact only

static {
autoScaleValue = System.getProperty (SWT_AUTOSCALE);

setUseSmoothScalingByDefaultProvider(() -> isMonitorSpecificScalingActive());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct, as it will erroneously set that behavior for MacOS and Linux as well.

@HeikoKlare HeikoKlare force-pushed the master-399 branch 2 times, most recently from 149ae24 to c287e90 Compare November 13, 2025 15:53
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

The adaptations now looks fine to me.

I adapted the PR to...

  • not expose getAutoScaleValue()
  • provide a correct documentation for isSetupCompatibleToMonitorSpecificScaling() (the given was still contained the further limitation of auto-scale values which is not part of this PR)
  • have proper formatting

Move some methods from Win32DPIUtil to DPIUtil to make them available
OS-independently such that they can be accessed from OS-independent
code.
@HeikoKlare HeikoKlare merged commit d268884 into eclipse-platform:master Nov 13, 2025
17 checks passed
@HeikoKlare HeikoKlare deleted the master-399 branch November 13, 2025 16:55
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.

Limit monitor-specific scaling to supported autoscale modes

3 participants