Bug 26374

(In reply to comment #0)
> Split a BaseCallChannel out of CallChannel
> 0e6717808feb31f545eabde72c7d73661d035c57
> +  /* automatically add creator to channel, but also ref them again (because
> +   * priv->creator is the InitiatorHandle) */
> +  g_assert (priv->creator != 0);
> +  tp_handle_ref (contact_handles, priv->creator);
> 
> This handle is never unreffed.
Code has changed around, handle reffing should be correct now

> Cope with not having a session when getting properties'
> 80385283026e9836f45b709d456f1f6d8305ca46
>        case PROP_MUTABLE_CONTENTS:
> -        g_value_set_boolean (value,
> -           gabble_jingle_session_can_modify_contents (priv->session));
> +        if (priv->session != NULL)
> +          g_value_set_boolean (value,
> +            gabble_jingle_session_can_modify_contents (priv->session));
> +        else
> +          g_value_set_boolean (value, TRUE);
>          break;
> 
> What's the reason for setting it to TRUE if there's no session?

Because i didn't write the code yet to figure out if the member we would create the session to was able to do non-mutalbe contents. This code is one big FIXME in my current branch and does indeed need extra work
 
> Make muji channels requestable and spawn at the right time
> 6af394a1e3eee210ee6f1dab73050ab358fbfd3e
> +void
> +gabble_muc_channel_request_call (GabbleMucChannel *gmuc,
> +    GHashTable *request,
> +    gboolean require_new,
> +    gpointer token,
> +    GAsyncReadyCallback callback,
> +    gpointer user_data)
> 
> Should this be cancellable?

Probably :)

> +  for (l = priv->call_requests ; l != NULL; l = g_list_next (l))
> +    {
> +      GSimpleAsyncResult *r = G_SIMPLE_ASYNC_RESULT (l->data);
> +
> +      if (error != NULL)
> +        g_simple_async_result_set_from_error (r, error);
> +
> +      g_simple_async_result_complete (r);
> +      g_object_unref (r);
> +    }
> 
> Should this be *_complete or *_complete_in_idle?

In the current code this should always get called from the mainloop so _complete is correct

> +  GabbleMucFactory *self = GABBLE_MUC_FACTORY (user_data);
> +  GabbleMucChannel *channel = GABBLE_MUC_CHANNEL (source);
> +  gpointer request_token;
> +  GError *error = NULL;
> +
> +
> +  if (!gabble_muc_channel_request_call_finish (channel,
> +      result, &request_token, &error))
> +    {
> +      tp_channel_manager_emit_request_failed (self,
> +        request_token, error->domain, error->code, error->message);
> +      g_error_free (error);
> +    }
> +
> +  /* No need to handle a successful request, this is handled when the muc
> +   * signals a new call cahnnel automagically */
> +}
> 
> Odd double newline.
> cahnnels that do automagic things are nice.

fixed

> Add a better for the wocky-muc property
> d8901f3ce0638aeffa13908e0a887267ff92b3a8
> 
> Add a better what?

name :)

> Start using seperate CallMember objects
> ca1f3cd95792bf99e73fdc3311638654326635be
>        case PROP_MUTABLE_CONTENTS:
> +      /* FIXME: this should probably move to the implementation class
>          if (priv->session != NULL)
>            g_value_set_boolean (value,
>              gabble_jingle_session_can_modify_contents (priv->session));
>          else
> +          */
>            g_value_set_boolean (value, TRUE);
>          break;
> 
> Seems kind of important.

Not for the initial phases that much, needs more work what now is the CallMember to figure out whether any potential session could have mutable context. It's slightly racy as well, needs some rethinking in both the spec and the code.

> +          GabbleJingleSession *session =
> +              gabble_call_member_get_session (member);
> +
> +          gabble_jingle_session_terminate (session,
> +           TP_CHANNEL_GROUP_CHANGE_REASON_NONE,
> +            NULL, NULL);
> 
> Verify that it's not null?

fixed

> +      content = gabble_call_member_create_content (member, name, type,
> &error);
> +
> +      if (content != NULL)
> +        path = gabble_base_call_channel_add_content (self,
> +            gabble_call_member_content_get_jingle_content (content),
> +            GABBLE_CALL_CONTENT_DISPOSITION_NONE,
> +              ((TpBaseConnection *) self->conn)->self_handle);
> +      break;
> 
> Debug if failed would be nice.

