Review Request 123482: ktp-common-internals status handler integration and global / multiple account presence fixes

Mon Mar 7 07:08:29 UTC 2016

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > I'm not really sure I want to have this incredibly complex feature-set for the little gain it provides.
> > 
> > I don't know what your overall goal with this set of changes actually is as there is no explanation
> > anywhere, just a bunch of patches. But I assume it has to do with the kded plugins and per-account
> > presences. I would actually like to just kill the kded plugins and leave just auto-away. And the plugins
> > presence handling actually becomes very simple - it's either set by a plugin or not. I don't see a need
> > for a whole dbus infrastructure to mess with presences.
> > 
> > As for your StatusHandlerController, that seems kinda useless. You either controll all accounts by
> > using the GlobalPresence, or you're using separate account presences, at which point the GlobalPresence
> > should simply have two states. You don't need no special class nor dbus interfaces to control per-account
> > presences, you can do that directly.
> > 
> > So, to summarize:
> > 
> > * GlobalPresence should have a "global" and "non-global" state
> > * All our presence controls should have the same two modes
> > * Setting the non-global mode from the UI will disable setting all accounts to the same presence
> > * Setting global presence from the UI will again set everything
> > * Auto-away overrides all presences

The KDE global / account presence setting should as far as possible be done compatibly with how TelepathyQt can accept incoming presences from other IM applications. This increases complexity a bit because we have to take into account our own automatic presences, and on top of this we do our own auto connect. This patchset throws in control and monitor functions that cover what sticking to the basics doesn't actually allow for, in addition to changing some existing shortcomings, and addressing other noticeable flaws.. Having a non-global mode to set is probably not a step in the right direction.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/global-presence.h, lines 111-112
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445120#file445120line111>
> >
> >     I really don't understand what this is supposed to do.
> >     
> >     We have GlobalPresence::requestedPresence() with coment "The most online requested presence for all accounts".
> >     
> >     Now this is GlobalPresence::requestedGlobalPresence() with "The account status helper requested global presence"
> >     
> >     I assume the "status helper" is your "statushandlercontroller", which makes it a confusing comment as it's called "status handler controller" (which itself is a strange name), not "status helper".
> >     
> >     This API becomes very unclear, what's more "global"? "GlobalPresence::requestedPresence()" or "GlobalPresence::requestedGlobalPresence()".

requestedPresence() can return the kded automatic presence, requestedGlobalPresence() never returns an automatic presence. This is the presence selected using the presence applet or the presence chooser in the contact list and is saved for auto-connect.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/global-presence.cpp, lines 42-43
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445121#file445121line42>
> >
> >     m_currentPresence = m_requestedPresence; ?

Telepathy-Qt, has an outstanding bug with patch to implement making these from PresenceSpec.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/global-presence.cpp, lines 59-61
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445121#file445121line59>
> >
> >     So what happens when new account is added (and enabled)?

The signals are connected, global properties updated, enabledAccountsChanged signal is emitted. When a new account is disabled, the reverse happens.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/global-presence.cpp, line 385
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445121#file445121line385>
> >
> >     I've noticed just now that all these are being flipped, why?

Code tidying.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/presence.h, line 74
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445122#file445122line74>
> >
> >     This seems needed only because you flipped the if arguments for seemingly no reason?

The original operator was unintuitively backwards. PresenceModel required its own modifications to get the status message display order useful after correcting this inconsistency, while the original operator wasn't useful (or used) for any clear purpose. This patchset implements both less than and greater than operators.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/presence.cpp, lines 128-134
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445123#file445123line128>
> >
> >     No. Hidden is still online; more available than away.

I had to move this because it was for e.g. masking away requested global presence for one single account at hidden presence.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/status-handler-controller.cpp, line 43
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445125#file445125line43>
> >
> >     PresenceHandler

