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

config: Handle unknown Action/SystemAction gracefully #68

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

Drakulix
Copy link
Member

@Drakulix Drakulix commented Jan 21, 2025

This is an attempt to solve the issue e.g. described in #55.

New Actions and SystemActions can currently be very annoying to add, especially if they are set in the default-keybindings or system-actions ron, as older version of cosmic-comp or cosmic-settings will simply refuse to launch, if they can't parse that file.

This adds an Unknown variant to the end of the respective enums, that matches every action. This has the potential to remove those actions from user-config files by any application writing those (e.g. cosmic-settings), but imo that is reasonable and I haven't found a good way to just store and write the unserialized data in the Unknown variant without breaking this work around one way or another.

This can be merged at any time, but cosmic-settings and cosmic-comp will need updates to depend on a cosmic-settings-config version containing this before we can just add new variants.

I plan to backport this to the cosmic-comp jammy branch as well, so we don't accidentally break that old version with new shortcuts.

@Drakulix Drakulix requested a review from ids1024 January 21, 2025 13:59
@Drakulix
Copy link
Member Author

Seems like this doesn't work with ron in particular. Still searching for a better solution.

@ids1024
Copy link
Member

ids1024 commented Jan 21, 2025

Using #[serde(other)] may work for deserializing system_actions.ron.

It will be lossy for cosmic-settings if it sees an action it doesn't recognize and then save changes. Not sure if that's likely to be an issue.

@Drakulix
Copy link
Member Author

Seems like this doesn't work with ron in particular. Still searching for a better solution.

Using #[serde(other)] may work for deserializing system_actions.ron.

Other doesn't work for externally tagged enums. And the latest stable ron version (0.8) doesn't support anything but externally tagged.

@Drakulix
Copy link
Member Author

Drakulix commented Jan 22, 2025

New approach, which is working now. Needs pop-os/libcosmic#797 merged

@Drakulix Drakulix changed the title config: Add unknown Action/SystemAction config: Handle unknown Action/SystemAction gracefully Jan 22, 2025
@mmstick mmstick merged commit b29dfc2 into master Jan 22, 2025
6 checks passed
@mmstick mmstick deleted the unknown-action branch January 22, 2025 15:19
@ids1024
Copy link
Member

ids1024 commented Jan 22, 2025

I guess we'll still need to make sure when a new default shortcut is added with a system action, cosmic-settings is updated to still be able to parse the default shortcuts. That's definitely better though.

@mmstick
Copy link
Member

mmstick commented Jan 22, 2025

Yeah, thankfully cosmic-settings can be reloaded without needing to log out.

@ids1024
Copy link
Member

ids1024 commented Jan 22, 2025

Though that could be an issue with cosmic-comp. Which packages the default bindings, but an update will pull in the new version but not restart the compositor.

@Drakulix
Copy link
Member Author

Though that could be an issue with cosmic-comp. Which packages the default bindings, but an update will pull in the new version but not restart the compositor.

Which will just mean that cosmic-comp will ignore the new bindings/system-action.

@Drakulix
Copy link
Member Author

I guess we'll still need to make sure when a new default shortcut is added with a system action, cosmic-settings is updated to still be able to parse the default shortcuts. That's definitely better though.

What am I missing here, shouldn't cosmic-settings also simply skip over actions it doesn't know about? Or is it parsing the file separately?

@ids1024
Copy link
Member

ids1024 commented Jan 22, 2025

Oh, I didn't see this was also changing the de-serialization of Shortcuts, so that should be good.

I guess serialization isn't an issue either since custom doesn't duplicate defaults, so the fact deserializating and then serialization would strip out unrecognized actions isn't a problem.

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