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 204
> > <https://git.reviewboard.kde.org/r/119297/diff/4/?file=300637#file300637line204>
> >
> >     again, we need this.
> 
> James Smith wrote:
>     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.
> 
> David Edmundson wrote:
>     but you're now including things that aren't players.

Looking in m_watchedPlayersInfo for the unregistering player is only slightly more expensive than checking for the mprisServicePrefix in serviceName and performs essentially identically. So instead, m_watchedPlayersInfo is now checked for a matching serviceName before disconnecting the relevant matching player, if any. This should be acceptable as a workaround, given the very large number of running players theoretically required for it to become prohibitively expensive.


> 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?
> 
> James Smith wrote:
>     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.
> 
> David Edmundson wrote:
>     >If it did detectPlayers() cleared every single player each time a player exited and then slowly rebuilt playback information from playback status updates.
>     
>     Well it's not slow, it does a query right away, no?

Keeping track of and deactivating players and their metadata mappings properly and accurately is still far more correct than removing and redetecting all players and their associated metadata after every single player deactivation.


- James


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


On Aug. 8, 2014, 3:30 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, 3:30 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/06632dc8/attachment.html>