Add MPRIS media control notification
ClosedPublic

Authored by mtijink on Dec 9 2017, 12:57 PM.

Details

Summary

BUG: 337485

Adds a notification to show and control mpris players. It shows the title, artist etc. (so depends on D9083, but can easily be adapted to work without it, but that leads to less features).

The notification appears as soon as one of your connected devices plays music. If multiple devices/players are playing, it shows the information and controls for only one of these. If it stops playing, it tries to switch to another playing device/player. If those do not exist, it shows the same player, but allows starting it again.

Dismissing the notification is only possible if the showed player is paused (as effect, only when all players are paused). It automatically closes if the device or player disappears or disconnects and no other players are playing. I think this behaviour is intuitive, other native android music players have similar behaviour.

About the implementation: there are two parts to this: the notification and the media session control API. The first shows the notification and its controls. The second allows lock screen controls on older Android versions and control using e.g. headphone buttons. Since nearly all code is shared between the two parts, and the rest is mostly straightforward, I put them in the same diff.

Test Plan

Tested on Android Nougat 7.1 (shows the notification with buttons, as expected; no lock screen controls, as expected) and Android Gingerbread 2.3 (shows a notification without buttons, lock screen controls work, so as expected).

I am not able to test with multiple desktops, so testing that would be appreciated.

Disabling buttons when not available should work, but all players I tested always allowed next/previous/play/pause.

Diff Detail

Repository
R225 KDE Connect - Android application
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mtijink requested review of this revision.Dec 9 2017, 12:57 PM
mtijink created this revision.

Thanks for working on this! I would like to have an option to disable the notification in the plugin settings because I can imagine some people may not like to have a persistent notification.

mtijink updated this revision to Diff 23754.Dec 11 2017, 11:04 AM

I added such a preference to enable/disable the notification. Initially I wanted to make this a per-device setting, but there does not seem to be any way to do that easily.

Works well so far :) Some small things I noticed: Toggling the option doesn't activate/deactivate the notification immediately. When my laptop screen turns off plasma pauses the media player, but the notification is still in play mode. Both are not critical, but a fix would be nice if it's not too complex.

nicolasfella added inline comments.Dec 12 2017, 2:50 PM
res/values/strings.xml
156

I would go with 'media players' instead of 'music' because it could be a video as well

mtijink updated this revision to Diff 23818.Dec 12 2017, 3:48 PM

Reworded notification preference description, as suggested.

mtijink marked an inline comment as done.Dec 12 2017, 3:59 PM

Works well so far :) Some small things I noticed: Toggling the option doesn't activate/deactivate the notification immediately.

I know (should have mentioned that, though). It's because the plugin doesn't reload and doesn't get notified in any way of the preference change. I figured that this is the simplest solution, as the notification appears/disappears on the next track/device/player list changes. In general, I think it's good if the plugin can be notified of such changes, or automatically reloads, but I think that's out of scope for this diff.

When my laptop screen turns off plasma pauses the media player, but the notification is still in play mode. Both are not critical, but a fix would be nice if it's not too complex.

Does that close the connection? If so, KDE Connect generally does not notice that immediately, so nothing this code can do about that. Closed connections that KDE Connect knows about are handled correctly, as far as I know. Could you supply some more information so I can look into it?

Could you supply some more information so I can look into it?

Not until next week

Did you have time to look at it yet?

While suspending (e.g. by closing the laptop lid) the connection seems to be interrupted, so it seems like we can't do anything about it. Both issues are minor and out of scope for this patch, so it looks good to me, but I would like to hear @albertvaka or @apol 's opinion.

Any further comments?

You could use the artwork as notification icon :)

You could use the artwork as notification icon :)

I was indeed planning to add that (and lockscreen album art), but as a follow-up diff since that depends on D9564.

Sorry that some reviews take so long. For large patches I like to wait for Albert's opinion because he has far more experience than me. He seems to be quite busy at the moment. I try to give valuable feedback, but sometimes I don't feel confident enough to give the final thumbs up. Nevertheless, keep up the good work :)

nicolasfella edited the summary of this revision. (Show Details)Jan 17 2018, 12:27 AM

Sorry that some reviews take so long. For large patches I like to wait for Albert's opinion because he has far more experience than me. He seems to be quite busy at the moment. I try to give valuable feedback, but sometimes I don't feel confident enough to give the final thumbs up. Nevertheless, keep up the good work :)

I understand, I'd feel the same. @albertvaka, could you look at this sometime? The request has been open for some time already.

Works well so far :) Some small things I noticed: Toggling the option doesn't activate/deactivate the notification immediately.

I know (should have mentioned that, though). It's because the plugin doesn't reload and doesn't get notified in any way of the preference change. I figured that this is the simplest solution, as the notification appears/disappears on the next track/device/player list changes. In general, I think it's good if the plugin can be notified of such changes, or automatically reloads, but I think that's out of scope for this diff.

Maybe https://developer.android.com/guide/topics/ui/settings.html#ReadingPrefs can help :)

mtijink updated this revision to Diff 27492.Feb 18 2018, 8:19 PM

Nice, that works!

nicolasfella requested changes to this revision.Feb 22 2018, 12:44 PM

Works pretty good, but I found one issue. To reproduce:

Install Plasma Browser Integration (other players might work too)
Go on Youtube and play a video while MPRISActivity is open
Go to Youtube start page
Hit pause

App crashes with:

FATAL EXCEPTION: main

Process: org.kde.kdeconnect_tp, PID: 23125
android.app.RemoteServiceException: Bad notification posted from package org.kde.kdeconnect_tp: Couldn't inflate contentViewsjava.lang.IllegalArgumentException: setShowActionsInCompactView: action 0 out of bounds (max -1)
    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1870)
    at android.os.Handler.dispatchMessage(Handler.java:105)
    at android.os.Looper.loop(Looper.java:164)
    at android.app.ActivityThread.main(ActivityThread.java:6809)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:240)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:767)
This revision now requires changes to proceed.Feb 22 2018, 12:44 PM

Another issue:

When the connection is lost (and we know about it) the notification keeps showing and cannot be closed (because it's in playing state).
The plugings onDestroy() method might be useful here.

mtijink updated this revision to Diff 27922.Feb 24 2018, 3:10 PM

Fix crash when no buttons were available to push

Another issue:

When the connection is lost (and we know about it) the notification keeps showing and cannot be closed (because it's in playing state).
The plugings onDestroy() method might be useful here.

The code already does that, and I can't reproduce it. Could you provide more details when this happens?

nicolasfella accepted this revision.Feb 24 2018, 3:39 PM

I can't reproduce it either right now. Let's merge it for now and see if we can reproduce it in daily usage

This revision is now accepted and ready to land.Feb 24 2018, 3:39 PM
This revision was automatically updated to reflect the committed changes.