This class is already named StatusHandler in the kded module.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/status-handler-controller.h, line 43
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445124#file445124line43>
> >
> >     This class is something I really would like to not have.
> >     
> >     First of all - "StatusHandlerController" - make it either "StatusHandler" or "StatusController", not both. And stick with that name. 
> >     
> >     Then we use "presence" rather than "status" everywhere, so this should really be "PresenceHandler".
> >     
> >     Lastly, this just duplicates GlobalPresence on a different level. I agree the GlobalPresence can use some improvements, but this is not the way. There needs to be one single class handling global presence, not two of them competing.

These classes are actually different entirely, GlobalPresence reflects the account's current presence(which may or may not be global or automatic in nature) and can set a global presence, while StatusHandlerController reflects the available automatic presence (which may not apply to that particular account, i.e. unempty requested account presence status message and now playing) and provides a number of ways to modify the account's (and hence global) requested presence if the kded module is available.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/types.h, line 94
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445126#file445126line94>
> >
> >     This is not "PluginQueue", more like "PluginState"

PluginQueue::Plugin* makes it clear that these are states for individual plugins, and does look like part of the PluginQueue kded class.

> On Feb. 29, 2016, 7:45 p.m., Martin Klapetek wrote:
> > KTp/Models/presence-model.cpp, lines 38-51
> > <https://git.reviewboard.kde.org/r/123482/diff/2/?file=445119#file445119line38>
> >
> >     Why? is presence < other not enough?

The < (>) operators were unintuitive and non-useful except in the presence model where presences with empty status message are placed before presences with non-empty status messages. This patchset made the < (>) operators correct and un-backwards but broke the PreseceModel sorting so the PresenceModel got a custom operator that reverses the status message ordering.

- James

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

On March 7, 2016, 6:53 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123482/
> -----------------------------------------------------------
> 
> (Updated March 7, 2016, 6:53 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> -------
> 
> New classes:
> -StatusHandlerPresence, an interface for StatusHandler PluginQueue and AccountStatusHelper DBus interfaces, and a number of control functions for the Presence / StatusMessageInserter plugins (per-app save / restore presence replacement).
> 
> New properties:
> -GlobalPresence: bool activeAutoPresence(), bool mixedPresenceMode(), KTp::Presence globalRequestedPresence().
> 
> New methods:
> -GlobalPresence: onAccountDisabled(Tp::AccountPtr)
> 
> New types:
> KTp::PluginQueue, for indicating the state of the plugin queue plugins, e.g. PluginDisabledOnLoad, PluginDisabled, PluginEnabled, PluginActive, etc.
> 
> New features:
> -pluginQueue active / inactive information in GlobalPresence, separate presenceQueueActiveChanged and statusMessageQueueActiveChanged signals.
> -More reliable account property signalling.
> -Reworked account enable / disable.
> -Presence priority improvements.
> 
> Other simple cleanups (Use KTp::Presence where possible, make function arguments conform, commenting clarifications, errant properties, etc.)
> 
> 
> Diffs
> -----
> 
>   KTp/CMakeLists.txt 85d578bc3cbc0ae5769e6ff9f466381da4e701a7 
>   KTp/Declarative/qml-plugins.cpp 4956a1c94fe60a526367f6a74a357d510e74e519 
>   KTp/Models/presence-model.cpp ddc1a7c75f1a452bf3ac2db1aecbd88a5d1ce519 
>   KTp/global-presence.h 46e6cc358b0f3cdaff35d3a7be25949646b400d3 
>   KTp/global-presence.cpp 8f1148b4ec339ab77f9c2fca0c87429ac14c5194 
>   KTp/presence.h 7da07d0120c75ed057f4b2f2f832f5a1f80bd528 
>   KTp/presence.cpp 6030db0ed821a4399603b5453ab124c741912a1f 
>   KTp/status-handler-presence.h PRE-CREATION 
>   KTp/status-handler-presence.cpp PRE-CREATION 
>   KTp/types.h 27eb8975a4149e78c1e545b7273572c7eb18bad3 
> 
> Diff: https://git.reviewboard.kde.org/r/123482/diff/
> 
> 
> Testing
> -------
> 
> Compile, run.
> 
> 
> Thanks,
> 
> James Smith
> 
>

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