Volume control on mouse scroll

[Batwam]

Preliminary implementation of the volume control with mouse scroll based on selected section of #26

For this initial commit, only the proference drop down and control for "Global" volume are implemented to start the conversation.

[Batwam]

The volume control based on mpris could be put in too but I wanted to check if there would be a way to implement using getMixerControl() instead as per gnome-volume-mixer

I am able to list streams using .get_streams(). (Edit: I am also able to adjust individual volume levels).

However, while the names don't really matter for the other extension or gnome settings, it turns out they don't exactly match either Identity or desktopEntry so matching them might be a challenge. There might be other info somewhere which might be more useful to use to match with mpris sources, not sure... (Edit: based on investigation, name seems to be the only source of info we can use).

See log below, the first section is the list of stream id/names from .get_streams(). Not that it's extremely relevant but the names match the ones listed in the Sounds section of Gnome Settings.

Also, if you have several Google Chrome tabs playing, they will all be called "Google Chrome". On the plus side, the browsers are listed so this definitely gives us the ability to control Chrome/Firefox through this since they can't be controlled via mpris (as far as I can tell). Any thoughts?

Some additional info here maybe: https://gitlab.gnome.org/rmader/gnome-shell/-/blob/main/js/ui/status/volume.js
and maybe here too: https://gitlab.gnome.org/GNOME/libgnome-volume-control/-/blob/master/gvc-mixer-control.c
even better: https://www.roojs.org/seed/gir-1.2-gtk-3.0/seed/Gvc.MixerStream.html

[Batwam]

I guess if we were to run a loose lowercase match with (stream name included in identity) or (identity included in stream name), this would work for all apps listed here and save us from doing any separate mpris volume control. Find the stream matching the identity field and update the volume for it. Maybe that's the way to go?

[Moon-0xff]

The object returned from get_stream is Gvc.MixerStream

My bet is: we can match desktop entries with properties of this object.
My first thought is looking if name matches DesktopAppInfo.get_generic_name or some field in the .desktop file.

I think your idea of loosely matching name with identity could work pretty well too, but that's not a straight forward implementation.
Although, we can probably implement it following a guide, or import it from somewhere else.

[Batwam]

I just had a look at the content of the .desktop as a quick way to extract info and got the following for main info for Firefox for example:

Name=Firefox Web Browser
Comment=Browse the World Wide Web
GenericName=Web Browser
Keywords=Internet;WWW;Browser;Web;Explorer
Icon=firefox

I was hoping we could somehow get a match with icon but turns out the firefox's stream icon is multimedia-volume-control... so, that's a non-starter for me as nothing appears to match.

I went down the fuzzy match route and I have a working draft now. What I had in mind for the match is more something simple like this:

let name = stream.get_name().toLowerCase();
let identity = this.player.identity.toLowerCase();
if (name.includes(identity) ||identity.includes(name)){
...
}

This implementation with this match within the _onScroll(event) is not the most efficient implementation as it cycles through the streams for each click of the scroll wheel it works so that's a start.

We could identify the stream_id in the player class. However, since it's a sequence, this number could change if another source gets deleted. There are ways to detect stream-added stream-removed which could be used to trigger an update in the background. On the plus side, we don't really need to know the stream_id for any source other than the current this.player so we would probably need to initialise it when this.player is changed, only match the active source and update based on the stream-added/removed event.

[Batwam]

ok, I think that I've got it now.

I moved the streamID identification out of the _onScroll(event) to only run when the stream list or player changes. Turns out the streamID doesn't actually change when another player is closed so if streamID11 is running and streamID 5 is closed, the first player remains 11. However, if you pause a player, it might lose its streamID so it's still useful to monitor stream-added. I am doing the stream_ID updates regardless of the volume mode so the streamID is already available if the user changes the settings from off/global to source.

I also wanted to keep the _onScroll(event) loop as lean as possible so I have moved some of the declarations out into _init() for constants. I also merged the code which was common between global and source mode to keep things clean.

I have left the logs so you can see what it does. to be removed before merge obviously. try to break me 👍

[Moon-0xff]

Good job figuring it out! Although the code is sprawling itself too much, it definitely needs a layer of abstraction.
I have some spare time so i will try to push some of the changes i think are worth making instead of explaining them.

[Batwam]

That sounds good. I just wanted to start step by step to make sure I have a working solution before cleaning up as you probably noticed in the last few commits.

A couple of things I was considering:

• can we store the stream object directly rather than id number?
• due to browser potentially having multiple tabs all with the same name, I think that we should set the same volume to all tabs when adjusting volume and increase/decrease them all with the scroll. For this, we might want to store the multiple active streams matching the Browser's name as an Array and adjust the volume with a forEach.

