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 option to toggle window control buttons #9083

Merged
merged 3 commits into from
Jul 23, 2023
Merged

Add option to toggle window control buttons #9083

merged 3 commits into from
Jul 23, 2023

Conversation

aleksa2808
Copy link
Contributor

@aleksa2808 aleksa2808 commented Jul 9, 2023

Objective

Implements #9082 but with an option to toggle minimize and close buttons too.

Solution

  • Added an enabled_buttons member to the Window struct through which users can enable or disable specific window control buttons.

Changelog

  • Added an enabled_buttons member to the Window struct through which users can enable or disable specific window control buttons.
  • Added a new system to the window_settings example which demonstrates the toggling functionality.

Migration guide

  • Added an enabled_buttons member to the Window struct through which users can enable or disable specific window control buttons.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@aleksa2808
Copy link
Contributor Author

aleksa2808 commented Jul 9, 2023

I tested the change on Windows 11 and macOS Ventura, both OSes disable the buttons correctly.

However, on macOS there is still an issue where you can double click the top bar of the window to maximize it even though the button is disabled. I'm looking into whether this can be disabled.

On Windows, if the maximize button is disabled and the window is not maximized double clicking the window top bar won't do anything. However, if the window is already maximized, double clicking the top bar will un-maximize it. I will accept this behavior. 😃

Edit: The double-click-bar-to-maximize behavior on macOS is controlled by the resizable member, so it can be disabled by setting that value to false (on Windows this is not the case, the double-click functionality there is controlled by the maximize button behing enabled or not (which to me makes more sense)). Additionaly, on macOS, if resizable is set to false, the maximize button will be permanently disabled even though it is enabled in the EnabledButtons struct. This might be a bit confusing so I will add it as a note to the enabled_buttons member docstring.

@aleksa2808 aleksa2808 marked this pull request as draft July 9, 2023 18:46
@aleksa2808 aleksa2808 marked this pull request as ready for review July 9, 2023 19:13
@Selene-Amanita Selene-Amanita self-requested a review July 9, 2023 19:35
@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jul 9, 2023

I just tested on Linux Mint Mate (derived from Gnome, X11), and the EnableButtons booleans do nothing. :(

But setting resizable to false does make the maximize button disappear (and greys out the option in the window menu), no matter what is in EnableButtons.

My "double click on the bar" is configured to do a kind of weird semi-minimize where only the title bar of the window remains, that is also still possible, I could try to change this behaviour to maximize but my guess is it would do the same (not maximize if resizable is false, do it if true, ignore EnableButtons.)

@aleksa2808
Copy link
Contributor Author

aleksa2808 commented Jul 9, 2023

I just tested on Linux Mint Mate (derived from Gnome, X11), and the EnableButtons booleans do nothing. :(

Yes, in essence this PR just exposes the winit functionality, which for some platforms is just an empty function. Here's the line for X11:
https://github.com/rust-windowing/winit/blob/master/src/platform_impl/linux/x11/window.rs#L1300

Maybe I can add a note about this discrepancy in the docstrings as well?
I'll also try to figure out why this is the case when obviously the disable button functionality is still there, at least for maximize.

Edit:

This might be the reason why on X11 the maximize button deactivates when resizable is set to false:
https://github.com/rust-windowing/winit/blob/master/src/platform_impl/linux/x11/window.rs#L1278

In general, this feature (among others) is straight up not implemented for X11 and Wayland which winit says in the docs. I haven't delved into the question whether it's actually possible or not.

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jul 9, 2023

I think it's fine if it's not supported on every platform, we just need to be transparent about it.

Edit: beside the remarks on documentation, looks good to me!

@aleksa2808
Copy link
Contributor Author

For posterity, I was confused as to why, in older bevy/winit versions, when setting resizable to false the maximize button would also be disabled even on Windows. I found out that this was done explicitly and was then removed when the enabled_buttons feature was added:
https://github.com/rust-windowing/winit/pull/2537/files#diff-6bbe536d32ae2b66fde18a908848021a8c511a1cc38a4c83d31b0dd6cd3eca33L249

@Selene-Amanita Selene-Amanita added C-Enhancement A new feature A-Windowing Platform-agnostic interface layer to run your app in C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 10, 2023
@github-actions
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jul 10, 2023

Can you put a simple migration guide to say that a new field is added to Window? ^^'

@Selene-Amanita Selene-Amanita added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 10, 2023
@aleksa2808
Copy link
Contributor Author

aleksa2808 commented Jul 10, 2023

Can you put a simple migration guide to say that a new field is added to Window? ^^'

Damn, I'm so used to using ..default() that I overlooked that this can actually be a breaking change! Will add it now!

Edit: added it now, @Selene-Amanita does it conform with other PRs?

@Selene-Amanita
Copy link
Member

Perfect thanks!

@Selene-Amanita Selene-Amanita added this to the 0.12 milestone Jul 17, 2023
@mockersf mockersf added this pull request to the merge queue Jul 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 22, 2023
@mockersf mockersf added this pull request to the merge queue Jul 23, 2023
Merged via the queue into bevyengine:main with commit 5e8ee10 Jul 23, 2023
24 checks passed
@aleksa2808 aleksa2808 deleted the enable-window-control-button-toggling branch July 23, 2023 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants