D12559: [WIP] Add MprisReceiverPlugin (Android)

[Matthijs Tijink]

mtijink requested changes to this revision.
mtijink added a comment.
This revision now requires changes to proceed.


  Generally looks quite good, but didn't test it yet.
  
  > What doesn't work:
  > 
  > - volume control
  
  Maybe we should expose a `canControlVolume` on our protocol: even though MPRIS doesn't support it, we can use it ourselves for these cases.
  
  > - length and position
  > - Album art
  > - can* abilities are not exposed, e.g. canGoNext, not sure if possible at all
  
  Maybe this is useful? https://developer.android.com/reference/android/support/v4/media/session/PlaybackStateCompat

INLINE COMMENTS

> MprisReceiverCallback.java:39
> +    private final MprisReceiverPlayer player;
> +    private MprisReceiverPlugin plugin;
> +

No `final` here?

> MprisReceiverPlayer.java:95
> +    int getVolume() {
> +        return (int) (((float) controller.getPlaybackInfo().getCurrentVolume()) / ((float) controller.getPlaybackInfo().getMaxVolume())) * 100;
> +    }

I think this can be simplified to `100 * controller.getPlaybackInfo().getCurrentVolume() / controller.getPlaybackInfo().getMaxVolume()`, removing all the casts.

> MprisReceiverPlugin.java:63
> +
> +            manager.addOnActiveSessionsChangedListener(MprisReceiverPlugin.this, new ComponentName(context, NotificationReceiver.class), new Handler(Looper.getMainLooper()));
> +

Best to check if the notifications plugin is enabled and has it's required permissions first. Also, check that in the plugin's requirements.

> MprisReceiverPlugin.java:152
> +        }
> +        createPlayers(controllers);
> +        sendPlayerList();

If controller get removed, does this get called too? If so, we don't ever remove controllers from the list.

> MprisReceiverPlugin.java:187
> +        NetworkPacket np = new NetworkPacket(MprisReceiverPlugin.PACKET_TYPE_MPRIS);
> +        np.set("player", player.getId());
> +        np.set("nowPlaying", player.getTitle());

Don't we have a "pretty name"?

> MprisReceiverPlugin.java:188
> +        np.set("player", player.getId());
> +        np.set("nowPlaying", player.getTitle());
> +        np.set("title", player.getTitle());

On the desktop, this sends `artist - title` (or `title` if the artist is empty).

REPOSITORY
  R225 KDE Connect - Android application

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

To: nicolasfella, #kde_connect, mtijink
Cc: mtijink, #kde_connect, yannux, Danial0_0, johnq, Pitel, adeen-s, SemperPeritus, ahmedbesbes, ndavis, daniel.z.tg, jeanv, seebauer, bugzy, MayeulC, menasshock, ach, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdeconnect/attachments/20180427/99a95c84/attachment.html>