Deprecate insufficient Display#setRescalingAtRuntime() method #1720

HeikoKlare · 2025-01-13T15:05:38Z

The method Display#setRescalingAtRuntime() allows to activate the monitor-specific scaling mechanism on Windows. This must, however, actually be activated before the Display is instantiated, like it is done during the Display constructor execution if the system property for global activation of monitor-specific scaling is specified. In consequence, calling #setRescalingAtRuntime() does not take the full intended effect.

This change deprecates the method so that the system property needs to be set for proper initialization of the monitor-specific scaling functionality. It also adapts the according tests to the deprecation of
the API and alternatives to be used.

⚠️ #1728 has been added as a preparation for this

HeikoKlare · 2025-01-13T15:05:50Z

@akoch-yatta

github-actions · 2025-01-13T15:16:11Z

Test Results

494 files ±0 494 suites ±0 10m 47s ⏱️ -29s
4 326 tests +2 4 308 ✅ +2 18 💤 ±0 0 ❌ ±0
16 536 runs +2 16 388 ✅ +2 148 💤 ±0 0 ❌ ±0

Results for commit 44a7067. ± Comparison against base commit e0b4fe9.

♻️ This comment has been updated with latest results.

akoch-yatta · 2025-01-14T09:21:27Z

bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java

 */
+@Deprecated(since = "2025-03", forRemoval = true)


Is that the correct value for "since"? I personally would expect the version number "3.129" instead of the release name here

I recall @HannesWell discussing this topic, but I don't remember where. I think the point was that most users have no clue what the number version number means and most importantly don't know when the removal will happen from just the version number. With a date-version like this they know (if they know the policy) that in 2027-03 it can end up being removed.

FWIW I agree with Ed. Since the removal will be done "in X amount of time" then having a direct map from version to point in time is more useful.

Nice to know that this topic has been discussed and this form of documenting it has been accepted.

I am of course fine with that, if there is in agreement about it. Still, it is at least in a conflict with javadoc
of the since attribute, isn't it? We are using the version number in the "since" in the javadoc

* Returns the version in which the annotated element became deprecated. * The version string is in the same format and namespace as the value of * the {@code @since} javadoc tag. The default value is the empty * string.

I just had never seen this kind of usage anywhere, so I wondered and wanted to ask. But, as you said, there was an agreement for that, I can live with that 😄

fedejeanne

LGTM I only have 1 question.

fedejeanne · 2025-01-15T07:23:13Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java

