From bug #21957 : """ > The current situation confuses Empathy and the Shell (which are both using > tp_account_manager_get_most_available_presence()) if only Idle accounts are > connected. As they don't implement SimplePresence, there is no "most available > presence" and so UIs claim we are offline. I don't think this is really Idle's fault: MC, telepathy-glib and clients should cope with this situation. Is get_most_available_presence actually returning OFFLINE, or is it really UNSET? If it's UNSET, I believe MC uses UNSET to mean "it's online but does not report any specific presence" - as a most available presence, UIs should treat that as online. Rationale for UNSET meaning that per-account: if we used AVAILABLE, then this situation: XMPP: BUSY IRC: online would be reported by get_most_available_presence as AVAILABLE, which is clearly wrong: it should be BUSY. With the special handling of UNSET, and UNSET prioritized below all online presences, it's reported as BUSY as expected. I see two possibilities to fix this: 1) Change get_most_available_presence to return AVAILABLE if it would have returned UNSET (implementation detail: you will want to store priv->most_available_presence == UNSET, and only do the translation in the API - otherwise certain state transitions will be wrong) 2) Change clients (Empathy, Shell) to treat UNSET as an online status Some things to test if you do (1): * XMPP offline, IRC online -> AVAILABLE (or UNSET for (2)) * XMPP BUSY, IRC online -> BUSY * XMPP offline, IRC online, XMPP changes to BUSY -> AVAILABLE changes to BUSY (That last one is an example of a transition that would break if you stored priv->most_available_presence == AVAILABLE for the only-IRC-online case.) """ According to my test, it does return Offline which doesn't fit with either 1) or 2).
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! ;-)
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.