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

Tue Sep 2 23:31:34 UTC 2014

> On Aug. 23, 2014, 3:45 p.m., David Edmundson wrote:
> > status-handler.cpp, line 200
> > <https://git.reviewboard.kde.org/r/116940/diff/23/?file=307051#file307051line200>
> >
> >     I haven't had an answer that I understand when I last asked this.
> >     
> >     Why are you turning plugins off? 
> >     The entire point of the queue is that some plugins have priority over others, if you make it so there's only one active there's not point having a queue.
> >     
> >     
> >     Also can you (using simple words) explain the difference between enabled and active in plugins.
> 
> James Smith wrote:
>     This is really useful for deactivating the entire stack of status message plugins at the same time, for instance by manually changing the requested presence with an active status message plugin. If the requested presence isn't manually altered and the active plugin deactivates due to inactivity, the queue should in theory bump to the next active plugin automatically.
>     
>     active is useful for plugin-specific changes e.g. new data events, etc. The now playing status message plugin sets active for each track change, and (de)active between track changes and (de)active for no upcoming tracks or players.
>     
>     enabled is for turning the output of a plugin on and off completely, so that it won't respond to new data events. (dis)abled overrides active.
>     
>     (de)active looks a lot like (dis)abled output-wise, but (de)active plugins still respond to new data events while (dis)abled plugins don't.
> 
> David Edmundson wrote:
>     This differs massively from the original code which was:
>      - enabled: turned on by the user in the config
>      - active:  currently has a presence that should be used.
>     
>     Then we set ourselves to the first thing in the queue which is active.
>     
>     If you start manipulating enabled from other code you're going to break following the user's settings surely?
> 
> James Smith wrote:
>     The  current status message plugin class nicely waits on any pending activate requests meaning it's probably safe to assume setEnabled() completed safely from outside the plugin. We already disable from the contact list on-the-fly in this way, so as long as a way is provided to re-enable the plugin, we don't have have a problem doing this by changing presences. It's simple to reenable the nowplaying plugin from the contactlist, and we currently do not save the on-the-fly state to disk. The user settings are confined to a single dialog when first starting the nowplaying plugin from the contactlist and are only presented if the plugin was disabled by the kcm, or the kcm is used to enable the plugin at logon. All other methods of activation use setEnable() in some way and work on-the-fly, without saving the state to disk. The same with disabling the plugin by changing the presence with either the presence applet or the contactlist. The only real way to disable the plugin entirely is still in the kcm.

R25 checks for the first active plugin's output to equal the global status message and if it doesn't then it checks which plugins to disable. This prevents disabling all plugins, thus upholding the queue.

> On Aug. 23, 2014, 3:45 p.m., David Edmundson wrote:
> > status-handler.cpp, line 36
> > <https://git.reviewboard.kde.org/r/116940/diff/23/?file=307051#file307051line36>
> >
> >     it's not really a buffer if it's only 1 item long.. I still don't understand what this is doing.
> 
> James Smith wrote:
>     I don't like the approach either of buffering to the presence plugin class. R20 doesn't do this at all. I haven't had a problem with status message plugins overwriting the disk last user presence, but the most recent implementation of this approach (presence plugin-specific) failed also as applied to presence plugins (screensaver, autoaway). I think a new approach is needed to address the presence message plugin part of this bug. R20 (and R24) fixes only the status message part of bug #334492, not the presence part.
>     
>     The basic idea is to keep a list of the past number of status message items to compare disk saves to, to prevent saving a non-user status message to disk. This is a bit hackish, but seems to work well for the status message plugins, having no false positives even with the occasional lock where the status message remained stuck. I'm trying similar infrastructure for the presence plugins, relying on the presence messages to match what not to write to disk. An occasional wrong presence on disk is probably the worst-case outcome of this workaround.

This is now dynamic, based on the number of plugins in the plugin lists.

- James

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

On Sept. 2, 2014, 10:48 p.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116940/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2014, 10:48 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
> -------
> 
> 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
> -----
> 
>   CMakeLists.txt 930267740d0bf26f42a48ea55d77148f87df5369 
>   status-handler.h 06240ff17e22148f2b128bc0eb8cec6d6abe68ff 
>   status-handler.cpp 4b9c25a2ccba451f6e608bb704626e33149108cc 
>   telepathy-kded-module-message-plugin.h PRE-CREATION 
>   telepathy-kded-module-message-plugin.cpp PRE-CREATION 
>   telepathy-kded-module-plugin.h 4c161696a706e82059a7eb314773c3644fe26bd7 
>   telepathy-kded-module-plugin.cpp daf73c66947bc946097de7a8e8a1518555131145 
>   telepathy-mpris.h 05b77c90a50372fd9ed66bde0ab8a287caf34b51 
>   telepathy-mpris.cpp 44b041fdd3764ee5f67598fcf555a2759d853bdd 
>   tests/CMakeLists.txt 7ec77495417a6790060ea5ea7126c46399dff755 
> 
> 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/20140902/01e4ceea/attachment.html>