add reset buttons in Preferences

[Batwam]

See first pass implementation of reset buttons. Note that I had to adjust some of the supporting text to make everything fit as it's getting relatively busy.

The buttons work for all widgets with the exception of the ComboBox which don't update to the default values. To be clear, it does reset gsettings but I can't figure out how to make the button also reset the comboBox value in a generic way which would work in the buildResetButton(). Would you mind having a look at it?

[Moon-0xff]

Looks good but I think it clutters the interface. I would prefer if the button is hidden unless the setting value has been changed, keeping the space where the button is for alignment. No idea how to do that though.

The buttons work for all widgets with the exception of the ComboBox which don't update to the default values

My bet is that combobox.set_active_id(settings.get_string(setting)) isn't declared on the right place.

With all the refreshless stuff I noticed calls in GJS signals sometimes don't work if declared in a wrong place/context? no idea.

[Batwam]

I made some changes quickly to see if it could be possible to only show the reset button if the user setting isn't default. The good thing is that when we press reset, the key is deleted so rather than compare the current setting Vs default, we can simply check if there is a user defined value or not to decide if we show the reset button or not.

The approach is a little different with the wide entry but I played around with the settings for secondary icons and it seems to work.

Just a rough test for proof of concept but it shoud give an idea. Once again, I focussed on entries/switch/spinbutton. it should only show a reset button for modified settings.

Also in the todo would be to hide the reset buttons for disabled double click drop downs.

[Batwam]

we could also use the rotated icon and put the reset button on the left to avoid the misalignment:

[Batwam]

it gets busy pretty quickly with multiple drop downs so we might not want to include it for addDoubleStringComboBox() or addTripleStringComboBox()

[Moon-0xff]

we could also use the rotated icon and put the reset button on the left to avoid the misalignment

Not a bad workaround.

it gets busy pretty quickly with multiple drop downs so we might not want to include it for addDoubleStringComboBox() or addTripleStringComboBox()

Well the reason we do that is to not lengthen the option list too much.

The triple combo box as it stands takes too much space and I don't like it, but the double one doesn't look bad on it's own.

Adding a reset button for each row instead of for each option sounds like a good compromise.

[Batwam]

The triple combo box as it stands takes too much space and I don't like it, but the double one doesn't look bad on it's own.

Note that is it currently set at a wider width than necessary. This was done to keep the single combo width the same as the spinbutton on the first page but could be reduced for the double and triple (all 3 are using buildStringComboBox() which is where the width is set manually to 105). I'll update so it only applies to single combobox.

[Moon-0xff]

I was thinking if we can avoid comboBoxes, they are difficult to work with.

I searched in the docs and it turns out comboBoxes are deprecated, the recommended alternative is dropDowns, which sounds like the same thing, but using them might give us less headaches.

[Batwam]

the other things I noticed is that the drop downs in gnome-settings have more of a flat look to them with no borders

I think that the modern way to do list is using Adw.ComboRow or ListBoxRow which is also what is being used in gnome-center as far as I can tell. The doc does say that it mirrors Gtk.DropDown

It would looks something like this:

The only challenge I have with these two is that it doesn't seem to be possible to select an item based on its value (at least not directly). You have to indicate the index of the item you want to select apparently. In our case, we have the value that we display but also the value that we save which aren't the same thing so I'm struggling to implement it. I found an example here if that helps you: https://github.com/dmo60/CoverflowAltTab/blob/a94345128f0841c178651e48a9ea15ffa0772ea8/src/prefs.js#L542

[Batwam]

btw, I just discovered the other day that if you are in an app like gnome-center and click on an element, you can then press Ctrl + Shift + i to find useful info like this. You can even change properties on the fly to see what that does:

You can do that on the Nautilus Preferences window and find that the dropsdowns are also Adw.ComboRow

[Moon-0xff]

the other things I noticed is that the drop downs in gnome-settings have more of a flat look to them with no borders

Looks a bit out of place. But if it does indeed give us less headaches than comboBoxes then it's a fair tradeoff.

I found an example here if that helps you

I can definitely see how we could implement after looking at it.
And if we can hookup the selection to a signal (and the reverse of that) then I think all the headaches are solved. I don't know if this is possible at all, but the hierarchy graph and the methods I found seems to allow such a thing.

