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 :+1:

[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.

[Batwam]

Note that with the current implementation, the mouse scroll doesn't work for me unless I scroll the mouse very quickly. A typical scroll for me has a delta between 0.125 and 0.375 and since the following if only captures exactly -1 or +1, the actions don't get triggered. The idea with the clamp was to prevent excessive volume change, not just to work out whether it was a volume increase or decrease.

What is the minimum delta value are you getting when you scroll very slowly? do we need to normalise this?

For me the value of the smooth even is that you can do fine adjustments of the volume or quickly change the volume depending on how fast you scroll. _changeVolume is designed to take any value. Allow variable speed of change would also be consistent with the way the mouse scroll work on the speaker icon for Global volume on the panel. Could we pass delta directly as this._changeVolume(delta) rather than going through this._activateButton('scroll-up-action')?

This would also mean removing the "Scroll up Action" and "Scroll down Action" drop down which look redundant to me. Honestly, I don't see why a user would want to activate play/pause or next with the scroll, it would send tens of play/pause to the source (actually, I just did it and it froze my system...). If you really want the user to be able to swap between up/down, you could have a drop down normal/reverse and apply -1 to the adjustment when calculating newVolume.

If you want, you could leave volume up as a click action in case someone wants to increase the volume by clicking rather than using the wheel, I would probably only use a delta of 0.5 for that though.

[Moon-0xff]

Hang on that's like 5 questions in a very dense paragraph

For starters I wasn't expecting values between 1 and -1, my bad.
I think my mouse has a pretty "digital" scroll wheel, raising or lowering is always done in steps, isn't that always the case?

[Batwam]

I'm getting a succession of delta values on mine, ranging from 0.125 if I scroll extremely slowly, to 1.25 if I go all in. So if I roll slowly, I get a succession of 0.125, if I scroll fast, a succession of .25, etc...

So when I scroll I get 5-10 scroll events at least (which is also why I removed the get_streamID from the onScroll(event) in 8614a33 so it doesn't get run 10-20 time to figure out the streamID which was worked out a fraction of a second prior.

[Moon-0xff]

I tested other mouse i had and the scroll wheel is definitely slower, which is pretty interesting! I suppose the value received depends on the mouse hardware and can't be assumed to behave in a particular way.

In any case we are dealing with a delta so for that we only need to know if the value is positive or negative i think.

[Batwam]

In a way yes, but you lose some of the sensitivity which is a shame. This is what a normal scroll looks for me, what sort of values do you get?

[Moon-0xff]

Yes, my mouse it's pretty digital, the values i get are just 1 and -1
Using the touchpad i get values with a lot of points of precision: -1.086129606111596 and the value depends of how fast i swipe on it.

[Moon-0xff]

In that case, I suppose the best approach is to pass the delta to _changeVolume directly.

[Batwam]

see commit for what I have in mind. Does this still work for you? I also removed the redundant drop downs. Feel free to reject/adjust or include an option to reverse the scroll.

as you can see from the previous screenshot, I am also sending 10-15 onScroll(event) per mouse scroll so running stream = this._getStream() on every single one of them is a little bit inefficient... which is why I removed the get_streamID from the onScroll(event) in 8614a33. It still works with the current implementation so it's not a deal breaker, it just doesn't feel very optimised considering that streamID for a given source is more or less a constant.

[Moon-0xff]

Yes, that's ok.

[Moon-0xff]

I'm pretty satisfied with the changes now, what do you think?

It looks like the scroll event can crash GNOME, I don't think we should worry about it too much but it would be nice to crash-proof the extension. We should try to add a way of dropping events if they are too close to each other, preferably in the vfunc_event method.

[Batwam]

Yeah, it's looking good. I was going to comment on the fact that the streamId needed to be updated in case of player switch last night but noticed that you already picked up on it 👍

Under what scenario do you think onScroll could crash gnome?

[Moon-0xff]

I don't think the extension could cope with a device that sends excessive amounts of events to the shell. At the same time i think it should be safe if gnome doesn't bother to slow down events, so we should dig into the gnome-shell repository to find the answer.

[Moon-0xff]

Well this method on volume.js is similar to ours and doesn't bother with slowing down the events (unless that functionality is abstracted away).
On the other hand i did find some situations where the scroll is purposely slowed down.

I suppose it's safe as long as the method isn't too slow? That sounds about right.

[Batwam]

I think that it's safe as long as all we so is adjusting the volume. The only time I manage to freeze gnome is when I picked play-pause or open menu as scroll open to stress test it.

I haven't felt any slowdown with volume control, even when we were checking the streamID on every scroll with my mouse which is sending 10-15 events per finger movement.

[Moon-0xff]

Ok, i think it's ready to merge, what do you think?

One thing though, is the stream match solid enough? Have you tested it with multiple players?

[Batwam]

we still have a potential challenge with browsers having multiple tabs open as mentioned in my earlier comment. Any thoughts on this? should we store streamIDs in an array and adjust all Browser tabs by the same amount?

Regarding _getStream() , what I suggested in 7e5a022 is to use the fuzzy match as fallback only if the direct lowercase match is unsucessful. Maybe an edge case but we wouldn't want a player called Wave to match with Shortwave if there is another stream called wave.

PS: I like the way you implemented the fallback in e84c071, very neat!

[Moon-0xff]

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?

[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