Refactor MediaController
Needs ReviewPublic

Authored by cblack on May 3 2020, 11:40 PM.

Details

Reviewers
None
Group Reviewers
Plasma
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.

Diff Detail

Repository
R120 Plasma Workspace
Branch
arcpatch-D29395_1
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 27040
Build 27058: arc lint + arc unit
cblack created this revision.May 3 2020, 11:40 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 3 2020, 11:40 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.May 3 2020, 11:40 PM
ngraham added a subscriber: ngraham.May 4 2020, 2:28 AM

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?

cblack added a comment.May 4 2020, 2:30 AM

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?

... what? This patch doesn't (or shouldn't) result in visual changes.

I don't notice any visual changes on my system.

trmdi added a subscriber: trmdi.May 4 2020, 3:32 AM

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?

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

broulik added a subscriber: broulik.May 4 2020, 3:35 PM

Generally +1
Nice idea with this Media singleton

applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
65

Coding style, space

99

Unrelated cleanup

applets/mediacontroller/contents/ui/Media.qml
7

Make this a QtObject

88

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

142

or map() and then unshift the multiplexer :p

188

Remove debug prints

198

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

applets/mediacontroller/contents/ui/main.qml
155–156

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

159–160

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

243

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)

cblack added inline comments.May 19 2020, 8:43 PM
applets/mediacontroller/contents/ui/ExpandedRepresentation.qml
99

a cleanup is unrelated in a cleanup patch?

cblack updated this revision to Diff 83073.May 19 2020, 9:44 PM
cblack marked 8 inline comments as done.

Address feedback

cblack added inline comments.May 20 2020, 6:25 AM
applets/mediacontroller/contents/ui/Media.qml
7

Seems to cause a segfault with the data source.

cblack updated this revision to Diff 83084.May 20 2020, 6:33 AM

Guess that segfault wants to resolve itself :D

Any update on this? I really like the idea.

meven added a subscriber: meven.Jun 16 2020, 8:53 AM

ping @cblack and reviewers

The commit message could mention the UI changes as well.

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