broulik added a comment.

  Other than those nitpicks would be good to go I guess

INLINE COMMENTS

> main.qml:91
>              if (root.state == "playing") {
> -                plasmoid.setAction("playPause", i18nc("Pause playback", 
> "Pause"), "media-playback-pause")
> -                plasmoid.action("playPause").enabled = Qt.binding(function() 
> {
> -                    return root.canPause;
> -                });
> +                if (canPause) {
> +                    plasmoid.setAction("pause", i18nc("Pause playback", 
> "Pause"), "media-playback-pause")

Shouldn't that be a binding? If the player cannot pause, the entry would just 
not show up rather than become disabled, ie. keep the action as "playPause" 
maybe?

> main.qml:149
> +                root.state === "paused" ?  "media-playback-pause" :
> +                                           "media-playback-stop"
>          active: compactMouse.containsMouse

Not a huge fan of that stopped icon in the panel

> main.qml:203
> +    function togglePlaying() {
> +        if (root.canPause) {
> +            root.action_playPause();

Please don't mix manual toggling and playPause, just do it manually then, 
otherwise the code becomes a bit convoluted

REPOSITORY
  R120 Plasma Workspace

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

To: kossebau, broulik
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

Reply via email to