Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
50 stars 9 forks source link

Double click feature #73

Closed michlampert closed 10 months ago

michlampert commented 11 months ago

Hi!

Sometimes I felt like I was missing buttons in my mouse (or touchpad) to trigger some options, so I decided to implement double click feature, which work similar to this in portable speakers or headphones.

After changes menu will contain additional options:

Before: image

After: image image

Moon-0xff commented 10 months ago

Hi @michlampert,

Thanks for the PR! This is a great addition to the extension!

And at a glance looks well implemented.

Just by looking at the code and images I have a couple of thoughts already:

michlampert commented 10 months ago

Thanks for your feedback @Moon-0xff!

I hope that changes will meet your expectations :smile:

Photo of menu for reference: image

Moon-0xff commented 10 months ago

I hope that changes will meet your expectations 😄

Indeed.

Normally I would prefer to avoid the ternary operator but it looks like a good fit here.

The changes to the menu are good.
For consistency I would underline "Single click" and "Double click" as "Mouse bindings" is underlined.
I would also put a blank space between the button bindings and the scroll binding (there's a commented line that produces a blank line somewhere).

edit: Didn't found it.

michlampert commented 10 months ago

image

Moon-0xff commented 10 months ago

Great! I don't have more changes to request. Good job!

I will soon inspect the code more closely and perhaps push to this branch, if isn't a problem on your end (is it?).

After that I will test it a bit and merge it.

michlampert commented 10 months ago

Sure! Thank you :smile:

Moon-0xff commented 10 months ago

I've added some inconsequential (or should be inconsequential) style changes.
I will add a few more tomorrow.

Let me know if you have any thoughts about, or if there's problems with my commits.
And don't be afraid of pushing!

Moon-0xff commented 10 months ago

I refactored the code to make it easier to understand (I hope).

The logic should be the same.

Moon-0xff commented 10 months ago

Alright, I think I'm done now. Please review the changes and if everything looks good to you I will merge it.

One thing to note, I didn't review on detail the prefs.js changes, it looks like it behaves like it should and we are thinking of redoing the whole thing (#67).

michlampert commented 10 months ago

Looks good to me :wink: I'm looking forward to see it in the next release!

Moon-0xff commented 10 months ago

Great! I will merge it then.

Thanks again for the PR @michlampert!