Probably, needs some changing around to work in the muc case though, for the TODO :)
 
> +++ b/src/base-call-channel.h
> @@ -25,7 +25,9 @@
> 
>  #include <extensions/extensions.h>
> 
> +#include "connection.h"
> 
> What in here needs connection.h?

improved
 
> +          switch (gabble_call_member_content_get_media_type (content))
> +            {
> +              case JINGLE_MEDIA_TYPE_AUDIO:
> +                cbase->initial_audio = TRUE;
> +                break;
> +              case JINGLE_MEDIA_TYPE_VIDEO:
> +                cbase->initial_video = TRUE;
> +                break;
> +              default:
> +                break;
> +            }
> 
> Assert or debug message would be nice.

Added a debug message

> @@ -145,6 +214,7 @@ gabble_call_channel_dispose (GObject *object)
> 
>    self->priv->dispose_has_run = TRUE;
> 
> +
>    if (G_OBJECT_CLASS (gabble_call_channel_parent_class)->dispose)
>      G_OBJECT_CLASS (gabble_call_channel_parent_class)->dispose (object);
>  }
> 
> :)
> 
> GabbleCallMemberContent isn't unreffing/freeing anything in _dispose or
> _finalize.

that's done in the base channel?

> +  if (audio_name != NULL)
> +    gabble_call_member_create_content (self, audio_name,
> +      JINGLE_MEDIA_TYPE_AUDIO, NULL);
> +
> +  if (video_name != NULL)
> +    gabble_call_member_create_content (self, video_name,
> +      JINGLE_MEDIA_TYPE_VIDEO, NULL);
> 
> Error handling would be nice.

Yeah, this code has changed around though

> +static void
> +muc_channel_new_call (GabbleMucChannel *muc,
> +    GabbleCallMucChannel *call,
> +    GSList *requests,
> +    gpointer user_data)
> +{
> +  GabbleMucFactory *fac = GABBLE_MUC_FACTORY (user_data);
> +
> +  DEBUG ("Emitting new Call channel");
> +
> +  tp_channel_manager_emit_new_channel (fac,
> +      TP_EXPORTABLE_CHANNEL (call), requests);
> +
> +  g_signal_connect (call, "closed",
> +    G_CALLBACK (muc_sub_channel_closed_cb), fac);
> +}
> 
> Any reason for it to {,not }be connect weak?

If the factory doesn't live longer then its channels we have bigger problems ;)
 
> +    {
> +      if (require_new)
> +        {
> +          g_set_error (error, TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
> +            "There is already a call in this muc");
> +          goto error;
> +        }
> +      else
> +        {
> +          tp_channel_manager_emit_request_already_satisfied (self,
> +            request_token,
> +            TP_EXPORTABLE_CHANNEL (call));
> +          goto out;
> +        }
> +    }
> +
> +  /* FIXME not coping properly with deinitialisation */
> +  gabble_muc_channel_request_call (muc,
> +      request_properties,
> +      require_new,
> +      request_token,
> +      call_muc_channel_request_cb,
> +      self);
> +out:
> +  return TRUE;
> +
> +error:
> +  return FALSE;
> 
> Is there a benefit to this using gotos?

I like function with single exit points

> +  for (c = gabble_jingle_session_get_contents (session) ;
> +      c != NULL; c = g_list_next (c))
> 
> Odd extra space before the semicolon :)



> +      if (priv->transport_ns == NULL)
> +        {
> +          g_object_get (content, "transport-ns",
> +            &priv->transport_ns,
> +            NULL);
> +        }
> 
> Are all contents transports in a call going to/supposed to be the same?

fixed

> +  c = gabble_jingle_session_add_content (priv->session,
> +      mtype, name, content_ns, priv->transport_ns);
> +
> +  g_assert (c != NULL);
> +
> +  content = gabble_call_member_content_from_jingle_content (c);
> 
> c is leaking here.

afaik we don't own that ref

