According to the spec, xesam:artist should be a string array, but
majority of players uses string instead. This patch adds support for
both string array and string options to allow new software development
according to the spec but also maintain compatibility with existing
solutions.
BUG: 405762
Details
Details
- Reviewers
broulik - Commits
- R120:1be4bb880fde: [Media Controller] Multiple artists support
Diff Detail
Diff Detail
- Repository
- R120 Plasma Workspace
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Thanks for your patch!
I am not sure if the property should be perhaps changed to a property var and always contain an Array. But that would mean changing the tooltip, full representation, among other things.
I think this is fine.
Do you have commit access? If not, I need an E-Mail address so I can land this for you.
Bonus points if you also look at media controls in task manager (the ones when you hover a window): https://cgit.kde.org/plasma-desktop.git/tree/applets/taskmanager/package/contents/ui/ToolTipInstance.qml#n70
and the lock screen https://cgit.kde.org/plasma-workspace.git/tree/lookandfeel/contents/lockscreen/MediaControls.qml#n73 :)
| applets/mediacontroller/contents/ui/main.qml | ||
|---|---|---|
| 61 | Perhaps this should be turned around to check Array.isArray(xesamArtist) and in doubt have it cast to string? | |
Comment Actions
Do you have commit access? If not, I need an E-Mail address so I can land this for you.
I don't, you can add my email: me@lxlz.space
Bonus points if you also look at media controls in task manager (the ones when you hover a window): https://cgit.kde.org/plasma-desktop.git/tree/applets/taskmanager/package/contents/ui/ToolTipInstance.qml#n70
and the lock screen https://cgit.kde.org/plasma-workspace.git/tree/lookandfeel/contents/lockscreen/MediaControls.qml#n73 :)
I can add the same path, but I don't really use those components so I'm not sure if I can test those properly.
| applets/mediacontroller/contents/ui/main.qml | ||
|---|---|---|
| 61 | That would make much more sence, but (at least for now) QStringList is array-like object and not an actual Array instance, and I don't really feel like I want to mess with all those "is-arrayish" javascript stuff. Strings are way easier to check. | |
| applets/mediacontroller/contents/ui/main.qml | ||
|---|---|---|
| 61 | That's odd, I thought a QStringList would just turn into a plain JS Array on QML side. Fair enough. Will land this for you tomorrow, thanks. | |
Comment Actions
Re: lockscreen/toolbar stuff: maybe the metadata parsing should go into the dataengine instead? I could write up a patch to move the metadata parsing logic there, and then we can have the actual QML bits refer to the specific parsed fields that will come pre-sanitized (probably always converted to a QStringList).