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

Only prompt for restart on zoom change when not rescaling at runtime #2126

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Jul 23, 2024

When the zoom of the primary monitor changes, the workbench asks for a restart to adapt the workbench windows to the changed zoom. With the runtime rescaling functionality recently introduced to SWT, this behavior is only desired if that rescaling functionality is not activated. This change adapts the functionality for showing the restart prompt to only come up if the rescaling functionality is deactivated.

⚠️ Requires recent SWT API changes, thus CI runs will only succeed after the next I-Build.

Copy link
Contributor

github-actions bot commented Jul 23, 2024

Test Results

 1 815 files  +  605   1 815 suites  +605   1h 34m 46s ⏱️ + 33m 13s
 7 683 tests ±    0   7 455 ✅ +    3  228 💤  -   3  0 ❌ ±0 
24 210 runs  +8 070  23 461 ✅ +7 834  749 💤 +236  0 ❌ ±0 

Results for commit bfaf2b0. ± Comparison against base commit 6fc79c1.

♻️ This comment has been updated with latest results.

@HeikoKlare HeikoKlare force-pushed the no-restart-prompt-rescaleatruntime branch from 7a09bad to d7a16e1 Compare July 24, 2024 06:08
@HeikoKlare HeikoKlare marked this pull request as ready for review July 24, 2024 07:28
@akoch-yatta
Copy link
Contributor

Looks good to me. I tested it on Windows and it works as expected for me, having the flag activated or not.

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 haven't tested yet but I have 2 points to consider:

  1. As briefly discussed in our daily, if activating the "auto scale on runtime" fails then the method Display::isRescalingAtRuntime should return false otherwise the restart dialog will not be shown even though the application is still running in "the old mode" i.e. not rescaling at runtime. Different PR though, this code is fine here.
  2. What would happen if one changes the mode programmatically via Display::setRescalingAtRuntime? Would the user be prompted again if one deactivates the rescaling at runtime (i.e. "would the old mode be completely restored, with prompt and everything")?

@fedejeanne
Copy link
Contributor

fedejeanne commented Jul 24, 2024

2. What would happen if one changes the mode programmatically via `Display::setRescalingAtRuntime`? Would the user be prompted again if one deactivates the rescaling at runtime (_i.e._ "would the old mode be completely restored, with prompt and everything")?

EDIT: 2. What would happen if one changes the mode programmatically via Display::setRescalingAtRuntime and sets it back to false? ...

@HeikoKlare
Copy link
Contributor Author

Thank you for the comments!

  • As briefly discussed in our daily, if activating the "auto scale on runtime" fails then the method Display::isRescalingAtRuntime should return false otherwise the restart dialog will not be shown even though the application is still running in "the old mode" i.e. not rescaling at runtime. Different PR though, this code is fine here.

Yes, makes sense. I will provide an according PR to SWT.

  • What would happen if one changes the mode programmatically via Display::setRescalingAtRuntime? Would the user be prompted again if one deactivates the rescaling at runtime (i.e. "would the old mode be completely restored, with prompt and everything")?

Then one has broken the contract of setRescalingAtRuntime(). We could also move the check inside the event handler, but then the check will be unnecessary unless some user broke the API.

@HeikoKlare
Copy link
Contributor Author

See eclipse-platform/eclipse.platform.swt#1363 for the first mentioned issue.

@fedejeanne
Copy link
Contributor

  • What would happen if one changes the mode programmatically via Display::setRescalingAtRuntime? Would the user be prompted again if one deactivates the rescaling at runtime (i.e. "would the old mode be completely restored, with prompt and everything")?

Then one has broken the contract of setRescalingAtRuntime(). We could also move the check inside the event handler, but then the check will be unnecessary unless some user broke the API.

You mean that part about the undefined behavior? (excerpt from the Javadoc of Display::setRescalingAtRuntime(boolean)

 * ... This is only safe to call as long as
 * no shell has been created for this display. When changing the value after a
 * shell has been created for this display, the effect is undefined.
...

In that case, wouldn't it be better to make Display::setRescalingAtRuntime non-api (either reduce visibility or mark it as @noreference)?

@fedejeanne
Copy link
Contributor

  • What would happen if one changes the mode programmatically via Display::setRescalingAtRuntime? Would the user be prompted again if one deactivates the rescaling at runtime (i.e. "would the old mode be completely restored, with prompt and everything")?

Then one has broken the contract of setRescalingAtRuntime(). We could also move the check inside the event handler, but then the check will be unnecessary unless some user broke the API.

You mean that part about the undefined behavior? (excerpt from the Javadoc of Display::setRescalingAtRuntime(boolean)

 * ... This is only safe to call as long as
 * no shell has been created for this display. When changing the value after a
 * shell has been created for this display, the effect is undefined.
...

In that case, wouldn't it be better to make Display::setRescalingAtRuntime non-api (either reduce visibility or mark it as @noreference)?

Ignore it, we spoke about it already.

@HeikoKlare HeikoKlare force-pushed the no-restart-prompt-rescaleatruntime branch from d7a16e1 to 2bc7198 Compare July 24, 2024 15:32
When the zoom of the primary monitor changes, the workbench asks for a
restart to adapt the workbench windows to the changed zoom. With the
runtime rescaling functionality recently introduced to SWT, this
behavior is only desired if that rescaling functionality is not
activated. This change adapts the functionality for showing the restart
prompt to only come up if the rescaling functionality is deactivated.
@HeikoKlare HeikoKlare force-pushed the no-restart-prompt-rescaleatruntime branch from 2bc7198 to bfaf2b0 Compare July 24, 2024 15:32
@HeikoKlare
Copy link
Contributor Author

Let's proceed with the discussion about the SWT API at SWT. I made the workbench behavior more "resilient" by always having the zoom change listener attached and make it check for the runtime rescaling behavior every time. In constrast to the efforts for actually rescaling the UI, this single check is irrelevant (performance-wise). This way the workbench implementation is able to adapt the restart behavior even if someone changes the rescaling mode at runtime (given that this will be possible at some point in time in the future).

@HeikoKlare HeikoKlare merged commit e30a52d into eclipse-platform:master Jul 24, 2024
16 checks passed
@HeikoKlare HeikoKlare deleted the no-restart-prompt-rescaleatruntime branch July 24, 2024 16:40
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.

3 participants