Make the player status a per-player object in the MPRIS plugin
ClosedPublic

Authored by mtijink on Nov 22 2017, 1:29 PM.

Details

Summary

This directly fixes a couple of bugs (where the UI was not updated in all cases) and will allow different code parts to use different players without clashing with each other.

This is required for sensible behaviour while using the media control notification (which I plan to work on).

Diff Detail

Repository
R225 KDE Connect - Android application
Branch
mpris-data-per-player
Lint
No Linters Available
Unit
No Unit Test Coverage
mtijink created this revision.Nov 22 2017, 1:29 PM

Any comments? I have a couple of changes depending on this, so I'd like to get this merged.

I think you should give MprisPlayer a reference to MprisPlugin and move the setters into MprisPlayer too.
Then change MprisActivity.targetPlayer to be a MprisPlayer.

Otherwise this looks like an improvement. Just my 2 cents.

src/org/kde/kdeconnect/Plugins/MprisPlugin/MprisActivity.java
156

targetPlayer is always null here. Please simplify beginning from line 143.

207

MprisPlayer could report correct info.

mtijink updated this revision to Diff 23087.Nov 28 2017, 1:06 PM

Thanks for the feedback! I incorporated the suggested changes (controlling a player via the new MprisPlayer object directly instead of via the plugin and letting the MprisPlayer object report correct properties even for Spotify).

The diff is a bit larger this way, but the end result is cleaner.

More suggestions:
MprisPlugin.sendAction(), MprisPlugin.setVolume(), MprisPlugin.setPosition(), MprisPlugin.seek() look quite alike and could be collapsed.
Add MprisPlayer.next(), MprisPlayer.previous() and MprisPlayer.playPause() in favour of sendAction("MAGIC").

mtijink updated this revision to Diff 23130.Nov 29 2017, 12:25 PM

Simplify MprisPlugin/MprisPlayer API

thomasp accepted this revision.Nov 29 2017, 2:01 PM

MprisPlayer.Seek() is the only one starting with a capital.
Then ship it.

This revision is now accepted and ready to land.Nov 29 2017, 2:01 PM
mtijink updated this revision to Diff 23141.Nov 29 2017, 2:42 PM

Done. Thanks for the feedback!

Could you commit it for me? I have no commit rights.

thomasp added a subscriber: apol.Nov 30 2017, 3:56 AM

I can not. Maybe you can ping @apol.

I'd like to know @albertvaka's opinion on it, I'm not that confident on the Android codebase just yet.

I'm a bit busy, and haven't tested it, but looks good to me. Thanks for the patch! @apol can you merge it?

@mtijink if you plan on contributing more to the app, you should consider asking for a KDE developer account, once you have some patches like this one in :)

Sorry for having code reviews like this one hanging for long recently, I've changed roles at work and I'm still getting used to it. Thankfully there is people taking care of the reviews without me :D

albertvaka accepted this revision.Nov 30 2017, 8:43 PM
This revision was automatically updated to reflect the committed changes.

This might come late, but is there a reason for the MprisPlayer class not to be in it's own file?

This might come late, but is there a reason for the MprisPlayer class not to be in it's own file?

The MprisPlugin requires private access to MprisPlayer to set its properties and MprisPlayer requires private access to MprisPlugin to send commands. This is the easiest way to do that, but it could of course be done in separate files.