The other solution would be to enclose comboBoxes in a wrapper, which seems like an easier solution.

[Moon-0xff]

I made this by referencing the link you shared and the comboBox function, it should be a drop-in replacement for the comboBoxes but it doesn't show a menu when clicked, it does show the selected option (I think) so something is working at least.

edit: note that these are ComboRows instead of DropDowns, they seem to be functionally the same. The above implementation might work if ComboRows are replaced with DropDowns (but probably not)

[Batwam]

yeah, if you replace Adw.ComboRow with Gtk.DropDown it shows the content of the list. It's strange that it doesn't work with Adw.ComboRow as they are meant to be equivalent and I prefer the look of ComboRow.

I have played around with the code a bit and have something which appears to be working. There are some edge issues (reset button appearing on startup) but generally it works. I'm still keen to keep a generic function to build the DropDown for 1, 2 or 3.

I haven't extended to the multiple drop downs yet but I agree with the idea of the single reset button for all 3 settings. One idea could be to have setting as an array and reset all items in the array in a forEach loop perhaps as part of buildResetButton()?

[Batwam]

ok, that's working pretty nicely for me. Main issue outstanding is the fact that the reset buttons always appear on startup but that shouldn't be too hard to solve.

you might also want to replacve the for loop which cycles through the settings for reset. I used it as it was simpler to implement and test in the short term.

I also haven't checked if the reset buttons at the bottom work as intended but since we have reet buttons everywhere now, we might want to remove them altogether?

[Moon-0xff]

I also haven't checked if the reset buttons at the bottom work as intended but since we have reset buttons everywhere now, we might want to remove them altogether?

There is a significant amount of settings for each page, and resetting each one individually would definitely be annoying.

Main issue outstanding is the fact that the reset buttons always appear on startup but that shouldn't be too hard to solve.

The logic to make the reset buttons appear is flawed, I tried to make them behave correctly but there wasn't a good way of making them work with comboBoxes.

[Batwam]

this last commit seems to fix it for the reset buttons on startup. I read through the doc and found settings.get_user_value() and settings.get_default_value() which greatly help figuring out whether the current settings are the same as default or not :-)

[Moon-0xff]

That's great! It doesn't react to the press of the page reset button though.
To solve that I'm thinking of re-structuring the code that handles the resets, or, make the page reset button virtually push all the individual buttons. Both are valid choices.

[Batwam]

yeah, getting the button to press all resets crossed my mind.

Note that I noticed that when change for example a switch and then put it back manually to the default value, the reset button doesn't go away which would be neat. I have implemented a solution for switches but extracting the default values and comparing is a bit tricky as each setting has a different type (integer, boolean, ...).

I have played around with another change which involves hidding the double click settings when the option is disabled in 46b8ce7. What do you think about it?

[Moon-0xff]

I have played around with another change which involves hidding the double click settings when the option is disabled in 46b8ce7. What do you think about it?

Well, the greyed out bindings serves as a prompt to enable the double clicks, which many people will probably want to do.

I suppose this change tries to keep the bindings as a single widget row, I guess that because I was trying to do the same.
My idea was to divide the bindings into two tabs, one for single-click and another for double-click, that would achieve both goals.

[Moon-0xff]

btw, I just discovered the other day that if you are in an app like gnome-center and click on an element, you can then press Ctrl + Shift + i to find useful info like this. You can even change properties on the fly to see what that does

It looks like it can be done in any Gtk application. Including our prefs window.

[Moon-0xff]

I tried and failed to store the default index of the dropDowns to trigger the reset painlessly. I will share the broken commit.

[Batwam]

I like the idea of saving this property, this is great. I've added a label for debugging and can confirm that the default index is indeed save so that's a start.

The dropdown reset loop doesn't appear to get executed though. Not sure why...

Btw, is it just me or it's not possible to get logs out of pref.js? I get that it doesn't loop but logs when the window is opened would be nice!

[Moon-0xff]

Btw, is it just me or it's not possible to get logs out of pref.js?

On my end log( ) works fine. I used it to work out what value was settings.get_default_value(setting) returning (a GLib.Variant object).

