MPRIS control: do not accumulate interface objects
ClosedPublic

Authored by kossebau on Mar 16 2018, 12:57 PM.

Details

Summary

If MPRIS players were appearing and disappearing multiple times, the
OrgFreedesktopDBusPropertiesInterface & OrgMprisMediaPlayer2PlayerInterface
instances created for listening to the signals had been accumulating and
thus resulting in X signals per X restarted player, because the instances
were not deleted when a player disappeared.
Additionally were instances of them created on the fly on the stack in
some of the methods, instead of reusing the existing ones.

This patch changes that by introducing a class MprisPlayer which holds all
data & instances per player. This allows to look up the respective
interfaces instances to reuse them as well as properly controlling their
lifetime.

Test Plan

Starting and restarting multiple MPRIS players (incl. multiple instances of
the same player app) works as expected as befire. They are listed on the
Android Media control as well as have proper states there when selected.
Additionally no longer multiple change signals are emitted if restarting a
player.

Diff Detail

Repository
R224 KDE Connect
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau requested review of this revision.Mar 16 2018, 12:57 PM
kossebau created this revision.
mtijink requested changes to this revision.Mar 16 2018, 5:43 PM
mtijink added a subscriber: mtijink.

Didn't test it yet, but it generally looks like a good patch!

plugins/mpriscontrol/mpriscontrolplugin.cpp
121

This loop can be replaced with std::find_if (also elsewhere in the code).

227

Instead of using raw deletes here, it's better to use std::unique_ptr (or a Qt equivalent?).

This revision now requires changes to proceed.Mar 16 2018, 5:43 PM

Thanks for first review :)

plugins/mpriscontrol/mpriscontrolplugin.cpp
121

I considered that, but did not find an example with QHash, so was unsure if that works in all versions of Qt that should be supported. Also from what i sketched did it not really simplify the code. But will try, then we/you can compare & comment :)

227

You are right, it makes sense to move memory management completely over to MprisPlayer instances (shared pointer though, given MprisPlayer is moved around by value). To shy before to go the full path :)

kossebau updated this revision to Diff 29704.Mar 16 2018, 8:48 PM
  • use std::find_if
  • let MprisPlayer completely life-time-manage the interface objects
mtijink accepted this revision.Mar 16 2018, 10:37 PM

Looks good to me!

This revision is now accepted and ready to land.Mar 16 2018, 10:37 PM
This revision was automatically updated to reflect the committed changes.