Add feature to import/export/reset settings #151

theoforger · 2024-10-08T18:28:39Z

Fixes #122.

Description

This PR implements the feature to import, export and reset settings, which can be accessed through the new buttons inside the preference page:

Implementation

The new class SettingsExport is responsible for all tasks related to this feature.
A list of excluded keys are defined. These keys are irrelevant to settings export.

Export

Run dconf dump in a subprocess: code
Remove keys in the excluded list: code
Save to file: code

Reset

Run dconf reset on keys that are not in the excluded list: code

Import

Reset to default: code
- This makes sure that, if the export file is missing a key, the corresponding option will be set to default
Turn off edge tiling and key binding: code
- This makes sure the overridden settings are restored back to system before importing anything
Run dconf load: code

Issues/Possible Improvements

Due to the workaround used for overridden settings, if the export file is missing the key active-screen-edges or enable-move-keybindings, these two options will remain disabled after import, instead of the default enabled state
Choose a specific file extension/suffix for export file

Feedback?

I am still new to coding. Any feedback or advice is appreciated 🙏

domferr · 2024-10-16T09:00:12Z

Hey @theoforger sorry for the late response. Your work is amazing and very well done! Thank you! ⭐
I'm going to use this features for a while to test them! I think this is already well made to land on the next update (not sure if I will name it 13.2 or 14) together with some other features I've implemented in the past weeks.

domferr · 2024-10-16T09:16:48Z

A quick update for you. I just discovered that some of the things you are doing via a subprocess can be done by using a Gio.Setting object. I'm leaving some comments so you know which one can be simplified and made more reliable by avoiding the use of a subprocess

src/settings/settingsExport.ts

src/prefs.ts

theoforger · 2024-10-17T15:42:25Z

Thanks for the review ❤️ ! I'll look into Gio.Setting. It does seem a lot easier than subprocesses.

Just in case you missed it, I left a comment on the issue about some strange behavoir I observed. Let me know if I should file them as a separate issue.

Also, any thoughts on these two?

Issues/Possible Improvements

Due to the workaround used for overridden settings, if the export file is missing the key active-screen-edges or enable-move-keybindings, these two options will remain disabled after import, instead of the default enabled state

Choose a specific file extension/suffix for export file

domferr · 2024-10-18T07:42:31Z

Thanks for the review ❤️ ! I'll look into Gio.Setting. It does seem a lot easier than subprocesses.

Just in case you missed it, I left a comment on the issue about some strange behavoir I observed. Let me know if I should file them as a separate issue.

Hey thank you! I didn't see you comment there. I left a reply there with all the details, thank you for spotting these bugs!

Choose a specific file extension/suffix for export file

What do you think about a simple .txt file?

Due to the workaround used for overridden settings, if the export file is missing the key active-screen-edges or enable-move-keybindings, these two options will remain disabled after import, instead of the default enabled state

Hmmmm, I'll investigate, hope to understand how to fix this soon!

domferr · 2024-10-18T07:56:29Z

src/settings/settingsExport.ts

+
+        // Flush overridden settings back into system
+        Settings.set_active_screen_edges(false);
+        Settings.set_enable_move_keybindings(false);


Due to the workaround used for overridden settings, if the export file is missing the key active-screen-edges or enable-move-keybindings, these two options will remain disabled after import, instead of the default enabled state

The reason behind this is that, during the import of settings, the settings are reset and then active-screen-edges and enable-move-keybindings are set to false.

I see you want to flush all the overridden settings back into system, you could use SettingsOverride.get().restoreAll()

Appreciate the suggestion!

SettingsOverride.get().restoreAll() works except for one situation: If either of these toggles are enabled both before and after the import, the overridden settings remain restored.

I can think of two solutions:

If the state of either is enabled both before and after import:
a. Apply overrides again (is there a function for this?), or
b. Toggle it off and on

What do you think?

Sorry, what do you mean with "the overridden settings remain restored"?

The value of overriden-settings is only populated when you enable the toggle.

In this case, SettingsOverride.get().restoreAll() restores the overridden settings, but it doesn't get populated again since the state of the toggle is unchanged.

Hope that clears things out!

