GNOME Bugzilla – Bug 790280
Volume up key sets volume back to 100%, if volume previously surpassed 100%
Last modified: 2018-07-31 18:21:25 UTC
This is the Shell part of https://bugzilla.gnome.org/show_bug.cgi?id=710424, where the general design discussion is. It adds a new widget and ensure all bars are using the same slider widgets for avoiding duplication.
Created attachment 363477 [details] [review] [PATCH 1/8] Add override capability to slider
Created attachment 363478 [details] [review] [PATCH 2/8] Allow slider to not have handle
Created attachment 363479 [details] [review] [PATCH 3/8] Reuse slider in OSD implementation
Created attachment 363480 [details] [review] [PATCH 4/8] Display amplified volume icon when amplified.
Created attachment 363481 [details] [review] [PATCH 5/8] Ensure scrolling on volume menu trigger OSD with override
Created attachment 363482 [details] [review] [PATCH 6/8] Ensure dbus call triggers OSD with override
Created attachment 363483 [details] [review] [PATCH 7/8] VolumeOverrideDialog to enable volume above 100%
Created attachment 363484 [details] [review] [PATCH 8/8] Trigger volume override dialog when scrolling on volume menu
FWIW, I've rejected the use of a dialogue in the gnome-settings-daemon portion of the patch, and I had doubts that "override" is a good name for that additional setting (showOSD is used by more than just the volume keys).
I did see some discussions on >100%-volume on #gnome-design. Is this taking those into account (if yes, are there mockup you can point to), or are these just the patches you ship in Ubuntu?
Yes, it's the mockups I guess (In reply to Florian Müllner from comment #10) > I did see some discussions on >100%-volume on #gnome-design. Is this taking > those into account (if yes, are there mockup you can point to), or are these > just the patches you ship in Ubuntu? Those aren't the patches we ship in ubuntu, those takes into account the design suggested by Allan on https://bugzilla.gnome.org/show_bug.cgi?id=710424#c7 and implemented those (the UI is exactly this). Note that I need to refresh them now that gsettings-desktop-schema part is committed and gnome-settings-daemon is getting review: - Adapting to the new key schema name in gsettings-desktop-schema - Adapt to the changes I just did on the OSD API for "max_level" instead of "override", and I'll remove the term override. I will handle those probably tomorrow. Then, there is the "how to trigger this behavior" discussion. Bastien seems to suggest that he would prefer a settings in Accessibility, which may makes it not really discoverable, but he doesn't like the "press volume up" interactions, and he has a point that it doesn't point to where resetting this behavior to default. I guess if you have Allan nearby (as I understood this is the week for this), maybe you can talk to him about it? I'll post the refreshed patch set once done.
Created attachment 363795 [details] [review] Slider: Add maximum value and overdrive capability to slider Sliders now has a capability to display an ovedrive area from which the bar will show a delimitor, and a different style. Nothing change in behavior if no maximum value is set. Overdrive is defined to trigger from >=1. This slider overdrive area is stylable through css. Generate a default style from the scss.
Created attachment 363796 [details] [review] Slider: Make the overdrive value limit customizable No overdrive value explicitly set assume that it's 1.0 for the slider (100%). We ensure that value is always less or equal than max value. Add accessibility property to get overdrive value.
Created attachment 363797 [details] [review] Slider: allow setting volume above 100% Read the new allow-volume-above-100-percent to define if we set the maximum volume allowed value above 100%. Do not set an explicit overdrive limit, as 100% is the desired starting point for the overdrive area.
Created attachment 363798 [details] [review] BarWithOverdrive: Separate bar drawing from slider logic As we will reuse the bar with overdrive capability in other pieces of the UI, like the OSD level bar, separate the slider piece (interactive) from the drawing portion, and make the class and other properties parameterizable. Draw an "end bar" arc radius, on which the handle (if slider) is drawn to end the bar. Adapt css style to it.
Created attachment 363799 [details] [review] BarWithOverdrive: Make border optional As not all bars will have border, rather than filling mock values, make the border colors and width optionals with a minimal value of 1.
Created attachment 363801 [details] [review] OSDWindow: Reuse BarWithOverdrive in OSD implementation This enable to reuse, in a non interactive way, the same BarWithOverdrive widget drawing area. We gain thus the overdrive capability and a similar behavior than Slider. We also expose the level to the a11y layer. Adapt css as well.
Created attachment 363802 [details] [review] Volume: Display above 100% volume icon when entering override area. Show the above 100% volume icon if volume is louder the max normalized one. Use a similar logic than gnome-settings-daemon to delimit values, restricted to output.
Created attachment 363803 [details] [review] Volume: Ensure scrolling on volume menu trigger OSD with override Send max_level when calling OSD when scrolling on volume menu. Send values as integer as the D-BUS API is sending them to avoid slight shift movement when going from multimedia keys to scrolling.
Created attachment 363804 [details] [review] ShellDBUS: Ensure dbus call triggers OSD with overdrive Send max_level overdrive settings to OSD when we get from dbus the corresponding parameter.
Created attachment 363805 [details] [review] VolumeOverdrideDialog: Add VolumeOverdrideDialog to enable volume above 100% This dialog is triggered by a dbus call and just change directly the gsettings allow-volumeabove-100-percent key if acked by the user. There is as well some description and link to the sound settings in Settings for people to check their global volume. The dbus call will be emitted by gnome-settings-daemon for instance if the volume plus media key is triggered multiple consecutive times at maximum volume.
Created attachment 363806 [details] [review] Volume: Trigger volume override dialog when scrolling on volume menu Implement similar behavior than when pressing volume up multimedia key repeatedly at maximum volume: trigger volume overdrive dialog, via a direct call.
Here we go with a refreshed patch set complying to latest gnome-settings-daemon API change patches. What's missing thus? * Allan needs to answer to Bastien's concern about the interaction to trigger the overdrive question (those are the 2 last patchs of this set). * We need Allan's asset for audio-volume-above-100-percent-symbolic.svg I guess that shouldn't stop the review, I'm going to port the theme changes in the sass repo, opening a bug for it, attach the changes and link it back here.
Created attachment 363808 [details] [review] style: Add Overdrive properties for generic sliders Add additional properties for generic slider to style an optional overdrive area.
Created attachment 363809 [details] [review] style: Change slider properties to bar after inroduction of BarWithOverdrive Slider now inherits from BarWithOverdrive, the styles have been renamed to this new element.
Created attachment 363810 [details] [review] style: Adapt to new osd slider implementation The OSD impementation is now reusing BarWithOverdrive. Adapt style to it as well as adding an overdive color area to it.
Created attachment 363811 [details] [review] style: Add theming for VolumeOverdriveDialog
I attached the gnome-shell-sass patches here as I couldn't find a gnome-shell-sass product on bugzilla. Note that in the GNOME Shell patch set, I didn't rename for the last 2 patches Override to Overdrive until we have more clues from Allan, if we still include thoses (and I'll rework them then). I think all the other patches are good for a review, keep me posted if there is anything unclear ;)
Comment on attachment 363805 [details] [review] VolumeOverdrideDialog: Add VolumeOverdrideDialog to enable volume above 100% We settled on not doing that, so marking appropriately to get the patch off the queue.
Comment on attachment 363806 [details] [review] Volume: Trigger volume override dialog when scrolling on volume menu We settled on not doing that, so marking appropriately to get the patch off the queue.
I've tried the patches and Jakub has looked at the visuals, and the behaviour seems good. The one issue I did see - if the volume is over 100% and you disable the volume overdrive gsetting, the volume isn't immediately lowered to 100%, as you might expect. This isn't a major issue for the moment, but might be something to look at in the future.
*** Bug 658248 has been marked as a duplicate of this bug. ***