> +  session = gabble_jingle_factory_create_session (
> +        priv->call->conn->jingle_factory,
> +        priv->target, resource, FALSE);
> +
> +  gabble_call_member_set_session (self, session);
> +  g_object_set (session, "dialect", dialect, NULL);
> 
> Session is leaking here.

We don't get a ref from create_session...

> -void
> -gabble_base_call_channel_set_session (GabbleBaseCallChannel *self,
> -    GabbleJingleSession *session)
> 
> The header is still in base-call-channel.h

Fixed

Pushed an updated branch, please have a look :)

>> @@ -145,6 +214,7 @@ gabble_call_channel_dispose (GObject *object)
>> 
>>    self->priv->dispose_has_run = TRUE;
>> 
>> +
>>    if (G_OBJECT_CLASS (gabble_call_channel_parent_class)->dispose)
>>      G_OBJECT_CLASS (gabble_call_channel_parent_class)->dispose (object);
>>  }
>> 
>> :)
>> 
>> GabbleCallMemberContent isn't unreffing/freeing anything in _dispose or
>> _finalize.
>
>that's done in the base channel?

Looks like I pasted the wrong bit as CallChannel is unreffing stuff. Check the CallMemberContent class instead.


Add a lot more API to properly proxy Jingle bits
653609aa565dc5f1e2eee0d3c4e7113037076b0b

+  param_spec = g_param_spec_object ("member", "CallMember",
+      "The call member  that has this as a content",
+      GABBLE_TYPE_CALL_MEMBER,
+      G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+  g_object_class_install_property (object_class, PROP_MEMBER,
+    param_spec);

Double-spaced docstring.


Add API to open a session
3308ebf689138a4a7fb0f8aeeacaa1af0747b4ce+/**
+ * Start a new session using the existing contents for this member. For now
+ * assumes we're using the latest jingle dialec and ice-udp
+ * FIXME: make dialect and transport selection more dynamic?
+ */

s/dialec/dialect/


+gboolean
+gabble_call_member_open_session (GabbleCallMember *self,
+    GError **error)
+{
+  GabbleCallMemberPrivate *priv = self->priv;
+  GabbleJingleSession *session;
+  gchar *jid;
+
+  jid = gabble_peer_to_jid (priv->call->conn, priv->target, NULL);
+
+  session = gabble_jingle_factory_create_session (
+        priv->call->conn->jingle_factory,
+        priv->target, jid, FALSE);
+  DEBUG ("Created a jingle session: %p", session);
+
+  g_object_set (session, "dialect", JINGLE_DIALECT_V032, NULL);
+
+  priv->transport_ns = g_strdup (NS_JINGLE_TRANSPORT_ICEUDP);
+
+  gabble_call_member_set_session (self, session);
+
+  return TRUE;
+}

jid appears to be being leaked here.


On incoming jingle sessions check if they belong to a muc call
ba98d7f0fb5796ba31b770ee214a14a6fd610957
+  member = gabble_base_call_channel_get_member_from_handle
+    (GABBLE_BASE_CALL_CHANNEL (self), session->peer);
+
+  if (member == NULL || gabble_call_member_get_session (member) != NULL)
+    {
+      gabble_jingle_session_terminate (session,
+        TP_CHANNEL_GROUP_CHANGE_REASON_NONE,
+        "Muji jingle session initiated while there already was one",
+        NULL);
+    }
+  else
+    {
+      gabble_call_member_set_session (member, session);
+    }
+}

Would a jingle session have been initiated already if member == NULL?


Simplify determining our local sending state a lot
3a3e025e3c6a02016acdad095943163723e83987
-      if (state == JINGLE_CONTENT_STATE_EMPTY && created_by_us)
-        {
-          local_state = GABBLE_SENDING_STATE_SENDING;
-        }
-      else if (created_by_us || state == JINGLE_CONTENT_STATE_ACKNOWLEDGED)
-        {
-          gpointer state_p;
-          gboolean exists;
-
-          exists = g_hash_table_lookup_extended (priv->senders,
-              GUINT_TO_POINTER (TP_BASE_CONNECTION (priv->conn)->self_handle),
-              NULL,
-              &state_p);
-
-          if (exists && GPOINTER_TO_UINT (state_p) ==
-              GABBLE_SENDING_STATE_SENDING)
-            local_state = GABBLE_SENDING_STATE_SENDING;
-          else
-            local_state = GABBLE_SENDING_STATE_PENDING_SEND;
-        }
+      if (state == JINGLE_CONTENT_STATE_ACKNOWLEDGED)
+        local_state = GABBLE_SENDING_STATE_SENDING;
       else
         local_state = GABBLE_SENDING_STATE_PENDING_SEND;

