Version 4 Development

[Batwam]

added button to turn on/off in settings. checks the status and hides the text if option is set and playback paused.

[Moon-0xff]

Your code seems to be very on point, however it breaks the control flow of the program.

Some statements are called way too early, breaking some other functionality, specifically: When the current playback is paused, you can't switch to another one and possibly there's some other hidden bug there.

There's some other nitpicks to do, but in any case this is a great start! Thanks for your help!

[Moon-0xff]

Refrain from refactoring your code any further, i'm going to push changes to your branch. Alternatively i can merge the pull request right away. It's your decision.

[Moon-0xff]

That's my work for the day. Last commit breaks the extension. Try to debug my stuff if you wish! Thanks again. edit: Also, use tabs instead of spaces edit 2: Fixed it

[Moon-0xff]

Added a very basic install script for faster development, only execute it from inside the directory. Use GNOME-xorg and reset the session with Alt+F2 -> r for better effects.

[Batwam]

Refrain from refactoring your code any further, i'm going to push changes to your branch. Alternatively i can merge the pull request right away. It's your decision.

feel free to merge right away if you are happy with the changes

[Moon-0xff]

I'm going to respond to your last comment here.

I wasn't aware of this case. It would indeed switch as the code is currently iterating from last to first. We could iterate from first to last as I assume that the browser would be added at the end of the list and this would then be consistent with current behaviour?

Not relevant anymore. Current implementation (same as the old one) only iterates playerList when selected mpris source no longer exists.

Yes, when two apps are playing at the same time, the tool won't know which one to pick but this is already the case. I am not sure how often people listen to multiple songs at the same time though.

People with faulty or badly designed amplifiers/stereos/headphones often play a "silent song" to prevent their devices to turn themselves off or buzzing. Also the aforementioned 'browsing reddit or whatever while playing music' scenario counts as two distinct mpris sources.

What would be the expected behaviour if multiple sources are playing, I assume that you would need to pick one as I doubt you would want to list them all?

You need to pick one.

[Moon-0xff]

We are done, i believe. But now, we could implement your unintended feature as an option:

When another source starts to play, the extension jumps to the new one, this could be particularly annoying if you are browsing reddit or whatever, although some people will probably prefer this behaviour (new option?)

Using playerList and the new activePlayers list, it should be easy.

[Moon-0xff]

Made a quick implementation, it works, but you can't switch sources with the option enabled. Gonna finish it tomorrow, unless you beat me to it.

[Batwam]

Hey, thanks for this. I'll let you complete the updates if that's ok as I don't want to mess it up. I will test how it's implemented it once it's done but it's looking good so far with the new activePlayers list filtering out the non-playing sources :-)

[Batwam]

ok, I've fixed up an issue with the AUTO_SWITCH_TO_MOST_RECENT under _loadData() section returning an error if all players are paused which I believe you had picked up on already. I checked with the two new options on/off and it all appears to work as expected.

[Moon-0xff]

ok, I've fixed up an issue with the AUTO_SWITCH_TO_MOST_RECENT under _loadData() section returning an error if all players are paused which I believe you had picked up on already

No, i was not aware of this. Gonna keep it in mind. Better yet, i'm going to look for another way of getting a last index

[Moon-0xff]

It looks like Array.prototype.at() works in gjs https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/at

[Moon-0xff]

Commit 8b981d6 has a fix for 669ce40. Didn't mention it

[Moon-0xff]

Yesterday, trying to implement the auto switch option did more or less the exact changes. Plus a really complicated and sloppy way of sorting activePlayers by activity that didn't work and sprawled itself to every corner of extension.js

The commits of today are the working parts of it. Plus some improvements.

I wasn't able to come up with a good way of sorting activePlayers correctly. getActivePlayers returns the list sorted by spawn order, meaning the time when the application which contains the mpris interface was opened.

I want this auto switch option to jump to the most recent instance of a player going from X to Playing Sorting a list isn't the best way of achieving that, but it's a lot more flexible.

