[Bug 26374] muc-calls branch review notes.

[bugzilla-daemon at freedesktop.org]

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

--- Comment #12 from Sjoerd Simons <sjoerd at luon.net> 2010-05-21 10:52:27 PDT ---
(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.

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