Are you absolutely sure about this? I seem to remember having to fiddle with it to no end to reach all cases

(In reply to comment #3)
> >> @@ -145,6 +214,7 @@ gabble_call_channel_dispose (GObject *object)
> >> 
> >>    self->priv->dispose_has_run = TRUE;
> >> 
> >> +
> >>    if (G_OBJECT_CLASS (gabble_call_channel_parent_class)->dispose)
> >>      G_OBJECT_CLASS (gabble_call_channel_parent_class)->dispose (object);
> >>  }
> >> 
> >> :)
> >> 
> >> GabbleCallMemberContent isn't unreffing/freeing anything in _dispose or
> >> _finalize.
> >
> >that's done in the base channel?
> 
> Looks like I pasted the wrong bit as CallChannel is unreffing stuff. Check the
> CallMemberContent class instead.

? /me confused

> Add a lot more API to properly proxy Jingle bits
> 653609aa565dc5f1e2eee0d3c4e7113037076b0b
> 
> +  param_spec = g_param_spec_object ("member", "CallMember",
> +      "The call member  that has this as a content",
> +      GABBLE_TYPE_CALL_MEMBER,
> +      G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
> +  g_object_class_install_property (object_class, PROP_MEMBER,
> +    param_spec);
> 
> Double-spaced docstring.

fixed

> Add API to open a session
> 3308ebf689138a4a7fb0f8aeeacaa1af0747b4ce+/**
> + * Start a new session using the existing contents for this member. For now
> + * assumes we're using the latest jingle dialec and ice-udp
> + * FIXME: make dialect and transport selection more dynamic?
> + */
> 
> s/dialec/dialect/

fixed

> +gboolean
> +gabble_call_member_open_session (GabbleCallMember *self,
> +    GError **error)
> +{
> +  GabbleCallMemberPrivate *priv = self->priv;
> +  GabbleJingleSession *session;
> +  gchar *jid;
> +
> +  jid = gabble_peer_to_jid (priv->call->conn, priv->target, NULL);
> +
> +  session = gabble_jingle_factory_create_session (
> +        priv->call->conn->jingle_factory,
> +        priv->target, jid, FALSE);
> +  DEBUG ("Created a jingle session: %p", session);
> +
> +  g_object_set (session, "dialect", JINGLE_DIALECT_V032, NULL);
> +
> +  priv->transport_ns = g_strdup (NS_JINGLE_TRANSPORT_ICEUDP);
> +
> +  gabble_call_member_set_session (self, session);
> +
> +  return TRUE;
> +}
> 
> jid appears to be being leaked here.

fixed

> On incoming jingle sessions check if they belong to a muc call
> ba98d7f0fb5796ba31b770ee214a14a6fd610957
> +  member = gabble_base_call_channel_get_member_from_handle
> +    (GABBLE_BASE_CALL_CHANNEL (self), session->peer);
> +
> +  if (member == NULL || gabble_call_member_get_session (member) != NULL)
> +    {
> +      gabble_jingle_session_terminate (session,
> +        TP_CHANNEL_GROUP_CHANGE_REASON_NONE,
> +        "Muji jingle session initiated while there already was one",
> +        NULL);
> +    }
> +  else
> +    {
> +      gabble_call_member_set_session (member, session);
> +    }
> +}
> 
> Would a jingle session have been initiated already if member == NULL?

No, that would mean we got a session from someone we don't think is a member. Which shouldn't happen.

