Implement Mouse Controls (Superseded by #29 and #30)

[Batwam]

As suggested in #4, see proposed implementation to control the volume of the active mpris source by scrolling up/down on the label.

[Batwam]

updated to include an option to control the global volume rather than the active source only. I tried to match the default volume step so it feels the same as scrolling the normal volume slider in quick menu.

Global Volume (replicates the default OSD):

Active Source Volume (include source name):

[Batwam]

Regarding sources not giving access to Volume, I'm not sure how to use Object.keys. I haven't found any source not providing Volume so far so if you have one, please let me know so I can test. What I noticed which is a bit of an oddity is that chrome allows to change the volume via mpris interface but while it does return a value which goes up and down on request, the change doesn't have any impact on the actual volume coming out of the speakers...

At the same time, the Settings panel does have a slider to Chrome (one per app actually) which does affect the volume level but doesn't match the volume level provided by mpris... I'm not sure what it's controlling but I did see that gnome-volume-mixer extension which allows to control individual apps is broken since Gnome 43 and the changes in the Quick Menu.

So, chrome looks like it's working since it provides a volume value and changes it on request, but in reality it's not. It's a bit difficult to exclude it without hardcoding an exclusion for "chromium" which could be done but isn't very pretty... We can probably use 'audio-volume-muted-symbolic' to indicate a source control which doesn't work. However, at the moment, chrome behaves like it's working, it just happens to do nothing when the volume is changed... unless the Object.keys can shed some light on this?

[Batwam]

ok, I checked with some extra players. Spotify, Celluloid,Shortwave, Rhythmbox are all of with mpris volume control. Firefox and Chrome give a volume which can be changed but it does nothing to the actual sounds.

I also noticed that if I go into a terminal and type dbus-send --print-reply --dest=org.mpris.MediaPlayer2.firefox.instance11471 /org/mpris/MediaPlayer2 org.freedesktop.DBus.Properties.Set string:org.mpris.MediaPlayer2.Player string:Volume variant:double:1 , I get org.mpris.MediaPlayer2.Player:Volume setting is not supported which could be used if we could get a similar info through the extension. Chrome on the other hand responds as it everything was fine.

Edit: ok, I got something out of Object.keys on this.proxy but it basically tells me that I have 3 items and lists the 3 interfaces we defined under mprisInterface so not exactly ground breaking.

[Batwam]

Ok, new update. I noticed that firefox will initially return a value null for this.proxy.Volume so I can make use of that to identify that we don't have volume control for that source. For Chrome however, it will return 1 initially which is a little bit tricky to exclude as it's a realistic value. I think that I might exclude a source if the value is 1 && address includes instance.

I could probably make it even simpler and exclude any source with instance in its address as it's a browser and it doesn't look like browsers work anyway but it might be a bit too broad and exclude sources which might otherwise work... or exclude if the volume is 1 and the address includes chromium... not sure... Any thoughts?

[Moon-0xff]

I checked and tested the code a bit. Implementation wise looks good, I'm not too familiar with events so i need to check more on that.

Style wise it looks unnecessarily abridged. I would prefer a slightly more verbose style. More inline with the rest of the code.

I suppose this is heavily influenced by the code of other extension. Care to share a link?

[Batwam]

yeah, it's a bit of a patchwork at the moment. I have used a little bit of the following and a few other ideas from the documentation to work out what the relevant commands were:

• Volume Scroller for the mouse scroll detection and OSD idea
• Volume Scroll for the Global volume but really only a line or two to work out how to control the global volume with get_default_sink().

Are you referring to the scroll detection in particular? Note that the second extension has a slightly different implementation of the mouse scroll which you might prefer (see row 55-64).

One thing to note is that also I tried to mirrorred the volume setting between Global and Source in the hope to combine them eventually into a single function. For instance, the VolumeMax for mpris sources is always 1 so we don't technically need a variable for this but it allows to use the same code for the following lines. We could probably summarise each section in 4-5 lines if they are going to remain split.

[Batwam]

Ok, I have re-written the section more based on the second extension's syntax. I also implemented the fallback with some sort of semi-smart solution to try to work out if a source is able to handle volume control or not. If it's source only and the source isn't capable, the OSD will show a message to explain that the source isn't compatible (so people don't think it's the extension's fault).

[Batwam]

It's all working pretty nicely as far as I can tell. In practice, I don't know if it's useful to keep "Source Only" mode without fallback since the alternative with fallback does the same thing but also works with browsers. It feel like Global sound would be the most popular which is why I set it as default but I still like the Source with Fallback mode as it allows the easily fix the volume individually for each.

I still don't know how Gnome controls chrome volumes. I can see that multiple sliders appear if there are multiple tabs with sources. These sliders are certainly doing something but it doesn't appears to be using mpris.

This reminds me of the whole DesktopEntry data in dbus. It looks like it was going to be great, until we realised that it was only partially implemented by the various apps and we had to develop a workaround... by the way, the Spotify slide in Settings goes move when I change the mpris volume by scrolling. It's just the browsers which seem to be managed differently.

[Moon-0xff]

Read the mpris specification (the freedesktop website one). I remember there's methods to know the mpris capabilities maybe there's one for the volume controls.

Gnome might have it's own way of controlling the audio, and if it's part of the shell then we could import it to the extension and use it.

Other way i can think of is by sending commands to the audio interface directly (pulseaudio,pipewire,jack) but that's too much for a workaround.

[Batwam]

Yeah, I really don't want to get into the weeds with pulseaudio/pipewire either! I am not sure how it works in Gnome but there are extensions which give you a menu so adjust sound for various sources so the solution must be out there somewhere.

PS: maybe even here but the fact that the extension is now broken kind of concerns me as we don't want to have to fix the extension every time there is a gnome update... hopefully it's the menu part which is broken and not the volume controls. https://github.com/mymindstorm/gnome-volume-mixer

[Moon-0xff]

That extension imports this code i haven't read it but might be the way gnome controls the audio.

[Batwam]

Yeah, this is the same library I'm importing to control the global volume. I'll have a look to see if this is what is being used to control individual sources too.

The problem we might have might be if the volume source is identified differently from the mpris source and we need some messy hack to transpose. If we are lucky, they are identified with the same address considering that there seems to be a slider appearing not just for the browser but for each source, well see...

[Batwam]

ok, I have found how to isolate Left/Middle/Right click. it was a bit fiddly as the menu kept opening but I got there in the end. I have also implemented play/pause toggle on left click and I'm finding it pretty useful. Ultimately, we could have a drop down in preferences for people to decide what they want to do with each button :-)

Other buttons are also recognised and additional functions could added, for instance, I have buttons recognised as 8 and 9 ("previous" and "next") on both of my mouses which could used for prev/next song.

by the way, do you know what the .bind(this) part in this.connect... of extension.js does? I deleted it for the button click and can't seem to see any difference.

[Moon-0xff]

That's great! I believed the menu implementation would completely deny the possibility of using left/right click for something else!

Regarding bind and connect I don't know.
I know one comes from standard javascript and the other from GObject.

[Batwam]

ok, merge conflicts all resolved so that's good.

With regards to figuring out in a neater way which sources can/cannot accept volume change, I've come accross functions like CanPlay as part of this latest implementation which is quite handy to check capabilities. However, it doesn't appear to be anything like CanSetVolume which would have been quite handy

I do note however that the spec says that if Volume is changed, "the signal org.freedesktop.DBus.Properties.PropertiesChanged is emitted" https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume I was thinking that perhaps, if we set the value to itself and the signal isn't emitted, this could be an indication that the source doesn't accept volume changes?

Do you know of a way to pick up this signal? More generally, this sounds like a way to remove the refresh altogether and only update when DBus tells us there has been a change perhaps?

[Moon-0xff]

PropertiesChanged is emitted when any property is changed so that might give lots of false positives/negatives. Is worth a try though.

It is the way of making this extension refreshless, plus the one for the DBusList. For a working implementation checkout gnome-shell's mpris.js

I hope think an implementation of this for the extension isn't difficult. We merely haven't tried. That's another thing for the list.

[Batwam]

yeah, I was just looking to see if I could set the volume to itseld and pick and the DBus signal. I'm looking at combining the following two:

• PropertiesChanges signal: https://dbus.freedesktop.org/doc/dbus-api-design.html
• connectSignal (scroll to the bottom): https://extensions.gnome.org/review/7884

It's not working (extension works fine but no signal emitted as far as I can tell). I'll push the code if you want to have a look. Also, I have tried but I haven't figured out how to merge the mprisInterface/entryInterface and dBusInterface/dBusPropertiesInterface

[Moon-0xff]

I think the signal "name" is different. Search in the mpris.js file something like propert that might point you to the exact code to use

[Batwam]

That's how it's called in this example and when I look in D-Feet:

but looking at the extension you linked above, I see that he is using this._playerProxy.connectObject('g-properties-changed', () => this._updateState(), this) instead, direclty on 'org.mpris.MediaPlayer2.Player' apparently. it's loading so much extra stuff it's a bit of a maze..

[Moon-0xff]

DbusUtils just includes a few helper functions as far as i can tell. Don't get too confused with the fancy and professional looking coding style. You might get a good grasp simply by reading the function and variable names and reading the code as it were plain english.

Edit: I didn't read your last message 😅

[Batwam]

ok, I've got it working now (just outputs a message in the log for now...). Turns out it was a one liner...

Next thing would ideally be to know which property actually changed which I was hoping to get from the previous implementation as it had <arg type="a{sv}" name="changed_properties"/>

I can also confirm that the signal is emitted if I change the volume on Spotify but not with Firefox or Chrome :-) Next step is to find a way to monitor this signal when trying to set a volume...

[Moon-0xff]

Maybe populating the parentheses (storing the return values in a variable) in the arrow function thing might give some extra information.

Change () => callback to (something,something2) => callback

I'm not quite sure if that's real javascript though, maybe i'm confusing things.

[Batwam]

I have a working version but there is probably a neater way to implement it:

Yeah, I tried to put info in the brackets but it returns undefined. need to dig further... I left the logs for testing purposes

[Batwam]

ok, I had a go at trying to manually implement a way to identify what changed by keeping the old values and comparing to the new for Volume, PlaybackStatus and Metadata. I've left the commented out logs for testing purposes.

It's not doing anything at the moment (except setting the canSetVolume property) but eventually, we should trigger label updates from _onPropertiesChanged, perhaps excluding cases of volume changes if we want to make it fancy, and make it essentially refreshless.

This said, if we are worried picking up changes manually might not be robust enough, a label update on each properties-changed signal would still be a significant reduction in the number of cycles.

[Batwam]

here is what I'm thinking about so this PR doesn't turn into a 3-week rewrite of the extension.

I think that while we are now closer to a refreshless implementation, we could keep that to a future PR. At the end of the day, it's not like the extension sluggish and it may not be as straightforward as we thing. Also, we still have the open PR on filtering which should presumably get merged first and I wouldn't want to spend too much time fixing merge conflicts. Actually, do you intend to merge this or the filtering changes first?

Anyway, my plan for this PR would be to implement a couple of actions like PlayPause and Menu we already have but also some basic one like Previous, Next. We can them put these in drop downs in Settings where people decide what each PRIMARY/MIDDLE/SECONDARY button to (I might add buttons 8/9 too if I can find a proper name for them).

That should do and this way, we can always add extra functionalities like opening the app to the drop down for people to select in the future without breaking everything.

Regarding controlling the browsers tab with whatever way gnome handles it, I am not sure how to implement it (yet). What we have is Global/Source with Fallback/Source without Fallback. Would that be enough for a merge?

[Moon-0xff]

Yeah, that sounds about right.

I don't have a preference regarding what PR gets merged first, and the stuff about controlling the volume the way gnome does it can be done later if it's too much trouble.

[Moon-0xff]

the extension wasn't loading on GNOME 3.38. Turns out connectObject is a rather recent addition to the shell. Checkout this commit and this merge request for some information

[Moon-0xff]

We can match players with running apps by matching desktop entries

[Batwam]

Nice. Would app.activate() also be the key for us to be able to launch the app on click?

By the way, I noticed that with the mpris sources listed in the Gnome notification area, clicking on Firefox will launch a new instance of firefox rather than the tab where the source is running so the Gnome implementation isn't perfect either.

[Moon-0xff]

activate_window looks like it does what we want.

[Batwam]

Ok, that makes sense since the app should already be open.

[Batwam]

would you know if Shell.AppSystem.get_default().lookup_app() looks through running apps only? If so, I would remove the get_running() check as it would be redundant.

also found some info on AppSystem capabilities here: https://www.roojs.org/seed/gir-1.2-gtk-3.0/seed/Shell.AppSystem.html

Edit: is seems like lookup_app() may not work for snaps (doesn't work for Firefox on my machine)... I'm commenting out the changes pending further testing.

[Moon-0xff]

this.metadataString breaks the extension if the source doesn't displays all three fields. I suggest to use buildLabel in a bit of a cheat move, might as well build the label string in players.js

[Batwam]

Interesting, I didn't come across that. Yes, we could build the label and see if it changes but if we are going to do that, we might as well refresh the label on every 'g-properties-changed' received and save ourselves the hassle?

Alternatively, couldn't we simply concatenate by excluding empty metadata[field]? The current implementation assumed as all values are available which, you are right, isn't always the case.

[Moon-0xff]

We could cache the result and call buildLabel when it's needed. This should be done in getMetadata, plus all the GLib.Variant stuff.

[Moon-0xff]

I started a new branch to implement the basic controls (play/pause, prev and next). It should be easier to merge.

[Batwam]

I started a new branch to implement the basic controls (play/pause, prev and next). It should be easier to merge.

would I need to rebase this PR against your new branch then? Will you be making changes to your branch or this one?

[Moon-0xff]

They are not changes. Simply merging the simple parts of this PR earlier.

The changes we have are release worthy and the time is right. 44 is scheduled for release in a month and i have the gut feeling we will take about the same time to get a main worthy of release again.

So for the sake of not annoying the users with repeated updates and of delivering them our work quickly:
I want to release an update soon.

[Batwam]

_activatePlayer() appears to be a little bit more complicated than I would have liked... I've got it working for most sources but some like Firefox and Rhythmbox don't work. lookup_app() is returning an object but it isn't in the list of running apps and .activate() doesn't seem to be doing anything.

Generally, this seems to be problematice since even the Gnome notification area opens a new Firefox Window rather than focussing the app. It however manages to open Rhythmbox.

[Batwam]

ok, I guess this nuts wasn't hard enough to crack. I worked around it with an alternative method using global.get_window_actors() which works for all apps. This works better than the Gnome notification menu as it even opens Firefox properly!

I have commented out the original code and it works without it. if you want, we could keep it and have the second method as fallback?

[Batwam]

Ok, we are getting close.

Next step is the Settings dialog. Could we group all mouse controls under a new tab in settings? I was thinking we could have drops downs for Primary/Middle/Secondary(maybe backward and forward buttons too) and the user can chose Open Application, Play-Pause, Previous, Next for each.

[Moon-0xff]

That's great!

I was also thinking of grouping the controls in another tab, but might be just too few options to justify it.
We can also add back the players.next() method as an option.

[Moon-0xff]

The basic controls branch is mostly done but the menu opens with any button click regardless of the selected action.

The branch should be easily merge-able with this one, the branch is basically a sort of a cherry pick of this one, plus the settings part.

[Batwam]

I just had a look at the branch and noticed that you already implemented a good chunk of the settings part which is great. On that basis, I won't redoing it here.

What I would change is include the player switch like you said but also even "Menu" to the drop down (if people are happy to drop the menu to use the right click for "next" for instance and rely on "Switch Automatically"). Also, I would add actions for backward (8) and forward(9) mouse buttons. That's 5 drops downs so probably worthy of its own tab.

[Batwam]

Backward (8) and Forward (9) buttons are now enabled.

I found how to get the .desktop of the active window using this page and realised that the .desktop we have selected don't always match the window's .desktop. Turns out matchedEntries[0][0] isn't necessarily a good match for the .desktop. For Firefox and Rhythmbox on my machine, the best match is actually matchedEntries[0][1].

I have included a check which cyclle through each matchedEntries to pick from the results the .desktop which matches one of the open apps.

This means that the previous implementation of _activatePlayer() worked. It's simply that we didn't have the correct .desktop names since Firefox and Rhythmbox were the two players which required selecting matchedEntries[0][1]. That's why they couldn't be activated. I've put the original activate code back as it's simpler back but left the window cycling one as fallback just in case as it feels more robust.

PS: I could simply have taken the result from Shell.WindowTracker.get_default().focus_app.id to get the name of the .desktop directly when loading the source as the source's window would most likely be focussed. However, this felt more risky and wouldn't work if the extension is loaded while the sources are already open .

[Moon-0xff]

Amazing work you are doing here but I'm starting to get no hopes of merging this PR, it's too big and wide in scope. Can we split it in let's say three different ones?

• Volume scroll
• Open the player window on click
• The rest

That would be a lot easier to merge, review and polish (and everything in between)

I would also propose to create a new branch/PR for a proper, refresh-less label update with the g-properties-changed signal and try to get it merged first.

[Moon-0xff]

In the spirit of making a clearer commit log (and a more useful git blame output) i will push to main the basic-controls branch with the "menu opens with any click" error intact, as I'm thinking of adding a more robust solution for that and other problems/limitations we deal with because of the underlying button implementation.

[Moon-0xff]

Pushed to main and started working on the refresh-less implementation

[Batwam]

yeah, there are quite a few conflicts now for both pull requests. The refresh-less updates are going to change even more of the underlying base code.

Shouldn't we resolve these conflicts and merge the filtering/controls PR first? We could make the refresh-less changes a separate PR to include once this is done.

[Moon-0xff]

A refresh-less label update shouldn't be a big update. I already have a "complete" version but i haven't tested it and it seems to me I'm missing something. I will push the branch so you can view it. edit: done