I'm also not a fan of how i ultimately implemented the trigger of the auto switch in commit 1d6dedc

[Moon-0xff]

This is(was) my approach:

1. Get activePlayers from getActivePlayers (once)
2. Detect when a player changes it's playback status to Playing comparing activePlayers with the return value of getActivePlayers and move it to the end of activePlayers
3. Repeat step 2 every cycle

[Batwam]

Note that I noticed that when I just start an app like Spotify and start playing music, the text doesn't appear. Only when the song changes (on it own or if I switch to the next song manually), the text start to appears. There is no issue if the player is open when i load the extension. I can't quite figure out what is happening but based on my investigations, this.proxy.Metadata[field].get_strv()[0] is empty when the app is just started so parseMetadataField(data) returns "". The player status itself does correctly report “Playing” if you log it so it’s purely with the Metadata.

I haven't tried the official extension but I tried with an older version of this commit I had backed up on the 31-Oct and the issue was already there so it's unlikely to be something which has been introduced recently. I’m guessing it’s some sort of initialisation issue somewhere.

[Moon-0xff]

A year ago when testing this extension Spotify returned at startup xesam:something and i had to manually filter it out in the code. It wouldn't surprise me if today this startup sequence of sorts it's faulty in some way or another. That's my theory at least. edit: Hang on, an app like spotify? Does this means you experienced this issue with another application?

[Moon-0xff]

Perhaps removing this from parseMetadataField and testing it again could reveal more information

    if (data.includes("xesam:") || data.includes("mpris:"))
        return ""

[Batwam]

Just to confirm, I have only experienced the issue with Spotify. As far as I can tell, data fed to parseMetadataField(data) is empty so the code stops are the previous check since data.length == 0. I fired up D-Feed to inspect the Bus info and the Metadata does appear to be loaded so the info is there but while getPlayerStatus(playerAddress) is returning the updated status (shows Playing if I click start the music), this.proxy.Metadata[field].get_strv()[0] remains blank so something doesn't seem to get loaded up...

edit: ok, I have transposed the code from getPlayerStatus to getMetadata and it's working now :-) I'll let you have a look and see what you think. I'm guessing there might be a simpler/neater way to make this work but it's only 2 lines and it's working as far as I can tell :-)

[Moon-0xff]

Nice catch!

[Moon-0xff]

Doing this changes to improve in all the D-bus stuff in the future edit2: also (probably) simplifies the code

[Moon-0xff]

Well now I'm finally satisfied with the implementation. It's not the best but certainly it's very flexible. The auto switch it's behaving exactly as i want:

I want this auto switch option to jump to the most recent instance of a player going from X to Playing

And it doesn't seem to spam the logs with warnings or errors. I don't trust it though. It definitely needs some serious debugging and a lot of testing. edit: Just from some casual usage noticed already some problems

[Batwam]

Looking good so far. The only warning I noticed is Usage of object.actor is deprecated for MprisLabel.

[Batwam]

ok, I've done a partial attempt to update the depreciated calls based on best-guess for the syntax by taking example from this pull request. It doesn't seem to have any adverse effect on the extension but the error is still there, possibly due to the use of .remove_actor(), not sure... some info here which might make more sense to you :-) if it's not deemed life threatening, feel free to ignore.

[Moon-0xff]

Updated it completely now. Guessing work honestly. Just changing remove_actor to remove_child goes well and changing extension place through settings doesn't throw any warnings or errors, however when disabling the extension it sometimes throws this.actor is null Using this.container.destroy instead of this.destroy makes the error appear less often? There's probably a better way though, perhaps super.*something*

[Moon-0xff]

Really i should have pushed it with a extended commit message so here's one: _init: Remove unnecessary lines _updatePlayerList: Fix the code and condense some loops _pickPlayer: Pick a player when no active players are present, and rename a variable for clarity _buildLabel: Changes for clarity

[Moon-0xff]

This one maintains both the ability to cycle sources AND auto-switching to the most recent source, however it only deals and switches to active ones. Not a bad trade-off.

