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

[James Smith]

> On May 8, 2014, 9:01 p.m., David Edmundson wrote:
> > status-handler.cpp, line 215
> > <https://git.reviewboard.kde.org/r/116940/diff/11/?file=271992#file271992line215>
> >
> >     
> >     If we have:
> >     Now playing 
> >     
> >     Then AutoAway starts
> >     
> >     Then AutoAway ends
> >     
> >     we're no longer in now playing.

No. With an empty AutoAway message the now playing plugin still functions. And returns when AutoAway is disabled.


> On May 8, 2014, 9:01 p.m., David Edmundson wrote:
> > telepathy-mpris.cpp, line 280
> > <https://git.reviewboard.kde.org/r/116940/diff/11/?file=271995#file271995line280>
> >
> >     Why do we have
> >     
> >     enabled
> >     active
> >     and enabledInConfig 
> >     
> >     as 3 completely separate things?
> >     Surely 1 of these must be pointless.
> >

enabled is session-only, enabledInConfig is disk-and-then-session, and active is only when there is a valid status message output.


> On May 8, 2014, 9:01 p.m., David Edmundson wrote:
> > telepathy-mpris.cpp, line 281
> > <https://git.reviewboard.kde.org/r/116940/diff/11/?file=271995#file271995line281>
> >
> >     Surely, this wouldn't be needed if you hadn't changed line 274
> >     
> >     Changing it clearly complicates things

It's a required change to allow the contact list to gracefully transition to the next presence post-active status message plugin. Otherwise the selected presence is ignored and the presence becomes what was used to set the status plugin active. This change makes certain that the kcm also still functions to enable / disable the now playing plugin.


> On May 8, 2014, 9:01 p.m., David Edmundson wrote:
> > status-handler.cpp, line 77
> > <https://git.reviewboard.kde.org/r/116940/diff/11/?file=271992#file271992line77>
> >
> >     Instead of having the extra method setPluginEnabled which doesn't have the signal we could change this to a 
> >     
> >     Qt::QueuedConnection.
> >     
> >     This way it would only run after all the plugins are finished being disabled and therefore not do anything.

Can this be accomplished with a boolean for setEnabled?


> On May 8, 2014, 9:01 p.m., David Edmundson wrote:
> > telepathy-kded-module-plugin.h, line 48
> > <https://git.reviewboard.kde.org/r/116940/diff/11/?file=271993#file271993line48>
> >
> >     I'm still not allowing a second very similarly named setMethod() that behaves slightly differently.
> >     
> >     It's confusing API

I actually agree, though enabled / active are still private api, setPluginEnabled() is a public function. setPluginEnabled() is useful to keep the presence in proper user control, the contact list and presence interaction is "broken" from the moment the presence is changed from the now playing plugin presence due to the extra presence change that shouldn't happen on a status message disable. Can it be more descriptively named? Or possibly complete duplication of functionality into status-message-only equivalents of the existing functions? setActive() can't be used on its own in the case of the now playing plugin because the next track change will just re-enable the plugin.


- James


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


On May 8, 2014, 10:39 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116940/
> -----------------------------------------------------------
> 
> (Updated May 8, 2014, 10:39 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Bugs: 332082 and 334492
>     http://bugs.kde.org/show_bug.cgi?id=332082
>     http://bugs.kde.org/show_bug.cgi?id=334492
> 
> 
> 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/20140509/a99d3506/attachment-0001.html>