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

Mon Feb 29 19:45:12 UTC 2016

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

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

KTp/Models/presence-model.cpp (lines 38 - 51)
<https://git.reviewboard.kde.org/r/123482/#comment63387>

    Why? is presence < other not enough?

KTp/global-presence.h (lines 109 - 110)
<https://git.reviewboard.kde.org/r/123482/#comment63374>

    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()".

KTp/global-presence.cpp (lines 42 - 43)
<https://git.reviewboard.kde.org/r/123482/#comment63375>

    m_currentPresence = m_requestedPresence; ?

KTp/global-presence.cpp (lines 56 - 58)
<https://git.reviewboard.kde.org/r/123482/#comment63376>

    So what happens when new account is added (and enabled)?

KTp/global-presence.cpp (lines 318 - 326)
<https://git.reviewboard.kde.org/r/123482/#comment63377>

    This is really uneffective and wrong solution.

    Q_FOREACH(const Tp::AccountPtr &account, m_enabledAccounts->accounts()) {
        if (account->isChangingPresence()) {
            changing = true;
            break;
        }
    }

KTp/global-presence.cpp (lines 344 - 351)
<https://git.reviewboard.kde.org/r/123482/#comment63379>

    Same as above, set the var directly and break.

KTp/global-presence.cpp (line 355)
<https://git.reviewboard.kde.org/r/123482/#comment63378>

    I've noticed just now that all these are being flipped, why?

KTp/presence.h (line 74)
<https://git.reviewboard.kde.org/r/123482/#comment63380>

    This seems needed only because you flipped the if arguments for seemingly no reason?

KTp/presence.cpp (lines 128 - 132)
<https://git.reviewboard.kde.org/r/123482/#comment63381>

    No. Hidden is still online; more available than away.

KTp/status-handler-controller.h (line 43)
<https://git.reviewboard.kde.org/r/123482/#comment63382>

    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.

KTp/status-handler-controller.h (lines 82 - 86)
<https://git.reviewboard.kde.org/r/123482/#comment63383>

    What queue? Where is that queue? Who controls this queue? Why is it a queue when it returns a single presence?

KTp/status-handler-controller.h (lines 96 - 114)
<https://git.reviewboard.kde.org/r/123482/#comment63384>

    A queue is a queue, that means a list with FIFO, you cannot have a map be called "queue".

KTp/status-handler-controller.cpp (line 43)
<https://git.reviewboard.kde.org/r/123482/#comment63385>

    PresenceHandler

KTp/types.h (line 94)
<https://git.reviewboard.kde.org/r/123482/#comment63386>

    This is not "PluginQueue", more like "PluginState"

- Martin Klapetek

On Feb. 23, 2016, 8:14 a.m., James Smith wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123482/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2016, 8:14 a.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-common-internals
> 
> 
> Description
> -------
> 
> New classes:
> -StatusHandlerController, an interface for the status handler's 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 pluginQueueActive(), 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-controller.h PRE-CREATION 
>   KTp/status-handler-controller.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/20160229/1db1e83e/attachment-0001.html>