Bug 23963

I think a convention could be used with existing interfaces. The client would register interest in particular handles' presence by using SimplePresence or its Contacts equivalent with hold parameter set to true, and signal it's no longer interested by calling ReleaseHandles.

We have a very simple version of this in Gabble git master (and 0.9.13 when released), which detects Maemo device idleness by itself, without UI intervention. Long-term it'd be better to have something central (an AccountManager plugin?) detect device-specific idleness and kick all the CMs, though.

(In reply to comment #0)
> For the power-saving needs of mobile platforms, and to optimise network usage
> in general, some IM protocols may offer control over delivery of presence
> updates for contacts in the subscription list. Consider adding two possible
> optimizations to SimplePresence or a dedicated extension interface:
> 1. Possibility to disable remote presence updates completely (e.g. when the
> screen is locked).

So this is basically a binary setting: on/off right? Either the device is active or it is inactive.

I filed a bug for a connection interface like this, bug #29914.

> 2. Explicit control over the list of contacts for which presence updates are to
> be received.

What would the use-case look like? How would the VIP list of contacts be determined? This would have an element in the UI, or would this be something the CM determines with internal heuristics?

> 
> On support in well-known protocols:
> * As a general case, no optimizations are offered. This means the feature
> should be negotiable and optional to implement.
> * In XMPP, we know no XEPs to control presence updates, though such an
> extension is not inconceivable.

Don't know of any server support for this:
http://xmpp.org/extensions/xep-0273.html

Google talk supports this (and gabble relies on it):
http://web.archiveorange.com/archive/v/08AqeIGhT7exfi88k8jy

> * As one existing, but not really compelling use case, a non-XCAP presence
> implementation in SIP could subscribe to select SIP URIs. Some degrees of
> fine-grained control should be possible with XCAP as well, but I can't confirm
> this before reading up the voluminous specifications, and server-side support
> may be spotty.

(In reply to comment #3)
> > 1. Possibility to disable remote presence updates completely (e.g. when the
> > screen is locked).
> 
> So this is basically a binary setting: on/off right? Either the device is
> active or it is inactive.

Yep.

> > 2. Explicit control over the list of contacts for which presence updates are to
> > be received.
> 
> What would the use-case look like? How would the VIP list of contacts be
> determined? This would have an element in the UI, or would this be something
> the CM determines with internal heuristics?

I was thinking about a desktop widget showing status for favorite contacts. Not unlike the ones we have in Maemo. While no other contacts are requested by any client, presence updates for those could be disabled. But this is a lower priority case, as support for this in real-world services is not yet certain.

(In reply to comment #4)
> (In reply to comment #3)
> > > 1. Possibility to disable remote presence updates completely (e.g. when the
> > > screen is locked).
> > 
> > So this is basically a binary setting: on/off right? Either the device is
> > active or it is inactive.
> 
> Yep.

-- This usecase sounds good for optimization.

> 
> > > 2. Explicit control over the list of contacts for which presence updates are to
> > > be received.
> > 
> > What would the use-case look like? How would the VIP list of contacts be
> > determined? This would have an element in the UI, or would this be something
> > the CM determines with internal heuristics?
> 
> I was thinking about a desktop widget showing status for favorite contacts. Not
> unlike the ones we have in Maemo. While no other contacts are requested by any
> client, presence updates for those could be disabled. But this is a lower
> priority case, as support for this in real-world services is not yet certain.

-- In this usecase we really need to inform the user how the presence will work, even though we plan about selecting the favorite contacts, because for many users favorite does not mean they are not interested in real time presence update for other contacts. Its just we can show those favorite contacts top in to the list or something...

(In reply to comment #5)
> > While no other contacts are requested by any
> > client, presence updates for those could be disabled. But this is a lower
> > priority case, as support for this in real-world services is not yet certain.
> 
> -- In this usecase we really need to inform the user how the presence will
> work, even though we plan about selecting the favorite contacts, because for
> many users favorite does not mean they are not interested in real time presence
> update for other contacts. Its just we can show those favorite contacts top in
> to the list or something...

Yes, but _when_ a client requests other contacts the presence for those could be refreshed. Contacts which no client is interested in do not need to receive updates.

(In reply to comment #4)
> While no other contacts are requested by any
> client, presence updates for those could be disabled. But this is a lower
> priority case, as support for this in real-world services is not yet certain.

Update: the SIFT XEP is the only mechanism known to us in protocols we're interested about, that can support this case.

In case power-saving is not supported by server (everything except gtalk AFAIK), we could queue requests/updates locally into the CM to reduce client processes wakeup.

Take for example avatar requests:
1) CM emits token update signal.
2) telepathy-glib see that token is not in avatar cache and if the TpContact has the AVATAR_DATA it does RequestAvatars to the CM. (tp-qt4 does exactly the same).
3) the CM request avatar to server, which is really BAD in power saving mode!

From a client-side POV the problem is that features on contacts are not removable, so Tp::Contact have to know we are in power saving and delay the avatar request there...

I think the best solution would be to queue all non-essential updates into the CM so no client is even wakeup. But that's a problem if we want a logger of such updates that works even if idle. But do we want that really?

(In reply to comment #8)
> In case power-saving is not supported by server (everything except gtalk
> AFAIK), we could queue requests/updates locally into the CM to reduce client
> processes wakeup.

Not such a big deal, because if the server is sending us presence, our network device and Gabble have to be awake, so we're awake already. The important edge is between 0 and 1 causes to wake up. I agree that once awake, doing avatar downloads would keep us awake for longer, though.

Queueing presence while in pseudo-power-saving mode would have the side-effect of queueing avatar changes.

If not done carefully, this would re-order XMPP stanzas, which I think is undesirable.

If you implement this, I think the right place would be Wocky? Like so:

* power-saving user provides a callback, gboolean (*WockyInterestingStanzaPredicate) (WockyStanza *stanza, gpointer user_data) or some such

* all uninteresting stanzas (for Gabble this would be <presence> and a whitelist containing most known PEP notifications, I think?) are pushed onto a queue in the porter instead of being dispatched

* when an interesting stanza arrives, the porter dispatches all the queued uninteresting stanzas (to preserve ordering), then dispatches the interesting stanza, then goes back to sleep

(In reply to comment #8)
> But that's a problem if we want a logger of
> such updates that works even if idle. But do we want that really?

PowerSaving is defined to be allowed to break such things; if you care about them, don't set power saving mode.

Moving to Gabble, since the spec already has a way for MC to tell the CM to go into idle mode. Senko has some branches:


Stanza queueing in wocky:
http://git.collabora.co.uk/?p=user/ptlo/wocky/.git;a=shortlog;h=refs/heads/stanza-queueing

Using the above wocky API additions to queue presence stanzas:
http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=shortlog;h=refs/heads/pm

(In reply to comment #10)
> Moving to Gabble, since the spec already has a way for MC to tell the CM to go
> into idle mode. Senko has some branches:
> 
> 
> Stanza queueing in wocky:
> http://git.collabora.co.uk/?p=user/ptlo/wocky/.git;a=shortlog;h=refs/heads/stanza-queueing

I'm slightly surprised that there's a global “is this stanza important?” hook, rather than each handler specifying whether it minds being delayed. I'd envisaged something more like:

typedef enum {
  WOCKY_DELIVER_IMMEDIATELY,
  WOCKY_ALLOW_QUEUEING
} WockyStanzaImportance;

wocky_porter_register_handler (..., WockyStanzaImportance i, ...);

and

wocky_porter_enable_queueing()
wocky_porter_disable_queueing().

This might mean for slightly more code when registering handlers, but would avoid having to encode all the knowledge about interestingness in one function in the application. For example, Gabble doesn't actually handle PEP events itself, it uses WockyPepService. With this branch, it suddenly has to re-gain knowledge of what PEP events look like, so that it can decide whether a Wocky component cares about them.

It also makes dispatch quite hairy when queueing is enabled: you'd find matching handlers, but if none of them mind waiting a bit, you just push the stanza on a queue. If one *does* want the stanza immediately, you have to go back and dispatch all queued stanzas, and then to all the previous matching handlers for this stanza, then continue as normal. So I can see benefits to both ways…

If we do it the way you've implemented:

+void
+wocky_porter_register_important_handler (WockyPorter *self,
+    WockyPorterHandlerFunc callback,
+    gpointer user_data)

This should say ‘set’, not ‘register’; register implies that there can be more than one. wocky_porter_set_stanza_importance_func()?

+  priv->important_stanza_handler = stanza_handler_new (WOCKY_STANZA_TYPE_NONE,
+      WOCKY_STANZA_SUB_TYPE_NONE, NULL, 0, NULL, callback, user_data);

Reusing the StanzaHandler struct when actually only two fields of it are relevant feels like a bit of a hack.

The patch “WockyPorter: properly ref/unref queued stanza stanza-queueing” should have been squashed into “WockyPorter: support for queueing unimportant stanza”.

> Using the above wocky API additions to queue presence stanzas:
> http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=shortlog;h=refs/heads/pm

<http://dave.cridland.net/xeps/google-queue.html#sect-id280551>, Dave Cridland's proposed historical XEP about google:queue, suggests that only <presence> and <presence type='unavailable'/> should be queued. This seems sensible: subscription requests should come through immediately.

In “conn-power-saving: rename google-specific methods”:

@@ -127,14 +127,15 @@ conn_power_saving_set_power_saving (
       return;
     }
 
+
   queueing_context = g_slice_new0 (ToggleQueueingContext);

Why the extra newline?

+  /* FIXME: we want this cached since it's used on every incoming stanza
+   * if in power-saving mode; but this will leak the pattern on exit */
+  if (pep_pattern == NULL)
+    {
+      pep_pattern = wocky_stanza_build (WOCKY_STANZA_TYPE_MESSAGE,
+          WOCKY_STANZA_SUB_TYPE_NONE, NULL, NULL,
+          '(', "event",
+            ':', WOCKY_XMPP_NS_PUBSUB_EVENT,
+          ')',
+          NULL);
+    }

If the importance was specified when the handler was registered, this would be a non-issue. :)

+      /* FIXME: copy-pasted from toggle_google_queueing_cb, refactor */
+      if (enable != enabled)
+        {
+          g_object_set (G_OBJECT (self), "power-saving", enable, NULL);
+          tp_svc_connection_interface_power_saving_emit_power_saving_changed (
+              self, enable);
+        }

So yeah. Refactoring? :)

(In reply to comment #11)
> I'm slightly surprised that there's a global “is this stanza important?” hook,
> rather than each handler specifying whether it minds being delayed. I'd
> envisaged something more like:
> 
> typedef enum {
>   WOCKY_DELIVER_IMMEDIATELY,
>   WOCKY_ALLOW_QUEUEING
> } WockyStanzaImportance;
> 
> wocky_porter_register_handler (..., WockyStanzaImportance i, ...);

Or alternatively, a separate function for registering queue-tolerant handlers.

C/P from IRC discussion:

<wjt> re queueing stanzas, i've realised there's a third case:
<wjt> for incoming IMs, we need to deliver them immediately, and flush the queue; for PEP notifications, we can delay them, and preserve ordering; for incoming disco queries (for example) we want to reply immediately, but not flush the queue
<wjt> a first approximation would make disco queries flush the queue. but we don't need to preserve ordering of them versus anything else. ditto xmpp ping: we should reply sooner rather than later, or risk disconnection, but we shouldn't need in a perfect world to flush the queue

<ptlo> wjt, wrt each handler specifying whether it minds the delaying, i think it could get quite hairy, not only for the examples you gave; one other example is what if my handler says it's unimportant, but i only optionally handle the stanza, and the next handler that's going to be called (if i don't handle it) actually thinks its important; also, you would need to pepper both wocky and gabble with knowledge of what can and cannot be delayed, and i'm not 100% sure that's not dependant on the use case so that would basically encode powersaving policy into wocky and gabble (parts in each)
<ptlo> and after some time i fear it might become difficult to reason what exactly happens on which combination of important and unimportant handlers

<wjt> i don't think it would
<wjt> each handler would register whether it likes stanzas immediately, immediately and out of order, or not immediately
<wjt> for each stanza, we scan all available handlers
<wjt> if any of them fall into class a, flush the queue
<wjt> if some but not all fall into class b, flush
<wjt> if all fall into b, don't flush
<wjt> if all are in c, queue
<wjt> i guess that's not exactly simple :)
<wjt> but i think the information on how important dispatching a stanza is lives with its handler

So I think we have two possible ways of doing this:
  * preserving the stanza ordering and having Wocky just ask the user whether it wants it now or later (Simon's original proposal and my current implementation)
  * building the information about stanza importance (deliver now in-order, now out-or-oder, or can be queued) into handlers, thus making Wocky more intelligent about how to behave with handling the stanzas, but messing with stanza ordering

(the in-between approach (specify stanza importance for each handler, but always do in-order) sounds to like it has drawbacks of both of these approaches).

The former solution is simpler (less chance to mess up) but requires the user to know the stanza details in order to specify what to queue. The latter would probably have better performance (due to possibility of doing some things out of order), but is more complex and (IMHO) might be a bit harder to reason about later on.

So, we first need to decide what we want to do :-)

We've decided to modify the original proposal a bit. The issue here is that
while the code works, it's not very elegant, and would propose additional
maintenance burder in the future.

The modified version is very similar in the effect, but has the important
stanza handler in wocky, too. This means that the XMPP implementation details
are hidden from gabble, and can easily be modified/amended/updated/enhanced in
the future (for example, to do more aggressive power-management support by
reordering XMPP stanzas that may be reordered, so the queue flushes are
minimised).

The changes implementing this are in the updated (rebased) branches in my git repos). Since the underlying Wocky code changed while I was doing this (Porter split into Porter the interface and Porter-C2S the implementation), I'll have to rebase the wocky changes once again before they can be reviewed/merged.

Updated branches:

wocky:

http://git.collabora.co.uk/?p=user/ptlo/wocky/.git;a=commit;h=1617a527eb7c6b1d29cde9029045dcd9ce0210bf

gabble:

http://git.collabora.co.uk/?p=user/ptlo/telepathy-gabble/.git;a=shortlog;h=refs/heads/pm

(In reply to comment #15)
> http://git.collabora.co.uk/?p=user/ptlo/wocky/.git;a=commit;h=1617a527eb7c6b1d29cde9029045dcd9ce0210bf

+ * wocky_c2s_porter_enable_power_saving_mode:
+ * @porter: a #WockyC2SPorter

+void
+wocky_c2s_porter_enable_power_saving_mode (WockyC2SPorter *self,
+    gboolean enable)

+void wocky_c2s_porter_enable_power_saving_mode (WockyC2SPorter *self,
+    gboolean enable);

gtk-doc won't like you for that.

Looks pretty good! I've just got a few style comments it'll take no time to fix up.

Senko's updated wocky branch is:

http://git.collabora.co.uk/?p=user/ptlo/wocky/.git;a=shortlog;h=refs/heads/stanza-queueing-c2s

(In reply to comment #15)
> wocky:

+  gboolean power_saving_mode;
+  /* Queue of (owned WockyStanza *) */
+  GQueue *unimportant_queue;
+  /* List of (owned WockyStanza *) */
+  GList *queueable_stanza_patterns;
+
+
   WockyXmppConnection *connection;

I like your comments, nice going. Now just remove the extra newline. :-)

+  for (l = priv->queueable_stanza_patterns; l; l = l->next)
+      g_object_unref (WOCKY_STANZA (l->data));

Bad indentation, should only be two spaces, and "l" should be "l != NULL". Also g_object_unref takes a gpointer, you don't need to cast the data to a WockyStanza. But... you could just replace this with:

g_list_foreach (priv->queueable_stanza_patterns, (GFunc) g_object_unref, NULL);

and you'd avoid the annoying GList *l, and the typecheck.

+  while (NULL != (stanza = g_queue_pop_head (priv->unimportant_queue)))

This takes a little while to read. For clarity in a few years, could you write it out as something like:

for (stanza = q_queue_pop_head (...); stanza != NULL; stanza = g_queue_pop_head (...))

Hm, that's unfortunate we have to call pop_head twice. Well, yeah, or something like that.

+ * Enable or disable power saving. In power saving mode, Wocky will
+ * attempt to queue "uninteresting" stanza until it is either manually
+ * flushed, until important stanza arrives, or until the power saving
+ * mode is disabled.

It'd be nice if you could add a few notes as to what uninteresting stanzas are please.

+void wocky_c2s_porter_flush_unimportant_queue (WockyC2SPorter *porter);

Does this need to be public?

+is_stanza_important (WockyC2SPorter *self, WockyStanza *stanza)

Each argument on a new line for declarations.

+      if (!ptype || !wocky_strdiff (ptype, "unavailable"))

ptype != NULL

+  if (!priv->queueable_stanza_patterns)

priv->queueable_stanza_patterns != NULL

+  for (l = priv->queueable_stanza_patterns; l; l = l->next)

l != NULL

Other than those little niggles it's basically fine.

(In reply to comment #15)
> gabble:

+#include <wocky-namespaces.h>
+#include <wocky-c2s-porter.h>

#include <wocky/wocky-foo.h> please. I'm actually kind of surprised that worked?

All your other gabble changes look good.

(In reply to comment #17)
> Looks pretty good! I've just got a few style comments it'll take no time to fix up.

Thanks for the reviews. I've updated my branches based on this and I'm in the process of merging them.

To be able to merge wocky I'll need permissions to push to master there, for which I've opened a fd.o# and set it as blocking this one.

This was merged.