GNOME Bugzilla – Bug 633005
volumeIcon: Unmute on manual volume changes
Last modified: 2011-03-19 21:40:27 UTC
Quoting IRC conversation: --------- <drago01> hadess: just wanted to ask you what your opinion is on how volume changes while muted should work .. g-v-c-applet unmutes the sink when the user changes the volume while muted, while the shell implementation does not <hadess> drago01: it probably should <hadess> drago01: the status icon behaviour is what jon wanted <hadess> drago01: so it's probably the right thing to do with the shell as well ---------
Created attachment 173094 [details] [review] volumeIcon: Unmute on manual volume changes Unmute when the user changes the volume (using the slider or mousewheel), to reflect the icon state and match the gnome-volume-control-applet behaviour.
Created attachment 173095 [details] [review] volumeIcon: Unmute on manual volume changes Unmute when the user changes the volume (using the slider or mousewheel), to reflect the icon state and match the gnome-volume-control-applet behaviour. --- Remove unrelated hunk.
Created attachment 173097 [details] [review] volumeIcon: Unmute on manual volume changes Unmute when the user changes the volume (using the slider or mousewheel), to reflect the icon state and match the gnome-volume-control-applet behaviour. --- Fix whitespace (should probably not write patches at 00:20 am ;) ).
Review of attachment 173097 [details] [review]: ::: js/ui/status/volume.js @@ +84,3 @@ } + + if (this._output.volume > 0) { Why if > 0? I think it should be only if the volume is louder (this is what g-v-c-a does for scrolling, whereas the slider is reset to 0 when muted) @@ +85,3 @@ + + if (this._output.volume > 0) { + this._output.change_is_muted(false); You also need to set this._output.is_muted, or manually update this._outputSwitch @@ +186,3 @@ + if (this[property].volume > 0) { + this[property].change_is_muted(false); + this._notifyVolumeChange(); You cannot do this here, as ::value-changed is emitted for every micro change (you would hear a continuous beep while dragging). Move it to the ::drag-end signal handler.
(In reply to comment #4) > Review of attachment 173097 [details] [review]: > > ::: js/ui/status/volume.js > @@ +84,3 @@ > } > + > + if (this._output.volume > 0) { > > Why if > 0? > I think it should be only if the volume is louder (this is what g-v-c-a does > for scrolling, whereas the slider is reset to 0 when muted) OK > @@ +85,3 @@ > + > + if (this._output.volume > 0) { > + this._output.change_is_muted(false); > > You also need to set this._output.is_muted, or manually update > this._outputSwitch OK > @@ +186,3 @@ > + if (this[property].volume > 0) { > + this[property].change_is_muted(false); > + this._notifyVolumeChange(); > > You cannot do this here, as ::value-changed is emitted for every micro change > (you would hear a continuous beep while dragging). > Move it to the ::drag-end signal handler. Well dragging isn't the only way to change a sliders value (you can scroll or even use the arrow keys).
(In reply to comment #5) > (In reply to comment #4) > > @@ +186,3 @@ > > + if (this[property].volume > 0) { > > + this[property].change_is_muted(false); > > + this._notifyVolumeChange(); > > > > You cannot do this here, as ::value-changed is emitted for every micro change > > (you would hear a continuous beep while dragging). > > Move it to the ::drag-end signal handler. > > Well dragging isn't the only way to change a sliders value (you can scroll or > even use the arrow keys). Unfortunately, we cannot know when scrolling ends, so both g-v-c-a and new volume indicator have no beep for scrolling or key press. (I'm referring only to this._notifyVolumeChanged here)
I'm tentatively obsoleting this because the shell has no more a separate muted button, and when the slider is 0 (muted), moving it anywhere changes volume.
(In reply to comment #7) > I'm tentatively obsoleting this because the shell has no more a separate muted > button, and when the slider is 0 (muted), moving it anywhere changes volume. Yeah makes sense, that's the reason why I haven't bothered to update the patch.