> On Feb. 15, 2014, 1: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. > > Martin Klapetek wrote: > 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)).
Warning the user to enable nowplaying from an empty status message would help keep the code readable. There's a ton of return-state logic needed to make certain nowplaying relinquishes properly in the contact list. Also I like the checkbox idea instead of trying to hide the entry in the contact list. - James ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115425/#review49821 ----------------------------------------------------------- On Feb. 15, 2014, 1: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, 1: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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
