Bug 45120

Created attachment 56034 [details]
Manual test script

As a client developper, I'd be tempted to go for 1). It doesn't perfectly fit the spec but that's what everyone will expect and actually care about.

(In reply to comment #2)
> As a client developper, I'd be tempted to go for 1). It doesn't perfectly fit
> the spec but that's what everyone will expect and actually care about.

get_most_available_presence() doesn't directly correspond to anything in the spec, so you can make it do whatever you like without contradicting the spec (bonus points for documenting its behaviour).

Created attachment 56075 [details] [review]
account-mgr: pretend that we are AVAILABLE if the best presence is UNSET

This is what most UI would expect; they just care if we are connected, not if
SimplePresence is actually implemented by the underlying CM.

This really deserves an automated regression test: it's the sort of thing I can see going wrong whenever something touches it.

Does your patch handle this transition correctly? It looks as though it wouldn't...

    XMPP OFFLINE, Idle OFFLINE -> XMPP OFFLINE, Idle UNSET

(In reply to comment #5)
> This really deserves an automated regression test: it's the sort of thing I can
> see going wrong whenever something touches it.

I was planning to write one but this function was already untested and I suspect it will be a pain to test. :\

> Does your patch handle this transition correctly? It looks as though it
> wouldn't...
> 
>     XMPP OFFLINE, Idle OFFLINE -> XMPP OFFLINE, Idle UNSET

Yeah, seems to work fine when testing with Empathy

Comment on attachment 56075 [details] [review]
account-mgr: pretend that we are AVAILABLE if the best presence is UNSET

>From 0b2c07591bff1591ec57b2104871632c6e60781c Mon Sep 17 00:00:00 2001
>From: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
>Date: Tue, 24 Jan 2012 14:27:24 +0100
>Subject: [PATCH] account-mgr: pretend that we are AVAILABLE if the best
> presence is UNSET
>
>This is what most UI would expect; they just care if we are connected, not if
>SimplePresence is actually implemented by the underlying CM.
>
>https://bugs.freedesktop.org/show_bug.cgi?id=45120
>---
> telepathy-glib/account-manager.c |   51 ++++++++++++++++++++++++++++++++++++-
> 1 files changed, 49 insertions(+), 2 deletions(-)
>
>diff --git a/telepathy-glib/account-manager.c b/telepathy-glib/account-manager.c
>index 5fc2449..dc3d130 100644
>--- a/telepathy-glib/account-manager.c
>+++ b/telepathy-glib/account-manager.c
>@@ -327,6 +327,7 @@ _tp_account_manager_update_most_available_presence (TpAccountManager *manager)
>   TpAccount *account = NULL;
>   GHashTableIter iter;
>   gpointer value;
>+  TpAccount *has_unset_presence = NULL;
> 
>   /* this presence is equal to the presence of the account with the
>    * highest availability */
>@@ -339,6 +340,14 @@ _tp_account_manager_update_most_available_presence (TpAccountManager *manager)
> 
>       p = tp_account_get_current_presence (a, NULL, NULL);
> 
>+      if (p == TP_CONNECTION_PRESENCE_TYPE_UNSET)
>+        {
>+          has_unset_presence = a;
>+          /* There is no point comparing the presence as UNSET is the
>+           * 'smallest' presence of all */
>+          continue;
>+        }
>+
>       if (tp_connection_presence_type_cmp_availability (p, presence) > 0)
>         {
>           account = a;
>@@ -346,6 +355,14 @@ _tp_account_manager_update_most_available_presence (TpAccountManager *manager)
>         }
>     }
> 
>+  if (presence == TP_CONNECTION_PRESENCE_TYPE_OFFLINE &&
>+      has_unset_presence != NULL)
>+    {
>+      /* Use an account having UNSET as presence as the 'best' one,
>+       * see tp_account_manager_get_most_available_presence() */
>+      account = has_unset_presence;
>+    }
>+
>   priv->most_available_account = account;
>   g_free (priv->most_available_status);
>   g_free (priv->most_available_status_message);
>@@ -865,6 +882,9 @@ _tp_account_manager_account_presence_changed_cb (TpAccount *account,
> {
>   TpAccountManager *manager = TP_ACCOUNT_MANAGER (user_data);
>   TpAccountManagerPrivate *priv = manager->priv;
>+  TpConnectionPresenceType p;
>+  gchar *s;
>+  gchar *msg;
> 
>   if (tp_connection_presence_type_cmp_availability (presence,
>           priv->most_available_presence) > 0)
>@@ -888,10 +908,19 @@ _tp_account_manager_account_presence_changed_cb (TpAccount *account,
>     }
> 
>   return;
>+
> signal:
>+  /* Use tp_account_manager_get_most_available_presence() as the effective
>+   * most available presence may differ of the one stored in
>+   * priv->most_available_presence. */
>+  p = tp_account_manager_get_most_available_presence (manager,
>+      &s, &msg);
>+
>   g_signal_emit (manager, signals[MOST_AVAILABLE_PRESENCE_CHANGED], 0,
>-      priv->most_available_presence, priv->most_available_status,
>-      priv->most_available_status_message);
>+      p, s, msg);
>+
>+  g_free (s);
>+  g_free (msg);
> }
> 
> static void
>@@ -1127,6 +1156,10 @@ tp_account_manager_set_all_requested_presences (TpAccountManager *manager,
>  * If no accounts are enabled or valid the output will be
>  * (%TP_CONNECTION_PRESENCE_TYPE_OFFLINE, "offline", "").
>  *
>+ * Since 0.UNRELEASED, if the only connected accounts does not implement
>+ * %TP_IFACE_CONNECTION_INTERFACE_SIMPLE_PRESENCE, the output will be
>+ * (%TP_CONNECTION_PRESENCE_TYPE_AVAILABLE, "available", "").
>+ *
>  * The return value of this function is not guaranteed to have been retrieved
>  * until tp_proxy_prepare_async() has finished; until then, the
>  * value will be the same as if no accounts are enabled or valid.
>@@ -1148,6 +1181,20 @@ tp_account_manager_get_most_available_presence (TpAccountManager *manager,
> 
>   priv = manager->priv;
> 
>+  if (priv->most_available_presence == TP_CONNECTION_PRESENCE_TYPE_UNSET)
>+    {
>+      /* The best we have is an account having UNSET as its presence, which
>+       * means it's connected but does not implement SimplePresence; pretend
>+       * we are available. */
>+      if (status != NULL)
>+        *status = g_strdup ("available");
>+
>+      if (message != NULL)
>+        *message = g_strdup ("");
>+
>+      return TP_CONNECTION_PRESENCE_TYPE_AVAILABLE;
>+    }
>+
>   if (status != NULL)
>     *status = g_strdup (priv->most_available_status);
> 
>-- 
>1.7.7.6

Created attachment 56137 [details] [review]
simple-account: add API to change the presence

Created attachment 56138 [details] [review]
coding style fix

Created attachment 56139 [details] [review]
more tp_account_manager_get_most_available_presence() tests

Created attachment 56140 [details] [review]
account-mgr: pretend that we are AVAILABLE if the best presence is UNSET

This is what most UI would expect; they just care if we are connected, not if
SimplePresence is actually implemented by the underlying CM.

Comment on attachment 56137 [details] [review]
simple-account: add API to change the presence

Review of attachment 56137 [details] [review]:
-----------------------------------------------------------------

r+

Comment on attachment 56138 [details] [review]
coding style fix

Review of attachment 56138 [details] [review]:
-----------------------------------------------------------------

r+

Comment on attachment 56139 [details] [review]
more tp_account_manager_get_most_available_presence() tests

Review of attachment 56139 [details] [review]:
-----------------------------------------------------------------

r+

Comment on attachment 56140 [details] [review]
account-mgr: pretend that we are AVAILABLE if the best presence is UNSET

Review of attachment 56140 [details] [review]:
-----------------------------------------------------------------

r+, looks good to merge

Merged to master, will be in 0.17.5; thanks!

Cool! ;-)