[Batwam]

Just needed to put the dropdown selection within the 'clicked' callback rather than the Reset button creation.

Still need to find a way to get these pesky reset buttons to disappear when the bigger button is clicked! It's not the end of the world and they would go away next time the window is opened but it would still be nice for them to go away...

[Batwam]

would you consider using a similar secondary_icon_name with 'edit-clear-symbolic' icon for addEntry rather than the reset button? or is it better to keep consistent with the reset of the widgets on the page and use the reset button?

[Moon-0xff]

would you consider using a similar secondary_icon_name with 'edit-clear-symbolic' icon for addEntry rather than the reset button? or is it better to keep consistent with the reset of the widgets on the page and use the reset button?

I think the context is subtly different. I wouldn't expect both buttons to behave the same way. Though I'm saying this after looking at the question and picture.

I suppose I wouldn't mind the slight inconsistency if it gives us an easier implementation, a considerably bigger text field, or any other advantage.

[Moon-0xff]

Last problems I think are better solved by re-structuring prefs. Not something to do on this PR though.

We should start to tidy up the code and get it ready for a merge.

[Batwam]

I'm not a big Glib.Variant expert and struggled to extract values from get_default_value the same way for all variant types but found that with .print(true) we can produce a string with the settings value which can then be used for comparison. This is handy as it allows generic code without having to manage individually the various integer/boolean/string settings. I'm not sure if there is a neater way to get the values in their original type using Object or Glib.Variant functions but this seems to compare Default Vs User values works at least.

please try this latest commit. it shoud clear the reset buttons for Switches/SpinButton/Entries by checking if the values matches the default. So if a user changes the album size to 100, then back to 60 manually, it will remove the button when it is but back to 60 as this matches the default.

The Reset buttons trigger the signals so it only needs to be coded once in the GtkWidget.connect() and all individual reset buttons get cleared automatically on page Reset.

It should also make sure reset buttons don't appear when the settings matches default when the window is first open (separate code which checks for match on widget creation).

I haven't tried for DropDowns yet but the same logic can hopefully apply.

[Batwam]

ok, I've implemented the reset button removal when the values match the default for DropDowns too. This made the section of code hiding the buttons on reset individual button click redudant so I removed that and did some general varaible name cleanup for consistency.

I believe that the original objective of the PR is now achieved although I would stil consider using the secondary icon for Gtk.Entry as mentioned above.

Just one other thing but I don't believe that it is direclty related to this PR. Is it me of the "use symbolic icon" option no longer works? I haven't looked at it in detail but I noticed that it wasn't changing when I switched.

[Moon-0xff]

I'm not a big Glib.Variant expert and struggled to extract values from get_default_value the same way for all variant types but found that with .print(true) we can produce a string with the settings value which can then be used for comparison. [...]

Thanks for the explanation. When I first saw the commit I didn't understand the use of .print(true)

I believe that the original objective of the PR is now achieved although I would stil consider using the secondary icon for Gtk.Entry https://github.com/Moon-0xff/gnome-mpris-label/pull/76#issuecomment-1716019237

I think it's a bit more consistent as it stands.

Just one other thing but I don't believe that it is direclty related to this PR. Is it me of the "use symbolic icon" option no longer works? I haven't looked at it in detail but I noticed that it wasn't changing when I switched.

Works fine on my end.

[Batwam]

Yep ok, for the symbolic icon, it looks like it must be because my Spotify package didn't have the icon as it works with Rhythmbox. All good although we could probably add "(if available)" to the description.

[Batwam]

I don't have much else to add to do PR. Any final comments/suggestions?

[Moon-0xff]

I think some comments will help a lot. But the not-so-clear-code is sprinkled and repeated in a lot of functions. I think something like a general statement as to how we access and reset functions, how we deal with dropdowns, etc can do the trick.

I'm not satisfied with the code. It's a bit hard to understand and it feels like it can be written in a less complex way, but I haven't come up with anything and I'm also getting lazy busy.
I blame GNOME, the root of the problems are with the API itself.

[Moon-0xff]

It might be better to add an explanation on HACKING.md.

As for the code I inspected several times now, and while I'm not satisfied with it I don't see a problem to it. I think I'll merge this now.