edit: I have been testing it for a while and this version holds itself pretty good. So far i haven't noticed missing behaviour or any warning or error (from extension.js). there's three warnings in prefs.js. I don't dare to solve them because it could break compatibility with gnome 40 and/or 41. edit 2: Luck is on our side! It started to spam this.player is null

[Batwam]

See proposed update for the spam issue.

yeah, I agree with not going over the top on depreciated functions as this could have the unintended effect of breaking compatibility with older gnome versions. As they say, if it's not broken, don't fix it...

[Moon-0xff]

Ha! I didn't noticed that! Anyways, have you been running your latest commit? Any problems so far? Talking about problems the one about the extension throwing this.actor is null looks like it has nothing to do with the extension

JS ERROR: TypeError: this.actor is null
        _syncEnabled@resource:///org/gnome/shell/ui/windowManager.js:138:25
        onStopped@resource:///org/gnome/shell/ui/windowManager.js:150:35
        _makeEaseCallback/<@resource:///org/gnome/shell/ui/environment.js:150:22
        _easeActorProperty/<@resource:///org/gnome/shell/ui/environment.js:316:60
        _destroyWindowDone@resource:///org/gnome/shell/ui/windowManager.js:1596:21
        onStopped@resource:///org/gnome/shell/ui/windowManager.js:1564:39
        _makeEaseCallback/<@resource:///org/gnome/shell/ui/environment.js:150:22
        _easeActor/<@resource:///org/gnome/shell/ui/environment.js:239:64

As we were at the time fidgeting with the actor stuff i just assumed this had something to do with the recent changes to the extension. If that were the case i believe the log would be something like this:

JS ERROR: TypeError: this.actor is null
        disable@$HOME/.local/share/gnome-shell/extensions/mprisLabel@moon-0xff.github.com/extension.js

It could be that indeed it has something to do with the extension, but seems unlikely, and in that case the fault is on GNOME.

[Batwam]

Hmmm ok, yeah, I did notice that it wasn't going away despite removing the .actor stuff... should be ok then.

No issue on my side with the latest commit so far. All working as intended.

[Moon-0xff]

Well then, we are almost done here.

The question about using this.destroy, this.container.destroy or something else is still open. I believe it doesn't matter, but in the face of ambiguity it's probably wiser to revert it to this.destroy

As this latest version looks good and stable, and unless you want to contribute some additional refactoring, it's time to test it with some long casual use, i believe a week would suffice.

[Batwam]

Yeah, let's give it a week. I'll let you know if I encounter any issues. Feel free to update the code if needed and I'll update to the latest.

[Moon-0xff]

As the commit says, based purely on my own speculation. Here's my reasoning:

