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

Move tiling exceptions to configuration file #726

Merged
merged 7 commits into from
Sep 4, 2024

Conversation

Tebro
Copy link
Contributor

@Tebro Tebro commented Aug 14, 2024

This moves out the hard coded tiling exceptions into a new separate config file under CosmicComp

@Drakulix
Copy link
Member

Sorry, but we probably won't do this as one single config file.

Reason being that we want to ship the default rules and also be able to update them. So we likely want a new config file for rules added by users and default rules to be explicitly disabled.

Externalizing the default rules into a separate config file still makes sense, given we want to allow distros overwriting them.

I don't have the time to do a thorough review on your work now and will try to do so next week. In any case thanks for working on this!

@Tebro Tebro force-pushed the feat/exceptions-config branch from 310ac52 to cee0ee3 Compare August 17, 2024 10:38
@Tebro
Copy link
Contributor Author

Tebro commented Sep 1, 2024

@Drakulix is there something I can do for this at this time?

@Drakulix
Copy link
Member

Drakulix commented Sep 3, 2024

@Drakulix is there something I can do for this at this time?

Yes, so looking at this further, what needs to be done to move this PR along is split the config file into two. One for the defaults and one for user-overrides (so either disabling existing rules from the defaults file or adding additional ones), because we want distros to provide the former and be able to update them without disrupting user configuration.

For this you can take inspiration from our shortcuts-config, which does the same (but is located in the cosmic-settings-daemon-crate: https://github.com/pop-os/cosmic-settings-daemon/blob/master/config/src/shortcuts/mod.rs#L53).

That also adds a new config namespace (com.system76.CosmicSettings.Shortcuts), we probably want the same for this, something like com.system76.CosmicSettings.WindowRules (anticipating, that we might have different rules in the future) and then tiling-exception-defaults and tiling-exception-custom for the files.

Given these should be configurable from cosmic-settings and cosmic-applet-tiling in the future, it might also make sense to move the config into the cosmic-settings-daemon repo (splitting this feature into two PRs).

The defaults file could still be installed by cosmic-comp, like the shortcuts file.
I don't have a problem with the whole config format (at least for the defaults file, the custom file would need to be slightly different to allow disabling existing rules), but I would prefer, if we didn't skip the whole config, if one regex fails to parse. I would appreciate it, if the deserialization code could skip over broken rules and just log a warning.

I understand, that this is a far bigger tasks, than you probably imagined opening this PR, so please don't see this as a demand. If you don't feel like working on this, we will eventually implement said feature. That said your effort and contribution is very appreciated.

@Tebro
Copy link
Contributor Author

Tebro commented Sep 4, 2024

Thanks for the response. Will try to find some time to look into all of this. It would be nice to finish what was started.

@Tebro
Copy link
Contributor Author

Tebro commented Sep 4, 2024

This is now updated, needs the PR in cosmic-settings-daemon to be merged first for the checks to pass.

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

This looks great!

I think there is just one thing missing, it doesn't appear to reload the config file at runtime. To do that you need to add some code to watch the config file, re-assemble the exceptions again and update the data in Shell.

You can see the code doing that for the shortcuts just below your new code in Config::load (config.rs).

@Tebro
Copy link
Contributor Author

Tebro commented Sep 4, 2024

Got that working now.

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

Almost, just a small nitpick. Thanks for working on this :)

src/shell/layout/mod.rs Outdated Show resolved Hide resolved
src/shell/mod.rs Outdated Show resolved Hide resolved
src/shell/mod.rs Outdated Show resolved Hide resolved
@Drakulix
Copy link
Member

Drakulix commented Sep 4, 2024

pop-os/cosmic-settings-daemon#50 merged, this needs a cargo update -p cosmic-settings-config

@Tebro
Copy link
Contributor Author

Tebro commented Sep 4, 2024

Did the cargo update, some of the version moves in the diff looked confusing, such as libloading 0.8.5 -> 0.7.4. Pushed it up in case it is correct.

Cargo.lock Show resolved Hide resolved
@Tebro Tebro force-pushed the feat/exceptions-config branch from 03a039a to a7970ce Compare September 4, 2024 18:42
Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this!

@Drakulix Drakulix merged commit 79ae56c into pop-os:master Sep 4, 2024
1 check passed
@Tebro Tebro deleted the feat/exceptions-config branch September 4, 2024 18:51
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