D29395: Refactor MediaController

[Ismael Asensio]

iasensio added a comment.


  Any chance to retake this and move it to invent?
  
  This is really useful for an extension of the media controller I'm 
mantaining, and will help to eventually upsream it (al least partially)
  I have a branch based on this MR with some fixes to it, and collaboration in 
gitlab seems easier

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: meven, kmaterka, iasensio, broulik, trmdi, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D29395: Refactor MediaController

[Méven Car]

meven added a comment.


  ping @cblack and reviewers
  
  The commit message could mention the UI changes as well.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: meven, kmaterka, iasensio, broulik, trmdi, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D29395: Refactor MediaController

[Konrad Materka]

kmaterka added a comment.


  Any update on this? I really like the idea.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: kmaterka, iasensio, broulik, trmdi, ngraham, plasma-devel, Orage, 
LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, 
ZrenBot, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D29395: Refactor MediaController

[Carson Black]

cblack updated this revision to Diff 83084.
cblack added a comment.


  Guess that segfault wants to resolve itself :D

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29395?vs=83073=83084

BRANCH
  arcpatch-D29395_1

REVISION DETAIL
  https://phabricator.kde.org/D29395

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/Media.qml
  applets/mediacontroller/contents/ui/main.qml
  applets/mediacontroller/contents/ui/qmldir

To: cblack, #plasma
Cc: iasensio, broulik, trmdi, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29395: Refactor MediaController

[Carson Black]

cblack added inline comments.

INLINE COMMENTS

> broulik wrote in Media.qml:6
> Make this a `QtObject`

Seems to cause a segfault with the data source.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: iasensio, broulik, trmdi, ngraham, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D29395: Refactor MediaController

[Carson Black]

cblack updated this revision to Diff 83073.
cblack marked 8 inline comments as done.
cblack added a comment.


  Address feedback

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29395?vs=81831=83073

BRANCH
  arcpatch-D29395

REVISION DETAIL
  https://phabricator.kde.org/D29395

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/Media.qml
  applets/mediacontroller/contents/ui/main.qml
  applets/mediacontroller/contents/ui/qmldir

To: cblack, #plasma
Cc: broulik, trmdi, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Carson Black]

cblack added inline comments.

INLINE COMMENTS

> broulik wrote in ExpandedRepresentation.qml:99
> Unrelated cleanup

a cleanup is unrelated in a cleanup patch?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: broulik, trmdi, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Kai Uwe Broulik]

broulik added a comment.


  Generally +1
  Nice idea with this `Media` singleton

INLINE COMMENTS

> ExpandedRepresentation.qml:65
> +}
> +Media.lockPositionUpdate= false
> +}

Coding style, space

> ExpandedRepresentation.qml:99
>  seekSlider.value = Math.max(0, seekSlider.value - 500) 
> // microseconds
> -seekSlider.moved();
> +seekSlider.moved()
>  } else if (event.key === Qt.Key_Right || event.key === Qt.Key_L) 
> {

Unrelated cleanup

> Media.qml:6
> +
> +Item {
> +id: media

Make this a `QtObject`

> Media.qml:87
> +
> +readonly property int play: 0
> +readonly property int pause: 1

Just pass the action string through, since all we do below is map the number 
back to a string

> Media.qml:141
> +sources.filter(source => source !== mpris2Source.multiplexSource)
> +   .forEach(source => {
> +   model.push({

or `map()` and then `unshift` the multiplexer :p

> Media.qml:187
> +function togglePlaying() {
> +print(Media.state)
> +if (Media.state === "playing" && Media.canPause) {

Remove debug prints

> Media.qml:197
> +
> +states: [
> +State {

Turn this into a property with an `if` when this is no longer an `Item`

> main.qml:155
>  name: "playing"
> -when: !root.noPlayer && mpris2Source.currentData.PlaybackStatus 
> === "Playing"
> +when: Media.state == "playing"
>  

You can just set `state: Media.state` rather than `when` on every state

> main.qml:159
>  target: plasmoid
> -icon: albumArt ? albumArt : "media-playback-playing"
> -toolTipMainText: track
> -toolTipSubText: artist ? i18nc("by Artist (player name)", 
> "by %1 (%2)", artist, identity) : identity
> +icon: Media.albumArt ? Media.albumArt : 
> "media-playback-playing"
> +toolTipMainText: Media.currentTrack

We don't set album art on the icon anymore, cf D28917 


> main.qml:243
> -
> -function action_open() {
> -serviceOp(mpris2Source.current, "Raise");

You can't remove these, they are wired up to the `plasmoid.setAction` calls 
above. (Yes, I'd like to have an API to pass a JS callback :p but this is how 
it works right now)

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: broulik, trmdi, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Tranter Madi]

trmdi added a comment.


  In D29395#662576 , @ngraham wrote:
  
  > Now the inner image has a greater top margin than bottom margin. Also, 
could the artist and album be allowed to become two-line strings before 
eliding, maybe? F8282138: Screenshot_20200503_202710.png 

  
  
  There seems to be lots of wasted space whereas the text is truncated ?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: trmdi, ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Carson Black]

cblack added a comment.


  In D29395#662576 , @ngraham wrote:
  
  > Now the inner image has a greater top margin than bottom margin. Also, 
could the artist and album be allowed to become two-line strings before 
eliding, maybe? F8282138: Screenshot_20200503_202710.png 

  
  
  ... what? This patch doesn't (or shouldn't) result in visual changes.
  
  I don't notice any visual changes on my system.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Nathaniel Graham]

ngraham added a comment.


  Now the inner image has a greater top margin than bottom margin. Also, could 
the artist and album be allowed to become two-line strings before eliding, 
maybe? F8282138: Screenshot_20200503_202710.png 


REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D29395

To: cblack, #plasma
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D29395: Refactor MediaController

[Carson Black]

cblack created this revision.
cblack added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
cblack requested review of this revision.

REVISION SUMMARY
  MediaController is refactored to:
  
  1. make the code easier to read and therefore more maintainable
  2. stop dumping errors like there's no tomorrow into the console

TEST PLAN
  Use, ensure no regressions are found.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  cblack/refactor-mediacontroller (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29395

AFFECTED FILES
  applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
  applets/mediacontroller/contents/ui/Media.qml
  applets/mediacontroller/contents/ui/main.qml
  applets/mediacontroller/contents/ui/qmldir

To: cblack, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart