Differential D3667

[MPRIS Dataengine] Let clients distinguish media players by process id more easily
ClosedPublic
Actions

Authored by romangg on Dec 13 2016, 4:36 PM.

Details

Reviewers
davidedmundson
Group Reviewers
Plasma
Commits
R871:d78b63b9ea77: [MPRIS Dataengine] Export process id of player instance as separate data entry
R120:d78b63b9ea77: [MPRIS Dataengine] Export process id of player instance as separate data entry
Summary

The MPRIS specification recommends to media players to distinguish instances of them by appending its process id. For example this is supported by the Dragon Player. The patch makes it possible for connected clients of MPRIS Dataengine to query this directly and by that distinguish multiple instances of a player.

While clients were able to do this earlier already by checking the source name and doing the same string operations as here, it's more convenient with this patch, since they then don't have to do it on their own anymore but just need to query the data.

Test Plan

Tested it with my prototype of the redesigned taskbar.

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
romangg updated this revision to Diff 8974.Dec 13 2016, 4:36 PM
romangg retitled this revision from to [MPRIS Dataengine] Let clients distinguish media players by process id more easily.
romangg updated this object.
romangg edited the test plan for this revision. (Show Details)
romangg added a reviewer: Plasma.
romangg set the repository for this revision to R120 Plasma Workspace.
romangg added a project: Plasma.
romangg added a subscriber: plasma-devel.
broulik added a subscriber: broulik.Dec 13 2016, 6:16 PM

LGTM overall

"Prototype of redesigned taskbar" made me curious ;)

dataengines/mpris2/playercontainer.cpp
123

QLatin1Char?

124

Can you use rightRef and the like, if possible?

128
bool ok;

is sufficient in that context imho

davidedmundson added a subscriber: davidedmundson.Dec 13 2016, 6:44 PM
davidedmundson added inline comments.
dataengines/mpris2/playercontainer.cpp
119

There is a way to do it which will work for all players much more reliably:

auto message = QDBusMessage::createMethodCall("org.freedesktop.DBus", "/", "org.freedesktop.DBus", "GetConnectionUnixProcessID");
message.setArguments({m_dbusAddress});

QDBusReply<uint> reply = QDBusConnection::sessionBus().call(message);

//safe to be blocking as we're calling a method on DBus server not a client, so no different from QDBusConnection::connect()

 if (!reply.isError) {
   secondaryInstancePid = reply.value();
}
davidedmundson added inline comments.Dec 13 2016, 6:52 PM
dataengines/mpris2/playercontainer.cpp
119

Ninja edit.

If you do go with that approach don't use the code above.

Use
secondaryInstancePid = QDBusConnectionInterface::servicePid(m_dbusAddress).value();

romangg updated this revision to Diff 9024.Dec 14 2016, 10:58 PM

Using David's superior method. With this method it's also possible to have in the VLC case, where they don't append the pid, atleast the one controllable media player instance identified.

romangg updated this revision to Diff 9025.Dec 14 2016, 11:09 PM
davidedmundson accepted this revision.Dec 15 2016, 11:48 AM
davidedmundson added a reviewer: davidedmundson.
This revision is now accepted and ready to land.Dec 15 2016, 11:48 AM
Closed by commit R120:d78b63b9ea77: [MPRIS Dataengine] Export process id of player instance as separate data entry (authored by romangg). · Explain WhyDec 15 2016, 12:26 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 9031

View Options

dataengines/mpris2/playercontainer.cpp

Loading...