> Simplify determining our local sending state a lot
> 3a3e025e3c6a02016acdad095943163723e83987
> -      if (state == JINGLE_CONTENT_STATE_EMPTY && created_by_us)
> -        {
> -          local_state = GABBLE_SENDING_STATE_SENDING;
> -        }
> -      else if (created_by_us || state == JINGLE_CONTENT_STATE_ACKNOWLEDGED)
> -        {
> -          gpointer state_p;
> -          gboolean exists;
> -
> -          exists = g_hash_table_lookup_extended (priv->senders,
> -              GUINT_TO_POINTER (TP_BASE_CONNECTION (priv->conn)->self_handle),
> -              NULL,
> -              &state_p);
> -
> -          if (exists && GPOINTER_TO_UINT (state_p) ==
> -              GABBLE_SENDING_STATE_SENDING)
> -            local_state = GABBLE_SENDING_STATE_SENDING;
> -          else
> -            local_state = GABBLE_SENDING_STATE_PENDING_SEND;
> -        }
> +      if (state == JINGLE_CONTENT_STATE_ACKNOWLEDGED)
> +        local_state = GABBLE_SENDING_STATE_SENDING;
>        else
>          local_state = GABBLE_SENDING_STATE_PENDING_SEND;
> 
> Are you absolutely sure about this? I seem to remember having to fiddle with it
> to no end to reach all cases

It passes all the tests ;) It should be correct and i was just not thinking very well when i wrote the intiial version (also one of the tests was wrong instead of the code).. I also realized we probably want an extra state which says: going to send asap

Implement the basic muji protocol logic
09de6a32012d93bdb842d0e9c9df2efaaaaa7000
+  /* The list of members who should sent an update before us */
+  GQueue *before;
+  GQueue *after;
+
+  /* List of members we should initial a session to after joining */
+  GQueue *sessions_to_open;

why GQueues? (no problem, just curious of your reason)


+  /* Internally preparing before we can sent muji information to the muc, only
+   * happens on the initial join */

s/sent/send/?


+  while ((m = g_queue_pop_head (priv->sessions_to_open)) != NULL)
+    gabble_call_member_open_session (m, 0);

Seems odd to use 0 instead of NULL and/or should this actually print the error (even though open_session doesn't set it yet)?


+        /* Start preperation of the next round */

s/preperation/preparation/


Forbid presences with muji untill we call Accept
316c8c44a8dd4b0e68636143f597fa6ff817091c
+    # Not allowed to have muji releated presences before we accept the channel

s/releated/related/

Test a remote party adding a video stream
dc575e2aee5c4387c5c8fb9130542557c65b43c0
+    presence = make_muc_presence('owner', 'moderator', muc, 'bob')
+    muji =  ('muji', ns.MUJI, {},
+        [('content', ns.MUJI, { "name": "Voice" },
+            [( 'description', None, {"media": "audio"},
+            jt.generate_payloads(jt.audio_codecs))]),
+         ('content', ns.MUJI, { "name": "Camera" },
+            [( 'description', None, {"media": "video"},
+            jt.generate_payloads(jt.video_codecs))]),
+        ])

Technically the description element should have the "urn:xmpp:jingle:apps:rtp:0" namespace.


Assert that the CallState changes once our muji state is stable
0477008786f63041d951ddb0571a6f5641aa80bc
+  if (!priv->sessions_opened)
+    {
+      /* At the point where we opened the sessions we're accepted i
+         in the call ? */
+      gabble_base_call_channel_set_state ( GABBLE_BASE_CALL_CHANNEL (self),
+          GABBLE_CALL_STATE_ACCEPTED);
+    }

This comment doesn't instill a great deal of confidence and/or you've got a dangling 'i'.


Implement hanging up properly
1415b81a5d093c8b7a3733bbb805181cf0681415
+    # happyness.. Now let's hang up
+    channel.Hangup (0, "", "", dbus_interface=cs.CHANNEL_TYPE_CALL)

s/happyness/happiness/


Add API to shut down members
8685e7f1577de05e5bbd4d15133af5c587d67782

Jingle function calls are almost completely factored out :)

