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 - 5000000) // 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 <https://phabricator.kde.org/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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200504/32ac519c/attachment-0001.html>