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

(feature) Add Stage Manager (com.apple.WindowManager) settings #1002

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

malko42
Copy link
Contributor

@malko42 malko42 commented Jul 11, 2024

All credits go to @AlexOwl. Their PR looked abandonned so I reported their changes and addressed the change requests.

Copy link
Contributor

@Samasaur1 Samasaur1 left a comment

Choose a reason for hiding this comment

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

Assuming that you've tested it and that those are still the correct defaults keys to use for those settings, this looks good to me

@malko42
Copy link
Contributor Author

malko42 commented Jul 11, 2024

Hi @Samasaur1 and thanks for the review!
If you compare this PR with the orginal one the options are not the same. I added and tested only the options that I was interested in. I'm not sure about the default values though I took my own system as reference since I haven't touched those options AFAIK but still not 100% sure.
Is there an up-to-date list somewhere listing all com.apple.* settings ?

Also there are a few commonly defined options in Nix config files do you want me to add them in a later PR or on this one ?

@Samasaur1
Copy link
Contributor

Hi @Samasaur1 and thanks for the review!

If you compare this PR with the orginal one the options are not the same. I added and tested only the options that I was interested in. I'm not sure about the default values though I took my own system as reference since I haven't touched those options AFAIK but still not 100% sure.

I think you're right about the default values. I also don't think it really matters all too much because I think if people are looking at these options, they're probably planning to set them anyway.

I did look again at your EnableStandardClickToShowDesktop option, and I think you should update the docs you wrote for it to say that false means "Only in Stage Manager" and true means Always

Is there an up-to-date list somewhere listing all com.apple.* settings ?

Wouldn't that be nice! I wish. I know there's https://macos-defaults.com/ and maybe other sites like it, but I don't think they're comprehensive

Also there are a few commonly defined options in Nix config files do you want me to add them in a later PR or on this one ?

I'd say you should feel free to add any options under com.apple.WindowManager in this PR, but other defaults options should have their own PRs

Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

Can you rebase your branch on top of master and squash all your commits into a singular one?

modules/system/defaults/WindowManager.nix Outdated Show resolved Hide resolved
modules/system/defaults/WindowManager.nix Outdated Show resolved Hide resolved
@malko42
Copy link
Contributor Author

malko42 commented Jul 21, 2024

Few things to note:

  • I added all the options I could effectively test
  • I named the options after the ones from Apple but I'm not happy with it. I find them at best non explicit and most of the time logically opposite of their UI counterpart

Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

modules/system/defaults-write.nix Show resolved Hide resolved
…tem.defaults.windowmanager

All credits go to @AlexOwl. Their [PR](LnL7#505)
looked abandonned so I reported their changes and addressed the change
requests.
Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution

@Enzime Enzime merged commit f7142b8 into LnL7:master Aug 2, 2024
6 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