(In reply to comment #6)
> Test a remote party adding a video stream
> dc575e2aee5c4387c5c8fb9130542557c65b43c0
> +    presence = make_muc_presence('owner', 'moderator', muc, 'bob')
> +    muji =  ('muji', ns.MUJI, {},
> +        [('content', ns.MUJI, { "name": "Voice" },
> +            [( 'description', None, {"media": "audio"},
> +            jt.generate_payloads(jt.audio_codecs))]),
> +         ('content', ns.MUJI, { "name": "Camera" },
> +            [( 'description', None, {"media": "video"},
> +            jt.generate_payloads(jt.video_codecs))]),
> +        ])
> 
> Technically the description element should have the
> "urn:xmpp:jingle:apps:rtp:0" namespace.

urn:xmpp:jingle:apps:rtp:1 even these days, good catch thanks

> Assert that the CallState changes once our muji state is stable
> 0477008786f63041d951ddb0571a6f5641aa80bc
> +  if (!priv->sessions_opened)
> +    {
> +      /* At the point where we opened the sessions we're accepted i
> +         in the call ? */
> +      gabble_base_call_channel_set_state ( GABBLE_BASE_CALL_CHANNEL (self),
> +          GABBLE_CALL_STATE_ACCEPTED);
> +    }
> 
> This comment doesn't instill a great deal of confidence and/or you've got a
> dangling 'i'.

fixed

> Implement hanging up properly
> 1415b81a5d093c8b7a3733bbb805181cf0681415
> +    # happyness.. Now let's hang up
> +    channel.Hangup (0, "", "", dbus_interface=cs.CHANNEL_TYPE_CALL)
> 
> s/happyness/happiness/

fixed

>  Add API to shut down members
> 8685e7f1577de05e5bbd4d15133af5c587d67782
> 
> Jingle function calls are almost completely factored out :)

:)

(In reply to comment #5)
> Implement the basic muji protocol logic
> 09de6a32012d93bdb842d0e9c9df2efaaaaa7000
> +  /* The list of members who should sent an update before us */
> +  GQueue *before;
> +  GQueue *after;
> +
> +  /* List of members we should initial a session to after joining */
> +  GQueue *sessions_to_open;
> 
> why GQueues? (no problem, just curious of your reason)

They're an underused data-structure in glib and prevent common mistakes not using the return value of g_list_append. Also appending is O(1) instead of O(n), which in practise doesn't matter as our list are small, but is still nice to have.
 
> 
> +  /* Internally preparing before we can sent muji information to the muc, only
> +   * happens on the initial join */
> 
> s/sent/send/?
>
fixed

> 
> +  while ((m = g_queue_pop_head (priv->sessions_to_open)) != NULL)
> +    gabble_call_member_open_session (m, 0);
> 
> Seems odd to use 0 instead of NULL and/or should this actually print the error
> (even though open_session doesn't set it yet)?

It can't fail currently, but yes i have no clue why i put in 0 instead of NULL there. fixed.
 
> +        /* Start preperation of the next round */
> 
> s/preperation/preparation/

fixed

> Forbid presences with muji untill we call Accept
> 316c8c44a8dd4b0e68636143f597fa6ff817091c
> +    # Not allowed to have muji releated presences before we accept the channel
> 
> s/releated/related/

fixed

It all looks good :)

Except I just ran make check and jingle/call-codecoffer.py is consistently failing.

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.

(In reply to comment #11)
> 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?

The call channel, no. Well the GObject could have been created but it wasn't at a point
where you'd call it finished at the point the muc is closed.

> +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)?

Historical raisons mostly. I've changes it to be call in both places as i find that slightly nicer,
but there isn't much in it.

> 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?

As a data structure you mean or ?

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

fixed

> 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.

Would you suggest an array instead? In practise members always leave one by one, 
so a single TpHandle seems better. The other option is to only signal CallMembersChanged
with a non-empty list of removals in gabble_base_call_channel_remove_member directly, but 
that just seems like it would duplicate code for no good reason.

(In reply to comment #12)
> > 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?
> 
> As a data structure you mean or ?

The object, but I looked at the object again and its more for handling if the object is freed before the async request finishes. Since that's not happening here, it's not really any extra useful.
 
> > 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.
> 
> Would you suggest an array instead? In practise members always leave one by
> one, 
> so a single TpHandle seems better. The other option is to only signal
> CallMembersChanged
> with a non-empty list of removals in gabble_base_call_channel_remove_member
> directly, but 
> that just seems like it would duplicate code for no good reason.

All I was saying was that it was not very obvious as to what the handle was for, being that it's called "handle". I didn't realize quite what this was doing though. Being that it's for emitting the CallMembersChanged, it makes less non-obvious. It should be fine as is then.

Looks good and all tests pass make check :)

Merged