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

_______________________________________________
KDE-Telepathy mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/kde-telepathy

Reply via email to