-
Notifications
You must be signed in to change notification settings - Fork 160
Install a default tiling_exceptions_custom config to /share
#1603
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
Conversation
Fixes "tiling exceptions custom config error" error that cosmic-comp was printing. Alternately we could modify `cosmic-settings-dameon-config` to ignore the error for this particular setting. But if we generally expect global config files to be present for Cosmic settings, it seems best to just be consistent and add a simple file here.
|
Hmm, "custom" sounds like user-provided for me, so I don't really see the point of providing an empty file... Maybe we should downgrade that error to a debug-statement. |
Hey, if I can add something to it :) From my perspective as an user. I was needed to edit this file. I did not know the name of it so I had to copy file from /usr/share (the original one). If it's helps, the empty file would be good idea at least I think like that :D |
|
Yeah, we could ignore that error, or have it as a debug print, while still having an error if
Yeah, this could make things clearer if someone wants to manually edit the configuration. This shows what the file should be named, and then But I don't have any strong opinion on this. |
Ideally you don't have to do that though, as the tiling applet should provide a window chooser to add new exceptions. Sure it is nice, if there is a file you can edit manually, but it doesn't even have the same format as (https://github.com/pop-os/cosmic-settings-daemon/blob/master/config/src/window_rules/mod.rs#L41-L44) Or you read about it in some issue, which likely points you to the right file anyway. Which brings me to my second point, we want to have and should have documentation on the config files at some point. So with a GUI-way to add exceptions and some docs, the empty file would again make no sense really.
Yeah, we definitely want an error if defaults is missing though. If a distribution chooses to ship an empty array, that is fine, but I want the error to be a hint, that the file should be there for packaging. |
|
Wow did know about it: (https://github.com/pop-os/cosmic-settings-daemon/blob/master/config/src/window_rules/mod.rs#L41-L44) Yeah I can agree with you @Drakulix :) Keep going what you are all guys doing because it's an incredible work :) |
As noted in pop-os/cosmic-comp#1603, there's no need to a system default config file for this particular setting. So as an alternative to that PR, we can ensure this isn't printed as an error. As noted in the description of pop-os/libcosmic#948, if the system default is not present, `Error::GetKey` is returned, without mapping to the `Error::NotFound` variant. So check for that, as well as `.is_err()`.
|
We could also document this by adding a default file with a commented example. But there are perhaps better ways to document configuration (when we get around to that), and in any case it's reasonable to just not error here: pop-os/cosmic-settings-daemon#100. |
Fixes "tiling exceptions custom config error" error that cosmic-comp was printing.
Alternately we could modify
cosmic-settings-dameon-configto ignore the error for this particular setting. But if we generally expect global config files to be present for Cosmic settings, it seems best to just be consistent and add a simple file here.