For info, I did some checks opening multiple tabs in chrome and when a new tab is open, it is given the same volume level as the last one so by default, they should all have the same volume level. If we don't update all matching streams for a given Browser together, we could be adjusting the volume to the wrong stream.

[Batwam]

sorry, just had to make a couple of fixes:

• volumeStep had been deleted by mistake in the last commit. I put it back into onScroll as it's a quick calc
• monitor put back into onScroll as there is a risk the monitor may not match the panel's monitor is set during _init(). When set on scroll, the mouse has to be on the panel and the OSB will therefore show on the correct screen.

over to you now ;-)

[Moon-0xff]

Ok, for a brief explanation i have this diff stashed and i will merge it with this branch, as the mpris method is discarded for now and the code for changing the volume for the application looks like it benefits greatly if we bind it with the player object, all that code will be moved to the players.js file.

[Batwam]

Yeah, moving the code into player.js crossed my mind. I'm not sure how many lines of code that would save though.

One think to watch out is that the streams may not be created at the same time as the mpris sources so the stream_id or object may not exist when you first initialise a source. Worth checking just in case.

[Batwam]

Couple of comments on the diff:

• I think that the clamp method is neater then if to keep the volume between 0 and 1 as it's built-in and does the same thing in a single line
• I like that the mouse controls will have their own tab, this will be clearer and allow to reset just these settings
• I am not convinced we need separate actions for scroll-up and scroll-down. Defining the mouse scroll as controlling the volume should be sufficient, or you want to be allow some sort of reverse scroll?

[Moon-0xff]

Math.clamp isn't a standard method, not even in gjs, this isn't a problem but it's something to keep in mind. That's the reason why in the diff I used an else if statement instead.

[Moon-0xff]

The scroll up and down clutter events are problematic and always (on my testing) fired alongside a smooth event. So i think it's better to simply ignore them.

[Moon-0xff]

And for the _getStream thing i agree with your thoughts.

[Batwam]

ok, see proposed updates. I was actually surprised about how easy it was to implement the stream Array as it worked right away. I'm either getting good at this or I must have missed something (probably option 2)...

[Batwam]

True. I forgot the browser thing. I suppose giving priority to the first, the last or your idea are all equally valid approaches. Maybe we should pass the decision to the user?

Streams are created/removed as the sources are stopped so the concept of first/last doesn't carry a lot of meaning.

The safest would be to pick the one with the lowest volume as it's better to have the sound drop than blasting sound full volume all of a sudden. There's always the OSD to show the volume level so if the user can't hear anything all of a sudden as the sources get realigned, he/she will know why. Again, by default they will all match so the risk is low.

[Moon-0xff]

Sounds about right.

The best way i see to do this would be to only raise/lower the volume of the stream that corresponds with the mpris information.
As far as i know we can't correlate streams with mpris information very accurately but it's worth mentioning.

[Batwam]

As you said that, I just thought I might check what comes out of _get_description() again in case we could do a match again some of the mpris info for browsers at least. Unfortunately, chrome returns null so that's not going to help I'm afraid.

[Batwam]

I added an option to toggle mute on click. I quite like being able to do it with a middle button click, it feels quite natural when combine with the volume up/down on scroll. I embedded it within the current _changeVolume() for minimal code change.

[Moon-0xff]

Have you tried to pass a very high negative delta instead? That wouldn't require the extra if statement. Although the code might not handle high deltas correctly, we should address it if that's the case.

[Batwam]

The purpose of the 0 delta is so that the existing code remains and the volume isn't affected since newVolume = volume + step * delta. This way, when we unmute, the volume remains what it was previously. Otherwise we would have to store/restore the value of the unmuted stream which would be a pain, especially if the user switched stream in the meantime... if you look at the code, the only thing I'm changing beside the is_muted status is VolumeRatio at the very end, and that's only so it shows the mute icon, nothing else.

[Moon-0xff]

I see. I wonder if devices can send a delta of 0. Sounds like a possibility, although i don't think a lot of devices would bother to send a useless value even if they can (i hope?).

I wonder what's the specification.

[Batwam]

I know that my mouse can send a scroll delta of 0 as it would occasionally trigger mute if I scrolled very slowly before I put the if(!delta ==0) statement in. It goes in increments of 0.125. The reason I excluded it in onScroll is because there is no reason for that value to be useful in the context of adjusting the volume up/down anyway.

[Batwam]

I guess the PR is now completed. Anything else we might have missed?

[Moon-0xff]

I got some errors in the logs, it looks like _getStream errors out when the shell is reloaded.

[Moon-0xff]

I want to use the same controls to change the volume of everything, not only players. I started another extension to do just that.

[Moon-0xff]

Well i think i cleaned all or most of them. I'm gonna go ahead and merge this.
edit: stream.get_name still errors out sometimes