Upcoming Version Roadmap (Version 13)

[Batwam]

@Moon-0xff ,
just so I can be clear on what's in/out in terms of features for the upcoming release and any future release when reviewing main:

• what features are to be included/excluded from the next release. Is is basically what is currently included so volume control / open player/label filter will only be implemented the following release?
• Are you simply looking to polish what is in the current main without additional features?
• Should I expect further commits to be added to main as you work through the open PRs?
• If you want to consolidate based on the current fetures added, is it too late to add forward/backward button to the drop down list? it's a pretty easy change and allows to add prev/next to the controls.
• Could we move Controls (click and later scroll) into a separate sheet under Settings? I still think that it would look neater. By the time we (hopefully) add backward/forward buttons + mouse scroll, that's 6 drop downs and the "Panel" tab is already a little busy.
• If you want to hold in implementing some of the other features recently implemented, what features will be pushed to the following release and what's your timeline for that?

No rush if you are still merging, I am just trying to see what to expect.

I am not 100% clear on the objective of the 'g-properties-changed' implementation as it's only partially refresh-less. The current implementation avoids doing the mpris request on each cycle but still requies the refresh loop for the label and players list. If we (you) wanted to, I feel like it should be possible to remove the refresh loop entirely. Would that be for a different version or you just don't feel that it's necessary? there would be a few ways to go about it but probably worth a separate discussion/PR.

[Moon-0xff]

The current main is what i want to submit as an update. I'm not looking to add further changes or even polish it (unless it's required or some things we added are too slow, unstable or something )

The next release will at least include the volume controls and will probably be released in a week or two, if it's too close to the GNOME 44 release then wait until we can test the extension on 44.

Currently the controls settings are too few to justify a new settings page (next release then)

I started working in a complete refresh-less implementation (more like a refresh-rate-less implementation), got very far but the extension expects a defined this.player (and probably some other things) in a number of places, this shouldn't be difficult to solve (i hope) but the required changes will create yet more conflicts with the PRs. It's best to do it later.

edit: I loaded the diff and checked the logs, it looks like i did solve the stuff about this.player and by removing the leftover this._removeTimeout call the extension loads correctly. Still it doesn't work and it looks like the signals aren't connected properly

[Moon-0xff]

I didn't make any commits so here's the diff against the latest main

