Skip to content

Deprecate insufficient Display#setRescalingAtRuntime() method #1720

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Deprecate insufficient Display#setRescalingAtRuntime() method #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions ...les/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/widgets/DisplayWin32Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,36 +8,46 @@
import org.junit.jupiter.api.extension.*;

@ExtendWith(PlatformSpecificExecutionExtension.class)
@ExtendWith(ResetMonitorSpecificScalingExtension.class)
public class DisplayWin32Test {

private Display display;

@BeforeEach
public void createDisplay() {
display = new Display();
@Test
public void monitorSpecificScaling_activate() {
DPIUtil.setMonitorSpecificScaling(true);
Display display = Display.getDefault();
assertTrue(display.isRescalingAtRuntime());
assertEquals(OS.DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2, OS.GetThreadDpiAwarenessContext());
}

@AfterEach
public void destroyDisplay() {
display.dispose();
@Test
public void monitorSpecificScaling_deactivate() {
DPIUtil.setMonitorSpecificScaling(false);
Display display = Display.getDefault();
assertFalse(display.isRescalingAtRuntime());
}

@Test
@SuppressWarnings("removal")
public void setRescaleAtRuntime_activate() {
Copy link
Member

@fedejeanne fedejeanne Jan 15, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 16, 2025

Choose a reason for hiding this comment

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

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.

Display display = Display.getDefault();
display.setRescalingAtRuntime(true);
assertTrue(display.isRescalingAtRuntime());
assertEquals(OS.DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2, OS.GetThreadDpiAwarenessContext());
}

@Test
@SuppressWarnings("removal")
public void setRescaleAtRuntime_deactivate() {
Display display = Display.getDefault();
display.setRescalingAtRuntime(false);
assertFalse(display.isRescalingAtRuntime());
assertEquals(OS.DPI_AWARENESS_CONTEXT_SYSTEM_AWARE, OS.GetThreadDpiAwarenessContext());
}

@Test
@SuppressWarnings("removal")
public void setRescaleAtRuntime_toggling() {
Display display = Display.getDefault();
display.setRescalingAtRuntime(false);
assertFalse(display.isRescalingAtRuntime());
assertEquals(OS.DPI_AWARENESS_CONTEXT_SYSTEM_AWARE, OS.GetThreadDpiAwarenessContext());
Expand Down
3 changes: 3 additions & 0 deletions bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/widgets/Display.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6858,7 +6858,10 @@ public boolean isRescalingAtRuntime() {
* @param activate whether rescaling shall be activated or deactivated
* @return whether activating or deactivating the rescaling was successful
* @since 3.127
* @deprecated this method should not be used as it needs to be called already
* during instantiation to take proper effect
*/
@Deprecated(since = "2025-03", forRemoval = true)
Copy link
Contributor

@akoch-yatta akoch-yatta Jan 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@merks merks Jan 14, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@fedejeanne fedejeanne Jan 14, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@akoch-yatta akoch-yatta Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 😄

public boolean setRescalingAtRuntime(boolean activate) {
// not implemented for Cocoa
return false;
Expand Down
3 changes: 3 additions & 0 deletions bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Display.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6294,7 +6294,10 @@ public boolean isRescalingAtRuntime() {
* @param activate whether rescaling shall be activated or deactivated
* @return whether activating or deactivating the rescaling was successful
* @since 3.127
* @deprecated this method should not be used as it needs to be called already
* during instantiation to take proper effect
*/
@Deprecated(since = "2025-03", forRemoval = true)
public boolean setRescalingAtRuntime(boolean activate) {
// not implemented for GTK
return false;
Expand Down
10 changes: 9 additions & 1 deletion bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Display.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,7 @@ protected void create (DeviceData data) {
checkSubclass ();
checkDisplay (thread = Thread.currentThread (), true);
if (DPIUtil.isMonitorSpecificScalingActive()) {
setRescalingAtRuntime(true);
setMonitorSpecificScaling(true);
DPIUtil.setAutoScaleForMonitorSpecificScaling();
}
createDisplay (data);
Expand Down Expand Up @@ -5263,6 +5263,7 @@ private long openThemeData(final char[] themeName) {
return OS.OpenThemeData(hwndMessage, themeName, dpi);
}
}

/**
* {@return whether rescaling of shells at runtime when the DPI scaling of a
* shell's monitor changes is activated for this device}
Expand All @@ -5288,8 +5289,15 @@ public boolean isRescalingAtRuntime() {
* @param activate whether rescaling shall be activated or deactivated
* @return whether activating or deactivating the rescaling was successful
* @since 3.127
* @deprecated this method should not be used as it needs to be called already
* during instantiation to take proper effect
*/
@Deprecated(since = "2025-03", forRemoval = true)
public boolean setRescalingAtRuntime(boolean activate) {
return setMonitorSpecificScaling(activate);
}

private boolean setMonitorSpecificScaling(boolean activate) {
Comment on lines +5297 to +5300
Copy link
Member

@fedejeanne fedejeanne Jan 15, 2025

Choose a reason for hiding this comment

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

Why the renaming in this PR?

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 15, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@fedejeanne fedejeanne Jan 15, 2025

Choose a reason for hiding this comment

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

Ah, you mean this one right?

image

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

Copy link
Contributor Author

@HeikoKlare HeikoKlare Jan 15, 2025

Choose a reason for hiding this comment

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

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.

int desiredApiAwareness = activate ? OS.DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2 : OS.DPI_AWARENESS_CONTEXT_SYSTEM_AWARE;
if (setDPIAwareness(desiredApiAwareness)) {
rescalingAtRuntime = activate;
Expand Down
Loading