Review Request 116940: Add a new status message plugin class and use it for the now Playing plugin

Fri Aug 8 03:03:42 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.
> 
> James Smith wrote:
>     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.

Buffering the status message into a QStringList seems to have solved this problem altogether, and may have to be done with the the presence plugin status messages as well, to keep them from being saved to disk as custom user-provided messages. This method works just as well with a list that only holds one item as opposed to a list which holds more than one item. Is there any reason to not extend buffering in this manner to the presence plugin status messages? And is this observation somehow caused by or related to using a QList to iterate through the plugins?

- James

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

On Aug. 7, 2014, 12:17 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116940/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2014, 12:17 a.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
> -------
> 
> Move status message plugins into a separate class. New infrastructure to help mitigate the occurrence of bug #334492 while using status message plugins: 1) buffering of the last n status messages, and 2) a limiter to help stop flooding the presence changer. Also strips redundant and disused data structures from both classes.
> 
> 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
> -----
> 
>   telepathy-kded-module-message-plugin.cpp PRE-CREATION 
>   CMakeLists.txt 930267740d0bf26f42a48ea55d77148f87df5369 
>   status-handler.h 06240ff17e22148f2b128bc0eb8cec6d6abe68ff 
>   status-handler.cpp 4b9c25a2ccba451f6e608bb704626e33149108cc 
>   telepathy-kded-module-message-plugin.h PRE-CREATION 
>   telepathy-kded-module-plugin.h 4c161696a706e82059a7eb314773c3644fe26bd7 
>   telepathy-mpris.cpp 44b041fdd3764ee5f67598fcf555a2759d853bdd 
>   tests/CMakeLists.txt 7ec77495417a6790060ea5ea7126c46399dff755 
>   telepathy-kded-module-plugin.cpp daf73c66947bc946097de7a8e8a1518555131145 
>   telepathy-mpris.h 05b77c90a50372fd9ed66bde0ab8a287caf34b51 
> 
> 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/20140808/7c2e02df/attachment-0001.html>