GUI for GSettings

[sei0o]

I'm working on #142 and trying to create the UI first (see components/settings/) then to integrate it with some structs in src/settings.rs.

Currently I am stuck at how to handle the AdwPreferencesWindow. I have set up XML file with some widgets, added a button in the user menu, but an empty window is shown:

[xou816]

Hi! Nice, thank you for working on this!

Haven't played with the Preferences stuff yet, but looks like you might need to have a PreferencesPage as a child of that PreferencesWindow first

(...also, the <child> tag should only have one <object> child!)

[jannuary]

You need to have the settings.ui file defined in the gresource schema, like this:

<?xml version="1.0" encoding="UTF-8"?>
<gresources>
  <gresource prefix="/dev/alextren/Spot">
...
    <file alias="components/settings.ui">app/components/settings/settings.ui</file>
...
  </gresource>
</gresources>

[sei0o]

It is working after applying your advice and some other tweaks. Thanks!

[sei0o]

So now the settings window is working and the updated GSettings properties are loaded on next startup.

I would like to make them reloaded during the execution of Spot, as many applications do. Based on my understanding, players and some other components make use of SpotSettings. On startup, the SpotSettings is cloned & passed down on those components. Therefore they are not synchronized with the original one or GSettings.

My idea is to put a SpotSettings in the AppState and configure necessary signals/actions to make reloading happen while the app is running (the design doc is quite helpful!). What do you think?

[xou816]

Neat!

Given that most if not all settings require a restart, it is not that big of a deal if we don't manage these settings through the shared AppState -- but you can absolutely do that if you want :)

As for persisting those settings, there's an example of that in settings.rs -- you could do it from pretty much anywhere I think.

[sei0o]

Well, yes, it seems we don't have to put the settings in AppState and PlayerNotifier is capable of make the player change its own settings (still I'm investigating).

As for persisting those settings, there's an example of that in settings.rs

Currently I'm binding the widgets to gsettings directly. The document on Gio.Settings says changing settings wakes up their backends (e.g. dconf) and lets them do lots of work, but I'm not quite sure how we have to care about this since many users won't fidget with this kind of settings.

[sei0o]

I've just pushed the latest commit, which allows us to apply new settings without restarting. The problem, however, is that the window cannot be closed twice. This bug has been there since I created an empty PreferencesWindow and is pretty weird...

[jannuary]

I've just pushed the latest commit, which allows us to apply new settings without restarting. The problem, however, is that the window cannot be closed twice. This bug has been there since I created an empty PreferencesWindow and is pretty weird...

It's a known issue, you need to have hide-on-close enabled on the PreferencesWindow.

[sei0o]

Yeah, hide-on-close did the trick!

It seems it is working as expected. If you are going to make changs in color scheme handling in anytime soon, I'm willing to fix GUI in accordance with it (though I don't see what to do exactly).

[xou816]

Hi! been a little busy but I'll have a look :)

[xou816]

nice work!! I'll try to be more reactive to questions and feedback as you go through the review!

[xou816]

I think you could store the Settings here as mentioned at some point :)

That way, the action could have no payload with it (just ChangePlayerSettings) and whenever that action is dispatched, we just do self.settings = SpotSettings::new_from_gsettings() since it's already synced probably?

that way you don't have to do the mapping from the Adwaita widgets to SpotSettings everytime

[sei0o]

Makes sense. Moved SpotSettings into SettingsState and SettingsWindow use it whenever possible, while there are still some redundant SpotSettings::new_from_gsettings() calls from the player and update_with handlers for SettingsState.

[xou816]

I am nitpicking a bit, but I think one action is enough here, and the state should decide whether to return no event or vec![SettingsEvent::PlayerSettingsChanged] if the player part of the settings have changed

[sei0o]

set_player_settings still has to emit PlaybackAction::Stop to stop playing (otherwise there will be some inconsistency in UI), so the two same ifs are put in update_with and Settings::new. It's not a nitpick because I had the same concern, just thought of avoiding the duplicate ifs at first.

[xou816]

Oh you're right absolutely!

[xou816]

Looking good!! :) one last comment above and a couple of compilation warnings to fix but otherwise it looks good to me!

[xou816]

     fn update_with(&mut self, action: std::borrow::Cow<Self::Action>) -> Vec<Self::Event> {
         match action.into_owned() {
             SettingsAction::ChangeSettings => {
-                let old_settings = self.settings.clone();
-                self.settings = SpotSettings::new_from_gsettings().unwrap_or_default();
-                if self.settings.player_settings == old_settings.player_settings {
-                    vec![]
-                } else {
+                let old_settings = &self.settings;
+                let new_settings = SpotSettings::new_from_gsettings().unwrap_or_default();
+                let player_settings_changed =
+                    new_settings.player_settings != old_settings.player_settings;
+                self.settings = new_settings;
+                if player_settings_changed {
                     vec![SettingsEvent::PlayerSettingsChanged.into()]
+                } else {
+                    vec![]
                 }
             }
         }

(you can avoid the extra clone here)

[sei0o]

Fixed in the latest commit :)

[xou816]

one last thing I just realized: source files should be tracked in src/meson.build, otherwise meson sometimes does not pick up on changes

[Diegovsky]

Great work! I can't wait for this to me merged :)

[xou816]

Thank you for your work and patience through the review! Merging this :)

[sei0o]

Learned a lot from this PR... I really appreciate reviews & advice!