Add options for OSD feedback
AcceptedPublic

Authored by sgoth on Wed, Mar 25, 2:49 PM.

Details

Reviewers
broulik
ngraham
drosca
cblack
Group Reviewers
VDG
Plasma
Summary

Extend configuration knobs for visual feedback by adding separate ones
for output volume, input volume and mute state.

BUG: 363983

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Branch
volumeosdcfg (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24233
Build 24251: arc lint + arc unit
sgoth created this revision.Wed, Mar 25, 2:49 PM
Restricted Application added a project: Plasma. · View Herald TranscriptWed, Mar 25, 2:49 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
sgoth requested review of this revision.Wed, Mar 25, 2:49 PM
sgoth added a comment.Wed, Mar 25, 2:54 PM

Not sure about the actual wording and whether we would want all 3 of them.
Probably would use "input volume", "output volume" but i tried to match the existing "aural feedback" string.

Also i actually tried to implement the various qml functions a single one taking a QML enum but failed to do so. If that or a string based version is preferred, just tell me (how :) ).

ngraham added inline comments.
applet/contents/ui/main.qml
198

mutecOsd? :-)

sgoth updated this revision to Diff 78483.Wed, Mar 25, 5:36 PM

Fix typo

sgoth marked an inline comment as done.Wed, Mar 25, 5:39 PM

Oh.. good catch thanks :)

This works great!

In terms of the UI, it feels like we need a bit more logical separation between the groups now. I would adjust things (in this same patch) to be like this:

 Play audio feedback for changes to: [x] Audio volume

Show visual feedback for changes to: [x] Audio volume
                                     [x] Microphone sensitivity
                                     [x] Mute state
                                     [x] default output

You can add the spacing between groups with an item like this:

Item {
    Kirigami.FormData.isSection: true
}
sgoth added a comment.Wed, Mar 25, 7:53 PM

"default output"
"Default output"
"Default output device" ?

looks like this then:

sgoth updated this revision to Diff 78494.Wed, Mar 25, 7:53 PM

Change wording

sgoth updated this revision to Diff 78495.Wed, Mar 25, 7:55 PM

Add spacing

sgoth added a comment.Wed, Mar 25, 7:56 PM

now with spacing

ngraham accepted this revision.Wed, Mar 25, 7:59 PM
ngraham added a reviewer: drosca.
ngraham added a subscriber: drosca.

This looks great to me!

I have to admit I was skeptical when I first saw the patch, but I think making the options very granular like this actually helps to explain just what they'll do, and also disambiguates the visual vs audible feedback settings too.

Plasma people and/or @drosca, are folks okay with this?

This revision is now accepted and ready to land.Wed, Mar 25, 7:59 PM
sgoth added a comment.Wed, Mar 25, 8:01 PM

Yeah, was unsure too if it's a good idea but now with your sanitized wording i'd prefer it that way :)
But if i should go for a single OSD on/off i wouldn't mind either.

cblack requested changes to this revision.Wed, Mar 25, 8:14 PM
cblack added a subscriber: cblack.

Code looks good, bar one minor issue—instead of using global showOsdX functions, it would be more idiomatic to declare these functions on the OSD object itself. Other than that, this looks good.

applet/contents/ui/main.qml
179

These functions would probably be better declared on the OSD object like so:

VolumeOSD {
    id: osd

    function showVolume(text) {
        if (!Plasmoid.configuration.volumeOsd)
            return
        show(text)
    }
}
This revision now requires changes to proceed.Wed, Mar 25, 8:14 PM
sgoth updated this revision to Diff 78498.Wed, Mar 25, 8:25 PM

Refactor show functions to be members of VolumeOsd

sgoth marked an inline comment as done.Wed, Mar 25, 8:26 PM

Ah, that seems a lot better yes, thanks.

cblack accepted this revision.Wed, Mar 25, 8:29 PM

LGTM.

This revision is now accepted and ready to land.Wed, Mar 25, 8:29 PM
sgoth added a comment.Wed, Mar 25, 8:37 PM

Cool thanks!

As this is my first contribution via phabricator, can you give me a short advice on how to proceed?
Do i "arc land" it myself? To master?

Cool thanks!

As this is my first contribution via phabricator, can you give me a short advice on how to proceed?
Do i "arc land" it myself? To master?

IIRC you can only do that if you get an accepted application for becoming a "KDE Developer", otherwise you'll need a KDE Developer to land it for you, which will most likely happen anyway.

cblack added a comment.EditedWed, Mar 25, 8:39 PM

Cool thanks!

As this is my first contribution via phabricator, can you give me a short advice on how to proceed?
Do i "arc land" it myself? To master?

Since there's two pending reviews, you probably want to wait on them first.

However, you need to have a developer account in order to land your patches, and you most likely don't have one yet considering that this is your first contribution. When someone's landing your patch for you, they'll ask for authorship information (name, email) if it's missing and then land it for you.

EDIT: Speaking of authorship information, you should probably add yourself to the copyright headers. It's not for "credit me" purposes, but for keeping track of who's added copyrightable stuff to a file without needing git.

sgoth added a comment.Wed, Mar 25, 8:45 PM

I see. Then i'll let $things happen. Thanks!