[Media controller] Add simple volume control
ClosedPublic

Authored by Pitel on Mar 4 2018, 3:49 PM.

Details

Summary

Add simple volume control to Media Controller applet. Volume can by changed by:

  • mouse wheel on applet icon (3 % step), and
  • new global shortcuts (5 % step, unbound by default).

There is no mute support currently (mpris interface does not have a mute method so we would have to keep
track of old volume).

BUG: 386588

Test Plan

Tested in daily use with Cantata and SMPlayer. There is no UI change.

Diff Detail

Repository
R120 Plasma Workspace
Branch
mediacontroller
Lint
No Linters Available
Unit
No Unit Test Coverage
Pitel created this revision.Mar 4 2018, 3:49 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 4 2018, 3:49 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
Pitel requested review of this revision.Mar 4 2018, 3:49 PM

What about a volume slider in the applet?

Pitel added a comment.Mar 4 2018, 4:15 PM

The idea crossed my mind but I rarely use expanded version of the applet and there are two problems to deal with:

  • Where exactly to place the slider (I am bad at designing UI).
  • What should upper bound of slider be -- usually 1 is good option but mpris does not prevent apps from using larger volumes and there is not "getMaxVolume" method / property. It might be possible to just hardcode 1 as max but care would be needed not to mess the volume up if it was set to (say) 1.5 by other means. (And the user experience would be somewhat inconsistent, but I am not sure how many apps really uses volume > 1.)

So I decided to keep it simple at first try.

Pitel retitled this revision from [Media contoller] Add simple volume control to [Media controller] Add simple volume control.Mar 4 2018, 8:49 PM
broulik added a subscriber: broulik.Mar 5 2018, 8:45 AM

Thanks for your patch! I thought about doing that for a long time, actually.
You can add BUG: 386588 to your commit message (on its own line) to indiate that it fixes this bug.

Can you please also implement a volume OSD? We have a mediaPlayerVolumeChanged in plasmashell OSD service but never used it. I think this could be done in the SetVolume operation by adding a new argument showOsd or so that you then set. To implement an OSD you just need to place a DBus call like so:

QDBusMessage msg = QDBusMessage::createMethodCall(
    QStringLiteral("org.kde.plasmashell"),
    QStringLiteral("/org/kde/osdService"),
    QStringLiteral("org.kde.osdService"),
    QStringLiteral("mediaPlayerVolumeChanged")
);

msg.setArguments({
    50, // volume in percent (0-100)
    user visible player name (identity), // "VLC media player"
    icon name of player // "vlc"
});

QDBusConnection::sessionBus().asyncCall(msg);
applets/mediacontroller/contents/ui/main.qml
153

Did you check this works fine with touchpads? Here Qt sends a gazillion of tiny wheel events.

Please enforce a limit of e.g. 100%, I almost deafened myself last time I messed with volume on Mpris

154
volume = Math.max(0, volume);
dataengines/mpris2/multiplexedservice.cpp
136

No need to abbreviate

const qreal newVolume = ...
137

Also enforce a limit here,

qBound(0.0, vol, 1.0);
Pitel edited the summary of this revision. (Show Details)Mar 5 2018, 12:20 PM
Pitel updated this revision to Diff 28701.EditedMar 5 2018, 12:44 PM
  • Volume is bounded at max(oldVolume, 1.0).
  • Added OSD support.
  • Refactored osd & volume bounding logic into helper MultiplexedService::changeVolume.
  • Is there any way to get player name & icon without adding PlayerControl::container?
  • Maybe add default OSD icon when app does not have any?

I also tested it with touchpad at it work correctly (but my Qt is compiled without xinput2).

Pitel edited the summary of this revision. (Show Details)Mar 5 2018, 12:55 PM
Pitel updated this revision to Diff 28729.Mar 5 2018, 3:59 PM

The changeVolume helper must be in PlayerControl class and MultiplexedService must only forward calls to it in order to volume change by mouse wheel also work for other sources than only mutliplex one.

Pitel marked 2 inline comments as done.Mar 12 2018, 9:01 PM

Mostly good!

dataengines/mpris2/multiplexedservice.h
38 ↗(On Diff #28729)

Shouldn't this be a job? So you use the startOperationCall stuff used elsewhere (for e.g. Play and so on)

Pitel added inline comments.Mar 20 2018, 1:42 PM
dataengines/mpris2/multiplexedservice.h
38 ↗(On Diff #28729)

I guess it would look more consistent if it was a job but somehow I fail to see why any of this is job in the first place: all the actions are very simple so offloading work to other thread is not needed and it hides interface in ugly way... Do you have any insight why?

broulik added inline comments.Mar 27 2018, 3:56 PM
dataengines/mpris2/multiplexedservice.h
38 ↗(On Diff #28729)

That's how dataengines worked 10 years ago. I'm not a huge fan either but I'd like to keep it consistent(ly bad)

Pitel updated this revision to Diff 30963.EditedMar 30 2018, 7:06 PM

Convert changeVolume helper into a Job... Right now it does not check wheter the DBus call is successful - I tried to wire it in but ended with segfault. I will investigate it further. Except that I hope it is ok.

(I also started thinking how to change the job system into something easier to work with. Were there any attempts in this direction?)

broulik accepted this revision.Apr 3 2018, 10:51 AM

Thanks!

This revision is now accepted and ready to land.Apr 3 2018, 10:51 AM
This revision was automatically updated to reflect the committed changes.