From 22a4438161874e36e27282bd3d24a909f4b16f70 Mon Sep 17 00:00:00 2001 From: Xavier Claessens Date: Fri, 11 May 2012 14:08:03 +0200 Subject: TpContact does not always request avatar If AVATAR_TOKEN was already prepared and we upgrade to have AVATAR_DATA, the avatar won't be requested --- telepathy-glib/contact.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/telepathy-glib/contact.c b/telepathy-glib/contact.c index 6211cd25f..9166888a8 100644 --- a/telepathy-glib/contact.c +++ b/telepathy-glib/contact.c @@ -3560,16 +3560,21 @@ contacts_context_queue_features (ContactsContext *context) #endif } - if (!contacts_context_supports_iface (context, + if ((feature_flags & CONTACT_FEATURE_FLAG_AVATAR_TOKEN) != 0 && + !contacts_context_supports_iface (context, TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS) && tp_proxy_has_interface_by_id (context->connection, TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS)) { - if ((feature_flags & CONTACT_FEATURE_FLAG_AVATAR_TOKEN) != 0) - g_queue_push_tail (&context->todo, contacts_get_avatar_tokens); + g_queue_push_tail (&context->todo, contacts_get_avatar_tokens); + } - if ((feature_flags & CONTACT_FEATURE_FLAG_AVATAR_DATA) != 0) - g_queue_push_tail (&context->todo, contacts_get_avatar_data); + /* There is no contact attribute for avatar data, always use slow path */ + if ((feature_flags & CONTACT_FEATURE_FLAG_AVATAR_DATA) != 0 && + tp_proxy_has_interface_by_id (context->connection, + TP_IFACE_QUARK_CONNECTION_INTERFACE_AVATARS)) + { + g_queue_push_tail (&context->todo, contacts_get_avatar_data); } if ((feature_flags & CONTACT_FEATURE_FLAG_LOCATION) != 0 && @@ -3669,12 +3674,6 @@ tp_contact_set_attributes (TpContact *contact, } } - /* There is no attribute for avatar data. If we want it, let's just - * pretend it is ready. If avatar is in cache, that will be true as - * soon as the token is set from attributes */ - if (wanted & CONTACT_FEATURE_FLAG_AVATAR_DATA) - contact->priv->has_features |= CONTACT_FEATURE_FLAG_AVATAR_DATA; - if (wanted & CONTACT_FEATURE_FLAG_AVATAR_TOKEN) { s = tp_asv_get_string (asv, -- cgit v1.2.3 From 143b5ad6c4c9e17fff22652eaf93a45eb54b9596 Mon Sep 17 00:00:00 2001 From: Xavier Claessens Date: Fri, 11 May 2012 16:17:03 +0200 Subject: Add regression test for avatar data problem --- tests/dbus/contacts.c | 140 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 96 insertions(+), 44 deletions(-) diff --git a/tests/dbus/contacts.c b/tests/dbus/contacts.c index 617f0b97f..b35e007d3 100644 --- a/tests/dbus/contacts.c +++ b/tests/dbus/contacts.c @@ -532,22 +532,22 @@ test_avatar_requirements (Fixture *f, g_main_loop_unref (result.loop); } -static GFile * +static TpContact * create_contact_with_fake_avatar (TpTestsContactsConnection *service_conn, TpConnection *client_conn, - const gchar *id) + const gchar *id, + gboolean request_avatar) { Result result = { g_main_loop_new (NULL, FALSE), NULL, NULL, NULL }; TpHandleRepoIface *service_repo = tp_base_connection_get_handles ( (TpBaseConnection *) service_conn, TP_HANDLE_TYPE_CONTACT); - TpContactFeature features[] = { TP_CONTACT_FEATURE_AVATAR_DATA }; + TpContactFeature feature; const gchar avatar_data[] = "fake-avatar-data"; const gchar avatar_token[] = "fake-avatar-token"; const gchar avatar_mime_type[] = "fake-avatar-mime-type"; TpContact *contact; TpHandle handle; GArray *array; - GFile *avatar_file; gchar *content = NULL; handle = tp_handle_ensure (service_repo, id, NULL, NULL); @@ -557,35 +557,46 @@ create_contact_with_fake_avatar (TpTestsContactsConnection *service_conn, tp_tests_contacts_connection_change_avatar_data (service_conn, handle, array, avatar_mime_type, avatar_token); + if (request_avatar) + feature = TP_CONTACT_FEATURE_AVATAR_DATA; + else + feature = TP_CONTACT_FEATURE_AVATAR_TOKEN; + tp_connection_get_contacts_by_handle (client_conn, 1, &handle, - G_N_ELEMENTS (features), features, + 1, &feature, by_handle_cb, &result, finish, NULL); g_main_loop_run (result.loop); g_assert_no_error (result.error); - contact = g_ptr_array_index (result.contacts, 0); - if (tp_contact_get_avatar_file (contact) == NULL) - { - g_signal_connect_swapped (contact, "notify::avatar-file", - G_CALLBACK (finish), &result); - g_main_loop_run (result.loop); - } + contact = g_object_ref (g_ptr_array_index (result.contacts, 0)); - g_assert_cmpstr (tp_contact_get_avatar_mime_type (contact), ==, - avatar_mime_type); g_assert_cmpstr (tp_contact_get_avatar_token (contact), ==, avatar_token); - avatar_file = tp_contact_get_avatar_file (contact); - g_assert (avatar_file != NULL); - g_file_load_contents (avatar_file, NULL, &content, NULL, NULL, &result.error); - g_assert_no_error (result.error); - g_assert_cmpstr (content, ==, avatar_data); - g_free (content); + if (request_avatar) + { + GFile *avatar_file; + + /* If we requested avatar, it could come later */ + if (tp_contact_get_avatar_file (contact) == NULL) + { + g_signal_connect_swapped (contact, "notify::avatar-file", + G_CALLBACK (finish), &result); + g_main_loop_run (result.loop); + } - /* Keep avatar_file alive after contact destruction */ - g_object_ref (avatar_file); + g_assert_cmpstr (tp_contact_get_avatar_mime_type (contact), ==, + avatar_mime_type); + + avatar_file = tp_contact_get_avatar_file (contact); + g_assert (avatar_file != NULL); + g_file_load_contents (avatar_file, NULL, &content, NULL, NULL, + &result.error); + g_assert_no_error (result.error); + g_assert_cmpstr (content, ==, avatar_data); + g_free (content); + } reset_result (&result); g_main_loop_unref (result.loop); @@ -593,7 +604,7 @@ create_contact_with_fake_avatar (TpTestsContactsConnection *service_conn, tp_handle_unref (service_repo, handle); g_array_unref (array); - return avatar_file; + return contact; } static void @@ -652,29 +663,19 @@ haze_remove_directory (const gchar *path) return ret; } -#define RAND_STR_LEN 6 - static void test_avatar_data (Fixture *f, gconstpointer unused G_GNUC_UNUSED) { TpTestsContactsConnection *service_conn = f->service_conn; TpConnection *client_conn = f->client_conn; - gchar *dir; gboolean avatar_retrieved_called; GError *error = NULL; - GFile *file1, *file2; + TpContact *contact1, *contact2; TpProxySignalConnection *signal_id; g_message (G_STRFUNC); - /* Make sure g_get_user_cache_dir() returns a tmp directory, to not mess up - * user's cache dir. */ - dir = g_dir_make_tmp ("tp-glib-tests-XXXXXX", &error); - g_assert_no_error (error); - g_setenv ("XDG_CACHE_HOME", dir, TRUE); - g_assert_cmpstr (g_get_user_cache_dir (), ==, dir); - /* Check if AvatarRetrieved gets called */ signal_id = tp_cli_connection_interface_avatars_connect_to_avatar_retrieved ( client_conn, avatar_retrieved_cb, &avatar_retrieved_called, NULL, NULL, @@ -684,24 +685,58 @@ test_avatar_data (Fixture *f, /* First time we create a contact, avatar should not be in cache, so * AvatarRetrived should be called */ avatar_retrieved_called = FALSE; - file1 = create_contact_with_fake_avatar (service_conn, client_conn, - "fake-id1"); + contact1 = create_contact_with_fake_avatar (service_conn, client_conn, + "fake-id1", TRUE); g_assert (avatar_retrieved_called); + g_assert (contact1 != NULL); + g_assert (tp_contact_get_avatar_file (contact1) != NULL); /* Second time we create a contact, avatar should be in cache now, so * AvatarRetrived should NOT be called */ avatar_retrieved_called = FALSE; - file2 = create_contact_with_fake_avatar (service_conn, client_conn, - "fake-id2"); + contact2 = create_contact_with_fake_avatar (service_conn, client_conn, + "fake-id2", TRUE); g_assert (!avatar_retrieved_called); + g_assert (contact2 != NULL); + g_assert (tp_contact_get_avatar_file (contact2) != NULL); - g_assert (g_file_equal (file1, file2)); - g_assert (haze_remove_directory (dir)); + g_assert (g_file_equal ( + tp_contact_get_avatar_file (contact1), + tp_contact_get_avatar_file (contact2))); tp_proxy_signal_connection_disconnect (signal_id); - g_object_unref (file1); - g_object_unref (file2); - g_free (dir); + g_object_unref (contact1); + g_object_unref (contact2); +} + +static void +test_avatar_data_after_token (Fixture *f, + gconstpointer unused G_GNUC_UNUSED) +{ + TpTestsContactsConnection *service_conn = f->service_conn; + TpConnection *client_conn = f->client_conn; + const gchar *id = "avatar-data-after-token"; + TpContact *contact1, *contact2; + + g_message (G_STRFUNC); + + /* Create a contact with AVATAR_TOKEN feature */ + contact1 = create_contact_with_fake_avatar (service_conn, client_conn, id, + FALSE); + g_assert (contact1 != NULL); + g_assert (tp_contact_get_avatar_file (contact1) == NULL); + + /* Now create the same contact with AVATAR_DATA feature */ + contact2 = create_contact_with_fake_avatar (service_conn, client_conn, id, + TRUE); + g_assert (contact2 != NULL); + g_assert (tp_contact_get_avatar_file (contact2) != NULL); + + g_assert (contact1 == contact2); + + /* Cleanup */ + g_object_unref (contact1); + g_object_unref (contact2); } static void @@ -2924,9 +2959,20 @@ int main (int argc, char **argv) { + gint ret; + gchar *dir; + GError *error = NULL; + tp_tests_init (&argc, &argv); g_test_bug_base ("http://bugs.freedesktop.org/show_bug.cgi?id="); + /* Make sure g_get_user_cache_dir() returns a tmp directory, to not mess up + * user's cache dir. */ + dir = g_dir_make_tmp ("tp-glib-tests-XXXXXX", &error); + g_assert_no_error (error); + g_setenv ("XDG_CACHE_HOME", dir, TRUE); + g_assert_cmpstr (g_get_user_cache_dir (), ==, dir); + #define ADD(x) \ g_test_add ("/contacts/" #x, Fixture, NULL, setup, test_ ## x, teardown) @@ -2940,6 +2986,7 @@ main (int argc, ADD (by_id); ADD (avatar_requirements); ADD (avatar_data); + ADD (avatar_data_after_token); ADD (contact_info); ADD (dup_if_possible); ADD (subscription_states); @@ -2969,5 +3016,10 @@ main (int argc, g_test_add ("/contacts/self-contact", Fixture, NULL, setup_no_connect, test_self_contact, teardown); - return g_test_run (); + ret = g_test_run (); + + g_assert (haze_remove_directory (dir)); + g_free (dir); + + return ret; } -- cgit v1.2.3 From dacdc30b38a39ba2500ecfeb223bfb83e464340a Mon Sep 17 00:00:00 2001 From: Xavier Claessens Date: Fri, 11 May 2012 16:14:15 +0200 Subject: _dup_contact_by_id_async() and _upgrade_contacts_async(): drop custom implementation Simplify them by directly calling deprecated APIs, custom implementation suffers the same problem with avatar than fixed in previous commit --- telepathy-glib/contact.c | 280 +++++++---------------------------------------- 1 file changed, 39 insertions(+), 241 deletions(-) diff --git a/telepathy-glib/contact.c b/telepathy-glib/contact.c index 9166888a8..64690f8bc 100644 --- a/telepathy-glib/contact.c +++ b/telepathy-glib/contact.c @@ -4573,38 +4573,6 @@ tp_connection_get_contacts_by_id (TpConnection *self, G_GNUC_END_IGNORE_DEPRECATIONS } -typedef struct -{ - GSimpleAsyncResult *result; - ContactFeatureFlags features; - - /* Used for fallback in tp_connection_dup_contact_by_id_async */ - gchar *id; - GArray *features_array; -} ContactsAsyncData; - -static ContactsAsyncData * -contacts_async_data_new (GSimpleAsyncResult *result, - ContactFeatureFlags features) -{ - ContactsAsyncData *data; - - data = g_slice_new0 (ContactsAsyncData); - data->result = g_object_ref (result); - data->features = features; - - return data; -} - -static void -contacts_async_data_free (ContactsAsyncData *data) -{ - g_object_unref (data->result); - g_free (data->id); - tp_clear_pointer (&data->features_array, g_array_unref); - g_slice_free (ContactsAsyncData, data); -} - static void got_contact_by_id_fallback_cb (TpConnection *self, guint n_contacts, @@ -4615,83 +4583,43 @@ got_contact_by_id_fallback_cb (TpConnection *self, gpointer user_data, GObject *weak_object) { - ContactsAsyncData *data = user_data; + const gchar *id = user_data; + GSimpleAsyncResult *result = (GSimpleAsyncResult *) weak_object; GError *e = NULL; if (error != NULL) { - g_simple_async_result_set_from_error (data->result, error); - g_simple_async_result_complete_in_idle (data->result); - return; + g_simple_async_result_set_from_error (result, error); } - if (g_hash_table_size (failed_id_errors) > 0) + else if (g_hash_table_size (failed_id_errors) > 0) { - e = g_hash_table_lookup (failed_id_errors, data->id); + e = g_hash_table_lookup (failed_id_errors, id); if (e == NULL) { g_set_error (&e, TP_DBUS_ERRORS, TP_DBUS_ERROR_INCONSISTENT, "We requested 1 id, and got an error for another id - Broken CM"); - g_simple_async_result_take_error (data->result, e); + g_simple_async_result_take_error (result, e); } else { - g_simple_async_result_set_from_error (data->result, e); + g_simple_async_result_set_from_error (result, e); } - - g_simple_async_result_complete_in_idle (data->result); - return; } - if (n_contacts != 1 || contacts[0] == NULL) + else if (n_contacts != 1 || contacts[0] == NULL) { g_set_error (&e, TP_DBUS_ERRORS, TP_DBUS_ERROR_INCONSISTENT, "We requested 1 id, but no contacts and no error - Broken CM"); - g_simple_async_result_take_error (data->result, e); - g_simple_async_result_complete_in_idle (data->result); - return; + g_simple_async_result_take_error (result, e); } - - g_simple_async_result_set_op_res_gpointer (data->result, - g_object_ref (contacts[0]), g_object_unref); - g_simple_async_result_complete (data->result); -} - -static void -got_contact_by_id_cb (TpConnection *self, - TpHandle handle, - GHashTable *attributes, - const GError *error, - gpointer user_data, - GObject *weak_object) -{ - ContactsAsyncData *data = user_data; - TpContact *contact; - GError *e = NULL; - - if (error != NULL) + else { - /* Retry the old way, for old CMs does that not exist in the real world */ - G_GNUC_BEGIN_IGNORE_DEPRECATIONS - tp_connection_get_contacts_by_id (self, 1, - (const gchar * const *) &data->id, - data->features_array->len, - (TpContactFeature *) data->features_array->data, - got_contact_by_id_fallback_cb, - data, (GDestroyNotify) contacts_async_data_free, NULL); - G_GNUC_END_IGNORE_DEPRECATIONS - return; + g_simple_async_result_set_op_res_gpointer (result, + g_object_ref (contacts[0]), g_object_unref); } - /* set up the contact with its attributes */ - contact = tp_contact_ensure (self, handle); - g_simple_async_result_set_op_res_gpointer (data->result, - contact, g_object_unref); - - if (!tp_contact_set_attributes (contact, attributes, data->features, &e)) - g_simple_async_result_take_error (data->result, e); - - g_simple_async_result_complete (data->result); - contacts_async_data_free (data); + g_simple_async_result_complete_in_idle (result); + g_object_unref (result); } /** @@ -4727,55 +4655,18 @@ tp_connection_dup_contact_by_id_async (TpConnection *self, GAsyncReadyCallback callback, gpointer user_data) { - ContactsAsyncData *data; GSimpleAsyncResult *result; - ContactFeatureFlags feature_flags = 0; - const gchar **supported_interfaces; - - g_return_if_fail (tp_proxy_is_prepared (self, - TP_CONNECTION_FEATURE_CONNECTED)); - g_return_if_fail (id != NULL); - g_return_if_fail (n_features == 0 || features != NULL); - - if (!get_feature_flags (n_features, features, &feature_flags)) - return; result = g_simple_async_result_new ((GObject *) self, callback, user_data, tp_connection_dup_contact_by_id_async); - data = contacts_async_data_new (result, feature_flags); - data->id = g_strdup (id); - data->features_array = g_array_sized_new (FALSE, FALSE, - sizeof (TpContactFeature), n_features); - g_array_append_vals (data->features_array, features, n_features); - - if (tp_proxy_has_interface_by_id (self, - TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACTS)) - { - supported_interfaces = contacts_bind_to_signals (self, feature_flags); - - tp_cli_connection_interface_contacts_call_get_contact_by_id (self, -1, - id, supported_interfaces, got_contact_by_id_cb, - data, NULL, NULL); - - g_free (supported_interfaces); - } - else - { - /* Proceed directly to the slow path, do not pass Go, do not collect - * £200. contacts_bind_to_signals() relies on having Contacts. */ - - G_GNUC_BEGIN_IGNORE_DEPRECATIONS - tp_connection_get_contacts_by_id (self, 1, - (const gchar * const *) &data->id, - data->features_array->len, - (TpContactFeature *) data->features_array->data, - got_contact_by_id_fallback_cb, - data, (GDestroyNotify) contacts_async_data_free, NULL); - G_GNUC_END_IGNORE_DEPRECATIONS - } - - g_object_unref (result); + G_GNUC_BEGIN_IGNORE_DEPRECATIONS + tp_connection_get_contacts_by_id (self, + 1, &id, + n_features, features, + got_contact_by_id_fallback_cb, + g_strdup (id), g_free, G_OBJECT (result)); + G_GNUC_END_IGNORE_DEPRECATIONS } /** @@ -4799,41 +4690,7 @@ tp_connection_dup_contact_by_id_finish (TpConnection *self, } static void -got_contact_attributes_cb (TpConnection *self, - GHashTable *attributes, - const GError *error, - gpointer user_data, - GObject *weak_object) -{ - ContactsAsyncData *data = user_data; - GHashTableIter iter; - gpointer key, value; - - if (error != NULL) - { - g_simple_async_result_set_from_error (data->result, error); - g_simple_async_result_complete_in_idle (data->result); - return; - } - - g_hash_table_iter_init (&iter, attributes); - while (g_hash_table_iter_next (&iter, &key, &value)) - { - TpHandle handle = GPOINTER_TO_UINT (key); - GHashTable *asv = value; - TpContact *contact; - - /* set up the contact with its attributes */ - contact = tp_contact_ensure (self, handle); - tp_contact_set_attributes (contact, asv, data->features, NULL); - g_object_unref (contact); - } - - g_simple_async_result_complete (data->result); -} - -static void -upgrade_contacts_async_fallback_cb (TpConnection *self, +upgrade_contacts_fallback_cb (TpConnection *connection, guint n_contacts, TpContact * const *contacts, const GError *error, @@ -4841,11 +4698,20 @@ upgrade_contacts_async_fallback_cb (TpConnection *self, GObject *weak_object) { GSimpleAsyncResult *result = user_data; + GPtrArray *contacts_array; + guint i; + + contacts_array = g_ptr_array_new_full (n_contacts, g_object_unref); + for (i = 0; i < n_contacts; i++) + g_ptr_array_add (contacts_array, g_object_ref (contacts[i])); + + g_simple_async_result_set_op_res_gpointer (result, contacts_array, + (GDestroyNotify) g_ptr_array_unref); if (error != NULL) g_simple_async_result_set_from_error (result, error); - g_simple_async_result_complete (result); + g_simple_async_result_complete_in_idle (result); } /** @@ -4881,86 +4747,18 @@ tp_connection_upgrade_contacts_async (TpConnection *self, GAsyncReadyCallback callback, gpointer user_data) { - ContactsAsyncData *data; GSimpleAsyncResult *result; - ContactFeatureFlags feature_flags = 0; - guint minimal_feature_flags = G_MAXUINT; - const gchar **supported_interfaces; - GPtrArray *contacts_array; - GArray *handles; - guint i; - - g_return_if_fail (tp_proxy_is_prepared (self, - TP_CONNECTION_FEATURE_CONNECTED)); - g_return_if_fail (n_contacts >= 1); - g_return_if_fail (contacts != NULL); - g_return_if_fail (n_features == 0 || features != NULL); - - for (i = 0; i < n_contacts; i++) - { - g_return_if_fail (contacts[i]->priv->connection == self); - g_return_if_fail (contacts[i]->priv->identifier != NULL); - } - - if (!get_feature_flags (n_features, features, &feature_flags)) - return; - - handles = g_array_sized_new (FALSE, FALSE, sizeof (TpHandle), n_contacts); - contacts_array = g_ptr_array_new_full (n_contacts, g_object_unref); - for (i = 0; i < n_contacts; i++) - { - /* feature flags that all contacts have */ - minimal_feature_flags &= contacts[i]->priv->has_features; - - /* Keep handles of contacts that does not already have all features */ - if ((feature_flags & (~contacts[i]->priv->has_features)) != 0) - g_array_append_val (handles, contacts[i]->priv->handle); - - /* Keep a ref on all contacts to ensure they do not disappear - * while upgrading them */ - g_ptr_array_add (contacts_array, g_object_ref (contacts[i])); - } - - /* Remove features that all contacts have */ - feature_flags &= (~minimal_feature_flags); result = g_simple_async_result_new ((GObject *) self, callback, user_data, tp_connection_upgrade_contacts_async); - g_simple_async_result_set_op_res_gpointer (result, contacts_array, - (GDestroyNotify) g_ptr_array_unref); - - if (tp_proxy_has_interface_by_id (self, - TP_IFACE_QUARK_CONNECTION_INTERFACE_CONTACTS)) - { - supported_interfaces = contacts_bind_to_signals (self, feature_flags); - - if (handles->len > 0 && supported_interfaces[0] != NULL) - { - data = contacts_async_data_new (result, feature_flags); - tp_cli_connection_interface_contacts_call_get_contact_attributes ( - self, -1, handles, supported_interfaces, TRUE, - got_contact_attributes_cb, data, - (GDestroyNotify) contacts_async_data_free, NULL); - } - else - { - g_simple_async_result_complete_in_idle (result); - } - - g_free (supported_interfaces); - } - else - { - G_GNUC_BEGIN_IGNORE_DEPRECATIONS - tp_connection_upgrade_contacts (self, n_contacts, contacts, n_features, - features, upgrade_contacts_async_fallback_cb, - g_object_ref (result), g_object_unref, NULL); - G_GNUC_END_IGNORE_DEPRECATIONS - } - - g_object_unref (result); - g_array_unref (handles); + G_GNUC_BEGIN_IGNORE_DEPRECATIONS + tp_connection_upgrade_contacts (self, + n_contacts, contacts, + n_features, features, + upgrade_contacts_fallback_cb, + result, g_object_unref, NULL); + G_GNUC_END_IGNORE_DEPRECATIONS } /** -- cgit v1.2.3