[WIP] MPRIS control plugin: fix status init
AbandonedPublic

Authored by kossebau on Mar 15 2018, 7:11 PM.

Details

Reviewers
None
Group Reviewers
KDE Connect
Summary

On connecting to a new MPRIS service appearing the initial status of the
properties was not fetched and reported. So properties which never change
their status have been missed to get the proper value.

Test Plan

Use Gwenview from master and see all control buttons properly appear in the
Android KDE Connect media controller plugin.

Diff Detail

Repository
R224 KDE Connect
Branch
fixmprisstatusinit
Lint
No Linters Available
Unit
No Unit Test Coverage
kossebau requested review of this revision.Mar 15 2018, 7:11 PM
kossebau created this revision.

Actually not yet completly tested, currently fighting to get my custom kdeconnect-kde build running. Seems installs of D-Bus service files into custom folders hides them from dbus server, despite being in XDG* var paths, so kdeconnectd is not started. Any hints how to solve that?

kossebau retitled this revision from MPRIS control plugin: fix status init to [WIP] MPRIS control plugin: fix status init.Mar 15 2018, 7:35 PM

Solved my dbus issue for now by putting the d-bus service file in global path and removed the packaged kdeconncet.

To find this patch actually introduced a crash, eek. Sorry for premature pushing for review. At least you know now I am on this :)

Are you sure this is needed? It appears to transfer CanPause etc. just fine when I start an MPRIS player, without the patch. Do I need the git-master gwenview to reproduce the issue?

Actually not yet completly tested, currently fighting to get my custom kdeconnect-kde build running. Seems installs of D-Bus service files into custom folders hides them from dbus server, despite being in XDG* var paths, so kdeconnectd is not started. Any hints how to solve that?

You can start kdeconnectd manually (located in build/daemon). Not sure how it picks up plugins etc. in that case though.

kossebau updated this revision to Diff 29640.Mar 15 2018, 11:18 PM

non-crashing version, with a qDebug() where the (missing) data passing is

Are you sure this is needed? It appears to transfer CanPause etc. just fine when I start an MPRIS player, without the patch.

From the debug logs I looked at, I would still think this is needed. Even if it seems just one of a few issues which might need fixing.

While most players will change some of the properties after having been started, thus trigger the update signal, they do not need to do. Or if the KDE Connect MPRIS plugin is activated if the player is already running, then there might be even less state changes with related propertychanged signals coming in.

I left a qDebug() log in the updated (no longer crashing patch) which should show what I mean.

There also seems another issue which can be seen by that log: when closing and then starting a player again, there will be multiple invocations of MprisControlPlugin::propertiesChanged(...), as many times as the player had been restarted. Possibly the OrgFreedesktopDBusPropertiesInterface & OrgMprisMediaPlayer2PlayerInterface instances need some manual cleanup if a service disappears, otherwise they pile up for the same service name. But would be an orthogonal issue to the problem I am looking at.

Do I need the git-master gwenview to reproduce the issue?

Yes, from as latest master as possible, D10972, which brought the MPRIS stuff to gwenview, only landed a day ago.

It seems that the set of MPRIS data used is resulting in unexpected states with the Android KDE Connect media payer remote control plugin. Which then shows almost no buttons for the actions.
Sadly I have never done Android code, so cannot tell what could be wrong or even try to come up with patches. I just can tell that the Plasma media controller plugin works perfectly with Gwenview, so my initial working theory is that the issues are not in Gwenview :)

Actually not yet completly tested, currently fighting to get my custom kdeconnect-kde build running. Seems installs of D-Bus service files into custom folders hides them from dbus server, despite being in XDG* var paths, so kdeconnectd is not started. Any hints how to solve that?

You can start kdeconnectd manually (located in build/daemon). Not sure how it picks up plugins etc. in that case though.

Thanks. Turned to something similar meanwhile, starting the custom installed one manually in a shell, also to get all debug log there.

Do you run the latest Android version, specifically does it include https://cgit.kde.org/kdeconnect-android.git/commit/?id=959c7c722033d41cd8c625da45aa11c8c46f0e06 ?

