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

Add API for minimize/maximize on Window component #4581

Merged
merged 17 commits into from Feb 22, 2024

Conversation

rminderhoud
Copy link
Contributor

@rminderhoud rminderhoud commented Feb 7, 2024

Update the Window api to support programmatically minimizing and maximizing windows on the Qt and winit backends. The following methods are new:

  • Window::set_minimized
  • Window::minimized
  • Window::set_maximized
  • Window::maximized
  • Window::fullscreen

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2024

CLA assistant check
All committers have signed the CLA.

@rminderhoud
Copy link
Contributor Author

rminderhoud commented Feb 7, 2024

I've tried my best to test this as much as I can on my system which is Windows 10 64-bit x86_64. I don't have convenient access to MacOS or Linux currently so it might be beneficial to get more testing on those systems if there are concerns with the code. I made sure to test a combination of fixed size / preferred size for the window and the behavior is as expected. I've recorded some quick demos of both the winit and Qt backends showing the following behaviors:

  • Minimizing/Unminimizing & Maximizing/Unmaximizing using window decorations
  • Minimizing/Unminimizing programatically
  • Minimizing/ Unminimizing restoring previous maximized/fullscreen state
  • Toggling fullscreen restores previous state

Winit:

winit-min-max.mp4

Qt:

qt-min-max.mp4

Closes issue: #4400

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the patch. Looks very good. I really like the fact that you added comments with links to relevant issues.

This still need fixup to the JS and C++ bindings, but we can do that after.

However, i wonder if is_minimized, is_fullscreen and so on is not better api for the getter in rust on the Window. this mirrors the API in winit, and it cannot be confused with something that would, eg, return a minimized window.

#[napi(setter)]
pub fn set_minimized(&self, minimized: bool) {
self.inner.window().set_minimized(minimized)
}
Copy link
Member

Choose a reason for hiding this comment

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

change in this files need to be reflected ion index.ts, i think

@ogoffart
Copy link
Member

I'd like to go forward with this PR.
Would you mind renaming the getter on the Window to add the is_ prefix?

(We can fix the C++ and JS bindings after this PR.)

@rminderhoud
Copy link
Contributor Author

@ogoffart No problem, sorry for the delay but I was away this weekend. I've made the changes as requested, namely:

  • Renamed new window state apis to include is_ prefix for the getters (e.g. is_maximized)
  • Updated index.ts to support new bindings
  • Added ffi declaration for minimized/maximized in c api

Regarding the C changes. I only implemented as much as was previously implemented for fullscreen. Wasn't sure to what extent the full C++ API needed to be changed.

@@ -279,9 +279,19 @@ impl<'a> WindowProperties<'a> {
}

/// Returns true if the window should be shown fullscreen; false otherwise.
pub fn fullscreen(&self) -> bool {
pub fn is_fullscreen(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we can't rename this function since it is already public in previous version.
We'll have to re-add it as a deprecated function.

Or the alternative is to keep the old name and leave the is_ prefix out of WindowProperties. But i think it's better to be consistent and keep the is_ prefix for all the window states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I added back fullscreen() to WindowProperties as a deprecated method

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

Thanks for doing the changes.

@ogoffart ogoffart merged commit 9cb1a4a into slint-ui:master Feb 22, 2024
35 checks passed
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.

None yet

3 participants