Simplify DropDown functions

Moon-0xff / gnome-mpris-label

A music related GNOME extension.

GNU General Public License v3.0

50 stars 10 forks source link

Simplify DropDown functions #78

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

I didn't manage to slip that one in before the merge!

Anyway, it wasn't directly related to the reset button anyway. I never felt liked the way we have 3 functions doing essentially the same thing so I thought that it would be a good idea to combine addDropDown(), addDoubleStringDropDown() and addTripleStringDropDown() into a grand unified function. I also simplified the code within the addDropDown to avoid repeated code in the process. All in all, this removed 60+ line of duplicated code...

Moon-0xff commented 1 year ago

Indeed this is a great simplification of the code. But I liked the 3 functions as an interface. The new, unified function requires far more syntax.

Can this code or a derivation of it be wrapped on 3 separate functions with the same syntax (parameters) as before?

Batwam commented 1 year ago

ok, I have created 3 separate functions which then call the generic function. The only thing these functions do as this stage is set the default width which avoids having to define it when calling the individual functions.

We could go further and add some more case specific code into each of the three functions but I see some drawbacks to this:

remove the need to use an array in addSingleDropDown. The array isn't really required so we could go with the simple string but we then have a different format for addSingleDropDown Vs the other two which feels inconsistent
in addDoubleDropDown, assume that the options for both dropDowns are the same. This would be sufficient in our case but would make the function less generic.

Essentially, we can certainly maintain the original syntax but this means that we don't have syntax consistency between the 3 functions which I feel is a bit of a missed opportunity?

Batwam commented 1 year ago

Saw the updates. Fine with me 👍

Moon-0xff commented 1 year ago

I don't mind the inconsistency between parameters.

Saw the updates. Fine with me 👍

Great!

Moon-0xff commented 1 year ago

I noticed the new functions names aren't the same. But the String part isn't helpful. DropDowns aren't well suited for boolean or numeric types.

Other than that I suppose there's not much else to discuss/add for this PR is it?

Batwam commented 1 year ago

That should be it. Please check just in case that the reset button still works and that changing the values does indeed update the settings. If it all works we should be good to go.

Moon-0xff commented 1 year ago

No problems on my end as far as I tested.

And in any case I want to push an update with the new changes soon, so it's time to test main more thoroughly.

Time for the merge!

Moon-0xff commented 1 year ago

Thanks again @Batwam!