extension: Add SettingsOverrider to preserve user settings#244
extension: Add SettingsOverrider to preserve user settings#244Leleat merged 2 commits intoubuntu:mainfrom
Conversation
Allow to get GVariant values and reset them
The extension is overriding the user settings by writing on them, sadly this implies various issues because even if we reset them on extension unloading, a shell crash or mis-behavior could always lead to affect user settings. Tiling assistant replaces the user settings when running, but not to loose the user settings, we need to save them around so that when the extension gets unloaded they can be restored. So add a SettingsOverrider class that: - Replaces the user settings with the extensions one - Remembers what has been changed and saves it locally and in our settings - When the extension is unloaded the settings are restored or reset Now, in case gnome-shell is stopped or the extension has crashed for some reason, once the extension is reloaded we check if previous settings were saved and in case they are used to restore the user settings on destruction.
Leleat
left a comment
Leleat
left a comment
There was a problem hiding this comment.
Only small comments/questions. Otherwise it LGTM and I could merge it.
|
|
||
| _maybeUpdateOverriden(schemaId, key, value) { | ||
| if (this._wasOverridden) | ||
| return undefined; |
There was a problem hiding this comment.
Doesn't look like you actually need the return value from this method (and the name _maybeUpdateOverriden doesn't really suggest that there is a return value). So maybe remove it? (Same for L68)
| new GLib.VariantType('b'), null); | ||
|
|
||
| const savedSettings = this._settings.getUserValue('overridden-settings'); | ||
| this._wasOverridden = savedSettings !== null; |
There was a problem hiding this comment.
IMO this._wasOverridden doesn't really commumicate what it actually stands for. From what I can tell, this is only used if the extension (or GNOME Shell) was shutdown unexpectedly so that the extension's disable function wasn't called and the setting wasn't cleaned up properly. So how about something like _previousShutdownWasDirty, previousCleanupWasSkipped, savedSettingsWereCorrupted or something similiar
| remove(schema, key) { | ||
| const settings = this._originalSettings.get(schema); | ||
| if (!settings) | ||
| return; | ||
|
|
||
| const values = this._overrides.get(settings.schemaId); | ||
| const value = values?.get(key); | ||
|
|
||
| if (value === undefined) | ||
| return; | ||
|
|
||
| if (value) | ||
| settings.set_value(key, value); | ||
| else | ||
| settings.reset(key); | ||
|
|
||
| values.delete(key); | ||
| this._maybeUpdateOverriden(settings.schemaId, key, undefined); | ||
| } |
There was a problem hiding this comment.
This seems unused. Do you think it is needed in the future? I don't mind leaving it in either way, up to you.
2 participants
This is the "hacky" version of #243 in a way that can be supported by current gnome.
The extension is overriding the user settings by writing on them, sadly this
implies various issues because even if we reset them on extension unloading,
a shell crash or mis-behavior could always lead to affect user settings.
Tiling assistant replaces the user settings when running, but not to loose the
user settings, we need to save them around so that when the extension gets
unloaded they can be restored.
So add a SettingsOverrider class that:
Now, in case gnome-shell is stopped or the extension has crashed for some
reason, once the extension is reloaded we check if previous settings were
saved and in case they are used to restore the user settings on destruction.