aleho / gnome-shell-volume-mixer

GNOME Shell Extension allowing separate configuration of PulseAudio devices
https://extensions.gnome.org/extension/858/volume-mixer/
GNU General Public License v2.0
135 stars 32 forks source link

Make indicator consume scroll events #143

Closed mrEDitor closed 2 years ago

mrEDitor commented 3 years ago

like original indicator does: https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/js/ui/status/volume.js#L65

It will keep other extensions which listen to bar scrolling from consuming events already recognized as volume adjusting one; see https://github.com/mrEDitor/gnome-shell-extension-scroll-panel/issues/15

aleho commented 2 years ago

Your link the the line number points at the wrong line, correct link should be: https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/40.5/js/ui/status/volume.js#L407).

This Indicator is basically a copy/paste job from upstream.

The upstream code, as called by

this._primaryIndicator.connect('scroll-event',
            (actor, event) => Volume.Indicator.prototype._handleScrollEvent.apply(this, [VolumeType.OUTPUT, event]));

has conditions on return values:

        this._primaryIndicator.connect('scroll-event',
            (actor, event) => this._handleScrollEvent(VolumeType.OUTPUT, event));
        this._inputIndicator.connect('scroll-event',
            (actor, event) => this._handleScrollEvent(VolumeType.INPUT, event));

…

    _handleScrollEvent(type, event) {
        …
        return result;
    }

The arrow-function specified for the event always returns a value.

I'm not sure why this code, that basically does the same as the original indicator, would break other extensions or if it's a good idea to differ too much from the original implementation.

In other words: GSVM seems to do the same thing as the upstream implementation (unless I'm not seeing the actual problem). I'd rather not fix a problem in another extension by changing expected behavior.

mrEDitor commented 2 years ago

Sorry, didn't get the idea of wrapping original indicator at first. There is more elegant fix. You just should pass return value of super.scroll(event) call in it's overload for MasterSlider class, since it returns completion flag for clutter event propagation: https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/40.5/js/ui/status/volume.js#L115

aleho commented 2 years ago

This looks good now. Thanks!