Attachment 61341 Details for Bug 49371 – TpChannel: move group_remove_error handling to channel-contacts.c

[patch] TpChannel: move group_remove_error handling to channel-contacts.c

TpChannel-move-groupremoveerror-handling-to-channe.patch (text/plain), 17.26 KB, created by Xavier Claessens on 2012-05-10 03:39:24 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Xavier Claessens

Created: 2012-05-10 03:39:24 UTC

Size: 17.26 KB

patch

obsolete

>From 28ba45423e0144c354cfa0247425d3a347fb16df Mon Sep 17 00:00:00 2001
>From: Xavier Claessens <xavier.claessens@collabora.co.uk>
>Date: Mon, 7 May 2012 17:19:18 +0200
>Subject: [PATCH] TpChannel: move group_remove_error handling to
> channel-contacts.c
>
>It means that TP_CHANNEL_FEATURE_CONTACTS must be prepared to get
>the group error.
>
>https://bugs.freedesktop.org/show_bug.cgi?id=49371
>---
> telepathy-glib/channel-contacts.c |  180 +++++++++++++++++++++++++++++++++++--
> telepathy-glib/channel-group.c    |  145 ------------------------------
> tests/dbus/cli-group.c            |   29 ++++--
> 3 files changed, 198 insertions(+), 156 deletions(-)
>
>diff --git a/telepathy-glib/channel-contacts.c b/telepathy-glib/channel-contacts.c
>index f5e852d..31f60ad 100644
>--- a/telepathy-glib/channel-contacts.c
>+++ b/telepathy-glib/channel-contacts.c
>@@ -35,6 +35,32 @@
> #include "telepathy-glib/debug-internal.h"
> #include "telepathy-glib/util-internal.h"
> 
>+/**
>+ * TP_ERRORS_REMOVED_FROM_GROUP:
>+ *
>+ * #GError domain representing the local user being removed from a channel
>+ * with the Group interface. The @code in a #GError with this domain must
>+ * be a member of #TpChannelGroupChangeReason.
>+ *
>+ * This error may be raised on non-Group channels with certain reason codes
>+ * if there's no better error code to use (mainly
>+ * %TP_CHANNEL_GROUP_CHANGE_REASON_NONE).
>+ *
>+ * This macro expands to a function call returning a #GQuark.
>+ *
>+ * Since: 0.7.1
>+ */
>+GQuark
>+tp_errors_removed_from_group_quark (void)
>+{
>+  static GQuark q = 0;
>+
>+  if (q == 0)
>+    q = g_quark_from_static_string ("tp_errors_removed_from_group_quark");
>+
>+  return q;
>+}
>+
> static GArray *
> dup_handle_array (const GArray *source)
> {
>@@ -477,6 +503,99 @@ set_local_pending_info (TpChannel *self,
>       GUINT_TO_POINTER (tp_contact_get_handle (contact)), info);
> }
> 
>+/*
>+ * If the @group_remove_error is derived from a TpChannelGroupChangeReason,
>+ * attempt to rewrite it into a TpError.
>+ */
>+static void
>+_tp_channel_group_improve_remove_error (TpChannel *self,
>+    TpContact *actor)
>+{
>+  GError *error = self->priv->group_remove_error;
>+
>+  if (error == NULL || error->domain != TP_ERRORS_REMOVED_FROM_GROUP)
>+    return;
>+
>+  switch (error->code)
>+    {
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_NONE:
>+      if (actor == self->priv->group_self_contact ||
>+          actor == tp_connection_get_self_contact (self->priv->connection))
>+        {
>+          error->code = TP_ERROR_CANCELLED;
>+        }
>+      else
>+        {
>+          error->code = TP_ERROR_TERMINATED;
>+        }
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_OFFLINE:
>+      error->code = TP_ERROR_OFFLINE;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_KICKED:
>+      error->code = TP_ERROR_CHANNEL_KICKED;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_BUSY:
>+      error->code = TP_ERROR_BUSY;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_INVITED:
>+      DEBUG ("%s: Channel_Group_Change_Reason_Invited makes no sense as a "
>+          "removal reason!", tp_proxy_get_object_path (self));
>+      error->domain = TP_DBUS_ERRORS;
>+      error->code = TP_DBUS_ERROR_INCONSISTENT;
>+      return;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_BANNED:
>+      error->code = TP_ERROR_CHANNEL_BANNED;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_ERROR:
>+      /* hopefully all CMs that use this will also give us an error detail,
>+       * but if they didn't, or gave us one we didn't understand... */
>+      error->code = TP_ERROR_NOT_AVAILABLE;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_INVALID_CONTACT:
>+      error->code = TP_ERROR_DOES_NOT_EXIST;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_NO_ANSWER:
>+      error->code = TP_ERROR_NO_ANSWER;
>+      break;
>+
>+    /* TP_CHANNEL_GROUP_CHANGE_REASON_RENAMED shouldn't be the last error
>+     * seen in the channel - we'll get removed again with a real reason,
>+     * later, so there's no point in doing anything special with this one */
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_PERMISSION_DENIED:
>+      error->code = TP_ERROR_PERMISSION_DENIED;
>+      break;
>+
>+    case TP_CHANNEL_GROUP_CHANGE_REASON_SEPARATED:
>+      DEBUG ("%s: Channel_Group_Change_Reason_Separated makes no sense as a "
>+          "removal reason!", tp_proxy_get_object_path (self));
>+      error->domain = TP_DBUS_ERRORS;
>+      error->code = TP_DBUS_ERROR_INCONSISTENT;
>+      return;
>+
>+    /* all values up to and including Separated have been checked */
>+
>+    default:
>+      /* We don't understand this reason code, so keeping the domain and code
>+       * the same (i.e. using TP_ERRORS_REMOVED_FROM_GROUP) is no worse than
>+       * anything else we could do. */
>+      return;
>+    }
>+
>+  /* If we changed the code we also need to change the domain; if not, we did
>+   * an early return, so we'll never reach this */
>+  error->domain = TP_ERROR;
>+}
>+
> typedef struct
> {
>   GPtrArray *added;
>@@ -507,11 +626,16 @@ members_changed_prepared_cb (GObject *object,
> {
>   TpChannel *self = (TpChannel *) object;
>   MembersChangedData *data = user_data;
>+  TpChannelGroupChangeReason reason;
>+  const gchar *message;
>   GPtrArray *removed;
>   guint i;
> 
>   _tp_channel_contacts_queue_prepare_finish (self, result, NULL, NULL);
> 
>+  reason = tp_asv_get_uint32 (data->details, "change-reason", NULL);
>+  message = tp_asv_get_string (data->details, "message");
>+
>   for (i = 0; i < data->added->len; i++)
>     {
>       TpContact *contact = g_ptr_array_index (data->added, i);
>@@ -528,17 +652,12 @@ members_changed_prepared_cb (GObject *object,
>     {
>       TpContact *contact = g_ptr_array_index (data->local_pending, i);
>       gpointer key = GUINT_TO_POINTER (tp_contact_get_handle (contact));
>-      TpChannelGroupChangeReason reason;
>-      const gchar *message;
> 
>       g_hash_table_remove (self->priv->group_members_contacts, key);
>       g_hash_table_insert (self->priv->group_local_pending_contacts, key,
>           g_object_ref (contact));
>       g_hash_table_remove (self->priv->group_remote_pending_contacts, key);
> 
>-      reason = tp_asv_get_uint32 (data->details, "change-reason", NULL);
>-      message = tp_asv_get_string (data->details, "message");
>-
>       /* Special-case renaming a local-pending contact, if the
>        * signal is spec-compliant. Keep the old actor/reason/message in
>        * this case */
>@@ -608,6 +727,57 @@ members_changed_prepared_cb (GObject *object,
>       g_hash_table_remove (self->priv->group_local_pending_contacts, key);
>       g_hash_table_remove (self->priv->group_local_pending_contact_info, key);
>       g_hash_table_remove (self->priv->group_remote_pending_contacts, key);
>+
>+      if (contact == self->priv->group_self_contact ||
>+          contact == tp_connection_get_self_contact (self->priv->connection))
>+        {
>+          const gchar *error_detail = tp_asv_get_string (data->details,
>+              "error");
>+          const gchar *debug_message = tp_asv_get_string (data->details,
>+              "debug-message");
>+
>+          if (debug_message == NULL && message[0] != '\0')
>+            debug_message = message;
>+
>+          if (debug_message == NULL && error_detail != NULL)
>+            debug_message = error_detail;
>+
>+          if (debug_message == NULL)
>+            debug_message = "(no message provided)";
>+
>+          if (self->priv->group_remove_error != NULL)
>+            g_clear_error (&self->priv->group_remove_error);
>+
>+          if (error_detail != NULL)
>+            {
>+              /* CM specified a D-Bus error name */
>+              tp_proxy_dbus_error_to_gerror (self, error_detail,
>+                  debug_message == NULL || debug_message[0] == '\0'
>+                      ? error_detail
>+                      : debug_message,
>+                  &self->priv->group_remove_error);
>+
>+              /* ... but if we don't know anything about that D-Bus error
>+               * name, we can still do better by using RemovedFromGroup */
>+              if (g_error_matches (self->priv->group_remove_error,
>+                    TP_DBUS_ERRORS, TP_DBUS_ERROR_UNKNOWN_REMOTE_ERROR))
>+                {
>+                  self->priv->group_remove_error->domain =
>+                    TP_ERRORS_REMOVED_FROM_GROUP;
>+                  self->priv->group_remove_error->code = reason;
>+
>+                  _tp_channel_group_improve_remove_error (self, data->actor);
>+                }
>+            }
>+          else
>+            {
>+              /* Use our separate error domain */
>+              g_set_error_literal (&self->priv->group_remove_error,
>+                  TP_ERRORS_REMOVED_FROM_GROUP, reason, debug_message);
>+
>+              _tp_channel_group_improve_remove_error (self, data->actor);
>+            }
>+        }
>     }
> 
>   g_signal_emit_by_name (self, "group-contacts-changed", data->added,
>diff --git a/telepathy-glib/channel-group.c b/telepathy-glib/channel-group.c
>index 07eafc2..e3e2737 100644
>--- a/telepathy-glib/channel-group.c
>+++ b/telepathy-glib/channel-group.c
>@@ -615,100 +615,6 @@ tp_channel_got_group_properties_cb (TpProxy *proxy,
>     }
> }
> 
>-/*
>- * If the @group_remove_error is derived from a TpChannelGroupChangeReason,
>- * attempt to rewrite it into a TpError.
>- */
>-static void
>-_tp_channel_group_improve_remove_error (TpChannel *self,
>-    TpHandle actor)
>-{
>-  GError *error = self->priv->group_remove_error;
>-
>-  if (error == NULL || error->domain != TP_ERRORS_REMOVED_FROM_GROUP)
>-    return;
>-
>-  switch (error->code)
>-    {
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_NONE:
>-      if (actor == self->priv->group_self_handle ||
>-          actor == tp_contact_get_handle (
>-            tp_connection_get_self_contact (self->priv->connection)))
>-        {
>-          error->code = TP_ERROR_CANCELLED;
>-        }
>-      else
>-        {
>-          error->code = TP_ERROR_TERMINATED;
>-        }
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_OFFLINE:
>-      error->code = TP_ERROR_OFFLINE;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_KICKED:
>-      error->code = TP_ERROR_CHANNEL_KICKED;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_BUSY:
>-      error->code = TP_ERROR_BUSY;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_INVITED:
>-      DEBUG ("%s: Channel_Group_Change_Reason_Invited makes no sense as a "
>-          "removal reason!", tp_proxy_get_object_path (self));
>-      error->domain = TP_DBUS_ERRORS;
>-      error->code = TP_DBUS_ERROR_INCONSISTENT;
>-      return;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_BANNED:
>-      error->code = TP_ERROR_CHANNEL_BANNED;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_ERROR:
>-      /* hopefully all CMs that use this will also give us an error detail,
>-       * but if they didn't, or gave us one we didn't understand... */
>-      error->code = TP_ERROR_NOT_AVAILABLE;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_INVALID_CONTACT:
>-      error->code = TP_ERROR_DOES_NOT_EXIST;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_NO_ANSWER:
>-      error->code = TP_ERROR_NO_ANSWER;
>-      break;
>-
>-    /* TP_CHANNEL_GROUP_CHANGE_REASON_RENAMED shouldn't be the last error
>-     * seen in the channel - we'll get removed again with a real reason,
>-     * later, so there's no point in doing anything special with this one */
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_PERMISSION_DENIED:
>-      error->code = TP_ERROR_PERMISSION_DENIED;
>-      break;
>-
>-    case TP_CHANNEL_GROUP_CHANGE_REASON_SEPARATED:
>-      DEBUG ("%s: Channel_Group_Change_Reason_Separated makes no sense as a "
>-          "removal reason!", tp_proxy_get_object_path (self));
>-      error->domain = TP_DBUS_ERRORS;
>-      error->code = TP_DBUS_ERROR_INCONSISTENT;
>-      return;
>-
>-    /* all values up to and including Separated have been checked */
>-
>-    default:
>-      /* We don't understand this reason code, so keeping the domain and code
>-       * the same (i.e. using TP_ERRORS_REMOVED_FROM_GROUP) is no worse than
>-       * anything else we could do. */
>-      return;
>-    }
>-
>-  /* If we changed the code we also need to change the domain; if not, we did
>-   * an early return, so we'll never reach this */
>-  error->domain = TP_ERROR;
>-}
>-
> static void
> handle_members_changed (TpChannel *self,
>                         const gchar *message,
>@@ -821,57 +727,6 @@ handle_members_changed (TpChannel *self,
>       tp_intset_remove (self->priv->group_members, handle);
>       tp_intset_remove (self->priv->group_local_pending, handle);
>       tp_intset_remove (self->priv->group_remote_pending, handle);
>-
>-      if (handle == self->priv->group_self_handle ||
>-          handle == tp_contact_get_handle (tp_connection_get_self_contact (
>-              self->priv->connection)))
>-        {
>-          const gchar *error_detail = tp_asv_get_string (details, "error");
>-          const gchar *debug_message = tp_asv_get_string (details,
>-              "debug-message");
>-
>-          if (debug_message == NULL && message[0] != '\0')
>-            debug_message = message;
>-
>-          if (debug_message == NULL && error_detail != NULL)
>-            debug_message = error_detail;
>-
>-          if (debug_message == NULL)
>-            debug_message = "(no message provided)";
>-
>-          if (self->priv->group_remove_error != NULL)
>-            g_clear_error (&self->priv->group_remove_error);
>-
>-          if (error_detail != NULL)
>-            {
>-              /* CM specified a D-Bus error name */
>-              tp_proxy_dbus_error_to_gerror (self, error_detail,
>-                  debug_message == NULL || debug_message[0] == '\0'
>-                      ? error_detail
>-                      : debug_message,
>-                  &self->priv->group_remove_error);
>-
>-              /* ... but if we don't know anything about that D-Bus error
>-               * name, we can still do better by using RemovedFromGroup */
>-              if (g_error_matches (self->priv->group_remove_error,
>-                    TP_DBUS_ERRORS, TP_DBUS_ERROR_UNKNOWN_REMOTE_ERROR))
>-                {
>-                  self->priv->group_remove_error->domain =
>-                    TP_ERRORS_REMOVED_FROM_GROUP;
>-                  self->priv->group_remove_error->code = reason;
>-
>-                  _tp_channel_group_improve_remove_error (self, actor);
>-                }
>-            }
>-          else
>-            {
>-              /* Use our separate error domain */
>-              g_set_error_literal (&self->priv->group_remove_error,
>-                  TP_ERRORS_REMOVED_FROM_GROUP, reason, debug_message);
>-
>-              _tp_channel_group_improve_remove_error (self, actor);
>-            }
>-        }
>     }
> 
>   g_signal_emit_by_name (self, "group-members-changed", added,
>diff --git a/tests/dbus/cli-group.c b/tests/dbus/cli-group.c
>index 0798464..38877e2 100644
>--- a/tests/dbus/cli-group.c
>+++ b/tests/dbus/cli-group.c
>@@ -213,10 +213,26 @@ check_invalidated_unknown_error_cb (TpProxy *proxy,
>       REMOVED_MESSAGE);
> }
> 
>+static void
>+run_until_members_changed (TpChannel *channel)
>+{
>+  GMainLoop *loop = g_main_loop_new (NULL, FALSE);
>+  gulong id;
>+
>+  id = g_signal_connect_swapped (channel, "group-contacts-changed",
>+      G_CALLBACK (g_main_loop_quit), loop);
>+
>+  g_main_loop_run (loop);
>+
>+  g_signal_handler_disconnect (channel, id);
>+
>+  g_main_loop_unref (loop);
>+}
> 
> static void
> check_removed_unknown_error_in_invalidated (void)
> {
>+  GQuark features[] = { TP_CHANNEL_FEATURE_CONTACTS, 0 };
>   gchar *chan_path;
>   TpTestsTextChannelGroup *service_chan;
>   TpChannel *chan;
>@@ -238,7 +254,7 @@ check_removed_unknown_error_in_invalidated (void)
> 
>   g_assert_no_error (error);
> 
>-  tp_tests_proxy_run_until_prepared (chan, NULL);
>+  tp_tests_proxy_run_until_prepared (chan, features);
>   DEBUG ("ready!");
> 
>   g_signal_connect (chan, "invalidated",
>@@ -256,7 +272,7 @@ check_removed_unknown_error_in_invalidated (void)
> 
>   tp_clear_pointer (&details, g_hash_table_unref);
> 
>-  tp_tests_proxy_run_until_dbus_queue_processed (conn);
>+  run_until_members_changed (chan);
> 
>   details = tp_asv_new (
>       "message", G_TYPE_STRING, REMOVED_MESSAGE,
>@@ -267,7 +283,7 @@ check_removed_unknown_error_in_invalidated (void)
>   tp_group_mixin_change_members ((GObject *) service_chan, NULL,
>       self_handle_singleton, NULL, NULL, details);
> 
>-  tp_tests_proxy_run_until_dbus_queue_processed (conn);
>+  run_until_members_changed (chan);
> 
>   tp_cli_channel_call_close (chan, -1, NULL, NULL, NULL, NULL);
> 
>@@ -308,6 +324,7 @@ check_invalidated_known_error_cb (TpProxy *proxy,
> static void
> check_removed_known_error_in_invalidated (void)
> {
>+  GQuark features[] = { TP_CHANNEL_FEATURE_CONTACTS, 0 };
>   gchar *chan_path;
>   TpTestsTextChannelGroup *service_chan;
>   TpChannel *chan;
>@@ -329,7 +346,7 @@ check_removed_known_error_in_invalidated (void)
> 
>   g_assert_no_error (error);
> 
>-  tp_tests_proxy_run_until_prepared (chan, NULL);
>+  tp_tests_proxy_run_until_prepared (chan, features);
>   DEBUG ("ready!");
> 
>   g_signal_connect (chan, "invalidated",
>@@ -347,7 +364,7 @@ check_removed_known_error_in_invalidated (void)
> 
>   tp_clear_pointer (&details, g_hash_table_unref);
> 
>-  tp_tests_proxy_run_until_dbus_queue_processed (conn);
>+  run_until_members_changed (chan);
> 
>   details = tp_asv_new (
>       "message", G_TYPE_STRING, REMOVED_MESSAGE,
>@@ -358,7 +375,7 @@ check_removed_known_error_in_invalidated (void)
>   tp_group_mixin_change_members ((GObject *) service_chan, NULL,
>       self_handle_singleton, NULL, NULL, details);
> 
>-  tp_tests_proxy_run_until_dbus_queue_processed (conn);
>+  run_until_members_changed (chan);
> 
>   tp_cli_channel_call_close (chan, -1, NULL, NULL, NULL, NULL);
> 
>-- 
>1.7.9.5

Actions: View | Splinter Review

Attachments on bug 49371: 61177 | 61178 | 61179 | 61180 | 61181 | 61182 | 61183 | 61184 | 61185 | 61186 | 61187 | 61338 | 61339 | 61340 | 61341 | 61342 | 61343 | 61344 | 61345 | 61346 | 61347 | 61348

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.