• rename function disable inside MprisLabel class to _disable gnome-shell (or something else i don't know) looks in extension.js for functions disable and enable. It can encounter them (at least enable) inside MprisLabel (see commit 28b96ae and 487e187) , so perhaps it's safer to not repeat those names
• Don't assign this.player to null shouldn't matter
• Call destroy from disable Already tested it, used in most extensions, sounds safer to call destroy from the outside?

Been testing it a bit, no logs so far.

[Batwam]

ok, no issue with the two new options enabled.

However, interesting behaviour I have noticed if "Remove text when paused" is disabled:

• start source 1, title appears. pause, title stays. all good
• start source 2, title appears (all good so far). pause, title goes away (it shouldn't), showing the title of the source 1

[Moon-0xff]

If you have the auto-switch option enabled, that's expected, it's part of the auto-switch trade-off mentioned above. Really it shouldn't, it should switch only if a source changes state from X to Playing (minding that a source can spawn already Playing). But i wasn't able to do it properly.

[Batwam]

Yeah, I see what you mean. Rather than monitor status change, would a possible trade-off be that it would switch only if the other source is playing?

[Batwam]

Please try this last commit. I'm simply avoiding to switch if nothing else is active which I believe is the "logical" way to handle this. I also tried with the auto switch turned off (something I probably wouldn't use) for completeness and it's working as expected too.

[Batwam]

Also, maybe something for a future version but I'll put it out there. The new "auto switch" is a solid feature. The only reason I can think of where people may not way to activate "auto switch" or be bothered by it would be if something like their browser is taking over the text while they have the music playing and the browser on mute. Do you think that it would be worth considering an option which would do "Ignore browser media sources" and filter out mpris sources like chromium? Not something which affects me personally but I'd be interested to get your thoughts on this.

[Moon-0xff]

It's on the menu, yes. A filter list for mpris sources. I'm not eager to start working on it though. Really i should write a new to-do list / help wanted sort of thing.

[Batwam]

Yeah, please make a list, I'd be really interested to see it!

[Moon-0xff]

Nice to know, going to add it as soon as I'm able.

[Batwam]

ok, all good so far with the latest version. By the way, when you click on the Reset button in settings, all settings (SpinButton, Entry) are set back to defaults except the ComboBox values (extension-place, first-field,...). It looks like Combobox contents aren't handled correctly, perhaps because the values are blank so the extension doesn't know which field to select?

[Moon-0xff]

Good catch, it seems comboBox values are correctly set to default, however the values selected in the comboBoxes aren't changed. Last commit addresses this. Just copy pasted a few lines. Does this happen in GNOME 3.x too? edit: Added the to-do / help wanted list to the main repo as promised

[Batwam]

I can confirm that this works now. I haven't checked gnome3.x but I can fire up the old laptop when I get a chance as I believe that it is running gnome3.36.

[Batwam]

ok, works fine in gnome 3.36.9.

One thing I just noticed as I was testing it by having a tab with Spotify and another one with youtube is that with the new auto switch, the title changes when you switch tabs (not play/Pause, just switching tabs is enough to make it most recent). so if you have spotify in a tab and Youtube in another, when a source is activated, it becomes "latest source" and the extension switches the text. If you go back and forth between the tabs, so does the extension which can feel a bit weird.

This is logical based on the code but I am not sure a reger filter would block individual tabs as they might both show as "chromium"? If this is the case, rather than a filter list, it may be useful to consider implementing something like "ignore muted sources" so if people have music playing and a watch a video on mute, it's still showing the song info. This should be easy based on the dbus info which gives a volume of 0 for mute sources. Maybe something for a future release...

[Moon-0xff]

ok, works fine in gnome 3.36.9.

Good to know, would be a shame if compatibility with GNOME 3.3x wasn't maintained

If you go back and forth between the tabs, so does the extension which can feel a bit weird.

I'm not sure if that's the extension, have you checked how many chromium instances are in playerList? If there's only one, it means the selection is made by chromium

it may be useful to consider implementing something like "ignore muted sources" [...] This should be easy based on the dbus info which gives a volume of 0 for mute sources.

Good idea!

[Moon-0xff]

As it stands now, the auto switch option is more annoying than convenient. I believe it needs the guidance of a filter list (both blacklist and whitelist) and perhaps some sort of priority list to be real useful. But probably just disabling or ignoring the web browsers mpris sources would be enough. Maybe we should keep it off as default. edit: for now

[Moon-0xff]

A new option for a fancier behaviour, at least i think so. I haven't tested it against any edge case yet.

[Batwam]

I didn't realise we were still allowed to make changes! I know that it's done now but if you feel that it's better with a 3s delay, it wouldn't bother me to see it being hard coded as the list of options is getting pretty long.

For the chromium tab thing, I had a look and the address in the same so it's chromium spitting out data based on whichever tab is in focus so that's going to be difficult to filter...

[Batwam]

quick proof of concept to filter out muted sources. Just an idea for now, feel free to ignore. When testing, i noticed that it seems like some sources like chromium don't actually provide the volume info? I'm getting a value of 1 regardless of the volume the tab is set at within Youtube/Spotify. If the tab itself is muted, the source goes away and so does the text anyway so maybe the volume based filtering isn't such a good idea after all...