diff --git a/extension.js b/extension.js
index 432b243..145ee38 100644
--- a/extension.js
+++ b/extension.js
@@ -49,9 +49,14 @@ class MprisLabel extends PanelMenu.Button {
 		this.box.add_child(this.label);
 
 		this.players = new Players();
+		this.players.connect('notify::list',this.players.pick.bind(this));
+		this.players.connect('notify::selected',this._refresh.bind(this));
+		this.players.connect('selected-update',this._refresh.bind(this));
 
 		this._buildMenu();
 		this.connect('button-press-event',(_a, event) => this._onClick(event));
+		this.players.connect('notify::list',this._buildMenu.bind(this));
+		this.settings.connect('changed::auto-switch-to-most-recent',this._buildMenu.bind(this));
 
 		this.settings.connect('changed::left-padding',this._onPaddingChanged.bind(this));
 		this.settings.connect('changed::right-padding',this._onPaddingChanged.bind(this));
@@ -153,7 +158,6 @@ class MprisLabel extends PanelMenu.Button {
 					this.player.goPrevious();
 				break;
 			case 'open-menu':
-				this._buildMenu();
 				this.menu.toggle();
 				break;
 			case 'next-player':
@@ -222,15 +226,9 @@ class MprisLabel extends PanelMenu.Button {
 	}
 
 	_refresh() {
-		const REFRESH_RATE = this.settings.get_int('refresh-rate');
-
-		this.player = this.players.pick();
+		this.player = this.players.selected;
 		this._setText();
 		this._setIcon();
-		this._removeTimeout();
-
-		this._timeout = Mainloop.timeout_add(REFRESH_RATE, this._refresh.bind(this));
-		return true;
 	}
 
 	_setIcon(){
@@ -268,13 +266,6 @@ class MprisLabel extends PanelMenu.Button {
 		}
 	}
 
-	_removeTimeout() {
-		if(this._timeout) {
-			Mainloop.source_remove(this._timeout);
-			this._timeout = null;
-		}
-	}
-
 	vfunc_event(event){
 		return Clutter.EVENT_PROPAGATE;
 	}
diff --git a/players.js b/players.js
index 0093d5a..1c16d64 100644
--- a/players.js
+++ b/players.js
@@ -34,12 +34,17 @@ const dBusInterface = `
 	</interface>
 </node>`
 
-var Players = class Players {
-	constructor(){
+var Players = GObject.registerClass(
+	{ GTypeName: 'MprisLabel_Players', Signals: {'selected-update': {} } },
+class Players extends GObject.Object {
+	_init(){
 		this.list = [];
 		const dBusProxyWrapper = Gio.DBusProxy.makeProxyWrapper(dBusInterface);
 		this.dBusProxy = dBusProxyWrapper(Gio.DBus.session,"org.freedesktop.DBus","/org/freedesktop/DBus");
 		this.settings = ExtensionUtils.getSettings('org.gnome.shell.extensions.mpris-label');
+
+		this.connect('notify::selected',this._connectSelected.bind(this));
+		this.dBusProxy.connectSignal('NameOwnerChanged',this._updateList.bind(this));
 	}
 	pick(){
 		const REMOVE_TEXT_WHEN_PAUSED = this.settings.get_boolean('remove-text-when-paused');
@@ -142,10 +147,17 @@ var Players = class Players {
 
 		this.activePlayers = this.list.filter(element => element.playbackStatus == "Playing")
 	}
-}
+	_connectSelected(){
+		this.selected.connect('update', () => {
+			this.emit('selected-update');
+		});
+	}
+});
 
-class Player {
-	constructor(address){
+var Player = GObject.registerClass(
+	{ GTypeName: 'MprisLabel_Player', Signals: {'update' : {} } },
+class Player extends GObject.Object {
+	_init(address){
 		this.address = address;
 		this.statusTimestamp = new Date().getTime();
 
@@ -180,6 +192,7 @@ class Player {
 			this.playbackStatus = playbackStatus;
 			this.statusTimestamp = new Date().getTime();
 		}
+		this.emit('update');
 	}
 	getIcon(desktopApp){
 		const settings = ExtensionUtils.getSettings('org.gnome.shell.extensions.mpris-label');
@@ -221,5 +234,5 @@ class Player {
 		if (this.proxy.CanGoPrevious)
 			this.proxy.PreviousRemote()
 	}
-}
+});

Mind you. This doesn't work and requires debugging.
Use $ patch < diff to apply the changes

[Batwam]

ok, I haven't tried the patch above but I'm sure we can make something work after the current set of features has been implemented and things settle now. I have a few busy weeks ahead of me myself so I'll have a look when there is a branch to look at.

The version in main looks like. Here are my comments:

1. One thing which puzzled me though is with the fact that the Settings are set to be on Left click by default. Would it make more sense to have Setting on right click and play/pause on the left? Settings menus are usually accessible from Left click.
2. When swiching player by clicking on the menu, it does take quite a while for the label to refresh, maybe it is because it's forcing this._updateList() which may not strictly necessary since the list is already being updated several times per second as part of this.players.pick()?
3. Interestingly, the Play/pause, Next, .. controls also work using when using the browsers on sites like Spotify or Youtube music, at least with chrome, Firefox, Opera, Chromium... (presumably all browsers which is neat.Sites like tune-in don't appears to work for play/pause but there's is probably not much we can do about that.
4. I tested chrome, Firefox, VLC, Rhythmbox, Brave, Opera, Shortwave, Chromium, Vivaldi on Gnome 43.1. They all behave fine, the Menu title is fine and the correct app icons now loads up for all of them so the new icon identification is definitely more robust than the previous hack.

All in all, it looks fine, I'd just have a think about the default for mouse click controls if I were you once it's set it will probably be best to leave these default unchanged for future releases. if that feels natural to you then it's all good as I'm sure that the list update in item 2 will get an overhaul with the refreshless implementation.

[Moon-0xff]

Now that you mentioned it _updateList() looks like a slow function. specially because the proxy for entryInterface is called without a callback, and that might even cause shell freezes.

I'm already solving the issue making dBusList update only when it's needed, merging entryInterface to mprisInterface, and making the _updateList code more lightweight.

This is not a small change to the codebase at all and it will require time.

Well... It's better to delay the update than possibly cause some shell freezes! 😄

[Batwam]

Yeah, that could be improved but considering that this is being run several time per second at the moment, I'm pretty sure we would know by now if it was causing freezes. I remember that when we investigated Shortwave and I checked the time taken for mpris calls, getting the dbus info for list was very fast.

The real reason for the lag is that we have to wait for the next refresh for the info to be updated.

[Moon-0xff]

I tested it with shortwave using the time measuring code for #12
it shows around 195ms when retrieving the identity info.

By default identity is retrieved once so there's any noticeable problem, but when using source filters identity is retrieved every refresh so the shell gets a short freeze every refresh.

[Moon-0xff]

I'm still solving the issue and the update is well on the way now, i haven't been using git properly so i can't share a branch but here's a diff

[Batwam]

sounds good. I guess you can always create a new branch and apply the patch once you are done.

Just as a reminder as I look at this line, this.desktopApp = matchedEntries[0][0] doesn't always return the correct player's .desktop so you might want to have a look at dc17bc0. I also realised after the commit that I was only cross checking against running apps for the result of Gio.DesktopAppInfo.search(this.desktopEntry) but we should also run the same check to the result of Gio.DesktopAppInfo.search(this.identity) to make sure it is also part if the running apps.

This is useful to make sure the correct .desktop is selected so we can open the app on a click of the menu

[Batwam]

ok, I tried in gnome 44-beta using Gnome Nightly and there is no major issue. The only thing I noticed is that the Button Placeholder character didn't seem to get recognised:

it looks like an encoding issue. Perhaps we could use the regular _ (or even a double __ if you need something longer) instead of ＿ ?

For clarity, this is based on the current unpatched main. I'll recheck once main is settled as there seemed to be issues with loading the player names (Spotify below). I doubt this is Gnome 44 specific (D-Feet shows the info so it's not that it's missing, just a refresh issue)

In a nutshell,there shouldn't be any issue in Gnome 44 we aren't already able to see in 43.

[Moon-0xff]

The missing text looks like a font issue, probably not coming from GNOME, maybe missing files/packages.
Either way it looks like updating for 44 will be just a matter of adding 44 to metadata.json so that's great!

[Batwam]

Yeah, they actually publish a guide for extensions to know what the changes are, it's quite nice of them: https://gjs.guide/extensions/upgrading/gnome-shell-44.html

With regards to the Backgrounds app section, I noticed that app like Rhythmbox and Amberol keep running even when their window is closed (which is quite annoying if you ask me...). I wonder if they are included in the get_running() apps list or if we would need to check against the list of background apps as well. That would be worth checking when you include the "Open Player" code. If the background apps are included that would be neat as it means that you can close the window completely but still control and reopen the window using the extension.

[Moon-0xff]

But hopefully amberol and rhythmbox implements Raise so we don't have to implement such a workaround for just two non-conforming players.

Regarding the state of the update, here's another diff.
This time if applied it works correctly, there's some warnings/errors raised in the logs and _buildMenu sometimes stops working but overall it's almost finished (i think).

[Batwam]

I'll let you have a go at Raise(). I implemented it, it works well with Amberol, however for Rhythmbox, it just shows a notification saying that the player is ready...

In terms of players supporting it, it's a bit of a mixed bag also:

This extension has it (see row 270) but it's only implemented as a fallback to the .desktop lookup method. Quite frankly, I think that implementing Raise is a bit of a waste of code as it's inferior to what we already have as far as I can tell.

[Batwam]

as a follow up, I checked how the _activatePlayer() function would worked with Amberol and Rhythmbox when the window is closed. They are not included in the list of running apps so it would skip the first method as well as the fallback which cycles through the various open windows.

It does however re-open closed windows just fine on playerObject.activate() which is quite useful. So, it's perhaps best not to cross check against get_running() in the end, especially now that we have a fairly robust way to make sure we are selecting the correct .desktop file.

[Moon-0xff]

Yes, i was aware the code in #26 was better than a Raise implementation, i was hoping it could serve well enough as a fallback.

Tweaking the code for a more general solution (if it can be done) would be a lot better.

[Moon-0xff]

Patch finished, committed and pushed. I have some confidence that the problems from the past patches are solved now. I can't recall if i tested the filters though.

We could test the freshly pushed main thoroughly and submit it as an update or focus on #26

For #26 I'm thinking of "rewinding" the branch to 7a87d03 and work from there, trying to merge the volume controls part first.

[Batwam]

Thanks for the update. For #26, since the base code has been updated, I think that we might as well close it and either reimplement changes by copying code by hand or do some cherry pick of commits if it works.

I can create a branch if you need help, otherwise I'll let you implement whatever functionalities you need. Most of the code required should be available in the code from #26.

On the volume side, I was thinking earlier today that it might be best to keep things simple and simply control the global volume since the source-specific volume control involves some arounds. We can have mouse scroll Global Volume or Off for now. This would remove the need to the section the code where we try to work out if a source can control volume or not. If we really feel the need for it and we manage to have a clean way to reliably manage individual sources in the future (including browsers), then we can add it to the list in a future release.

I've simplified the function to open the player in the last commit. The main thing is to fix the .desktop identification which used to assume that [0][0] is correct. Once this is fixed, the correct .desktop files will be selected and the .activate() will work reliably. You can even remove the part which cycles through the windows.

[Batwam]

In terms of what gets included, I think that it would be nice to include the volume control since it was requested by a user. If we keep it to global volume and ignore the mpris volume, it's pretty straightforward and robust. Enabling Open Player is only a handful of lines if we ignore the fallback code.

I believe that these are the only features from #26 which aren't in main? If that's the case, we can probably implement them in a few days and include in the next release.

[Moon-0xff]

So making a more basic implementation of #26, although more robust. Sounds good.

I did some work to implement the volume scroll, i did include the mpris approach as i think merely adding the global volume controls it's a bit of repeating functionality, the shell does this already with the volume widget on the top panel.

I will push a branch soon enough.

[Moon-0xff]

I did some testing and now I'm not really happy with the mpris approach. Works well for the players that implement the method but that's all it does. Just using it a few minutes and already I'm starting to rely on it. Even if I shouldn't because it doesn't work for web browsers.

What if we try to implement a per-application volume control the way gnome does it? By inspecting volume.js it looks like the bridge between the underlying audio interface and GNOME is this object

[Batwam]

Yeah, I hear you about the global volume control being a bit of a duplicate. This is what drove me to the source specific control initially. It is however more simple and the label being larger, I'm finding I use it a lot more than trying to locate the mouse on the small speaker icon.

MixerControl is already what is being used in PR#26 to modify the global volume. I am not sure how the individual implemented in Gnome-Settings, it looks like it has something to do with streams.
https://github.com/GNOME/gnome-control-center/tree/main/panels/sound

It would be great if there a simpler way to modify the source directly like in Gnome-Settings, possibly by-passing the mpris control (and custom function to work out which source can/cannot be controlled using mpris) with a more generic way to do individual app volume control entirely through MixerControl/Stream

If the simpler method above is possible, we will still need to reliably link the player source as listed in the extension with the individual app/stream we want to control (a browser might have multiple sources). Hopefully the apps/stream are identified using the mpris source name.

It would be worth figuring out how Gnome-Settings does it. Alternatively, the method implemented currently in PR#26 works, it just feels like there should be a simpler way to achieve it...

[Batwam]

This extension should have some relevant code too: https://github.com/mymindstorm/gnome-volume-mixer

As per previous post, it seems to be broken for Gnome 43 but it might have more to do with the menu than the sound/stream parts.

[Batwam]

I was looking at the current main and noticed that the updated label filters weren't in it. Will you be implementing this before the next release?

[Moon-0xff]

Probably not. #25 is not finished(almost) and I prefer to include the features from #26 first.

My sort of priority list would be something like this:

1. Volume controls
2. Open player window button
3. Something else perhaps
4. Label filters

I don't have any goal for a release. We already have enough to justify a release so probably soon.

We have a sort of deadline on the last week of march because 44 release is scheduled for the 22 and after the big distros start to update their version we will have to submit a new release for compatibility support.

[Batwam]

ok, see #29 for item 2. Let's keep things simple, work through the list as separate PRs and merge the functionalities one by one.

[Batwam]

not sure if you saw the anouncement but EGO now shows the number of downloads:

If will be interesting to see the number of active users based on the jump when the next version is released.

[Batwam]

I'm posting this here as it's not related to the volume control PR. Did you have a think about adding backward(8) and forward(9) mouse button to the Pref.js list since we now have quite a few options to chose from in the drop down list? I'd be glad to use them for Previous/Next. It looks to me like they are pretty common these days.

[Moon-0xff]

Yes, that would be a good addition, although it strikes as weird that the buttons are written in code as 8 and 9. Have you rebinded your mouse before? Can you test if you get the same result with another mouse? (I can't).

[Batwam]

I have a cheap and a more expensive Logitech mouse and they both map backward/forward to 8/9. Clutter doesn't seem to have generic names for these buttons. Left/Middle/Right map to 1/2/3 (don't quote me on the order). I'm not sure if other mouses/mice have buttons which could map to 4/5/6/7. That would be good to know. Do you have these buttons? If so, what number to they correspond to?

If we wanted to make it more generic, we could have a drop down with numbers from 4 to 9 which the user could select to try an manually map I guess? Either that or some sort of super fancy dialog where you click, it recognises the number and sets it like with the shortcut recognition thing.

[Moon-0xff]

I do have those buttons but they are rebinded. In any case if two different mouses both map to 8 and 9 that's good enough evidence for me. By the way i have them rebinded to media previous and media next so you might imagine what's my suggestion for the default bindings 😉

[Batwam]

Yeah, generalising based on a sample of 2 seems more than appropriate 😄 hopefully that follows some sort of standard. That would seem fairly logical as they would normally be used to go forward/backward in browsers without requiring any specific mapping.

I haven't touched the mouse settings as the app doesn't exist on Linux so it's as set out of the box. I had them linked to previous/next in the previous PR as well, that seems fairly logical.

[Batwam]

Extra from Arch Wiki:

For most mice, this will be '1' for left button, '2' for middle, '3' for right. Other buttons will vary (e.g. for an Logitech MX Master 3 the scroll wheel is 4 & 5, thumb wheel is 6 & 7, the thumb-tip button is 9, and the inner-thumb button is 8).

As far as I can tell from other pages, this is pretty much the standard for modern mouse.

[Moon-0xff]

I haven't touched the mouse settings as the app doesn't exist on Linux

https://github.com/libratbag/piper

[Batwam]

ok, let's implement buttons 8/9 once the volume control PR is merged.

[Moon-0xff]

Merged #30.
For the 8/9 buttons i did push a branch with the bindings implemented, but I can't test it on my end (Well I can but I'm lazy).

It's time to test and polish the main branch and get it ready for a release. 44 releases in less than a week so we can wait until the update reaches distros and update accordingly.

[Moon-0xff]

As mentioned before, 44 releases very soon and distros will soon follow.

In terms of what i intend to include before release:

• #38 which is a simple change
• (Discarded) A fix for #36, if we can do it before the big distros start to update

#34 is out. Is not even close to being finished

From casual use i haven't experienced any crash (except in prefs as mentioned), and the logs have been silent since last pushed commit, but additional testing is still necessary, specially some stress testing.

[Batwam]

Sounds like a plan 👍

[Batwam]

results from testing on Gnome 44 in Gnome Nightly:

• Actions tested: mouse controls, volume control, player filter, auto switch, menu, open app, icons
• no issue with all players tested: Chromium, Google Chrome, Music, Spotify, Brave, Amberol, Shortwave, Firefox, Elisa
• With Switch Automatically turned on, some apps like Spotify or VLC would show in the menu but wont' be selected as active player when the music is started. VLC will only start displaying track info when I stopped playing and start playing again. For Spotify, the track info start to show when I click next or it switches to the next song. I think that have actually noticed this occasionally even on Gnome 43 when starting Spotify. Some players seem to load without any song info or there is a timing issue with the asynchronous loading of song info?
• no error in log
• Web/Epiphany does not appear as mpris source but I suspect that this is a browser issue
• I am still having this issue with the default paceholder not being recognised . Recommend to replace ＿ with __ or even 'Click to switch'
  

[Moon-0xff]

I am still having this issue with the default paceholder not being recognised

This is a font issue (missing fonts) and we shouldn't worry about it, can confirm.

[Moon-0xff]

VLC will only start displaying track info when I stopped playing and start playing again

Last time I checked, VLC starts as Stopped and doesn't update itself on startup. I failed to mention this at the time.

[Batwam]

I have the feeling whatever fix we find to reload empty metadata for Spotify might help the VLC issue. VLC isn't exactly core app people would expect to use to listen to music as it's more of a video player in my view so probably lower priority.

[Moon-0xff]

We are ready for release. #42 and #43 are unsolved and we should fix them soon, but main is in pretty good shape and i feel like we delayed this update too much already.

[Batwam]

Ok 👍

[Moon-0xff]

Great, the version got accepted, no changes requested.

[Batwam]

Impressive considering the volume if changes. You're getting good at this!

I'm still a little concerned with the Spotify regression to be honest. As this is one of the main apps to be used with this extension, we don't really want people's first experience to be that it's giving nothing. I guess Auto Switch is disabled by default at least.