[Bug 26374] muc-calls branch review notes.

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=26374

--- Comment #11 from Mike Ruprecht <cmaiku at gmail.com> 2010-05-10 05:05:29 PDT ---
Properly cleanup call requests when closing the underlying muc
fc49f692338ae7577836cabb9b41c67c974fe53c
(in muc-channel.c:close_channel)

+  muc_call_channel_finish_requests (chan, NULL, &error);
-----------------------------------------------------------------
(in muc_call_channel_finish_requests)

  if (call != NULL)
    {
      GSList *requests = NULL;

      DEBUG ("Call channel created");

      for (l = self->priv->call_requests ; l != NULL; l = g_list_next (l))
        requests = g_slist_append (requests,
          g_simple_async_result_get_op_res_gpointer (
            G_SIMPLE_ASYNC_RESULT(l->data)));

      g_signal_emit (self, signals[NEW_CALL], 0, self->priv->call,
          requests);
      g_slist_free (requests);
    }
  else
    {
      DEBUG ("Failed to create call channel: %s", error->message);
    }

This debug message seems inaccurate: "Failed to create call channel". Couldn't
the call already be successfully created and the muc-channel just be closing?


+muc_call_channel_finish_requests (GabbleMucChannel *self,
+  GabbleCallMucChannel *call,
+  GError *error)
 {
-  GabbleMucChannel *gmuc = GABBLE_MUC_CHANNEL (user_data);
-  GabbleMucChannelPrivate *priv = GABBLE_MUC_CHANNEL_GET_PRIVATE (gmuc);
   GList *l;
-  GError *error = NULL;
-
-  g_assert (priv->call == NULL);

-  priv->call = gabble_call_muc_channel_new_finish (source,
-    result, &error);
-
-  g_signal_connect (priv->call, "closed",
-    G_CALLBACK (muc_channel_call_closed_cb),
-    gmuc);
-
-  if (priv->call != NULL)
+  if (call != NULL)
     {
       GSList *requests = NULL;

       DEBUG ("Call channel created");

-      for (l = priv->call_requests ; l != NULL; l = g_list_next (l))
+      for (l = self->priv->call_requests ; l != NULL; l = g_list_next (l))
         requests = g_slist_append (requests,
           g_simple_async_result_get_op_res_gpointer (
             G_SIMPLE_ASYNC_RESULT(l->data)));

-      g_signal_emit (gmuc, signals[NEW_CALL], 0, priv->call,
+      g_signal_emit (self, signals[NEW_CALL], 0, self->priv->call,
           requests);
       g_slist_free (requests);

Call and priv->call seem to be being used interchangeably. Also there any
reason to have the call parameter here (except maybe above the above comment)?


Don't let the async operation try to remember the request token
6a1acd2cd7066ca9006177c3558680b1a5faa8cf
TpWeakRef could be used here. Would that be more beneficial in any way?


Pass a prefix instead of the full object path when creating a new channel
b61312fb4280d377408068bd1baf9bf67e16ca28
object_path_prefix is never freed.


Add basic support for removing members
ab0989e9bf2da328aeeb36e1a29ac78147e9d899
Adding a handle to base_call_channel_signal_call_members seems
hacky/non-obvious. Being that it's a static function, that's probably ok, just
feels weird.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.