[Applet] Make 'raise maximum volume' global
ClosedPublic

Authored by gvgeo on Dec 28 2019, 1:07 PM.

Details

Summary

Remove maximum volume control from configuration.
Remove individual maximum volume.
Add a checkbox in applet to enable raise maximum volume for all devices. (not applications)

Depends on D26271

Test Plan

Before:
2 options exist to raise maximum volume.
One options controls the default output device(by setting the limit).
And a second one controls individual devices/applications.
After:
A checkbox in the applet will raise the max volume for all devices to 150%.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
raise2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21511
Build 21529: arc lint + arc unit
gvgeo created this revision.Dec 28 2019, 1:07 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 28 2019, 1:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
gvgeo requested review of this revision.Dec 28 2019, 1:07 PM
gvgeo added a comment.EditedDec 28 2019, 1:12 PM

Have a problem that marked with a TODO comment in code, but the desired functionality is working.solved, will fix.
@ngraham I should mention that I still oppose to the functionality of this patch, and I believe the better way is D26234 that maybe missed cause of the status change.

gvgeo edited the summary of this revision. (Show Details)Dec 28 2019, 1:18 PM
gvgeo updated this revision to Diff 72295.Dec 28 2019, 2:12 PM

Fixed shortcut keys.

ngraham added a subscriber: drosca.

Thanks for this patch. I won't try to make you go forward with something you don't agree with, so let's continue the discussion here and hopefully we can reach a consensus of some sort.

IIRC VDG people decided that 1) it made sense to have only a single way to raise the maximum volume rather than two, 2) that this single method should be global rather than per-app/per-device because global is simpler, more comprehensible, and more likely to be what the user wants to do, and 3) that the option to do this should be more discoverable than being in the config window.

Adding VDG for more comments, and see T10470 for more details. Also adding @drosca as he's the maintainer.

gvgeo added a comment.Dec 28 2019, 6:54 PM

Actually I never touch volume control, so don't have strong opinion. Either way I'm good, no worries.
But in general believe, that should simplify UI and usability, while trying to keep features.

This patch removes 2 options.
While the other could have the mockup as a default, while also giving more options to advanced users.

But if it is about code maintenance, I will not disagree.

gvgeo updated this revision to Diff 72740.Jan 4 2020, 12:25 PM

Moved limit max volume, code part,from listbase.qml into the main.qml checkbox.

gvgeo retitled this revision from [WIP][Applet] Make 'raise maximum volume' global to [Applet] Make 'raise maximum volume' global.Jan 4 2020, 12:27 PM
gvgeo edited the summary of this revision. (Show Details)
gvgeo updated this revision to Diff 72751.Jan 4 2020, 3:46 PM

squash

ngraham requested changes to this revision.Jan 4 2020, 8:44 PM

Works great. Just a few code change requests:

applet/contents/ui/main.qml
543

Don't override this; if it looks bad with the default value, we should fix that in the checkbox control itself

544

This could evaluate to a fractional number. You probably want units.largeSpacing here (the spacing doesn't have to be literally identical to the mockup; it's more important to use standard spacing values)

546

Always use onToggled to handle user input, never onCheckedChanged

If you're deliberately using onCheckedChanged because it has the side effect of evaluating the logic in here when the applet is first loaded, then don't do that, and instead explicitly call that logic in an onCompleted block

This revision now requires changes to proceed.Jan 4 2020, 8:44 PM
gvgeo updated this revision to Diff 72855.EditedJan 6 2020, 9:42 AM

Made requested changes.

Issues:

  1. Check comment at the bottom of main.qml in D26256 These two strange margins (including defaultButton in this patch), are required to vertical align the icons in their center, and the texts with the slider.
gvgeo marked 3 inline comments as done.Jan 6 2020, 9:51 AM
ngraham accepted this revision.Jan 6 2020, 3:19 PM
This revision is now accepted and ready to land.Jan 6 2020, 3:19 PM
gvgeo updated this revision to Diff 72915.Jan 6 2020, 6:58 PM

rebase

gvgeo updated this revision to Diff 73920.Jan 20 2020, 9:53 AM

Raise maximum volume has the potential to aid in speaker blowout. Although this requires a system not properly configured (an amplifier too powerful with no limiter). I expect this to be the case only with DIY audio setups.

I know that in theory it should be safe, and can only create distortions. And damage to speaker can happen with a very high sound, without raise functionality.
Despite this I feel this option has no place to be so accessible in the applet(calling, for example, kids to try the 'Boost' button).

Because time has been already invested in these patches, the only change I made was disabling the checkbox. That way, YOU can easily enable it.
Sorry for the inconvenience, so late too.

gvgeo updated this revision to Diff 74192.Jan 23 2020, 8:14 AM

Fix volume icons

This revision was automatically updated to reflect the committed changes.