Attachment 86932 Details for Bug 69885 – 11/11] McdAccount: go back to using GetKnownAvatarTokens

[patch] 11/11] McdAccount: go back to using GetKnownAvatarTokens

0011-McdAccount-go-back-to-using-GetKnownAvatarTokens.patch (text/plain), 10.29 KB, created by Simon McVittie on 2013-10-01 18:04:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2013-10-01 18:04:26 UTC

Size: 10.29 KB

patch

obsolete

>From 18dd1962d8c3dc54f7b06ece1bbaeeaf5faf4c92 Mon Sep 17 00:00:00 2001
>From: Simon McVittie <simon.mcvittie@collabora.co.uk>
>Date: Tue, 1 Oct 2013 15:38:53 +0100
>Subject: [PATCH 11/11] McdAccount: go back to using GetKnownAvatarTokens
>
>Sadly, contact attributes aren't enough to distinguish between
>"this protocol doesn't store avatars and you haven't re-sent your
>avatar since you last connected", "this protocol stores avatars but
>the CM hasn't checked for your current avatar yet", and "this protocol
>stores avatars, but there is no avatar on the server for you".
>
>GetKnownAvatarTokens specifically excludes the middle option (blocking
>on a server round-trip if it needs to), and uses "avatar token missing"
>for the first and "avatar token empty" for the last.
>---
> src/mcd-account.c                               | 123 ++++++++++++++++++++----
> tests/twisted/account-manager/avatar-refresh.py |  23 -----
> 2 files changed, 102 insertions(+), 44 deletions(-)
>
>diff --git a/src/mcd-account.c b/src/mcd-account.c
>index fb03638..f9e874e 100644
>--- a/src/mcd-account.c
>+++ b/src/mcd-account.c
>@@ -172,6 +172,7 @@ struct _McdAccountPrivate
>     gboolean always_on;
>     gboolean changing_presence;
>     gboolean setting_avatar;
>+    gboolean waiting_for_initial_avatar;
>     gboolean waiting_for_connectivity;
> 
>     gboolean hidden;
>@@ -1415,6 +1416,13 @@ mcd_account_self_contact_notify_avatar_file_cb (McdAccount *self,
>       return;
>     }
> 
>+  if (self->priv->waiting_for_initial_avatar)
>+    {
>+      DEBUG ("Ignoring avatar change notification: we are waiting for the "
>+          "initial value");
>+      return;
>+    }
>+
>   prev_token = _mcd_account_get_avatar_token (self);
>   changed = tp_strdiff (prev_token, token);
>   g_free (prev_token);
>@@ -4813,28 +4821,49 @@ _mcd_account_get_old_avatar_filename (McdAccount *account,
> }
> 
> static void
>-mcd_account_process_initial_avatar_token (McdAccount *self)
>+mcd_account_process_initial_avatar_token (McdAccount *self,
>+    const gchar *token)
> {
>+  GArray *avatar = NULL;
>+  gchar *mime_type = NULL;
>   gchar *prev_token;
>-  const gchar *token;
> 
>   g_assert (self->priv->self_contact != NULL);
> 
>   prev_token = _mcd_account_get_avatar_token (self);
>-  token = tp_contact_get_avatar_token (self->priv->self_contact);
>+
>+  DEBUG ("%s", self->priv->unique_name);
>+
>+  if (prev_token == NULL)
>+    DEBUG ("no previous local avatar token");
>+  else
>+    DEBUG ("previous local avatar token: '%s'", prev_token);
>+
>+  if (avatar == NULL)
>+    DEBUG ("no previous local avatar");
>+  else
>+    DEBUG ("previous local avatar: %u bytes, MIME type '%s'", avatar->len,
>+        (mime_type != NULL ? mime_type : "(null)"));
>+
>+  if (token == NULL)
>+    DEBUG ("no remote avatar token");
>+  else
>+    DEBUG ("remote avatar token: '%s'", token);
>+
>+  _mcd_account_get_avatar (self, &avatar, &mime_type);
> 
>   /* If we have a stored avatar but no avatar token, we must have
>    * changed it locally; set it.
>    *
>-   * Meanwhile, if the self-contact's avatar token is empty, this is a
>-   * protocol like link-local XMPP where avatars don't persist.
>-   * Upload ours, if any. */
>-  if (tp_str_empty (prev_token) || tp_str_empty (token))
>+   * Meanwhile, if the self-contact's avatar token is missing, this is
>+   * a protocol like link-local XMPP where avatars don't persist.
>+   * We can distinguish between this case (token is missing, so token = NULL)
>+   * and the case where there is no avatar on an XMPP server (token is
>+   * present and empty), although it's ridiculously subtle.
>+   *
>+   * Either way, upload our avatar, if any. */
>+  if (tp_str_empty (prev_token) || token == NULL)
>     {
>-      GArray *avatar = NULL;
>-      gchar *mime_type = NULL;
>-
>-      _mcd_account_get_avatar (self, &avatar, &mime_type);
> 
>       if (avatar != NULL)
>         {
>@@ -4844,19 +4873,19 @@ mcd_account_process_initial_avatar_token (McdAccount *self)
>             DEBUG ("We have an avatar and the server doesn't");
> 
>           mcd_account_send_avatar_to_connection (self, avatar, mime_type);
>-          g_array_unref (avatar);
>-          g_free (mime_type);
>-          return;
>+          goto out;
>         }
>-
>-      tp_clear_pointer (&avatar, g_array_unref);
>-      g_free (mime_type);
>     }
> 
>   /* Otherwise, if the self-contact's avatar token
>    * differs from ours, one of our "other selves" must have changed
>    * it remotely. Behave the same as if it changes remotely
>-   * mid-session - i.e. download it and use it as our new avatar. */
>+   * mid-session - i.e. download it and use it as our new avatar.
>+   *
>+   * In particular, this includes the case where we had
>+   * a non-empty avatar last time we signed in, but another client
>+   * has deleted it from the server since then (prev_token nonempty,
>+   * token = ""). */
>   if (tp_strdiff (token, prev_token))
>     {
>       GFile *file = tp_contact_get_avatar_file (self->priv->self_contact);
>@@ -4873,15 +4902,52 @@ mcd_account_process_initial_avatar_token (McdAccount *self)
>        * notify::avatar-file will go off. */
>     }
> 
>+out:
>   g_free (prev_token);
>+  tp_clear_pointer (&avatar, g_array_unref);
>+  g_free (mime_type);
> }
> 
>+static void
>+account_conn_get_known_avatar_tokens_cb (TpConnection *conn,
>+    GHashTable *tokens,
>+    const GError *error,
>+    gpointer user_data,
>+    GObject *weak_object)
>+{
>+  McdAccount *self = g_object_ref (weak_object);
>+
>+  self->priv->waiting_for_initial_avatar = FALSE;
>+
>+  if (error != NULL)
>+    {
>+      DEBUG ("%s: GetKnownAvatarTokens raised %s #%d: %s",
>+          self->priv->unique_name, g_quark_to_string (error->domain),
>+          error->code, error->message);
>+    }
>+  else if (self->priv->self_contact == user_data)
>+    {
>+      TpHandle handle = tp_contact_get_handle (self->priv->self_contact);
>+
>+      mcd_account_process_initial_avatar_token (self,
>+          g_hash_table_lookup (tokens, GUINT_TO_POINTER (handle)));
>+    }
>+  else
>+    {
>+      DEBUG ("%s: GetKnownAvatarTokens for outdated self-contact '%s', "
>+          "ignoring",
>+          self->priv->unique_name, tp_contact_get_identifier (user_data));
>+    }
>+
>+  g_object_unref (self);
>+}
> 
> static void
> mcd_account_self_contact_upgraded_cb (GObject *source_object,
>     GAsyncResult *res,
>     gpointer user_data)
> {
>+  TpConnection *conn = TP_CONNECTION (source_object);
>   McdAccount *self = tp_weak_ref_dup_object (user_data);
>   GPtrArray *contacts = NULL;
>   GError *error = NULL;
>@@ -4891,8 +4957,7 @@ mcd_account_self_contact_upgraded_cb (GObject *source_object,
> 
>   g_return_if_fail (MCD_IS_ACCOUNT (self));
> 
>-  if (tp_connection_upgrade_contacts_finish (TP_CONNECTION (source_object),
>-          res, &contacts, &error))
>+  if (tp_connection_upgrade_contacts_finish (conn, res, &contacts, &error))
>     {
>       TpContact *self_contact;
> 
>@@ -4911,7 +4976,6 @@ mcd_account_self_contact_upgraded_cb (GObject *source_object,
>           tp_g_signal_connect_object (self_contact, "notify::avatar-file",
>               G_CALLBACK (mcd_account_self_contact_notify_avatar_file_cb),
>               self, G_CONNECT_SWAPPED);
>-          mcd_account_process_initial_avatar_token (self);
> 
>           tp_g_signal_connect_object (self_contact, "presence-changed",
>               G_CALLBACK (mcd_account_update_self_presence),
>@@ -4924,6 +4988,22 @@ mcd_account_self_contact_upgraded_cb (GObject *source_object,
>               tp_contact_get_presence_status (self_contact),
>               tp_contact_get_presence_message (self_contact),
>               self_contact);
>+
>+          /* We have to use GetKnownAvatarTokens() because of its special
>+           * case for CMs that don't always download an up-to-date
>+           * avatar token before signalling CONNECTED. */
>+          if (tp_proxy_has_interface_by_id (conn,
>+              TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS))
>+            {
>+              guint self_handle = tp_contact_get_handle (self_contact);
>+              GArray *arr = g_array_new (FALSE, FALSE, sizeof (guint));
>+
>+              g_array_append_val (arr, self_handle);
>+              tp_cli_connection_interface_avatars_call_get_known_avatar_tokens (
>+                  conn, -1, arr, account_conn_get_known_avatar_tokens_cb,
>+                  g_object_ref (self_contact), g_object_unref,
>+                  (GObject *) self);
>+            }
>         }
>       else if (self->priv->self_contact == NULL)
>         {
>@@ -5073,6 +5153,7 @@ _mcd_account_set_connection (McdAccount *account, McdConnection *connection)
>     tp_clear_object (&priv->tp_connection);
> 
>     priv->connection = connection;
>+    priv->waiting_for_initial_avatar = TRUE;
> 
>     if (connection)
>     {
>diff --git a/tests/twisted/account-manager/avatar-refresh.py b/tests/twisted/account-manager/avatar-refresh.py
>index 03a02b9..e1c7051 100644
>--- a/tests/twisted/account-manager/avatar-refresh.py
>+++ b/tests/twisted/account-manager/avatar-refresh.py
>@@ -148,29 +148,6 @@ class Account(object):
>             # Nobody has an avatar - nothing should happen
>             self.winner = None
> 
>-        # Hack around bugs... ideally, the tests would pass without these.
>-        if server_delays and local_avatar == 'old' and remote_avatar:
>-            # What we *should* do is wait for GetKnownAvatarTokens
>-            # (because GetContactAttributes isn't guaranteed to fetch
>-            # our own up-to-date avatar token from the server), then
>-            # download the remote avatar. We currently don't.
>-            self.winner = 'MC'
>-        elif server_delays and remote_avatar and not local_avatar:
>-            # What we *should* do is wait for GetKnownAvatarTokens
>-            # (because GetContactAttributes isn't guaranteed to fetch
>-            # our own up-to-date avatar token from the server), then
>-            # download the remote avatar. At the moment we never actually
>-            # download it at all.
>-            self.winner = None
>-        elif avatars_persist and local_avatar == 'old' and not remote_avatar:
>-            # What we *should* do is work out that the avatar on the
>-            # server has been deleted since we last signed in,
>-            # and delete our local avatar to match. (telepathy-spec
>-            # does provide a way to distinguish between this and
>-            # "the protocol doesn't store avatars", but it's
>-            # really subtle; it's hardly surprising if this is wrong.)
>-            self.winner = 'MC'
>-
>     def test(self, q, bus, mc):
>         expected_params = {
>                 'account': self.id,
>-- 
>1.8.4.rc3
>

Actions: View | Splinter Review

Attachments on bug 69885: 86921 | 86922 | 86923 | 86924 | 86925 | 86926 | 86927 | 86928 | 86929 | 86930 | 86932

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.