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.

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

Jump to bottom

On Windows and macOS, add API to enable/disable window controls #2537

Merged
merged 13 commits into from
Nov 29, 2022
Merged

On Windows and macOS, add API to enable/disable window controls #2537

merged 13 commits into from
Nov 29, 2022

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Oct 28, 2022 •
edited by madsmtm
Loading

  • Tested on all platforms changed
    • Windows
    • macOS
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Porting tauri-apps/tao@a50fd86

@amrbashir amrbashir mentioned this pull request Oct 28, 2022
Open
22 tasks
@kchibisov
Copy link
Member

I have a general question wrt that. Is this about the decoration thing or you can actually disable Minimize/Maximize, ect? What the goal for set_closable, etc? If that's all about buttons and has nothing to do with the ability to close the window it should be named differently. From now I see that set_closable(false) prevents the window close?

@amrbashir
Copy link
Contributor Author

Yeah, this is mainly about disabling the buttons itself so it can't be clicked but the window can still be closed in other ways.

@kchibisov
Copy link
Member

Then we should explicitly say that it's all about buttons in decorations. You can use bitflags setter and getter here, no need for so many methods returning bool. It's also better to manage bitflags than a bunch of bools.

kchibisov
kchibisov requested changes Oct 28, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

See above.

The bitflags should be used instead.
The api should explicitly mention decorations.

Sorry for that short of review here, don't have that many of time and I'm only capable to reveiw general changes in the API not the windows/macOS code anyway.

@amrbashir
Copy link
Contributor Author

amrbashir commented Oct 29, 2022 •
edited
Loading

Done, although I don't quite like the naming, so I am open to suggestions.

madsmtm
madsmtm reviewed Nov 2, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks! I agree with the bitflags API.

An example would be nice for testing the functionality?

kchibisov
kchibisov approved these changes Nov 5, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Api change is fine. The rest is up to windows/macOS maintainers.

@kchibisov kchibisov added this to the Version 0.28 milestone Nov 20, 2022
madsmtm
madsmtm requested changes Nov 22, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

macOS impl looks good, haven't tested it though; would you mind adding an example for it?

@amrbashir
Copy link
Contributor Author

Sure

madsmtm
madsmtm approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@madsmtm madsmtm 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 the example - it enabled me to find a bug which I've pushed a fix for ;)

kchibisov
kchibisov requested changes Nov 26, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Just a few minor notes. Should be fine afterwards.

msiglreith
msiglreith approved these changes Nov 26, 2022
View reviewed changes
Copy link
Member

@msiglreith msiglreith left a comment

Choose a reason for hiding this comment

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

Tested successfully on Windows 10
(interesting that Windows seems to automatically adjust the style towards a tool window once minimize and maximize are disabled)

kchibisov
kchibisov approved these changes Nov 26, 2022
View reviewed changes
Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

Could you rebase? Should be good to go afterwards.

@amrbashir
Copy link
Contributor Author

Done!

@madsmtm madsmtm merged commit 94688a6 into rust-windowing:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msiglreith msiglreith msiglreith approved these changes

@madsmtm madsmtm madsmtm approved these changes

@kchibisov kchibisov kchibisov approved these changes

@francesca64 francesca64 Awaiting requested review from francesca64

Assignees
No one assigned
Labels
None yet
Milestone
Version 0.28
Development

Successfully merging this pull request may close these issues.
4 participants
@amrbashir @kchibisov @msiglreith @madsmtm