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

[David Edmundson]

> 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.
> 
> James Smith wrote:
>     I think this should be (service.startsWith), not (!service.startsWith).

well this did
if (!service.startsWith) {
continue;
}
//rest of code

which is the same as

if (service.startsWith) {
 //rest of code;
}

I don't have a preference but you can't remove it.


> 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.

but you're now including things that aren't players.


> 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.

>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?


- David


-----------------------------------------------------------
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/8bc5f8c3/attachment.html>