search

Moon-0xff / gnome-mpris-label

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

Hide the label completely #69

Closed abisammy closed 1 year ago

abisammy commented 1 year ago

This implements the changes requested in order to hide the label when changed, mentioned in https://github.com/Moon-0xff/gnome-mpris-label/pull/68.

This branch can also be used to implement the auto switch option...

The difference between this is and the previous solution is that it uses the new optimised event system.

Moon-0xff commented 1 year ago

Looks pretty good! I will take a closer look to the code and test it later but at a glance I might accept it without further changes!

Just to nitpick there's an extra newline on 388.

Moon-0xff commented 1 year ago

Remove the timeout in disable
Like repositionTimeout

https://gjs.guide/extensions/review-guidelines/review-guidelines.html#remove-main-loop-sources

Moon-0xff commented 1 year ago

I paused a player to listen to other, but the button disappeared!

It should display the placeholder string, and keep the button on the panel.

Another one:
It looks like the player can't be switched.
edit: bug already present in dev-branch
edit2: might be better to solve it in another PR, unless the fix is trivial

abisammy commented 1 year ago

I paused a player to listen to other, but the button disappeared!

It should display the placeholder string, and keep the button on the panel.

Another one:
It looks like the player can't be switched.
edit: bug already present in dev-branch
edit2: might be better to solve it in another PR, unless the fix is trivial

The bug where button disappears is trivial, sinply need to set text to placeholder instead!

Will work on it later

Moon-0xff commented 1 year ago

Works very well! I will probably rewrite the last commit a bit as it uses javascript things that aren't so clear. edit: by unclear javascript things I'm referring to default parameters

Moon-0xff commented 1 year ago

Well I can't come up with a better solution on the spot. Any ideas? If not I'm happy to merge this regardless, not a big deal.

abisammy commented 1 year ago

Well I can't come up with a better solution on the spot. Any ideas? If not I'm happy to merge this regardless, not a big deal.

We could split it into three functions...

setTextBase(placeholder)
setText() -> calls setLabelBase(false)
setTextPlaceholder()  -> calls setLabelBase(true)
Moon-0xff commented 1 year ago

Sounds more complicated!

Moon-0xff commented 1 year ago

I suppose we could use clearTimeout to clear the timeouts on disable.
And setTimeout to create this._repositionTimeout.

but setTimeout is used just once on gnome-shell and clearTimeout never. I wonder why.
I also can't find where are they declared, I searched in the gnome-shell repo and gjs. Probably it's a binded C function.

edit: It's a built-in javascript function edit2: I expected to be a built-in helper function declared in gnome-shell or gjs edit3: the code in _disable probably needs to be rewritten edit4: I wonder if using those functions is against guidelines

Moon-0xff commented 1 year ago

I replaced setTimeout with GLib.timeout..., it's probably the better option.

Last commit is a partial commit, we still need to replace the call to clearTimeout

Moon-0xff commented 1 year ago

ae9c527d4239da7e0782c9cc72dc59c142d693c6 commit message is wrong, I reverted label.js to 9afb6190258adc61604a6dae43e113e38f578c6d

And the second line is more relevant than the first one.

Moon-0xff commented 1 year ago

I tried to call hide and show directly but journalctl shows a lot of warnings.

There's repeated code when disabling the timeouts, that might be better to solve in another PR.

I think the branch is now ready to be merged. What do you think?

abisammy commented 1 year ago

Yep look sgood to me!

Moon-0xff commented 1 year ago

Great!
Thanks again @abisammy !