D11021: [Media controller] Add simple volume control
This revision was automatically updated to reflect the committed changes. Closed by commit R120:bd5619e0155a: [Media controller] Add simple volume control (authored by Pitel). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11021?vs=30963=31231 REVISION DETAIL https://phabricator.kde.org/D11021 AFFECTED FILES applets/mediacontroller/contents/ui/main.qml dataengines/mpris2/mpris2.operations dataengines/mpris2/multiplexedservice.cpp dataengines/mpris2/playeractionjob.cpp dataengines/mpris2/playercontrol.cpp dataengines/mpris2/playercontrol.h To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
broulik accepted this revision. broulik added a comment. This revision is now accepted and ready to land. Thanks! REPOSITORY R120 Plasma Workspace BRANCH mediacontroller REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel updated this revision to Diff 30963. Pitel added a comment. 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?) REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11021?vs=28729=30963 BRANCH mediacontroller REVISION DETAIL https://phabricator.kde.org/D11021 AFFECTED FILES applets/mediacontroller/contents/ui/main.qml dataengines/mpris2/mpris2.operations dataengines/mpris2/multiplexedservice.cpp dataengines/mpris2/playeractionjob.cpp dataengines/mpris2/playercontrol.cpp dataengines/mpris2/playercontrol.h To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
broulik added inline comments. INLINE COMMENTS > Pitel wrote in multiplexedservice.h:38 > 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? 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) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel added inline comments. INLINE COMMENTS > broulik wrote in multiplexedservice.h:38 > Shouldn't this be a job? So you use the `startOperationCall` stuff used > elsewhere (for e.g. Play and so on) 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? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
broulik added a comment.
Mostly good!
INLINE COMMENTS
> multiplexedservice.h:38
>
> +Q_INVOKABLE void changeVolume(double delta, bool showOSD) {
> +if (m_control) m_control->changeVolume(delta, showOSD);
Shouldn't this be a job? So you use the `startOperationCall` stuff used
elsewhere (for e.g. Play and so on)
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D11021
To: Pitel, #plasma, broulik
Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel added a comment. ping REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel updated this revision to Diff 28729. Pitel added a comment. 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. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11021?vs=28701=28729 REVISION DETAIL https://phabricator.kde.org/D11021 AFFECTED FILES applets/mediacontroller/contents/ui/main.qml dataengines/mpris2/multiplexedservice.cpp dataengines/mpris2/multiplexedservice.h dataengines/mpris2/playercontrol.cpp dataengines/mpris2/playercontrol.h To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel added a reviewer: broulik. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel edited the summary of this revision. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma, broulik Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel updated this revision to Diff 28701. Pitel added a comment. - 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`? REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11021?vs=28604=28701 REVISION DETAIL https://phabricator.kde.org/D11021 AFFECTED FILES applets/mediacontroller/contents/ui/main.qml dataengines/mpris2/multiplexedservice.cpp dataengines/mpris2/multiplexedservice.h dataengines/mpris2/playercontrol.h To: Pitel, #plasma Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel edited the summary of this revision. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
broulik added a comment.
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);
INLINE COMMENTS
> main.qml:153
> +var volume = mpris2Source.currentData.Volume || 0.0
> +volume += (wheel.angleDelta.y / 120) * 0.03
> +if (volume < 0) volume = 0.0
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
> main.qml:154
> +volume += (wheel.angleDelta.y / 120) * 0.03
> +if (volume < 0) volume = 0.0
> +var service =
> mpris2Source.serviceForSource(mpris2Source.current)
volume = Math.max(0, volume);
> multiplexedservice.cpp:136
> +if (m_control) {
> +double vol = m_control->playerInterface()->volume() + 0.05;
> +m_control->playerInterface()->setVolume(vol);
No need to abbreviate
const qreal newVolume = ...
> multiplexedservice.cpp:137
> +double vol = m_control->playerInterface()->volume() + 0.05;
> +m_control->playerInterface()->setVolume(vol);
> +}
Also enforce a limit here,
qBound(0.0, vol, 1.0);
REPOSITORY
R120 Plasma Workspace
REVISION DETAIL
https://phabricator.kde.org/D11021
To: Pitel, #plasma
Cc: broulik, nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D11021: [Media controller] Add simple volume control
Pitel retitled this revision from "[Media contoller] Add simple volume control" to "[Media controller] Add simple volume control". REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D11021 To: Pitel, #plasma Cc: nicolasfella, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
