From 18dd1962d8c3dc54f7b06ece1bbaeeaf5faf4c92 Mon Sep 17 00:00:00 2001 From: Simon McVittie 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