No, only the APK v1.7.2 downloaded directly from F-Droid, Are there some nightly builds available somewhere?

nicolasfella added a comment.EditedMar 15 2018, 11:39 PM

We released 1.8 like 10 minutes ago :D We don't really do nightlys, but building from git isn't hard. Basically you just clone the repo, open it in Android Studio, connect your phone via USB, activate USB debugging on the phone and hit 'Play' in Android Studio. There should also be a way to build it without Android Studio, but I've never done that

kossebau added a comment.EditedMar 15 2018, 11:45 PM

We released 1.8 like 10 minutes ago :D

Edit: Congrats to the new release achievement :)

Any chance of some APK with that somewhere without hidden in a software store? Because... see below.

We don't really do nightlys, but building from git isn't hard. Basically you just clone the repo, open it in Android Studio, connect your phone via USB, activate USB debugging on the phone and hit 'Play' in Android Studio

Small challenge for myself: my phone runs Sailfish OS, no Android device currently around. Not sure I want to start with Android Studio & stuff though, rather would invest time into helping with the sailfish kdeconnect variant. So would be happy if I can save time for now with existing APKs :)

I see, I'll build one for you

nicolasfella added a comment.EditedMar 15 2018, 11:52 PM

Let me know whether it works, I didn't sign it, might need to do that as well

Let me know whether it works, I didn't sign it, might need to do that as well

Thanks @nicolasfella ! Yes, install worked.
And I can report in the app itself also the Previous/Next buttons now work as expected when remote controlling Gwenview :)
Small improvement suggestion: for applications which have CanSeek=false, it might be still nice to see a progressbar to have an idea how much of the track is played.
Cmp. similar change to Plasma media controller applet done today in D11356 (yes, same person poking there for same reason :) ).

As well as for Okular with D11250 applied :)
Though there the mpris:artUrl , which is a url to a file with the PNG thumbnail of the page in the /tmp dir, seems not result in the respective upload, will look more into that soon.

Still, by the debug log this patch here could still make sense for a proper values init.

nicolasfella added a comment.EditedMar 16 2018, 12:24 AM

Artwork support for local thumbnails is in progress D11018 & D11017

Artwork support for local thumbnails is in progress D11018 & D11017

I remembered I had seen such code, but so it was only in review requests. Thanks for pointers, put on radar.

apol added a subscriber: apol.Mar 16 2018, 12:59 AM

You can always run kdeconnectd by hand, no need to let the system automatically execute it.

Playing some more with 1.8 and various orders of app/devices appearing/disappearing I cannot longer reproduce any issues. So possibly the debug log here was misleading me, and the initial status data fed by way already I yet have to discover, Time to make myself more familiar with things. So would be discarding this patch tomorrow, unless I find it still makes sense.

At least I found the seemingly not garbage collected interface objects by my initial clueless poking, will hunt those down some more instead for now :)

sredman added a subscriber: sredman.EditedMar 16 2018, 1:57 AM
In D11366#226924, @apol wrote:

You can always run kdeconnectd by hand, no need to let the system automatically execute it.

Not to hijack this conversation, but I have (also) had trouble getting it to load plugins in the case I have compiled it myself... My current workflow is very inefficient (build a non-debug rpm...) Is there some trick to how a self-compile should be installed or run?

Well, what I am doing is uninstalling the packaged version and installing it in the 'normal' directories using

mkdir build && cd build
cmake .. -DCMAKE_INSTALL_PREFIX=/usr -DLIB_INSTALL_DIR=lib -DLIBEXEC_INSTALL_DIR=lib
sudo make install

There are ways to not override the default installation, but since its not a critical part of the system its not a problem if I mess something up

kossebau abandoned this revision.Mar 16 2018, 12:57 PM

okidoki. now I got a better idea of the code. And learned that the remote (e.g. the android mpris plugin) asks for a complete status update by the "requestNowPlaying" command once it is interested in a player. Thus this patch here is unneeded indeed.