Uuuuhhhh! I understand what do you mean...
Thanks a million, I believe that toggling it off and on, as you suggested, could be the best workaround (there isn't a function to override again). Can you put a comment near this workaround to remember these details in the future? Thanks again!

No problem. Will do!

theoforger · 2024-10-21T17:38:07Z

I noticed the fallback states for several get functions in src/settings/settings.ts don't match with the default states of the corresponding options. For example, get_enable_move_keybindings falls back to false, while the actual option defaults to enabled if the dconf key enable_move_keybindings is not present.

Is this intentional or an oversight?

Context: I'm trying to use these functions to determine the states before/after import.

domferr · 2024-10-22T07:25:29Z

I noticed the fallback states for several get functions in src/settings/settings.ts don't match with the default states of the corresponding options. For example, get_enable_move_keybindings falls back to false, while the actual option defaults to enabled if the dconf key enable_move_keybindings is not present.

Is this intentional or an oversight?

Context: I'm trying to use these functions to determine the states before/after import.

This was intentional at that time, however there isn't any reason anymore. Because of that, it would be better that the dconf's defaults matches with settings.ts's defaults. Thank you for spotting that!

theoforger · 2024-10-22T16:11:17Z

I just pushed some more code. But the workaround is not working right now. This is getting more complicated than I thought haha.

From my testing, Settings.set_enable_move_keybindings(false) restores overridden-settings back to system just fine, but Settings.set_enable_move_keybindings(true) doesn't populate overridden-settings.

The same goes for screen edges. Toggling it on in the GUI does the trick. It only doesn't work if I use the setter function to enable it.

Edit: Forgot to mention I added these two setter functions myself. Maybe I missed something there 😄

domferr · 2024-10-22T17:51:12Z

Hey thank you! I gave a look at your code and it seems great to me! It's unfortunate you still have that bug 😢

However, considering that I was planning a major release (v14) in the near future I wish to include your amazing changes as well. Because of that, I'm changing the target of your pull request from main to the branch named 13.2 (sorry, at the beginning I thought to do version 13.2 but now I changed my mind to version 14 ahahah)

Can you tell me if you have the same bug? That branch includes the changes made to fix the bugs you spotted, maybe they are related! Thank you once again for this amazing work ❤️

Another question, as much important as the other one: is it ok for you if I cite you and put a link to your GitHub account in the release notes which will include your changes? 😃

Edit: Forgot to mention I added these two setter functions myself. Maybe I missed something there 😄

I looked at them, and you did it perfectly, so the bug is somewhere else 🤔. I'll investigate and come back to you if I have news!

theoforger · 2024-10-22T22:59:34Z

Strange. Now I'm getting this error when I try to open the settings page:

ImportError: Unable to load file from: resource:///org/gnome/shell/extensions/extension.js (The resource at “/org/gnome/shell/extensions/extension.js” does not exist)

Stack trace:
  _init/GLib.MainLoop.prototype.runAsync/</<@resource:///org/gnome/gjs/modules/core/overrides/GLib.js:266:34

I'm using GNOME 46 on Fedora 40 if that's relevant. And thanks for being patient with me haha 😄

domferr · 2024-10-23T08:20:46Z

Strange. Now I'm getting this error when I try to open the settings page:
ImportError: Unable to load file from: resource:///org/gnome/shell/extensions/extension.js (The resource at “/org/gnome/shell/extensions/extension.js” does not exist)

Stack trace:
  _init/GLib.MainLoop.prototype.runAsync/</<@resource:///org/gnome/gjs/modules/core/overrides/GLib.js:266:34
  
I'm using GNOME 46 on Fedora 40 if that's relevant. And thanks for being patient with me haha 😄

Hey! I'm so sorry, that's my mistake 😢

I fixed this, you should see a new commit in the target branch (13.2)

theoforger · 2024-10-23T13:53:17Z

Can you tell me if you have the same bug? That branch includes the changes made to fix the bugs you spotted, maybe they are related! Thank you once again for this amazing work ❤️

I just tested and this bug still exists 😢

Another question, as much important as the other one: is it ok for you if I cite you and put a link to your GitHub account in the release notes which will include your changes? 😃

Of course, I'm okay with it 👍 (Sorry I missed this earlier)

domferr · 2024-10-23T17:46:24Z

Hey, I have good news for you. From my tests, I think I fixed the problem you had!

The idea is the following, when settings are reset:

set to false the settings about keybinding and active screen edges (enable_move_keybindings and active-screen-edges)
restore all the overridden settings. This will revert what was overridden back to the system. At this stage the settings overridden-settings becomes empty
finally reset all the settings. This will trigger all the work needed to override the settings

When settings are imported, resetting will override all the settings as well. If the imported file contains a false value for enable_move_keybindings or active-screen-edges, they are updated and this triggers the update of overridden-settings as well. Thank you for suggesting this idea about "turning off and back on" 😄.

You will see that I also made some changes to how the settings are overridden and how they are reverted: it is now more robust and I believe that code was not good at all and it was causing issues as well.

theoforger · 2024-10-23T18:41:51Z

Fantastic job! I tested it and everything seemed to work properly. I should've thought of restoring the keys during resets haha.

Anyway I'll change the PR status to ready 🎉

domferr · 2024-10-24T09:01:50Z

Amazing work @theoforger ! Thank you once again, I'll merge and include your changes in the upcoming version of Tiling Shell. You can expect it being released today or tomorrow, depending on the time needed by GSE to review. Can't wait to see people enjoy all the features you implemented! ❤️

Edit: I left the hacktoberfest-accepted label in case you are interested into it 😄

theoforger · 2024-10-24T12:04:37Z

And thanks for guiding me along the way! It's been such a pleasure working with you ❤️

thyttan · 2024-10-24T18:47:23Z

I was a little confused about how the file should be named, but worked out that "example.txt" worked. Maybe the Files-window could be pre-filled with e.g "tiling-shell-exported-settings.txt" as the file name.

Just a suggestion.

theoforger · 2024-10-29T16:11:37Z

@thyttan I think it's a great idea. I noticed the name confusion as well during development and tried to solve it with a filter. For some reason it didn't seem to do anything. Pre-filling might be the better solution here. 👍

@domferr We can use Gtk.FileChooserDialog.set_current_name. So just add something like:

fc.set_current_name('tiling-shell-settings.txt');

What do you think?

domferr · 2024-10-30T14:22:49Z

Hey @theoforger @thyttan I think this is a must have addition! @theoforger I tested your suggestion and it works perfectly, thanks! Next release will include this change 🔜

theoforger added 2 commits October 5, 2024 20:26

Implement import/export/reset

a088bc0

Handle system overrides

10b0e88

domferr reviewed Oct 16, 2024

View reviewed changes

src/settings/settingsExport.ts Outdated Show resolved Hide resolved

domferr reviewed Oct 16, 2024

View reviewed changes

src/settings/settingsExport.ts Outdated Show resolved Hide resolved

domferr reviewed Oct 16, 2024

View reviewed changes

src/settings/settingsExport.ts Outdated Show resolved Hide resolved

domferr reviewed Oct 16, 2024

View reviewed changes

src/prefs.ts Outdated Show resolved Hide resolved

domferr reviewed Oct 18, 2024

View reviewed changes

theoforger added 2 commits October 21, 2024 13:07

Use Gio.Settings where applicable

0b44ff3

Change suffix filter to txt

0cc8d51

theoforger added 3 commits October 22, 2024 11:52

Apply workaround to populate overridden-settings

0912095

Simplify subprocess code

6c6cd68

Change fallback states of boolean functions to reflect dconf default

dff1580

domferr changed the base branch from main to 13.2 October 22, 2024 17:51

Merge branch '13.2' into 122-dconf-keyfile

021b288

domferr and others added 5 commits October 23, 2024 10:21

Merge branch '13.2' into 122-dconf-keyfile

5dc628c

Merge branch '13.2' into 122-dconf-keyfile

d25d0f0

Enable translations

dd3f492

Mirror the changes in Layout section

0a7a78b

Remove unused pipes

6a85ff6

fix: reset and import settings correctly

8abcfd6

theoforger marked this pull request as ready for review October 23, 2024 18:41

domferr added the hacktoberfest-accepted label Oct 24, 2024

domferr merged commit ca8006a into domferr:13.2 Oct 24, 2024

domferr added the enhancement New feature or request label Oct 24, 2024

Uh oh!

Uh oh!

Add feature to import/export/reset settings #151

Add feature to import/export/reset settings #151

Uh oh!

Conversation

theoforger commented Oct 8, 2024

Description

Implementation

Export

Reset

Import

Issues/Possible Improvements

Feedback?

Uh oh!

domferr commented Oct 16, 2024

Uh oh!

domferr commented Oct 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theoforger commented Oct 17, 2024

Issues/Possible Improvements

Uh oh!

domferr commented Oct 18, 2024

Uh oh!

domferr Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

theoforger Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domferr Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

theoforger Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

domferr Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

theoforger Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

theoforger commented Oct 21, 2024

Uh oh!

domferr commented Oct 22, 2024

Uh oh!

theoforger commented Oct 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domferr commented Oct 22, 2024

Uh oh!

theoforger commented Oct 22, 2024

Uh oh!

domferr commented Oct 23, 2024

Uh oh!

theoforger commented Oct 23, 2024

Uh oh!

domferr commented Oct 23, 2024

Uh oh!

theoforger commented Oct 23, 2024

Uh oh!

domferr commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theoforger commented Oct 24, 2024

Uh oh!

thyttan commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theoforger commented Oct 29, 2024

Uh oh!

domferr commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

theoforger Oct 21, 2024 •

edited

Loading

theoforger commented Oct 22, 2024 •

edited

Loading

domferr commented Oct 24, 2024 •

edited

Loading

thyttan commented Oct 24, 2024 •

edited

Loading