Review Request 116940: State-affinity and engaging now Playing from a custom status message fixes.

Sat May 31 07:35:31 UTC 2014

> On May 8, 2014, 9:45 p.m., David Edmundson wrote:
> > status-handler.cpp, line 128
> > <https://git.reviewboard.kde.org/r/116940/diff/11/?file=271992#file271992line128>
> >
> >     Martin, is this true?
> >     This seems really bad
> 
> Martin Klapetek wrote:
>     This was not added by me --> 
>     
>     commit fffcc2e011b6380a5478a9833d4dcd331b7906e6
>     Author: James Smith <smithjd15 at gmail.com>
>     Date:   Thu Feb 13 11:22:40 2014 +0100
>     
>         Fix delayed status update for KTp
>     
>         Fixes bug where state changes are slow to be returned to user-set values
>         after autoaway / screensaveraway interruption.
>     
>         REVIEW: 114569
>     
>     
>     ...however it was reviewed by me.
>     
>     Patch: https://git.reviewboard.kde.org/r/114569/diff/
> 
> James Smith wrote:
>     I don't know if this is necessarily bad, but a consequence of mpris2 polling. I think the mpris2 plugin needs to be re-worked to better take into account multiple players and then it might also be possible to filter the extra ticks in the plugin. It's not a bad addition here.
> 
> David Edmundson wrote:
>     Seems you're right. Good spot.
>     Fixed properly hopefully.
> 
> Martin Klapetek wrote:
>     What mpris polling are you talking about? It's fully signal based, there is no polling...
> 
> James Smith wrote:
>     The Juk player combined with the VLC Phonon backend emits > 20 presence changes per track change. This is a fairly bad state of affairs. There should be a lower-level filter to prevent flooding MC account plugins with presence message updates.
> 
> Martin Klapetek wrote:
>     Right; that's that player's bug and should be fixed there in the first place. Have you filed a bug? Does any other player have similar problems?
> 
> James Smith wrote:
>     Most of the players I tested with GStreamer + Phonon emitted a track change between twice and six times. It varies between players with the general trend being no less than two emits per track change. Dragon, Juk, Clementine, Tomahawk all had this problem. Juk with VLC Phonon backend was the worst performer with ~26 track change emits, the VLC backend was often similarly performing with different players, emitting close to the same number of re-signals. The results were sometimes different between first play and later plays. Overall, I think wide variations in implementing the mpris2 specification cause this issue, and the best possible way to ignore it is by making sure that the presence change is only emitted once. A number of players even cause a presence change re-signal upon opening with no file playing. I don't know if it's worth going to the trouble of filing bugs, simply because of the massive number of players involved that implement mpris2 in any number of different languages to varying degrees of compliance.
> 
> Martin Klapetek wrote:
>     I just tested it - you're right. Juk is broken (or some part underneath), it sends "PropertiesChanged" signal which change different properties, but metadata actually stay the same (and for some reason are part of the dbus message). Couldn't reproduce at all with Clementine.
>     
>     Anyways, I've pushed a fix for this in both 0.8 branch and master, so should be fixed now.
> 
> James Smith wrote:
>     Excessive PropertiesChanged signals might have been implicated in bug #334492.

bug #334492 wasn't completely fixed by this commit, either. It looks like the kded module gets itself into a race condition with QtTelepathy where the boolean isn't properly set when used in onPresenceChanged to check for status message plugin enabled. Occasionally the isActiveStatusMessagePlugin() boolean returns inactive and the global presence is from a status message plugin. This is usually accompanied by unusually large numbers of status message changes from the now Playing plugin, but occurs much quicker with multiple players and large numbers of multiple mpris2 events where the kded module is processing a recent change of the global presence and the now Playing plugin has also recently setActive'd false. This allows the status-message-altered global presence to be saved to disk as user-set. Meanwhile the status message plugin could have yet another status change with an activate() in-progress to disable the now playing plugin or possibly a next track event involving a setActive to true with an activate(), both factors that could cause a global presence saturation and allow the isActiveStatusMessagePlugin() boolean to be out-of-sync with the global presence. With more than one player it must be easier to catch a false isActiveStatusMessagePlugin() with an active status-plugin-message-set status message part of the session's current global requested/current presence. This issue was also seen with the current code from master, not using boolean checks.

- James

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

On May 14, 2014, 11:56 p.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116940/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 11:56 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Bugs: 332082, 334492 and 334542
>     http://bugs.kde.org/show_bug.cgi?id=332082
>     http://bugs.kde.org/show_bug.cgi?id=334492
>     http://bugs.kde.org/show_bug.cgi?id=334542
> 
> 
> Repository: ktp-kded-module
> 
> 
> Description
> -------
> 
> This patch returns the ability to engage status message plug-ins from custom status messages. Also working is the disabling of non-visible status message plug-ins. State-affinity in the 95% of previously noted cases has been vastly improved also, the few remaining issues should be due to "lite" protocols that don't have a full complement of on-line presences.
> 
> 
> Diffs
> -----
> 
>   status-handler.h 06240ff 
>   status-handler.cpp 4b9c25a 
>   telepathy-kded-module-plugin.h 4c16169 
>   telepathy-kded-module-plugin.cpp daf73c6 
>   telepathy-mpris.cpp 69e8562 
> 
> Diff: https://git.reviewboard.kde.org/r/116940/diff/
> 
> 
> Testing
> -------
> 
> Disconnect / reconnect, autoconnect / no autoconnect, suspend / resume. Enable / disable via kcm module. Added a new custom presence and engaged the now playing plugin in the contact list from the new presence. Disabled the plugin by activating another presence.
> 
> 
> Thanks,
> 
> James Smith
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20140531/eac6a2d2/attachment.html>