Review Request 119297: Improve MPRIS2 status message plugin to better handle multiple players

[James Smith]

> On Aug. 8, 2014, 11:55 a.m., David Edmundson wrote:
> > telepathy-mpris.cpp, line 212
> > <https://git.reviewboard.kde.org/r/119297/diff/4/?file=300637#file300637line212>
> >
> >     Why did this approach not work?
> >     This seems like it'd do the same thing in a much simpler way than monitoring all players all the time?

We don't need to or want to clear m_watchedPlayers or its successor m_watchedPlayersInfo every time a player disappears. We now properly account for every player coming and going. This (was) still used when the plugin is activated, and I don't know if it ever actually functioned after a player was removed. If it did detectPlayers() cleared every single player each time a player exited and then slowly rebuilt playback information from playback status updates.


> On Aug. 8, 2014, 11:55 a.m., David Edmundson wrote:
> > telepathy-mpris.cpp, line 204
> > <https://git.reviewboard.kde.org/r/119297/diff/4/?file=300637#file300637line204>
> >
> >     again, we need this.

This filter drops all player unwatch events, only allowing new players. Unwatch events have to remain unfiltered, or the player won't be removed. I now only unwatch the service if it was found in m_watchedPlayersInfo.


> On Aug. 8, 2014, 11:55 a.m., David Edmundson wrote:
> > telepathy-mpris.cpp, line 112
> > <https://git.reviewboard.kde.org/r/119297/diff/4/?file=300637#file300637line112>
> >
> >     you can't delete this. This means we are watching every single dbus service regardless of whether it's a media player or not.

I think this should be (service.startsWith), not (!service.startsWith).


> On Aug. 8, 2014, 11:55 a.m., David Edmundson wrote:
> > telepathy-mpris.h, line 61
> > <https://git.reviewboard.kde.org/r/119297/diff/4/?file=300636#file300636line61>
> >
> >     this name seems backwards.
> >     
> >     setPresenceToTrack?

That can work.


- James


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119297/#review64053
-----------------------------------------------------------


On Aug. 8, 2014, 2:02 p.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119297/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2014, 2:02 p.m.)
> 
> 
> Review request for Telepathy and Martin Klapetek.
> 
> 
> Repository: ktp-kded-module
> 
> 
> Description
> -------
> 
> Adds metadata<->player mapping to better keep track of active players and their associated metadata and playback state. This helps with adding and removing of multiple concurrent players while keeping the output fluid and predictable.
> 
> 
> Diffs
> -----
> 
>   telepathy-mpris.h 05b77c90a50372fd9ed66bde0ab8a287caf34b51 
>   telepathy-mpris.cpp 44b041fdd3764ee5f67598fcf555a2759d853bdd 
> 
> Diff: https://git.reviewboard.kde.org/r/119297/diff/
> 
> 
> Testing
> -------
> 
> Compile, run. Pause / play on multiple players. Close / launch multiple players. Set / unset enabled.
> 
> 
> Thanks,
> 
> James Smith
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20140808/0dc33c7a/attachment-0001.html>