+	return setMonitorSpecificScaling(activate);
+}
+
+private boolean setMonitorSpecificScaling(boolean activate) {


Why the renaming in this PR?

Because of introducting the new method setMonitorSpecificScaling() and to make the existing one consistent with that one as they relate to the same feature which is otherwise named differently.

Ah, you mean this one right?

The intent wasn't clear in the description of this PR or in the commit message, hence my question.

Now I got the point. I lost several changes when rebasing the PR on master, which I now added again. The deprecation of API of course also means that usages have to be adapted (accepting deprecation or replacing it). I have now updated the PR with the according adaptations, which also includes that method "renaming", which just factors out the functionality into a new method with a proper name and visibility. "Proper" regaring the name means that it conform to the notion of "monitor-specific scaling" we use by now.

fedejeanne

Looks good, only minor comments

fedejeanne · 2025-01-15T09:06:11Z

bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/DisplayWin32Test.java

+		DPIUtil.setMonitorSpecificScaling(false);
+		Display display = new Display();
+		try {
+			assertFalse(display.isRescalingAtRuntime());


Is it unnecessary/incorrect to assert that the scaling is set to DPI_AWARENESS_CONTEXT_SYSTEM_AWARE just like the "old" test setRescaleAtRuntime_deactivate does?

Suggested change

assertFalse(display.isRescalingAtRuntime());

assertFalse(display.isRescalingAtRuntime());

assertEquals(OS.DPI_AWARENESS_CONTEXT_SYSTEM_AWARE, OS.GetThreadDpiAwarenessContext());

This asymmetry in the tests looks confusing to me.

Yes, it is incorrect.

Care to elaborate? I'm missing some pieces of the puzzle here.

The application runs in "default" mode in this test case, so the actual DPI awareness depends on what the JVM is configured to use as process-wide DPI awareness. That's why assuming a specific DPI awareness is incorrect.

Ah, the missing piece (in my mind) was the fact that Display::setRescalingAtRuntime actively overrides the system default both when passing true and false as a parameter while DPIUtil::setMonitorSpecificScaling only overrides the system value when passing true as a parameter.
The problem I see here (unrelated to this PR) is that both these methods have the same name but they act differently, which is odd:

org.eclipse.swt.internal.DPIUtil.setMonitorSpecificScaling(boolean): sets a system property

org.eclipse.swt.widgets.Display.setMonitorSpecificScaling(boolean): sets and overrides the mode.

This currently produces (AFAIK) no bug because Display.setMonitorSpecificScaling(boolean) is only called if the system property has been set (e.g. if DPIUtil.setMonitorSpecificScaling(true) has been called) but I think the naming of the method in Display could be improved in order to make more explicit what it is supposed to do and how.

How about renaming Display.setMonitorSpecificScaling to Display.forceMonitorSpecificScaling (in a separate PR) so that it is 100% clear that this method overrides the system value?

No further changes required for this PR. Approved on my side.

fedejeanne · 2025-01-15T09:14:14Z

bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/DisplayWin32Test.java

+	}
+
+	@Test
+	@SuppressWarnings("removal")
 	public void setRescaleAtRuntime_activate() {


NIT: in these cases, when it is essential that the test (or any piece of code for that matter) runs "under special conditions", I personally like to extract a method that makes this "special condition" explicit e.g.:

private void runInNewDisplay(Consumer<Display> consumer) { Display display = new Display(); try { consumer.accept(display); } finally { display.dispose(); } }

This makes it clearer that the new implementation now force these tests to run in a new display

In general, I agree that this pattern should be applied. In this case it was rather about proper cleanup than about running in a new display, but would still have been possible to realize it that way.
This, however, became obsolete by preparatory test method refactoring, which made this kind of construct completely obsolete, as displays are now automatically cleaned up by an according JUnit 5 extension.

fedejeanne · 2025-01-15T13:17:22Z

bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/DisplayWin32Test.java

+		DPIUtil.setMonitorSpecificScaling(false);
+		Display display = new Display();
+		try {
+			assertFalse(display.isRescalingAtRuntime());


Ah, the missing piece (in my mind) was the fact that Display::setRescalingAtRuntime actively overrides the system default both when passing true and false as a parameter while DPIUtil::setMonitorSpecificScaling only overrides the system value when passing true as a parameter.
The problem I see here (unrelated to this PR) is that both these methods have the same name but they act differently, which is odd:

org.eclipse.swt.internal.DPIUtil.setMonitorSpecificScaling(boolean): sets a system property

org.eclipse.swt.widgets.Display.setMonitorSpecificScaling(boolean): sets and overrides the mode.

This currently produces (AFAIK) no bug because Display.setMonitorSpecificScaling(boolean) is only called if the system property has been set (e.g. if DPIUtil.setMonitorSpecificScaling(true) has been called) but I think the naming of the method in Display could be improved in order to make more explicit what it is supposed to do and how.

How about renaming Display.setMonitorSpecificScaling to Display.forceMonitorSpecificScaling (in a separate PR) so that it is 100% clear that this method overrides the system value?

No further changes required for this PR. Approved on my side.

The method Display#setRescalingAtRuntime() allows to activate the monitor-specific scaling mechanism on Windows. This must, however, actually be activated before the Display is instantiated, like it is done during the Display constructor execution if the system property for global activation of monitor-specific scaling is specified. In consequence, calling #setRescalingAtRuntime() does not take the full intended effect. This change deprecates the method so that the system property needs to be set for proper initialization of the monitor-specific scaling functionality. It also adapts the according tests to the deprecation of the API and alternatives to be used.

HeikoKlare · 2025-01-16T13:17:00Z

I see the point regarding method naming. In my opinion, the name setMonitorSpecificScaling is reasonable for the Display class. forceMonitorSpecificScaling would be misleading, as it does actually not "force" anything. I'd rather see the change to improve naming in the DPIUtil class, as it might make sense to keep the naming conforming to the system property using the termin "updateOnRuntime" referring to "autoScale", which in my opinion might be even better naming in that class. Since this PR does not touch names of method sin DPIUtil, I would not adapt it here.
And we need to keep in mind that of course methods with the same name may have a different meaning depending on their context (i.e., the class/interface they belong to), as the context also gives them semantics. But as said, in this case I see some chance for improvement in DPIUtil.

HeikoKlare requested a review from fedejeanne January 13, 2025 15:05

HeikoKlare force-pushed the deprecate-faulty-rescaling-at-runtime-method branch from f7ce33b to 76ca6a6 Compare January 13, 2025 20:46

akoch-yatta reviewed Jan 14, 2025

View reviewed changes

fedejeanne approved these changes Jan 14, 2025

View reviewed changes

HeikoKlare force-pushed the deprecate-faulty-rescaling-at-runtime-method branch 2 times, most recently from 382fe01 to 1a7bc84 Compare January 14, 2025 18:59

fedejeanne approved these changes Jan 15, 2025

View reviewed changes

HeikoKlare force-pushed the deprecate-faulty-rescaling-at-runtime-method branch 3 times, most recently from eae4cb5 to 4fe6c71 Compare January 15, 2025 09:10

fedejeanne reviewed Jan 15, 2025

View reviewed changes

HeikoKlare mentioned this pull request Jan 15, 2025

Remove type hierarchy of Win32AutoscaleTestBase #1728

Merged

fedejeanne approved these changes Jan 15, 2025

View reviewed changes

HeikoKlare force-pushed the deprecate-faulty-rescaling-at-runtime-method branch from 4fe6c71 to 44a7067 Compare January 15, 2025 18:43

HeikoKlare merged commit 22eece1 into eclipse-platform:master Jan 16, 2025
14 checks passed

HeikoKlare deleted the deprecate-faulty-rescaling-at-runtime-method branch January 16, 2025 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate insufficient Display#setRescalingAtRuntime() method #1720

Deprecate insufficient Display#setRescalingAtRuntime() method #1720

HeikoKlare commented Jan 13, 2025 •

edited

Loading

HeikoKlare commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 •

edited

Loading

akoch-yatta Jan 14, 2025

merks Jan 14, 2025

fedejeanne Jan 14, 2025

akoch-yatta Jan 14, 2025 •

edited

Loading

fedejeanne left a comment

fedejeanne Jan 15, 2025

HeikoKlare Jan 15, 2025

fedejeanne Jan 15, 2025

HeikoKlare Jan 15, 2025

fedejeanne left a comment

fedejeanne Jan 15, 2025

HeikoKlare Jan 15, 2025

fedejeanne Jan 15, 2025

HeikoKlare Jan 15, 2025

fedejeanne Jan 15, 2025

fedejeanne Jan 15, 2025

HeikoKlare Jan 16, 2025

fedejeanne Jan 15, 2025

HeikoKlare commented Jan 16, 2025

	assertFalse(display.isRescalingAtRuntime());
	assertFalse(display.isRescalingAtRuntime());
	assertEquals(OS.DPI_AWARENESS_CONTEXT_SYSTEM_AWARE, OS.GetThreadDpiAwarenessContext());

Deprecate insufficient Display#setRescalingAtRuntime() method #1720

Deprecate insufficient Display#setRescalingAtRuntime() method #1720

Conversation

HeikoKlare commented Jan 13, 2025 • edited Loading

HeikoKlare commented Jan 13, 2025

github-actions bot commented Jan 13, 2025 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoch-yatta Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

fedejeanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedejeanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeikoKlare commented Jan 16, 2025

HeikoKlare commented Jan 13, 2025 •

edited

Loading

github-actions bot commented Jan 13, 2025 •

edited

Loading

akoch-yatta Jan 14, 2025 •

edited

Loading