Moon-0xff / gnome-mpris-label

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

Inconsistent tray position #17

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

as per comments on EGO and personnal experience, the icon doesn't always get loaded in the correct position in the tray. image

I have it set at index 0 on the right (with dash to panel extension on) and definitely experience this. My understanding is that while it gets allocated this position when loading, depending on the sequence of extension loading, other extensions may get loaded after mpris-label and will naturally go to the left of it. I can see somethign similar with the clipboard extension. it's in the middle when I normally log in, but if I turn it off and back on, it will go to the left.

image

I am not sure if there is a neat way to control this loading sequence. I was thinking perhaps some sort of delay but if it means that it just pauses everything, it may not help. Worth checking if adding 1-2s wait before applying the position while letting other extensions load would be possible?

In the meantime and as a bit of a test, I have added a line to force the update when the label is pressed just to make sure it's not an issue with the code. This works for me while we come up with a prettier fix.

Alternatively, we could place this._updateTrayPosition() strategically in a part of the code which is run infrequently but which we know would get executed for sure before any text appears (this way, the location gets updated before it becomes noticeable) like on a mpris source change.

PS: I also updated the version to 11 to avoid the extension getting automacally updated in the background. I noticed that the version on github is 7 but the one on EGO is 10 so I jumped to 11?

PPS: some potentially relevant info here https://bbs.archlinux.org/viewtopic.php?id=160505

Batwam commented 1 year ago

Ok, so this method works for me. There might be a prettier option which would involved getting the actual position Vs intended position and update if they don't match. I am not sure which one is the most taxing from a resources perspective.

Ideally, I'd keep this check out of the refresh loop and run it only once but we at least know that this works.

Moon-0xff commented 1 year ago

Hi @Batwam I feel like updating the extension index constantly could have unintended effects if other extension is somehow affecting the index order (intentionally or unintentionally).

So i'm going with this idea:

adding 1-2s wait before applying the position while letting other extensions load would be possible?

this is indeed possible (i think) using GLib.timeout_add and binding this._updateTrayPosition() as a callback

Batwam commented 1 year ago

Yeah, I agree. Technically, this is what the MR currently does as I also included a flag to make sure we are only updating once. If you can think of a prettier way to implement out of the refresh loop however, this would certainly be preferable. Depending on the number of extensions to be loaded, the delay may need the delay might need to be >2s but it most likely <5s.

Moon-0xff commented 1 year ago

Make "force tray position update on button press" an option, and the delay too. Don't worry about the growing numbers of options, i will try to "prettify" the prefs box in the next update. It's a good addition for the people that could need to tweak the parameters.

Moon-0xff commented 1 year ago

I would gladly stick around and help you with this but I'm going to bed right now. See you in the morning 10 hours or so.

Batwam commented 1 year ago

No worries, it's only 3pm here. I'll do the night shift then!

Batwam commented 1 year ago

Ok, I didn't do much coding but I had a think about general implementation. I don't really think that there is any value in making this optional considering that the button press would be needed just just once but every time the extension is loaded (ie on every login) which is frankly a pain. Also, in the future, we might want to use this button press for something else (display media data, open media app,...).

Have a look at the latest implementation as I might have confused you with the original button press implementation. I now have it to run automatically once in the first few iterations of _refresh and it works fine. If we can easily have it to run as part of init with a delay while other extensions finish loading that would be even better.

Moon-0xff commented 1 year ago

I don't think making the procedure optional is of any use. But a few people might need to tweak the delay value (2s, 5s, 10s...)

Moon-0xff commented 1 year ago

And the original button press implementation would be useful if other extension alters the index in the middle of a session.

Moon-0xff commented 1 year ago

I don't have this problem and I'm too lazy to install a bunch of extensions and test and i need to update the repository in other places so let me know if the re-implementation works. edit: my bad! apparently it breaks the extension edit2: fixed

Batwam commented 1 year ago

ok, I had a look at it's working fine. I simply renamed top panel to panel as it may not be at the top for everyone.

Regarding the duration of the delay itself. With 5s, there is a relatively noticeable gap between the time the extension is loaded and the time it is moved. If the music pplayer is already running, this means that you can see the label move which is a bit odd and can be avoided. Anything 3s and below is relocated before the planel is visible (in my case). What is interesting with this implementation is that it fixes the position even with a very short delay (0s even works) which means that you can't goo too short. This means that the duration is basically irrelevant as far as I can tell so we could even hardcode it at 0 or 1 and remove the option altogether. I've set it at 2 in the meantime as default value but we could just as well set it to 0.

Moon-0xff commented 1 year ago

In that case, 2 sound like a perfect starting value, maybe even 1. I don't think removing the option is a good choice, as slow hardware or a heavy init (with tons of extensions or very slow ones) might need more time than 1 second.

Batwam commented 1 year ago

Yeah, I think so too. Let's lock it in and see how it goes. If other extensions try to pull the same trick, being able to increase the delay might give us a chance to make sure mpris-label gets the last word 😁

Moon-0xff commented 1 year ago

I'm wondering if we can replace the button press update with an event listener or other asynchronous means of detecting if the extension index has changed and act accordingly.

Moon-0xff commented 1 year ago

If other extensions pull the same trick (both tricks) i'm picturing the top bar jumping all around the place. Preferably it should be a built-in way in GNOME for the end user to set up the preferred extension index order.

Batwam commented 1 year ago

Yeah, I did a bit of research on that (see forum article linked in first post) and it looks like it doesn't exist. Even the loading order seems to be somewhat random.

That's why I'm now leaning towards avoiding regular checks as it could get annoying for the user, combined with the Optional button press check.

Moon-0xff commented 1 year ago

OK, I guess the PR is done. I'm still thinking about replacing the button press thing with an asynchronous event listener or something that doesn't involve clicking the label, but to me it looks good already. What do you think?

Batwam commented 1 year ago

Yeah, I think that it's good to go.

Moon-0xff commented 1 year ago

Great! I will merge it in a bit. Thanks again @Batwam You should know that without your contributions this extension would still be stuck in version 3

Batwam commented 1 year ago

I'm glad I can help, hopefully we introduced more features than bugs!

Moon-0xff commented 1 year ago

That's the spirit!