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
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,
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.
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
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
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
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
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
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
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
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
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
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
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,
14 matches
Mail list logo
