Review Request 115425: Activate / deactivate Now Playing globally in KConfig when setting playback status in the contact list

[Martin Klapetek]

> On Feb. 15, 2014, 2:38 p.m., David Edmundson wrote:
> > global-presence-chooser.cpp, line 372
> > <https://git.reviewboard.kde.org/r/115425/diff/3/?file=244160#file244160line372>
> >
> >     So the summary of this patch is that you're removing the activateNowPlaying signals and instead relying on changing the config and sending a configChanged method.
> >     
> >     The slots still exist in the kded so this is very half finished.
> >     
> >     It seems like all of these changes are just going round in circles changing things rather than coming up with a understanding of how things are meant to work and coming up with a solid design.
> >     
> >     Martin: now playing is your area, can you please comment. There have been several people randomly hacking on this now and things are still broken. Can you please comment and fix this mess.
> >     What is the purpose of the config and the nowPlaying signals? How is it /meant/ to work?
> 
> James Smith wrote:
>     As long as the kded dbus interface isn't used elsewhere it's probably redundant code originally there to trigger the first status update after enabling the nowplaying and essentially provide control over a semi-autonomous process thereafter.
>     
>     The kconfig method (functionally) duplicates this transparently enough where nowplaying will enable when the config entry is updated and self-terminate when respectively disabled. The slots in the kded don't set the nowplaying config either, so its a fairly overlapping implementation. Can we remove the dbus logic or at least properly set the kconfig from it? We can have kconfig write the current enable status from activatenowplaying / deactivatenowplaying but that still requires kconfig also to a) check whether the plugin is enabled, and b) also to write out enabled status to kconfig, while still using dbus to activate / deactivate the plugin.

Originally there were two things - "enabled" and "active". Enabled was set/read from the config and when it was disabled, the nowplaying plugin would disconnect itself from dbus and would just sit there doing nothing. Active on the other hand was runtime only and was set by the contact list over dbus, it was meant to activate that plugin and "actively set the presence". At some point in history the enabled was actually sort of merged with active in the nowplaying plugin.

The original code - dbus activation signals - will continue working as the slots are still present in the nowplaying plugin and everything is still ok even without this patch afaics. But I think there's some merit in your patch. But as it now behaves as a checkbox and it's really unclear to the user that this is what will happen, I'd like the entry in the presence menu to have an actual checkbox and loose all the dialogs asking questions. Additionally as I stated before, the setting in the KCM should just hide the presence in the combobox altogether (yes I've given it some thought and reevaluated that bug (also note I've never disagreed nor actually posted anything in the bug comments)).


- Martin


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


On Feb. 15, 2014, 2:25 p.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115425/
> -----------------------------------------------------------
> 
> (Updated Feb. 15, 2014, 2:25 p.m.)
> 
> 
> Review request for Telepathy and Martin Klapetek.
> 
> 
> Repository: ktp-contact-list
> 
> 
> Description
> -------
> 
> Enables / disables Now Playing in systemsettings every time it is enabled / disabled in the contact list.
> 
> Fixes systemsettings kcm showing nowplaying enabled while the contact list has disabled its functionality.
> 
> 
> Diffs
> -----
> 
>   global-presence-chooser.cpp 2047473 
> 
> Diff: https://git.reviewboard.kde.org/r/115425/diff/
> 
> 
> Testing
> -------
> 
> Compile, run.
> 
> 
> Thanks,
> 
> James Smith
> 
>

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