GNOME Bugzilla – Bug 643656
Autoreject incoming calls when there is one in progress
Last modified: 2011-03-07 11:55:19 UTC
Currently, empathy doesn't handle multiple calls very well, see bug 623348. I propose to autoreject incoming calls when there are other ongoing calls. At some point we should implement what bug 623348 describes though.
Created attachment 182243 [details] [review] Autoreject calls when we're already on one http://git.collabora.co.uk/?p=user/pochu/empathy.git;a=shortlog;h=refs/heads/autoreject-calls-643656 We should make the observer delay the approvers when that is merged, see https://bugs.freedesktop.org/show_bug.cgi?id=32267
Not a review really, but: +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ No. +struct _EmpathyObserver { + GObject parent; + EmpathyObserverPriv *priv; +}; Indentation. You should probably only create the private structure in _init. Do the rest in constructed.
(In reply to comment #2) > Not a review really, but: > > +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ > > No. > > +struct _EmpathyObserver { > + GObject parent; > + EmpathyObserverPriv *priv; > +}; > > Indentation. Both fixed. > You should probably only create the private structure in _init. Do the rest in > constructed. 'probably' ? Can you explain why? I've had a look at the ->constructed documentation and I don't see why I'd need to do it there and not in _init().
Created attachment 182269 [details] [review] new patch
Review of attachment 182269 [details] [review]: ::: src/empathy-observer.c @@ +1,2 @@ +/* + * Copyright (C) 2011 Collabora Ltd. EmpathyObserver is a pretty generic name. I think it should be something like EmpathyCallObserver to make its purpose clearer. @@ +39,3 @@ +struct _EmpathyObserverPriv { + TpBaseClient *observer; + GList *channels; Please describe what's stored and how (borrowed or reffed). @@ +43,3 @@ + +G_DEFINE_TYPE (EmpathyObserver, empathy_observer, G_TYPE_OBJECT); + Please add a few lines of comment explaining the purpose of this component. @@ +67,3 @@ + if (error != NULL) + { + DEBUG ("Could not claim CDO"); display the error message. @@ +71,3 @@ + } + + tp_channel_close_async (channel, NULL, NULL); Shouldn't we use tp_channel_close_async (channel, TP_CHANNEL_GROUP_CHANGE_REASON_BUSY, "Already in a call", NULL, NULL) ? @@ +117,3 @@ + "Unknown channel type" }; + + DEBUG ("Failed to find the main channel; ignoring"); That's not a very clear error. "Didn't find any Call or StreamedMedia channel; ignoring" ? @@ +131,3 @@ + + /* Autoreject if there are other ongoing calls */ + for (l = self->priv->channels; l != NULL; l = l->next) I think this is worth an helper function: gboolean have_ongoing_call() or something. @@ +139,3 @@ + if (type == TPY_IFACE_QUARK_CHANNEL_TYPE_CALL && + tpy_call_channel_get_state (TPY_CALL_CHANNEL (chan), NULL, NULL) + == TPY_CALL_STATE_ENDED) SM channels are automatically closed when the call is terminated? @@ +141,3 @@ + == TPY_CALL_STATE_ENDED) + continue; + This deserve at least a DEBUG message explaining what we are doing to this poor channel and why. @@ +142,3 @@ + continue; + + tp_cli_channel_dispatch_operation_call_claim (dispatch_operation, Why not use tp_channel_dispatch_operation_claim_async() ? @@ +149,3 @@ + } + + g_signal_connect (channel, "invalidated", I think you should use tp_g_signal_connect_object() to avoid to be called after the object has been destroyed. @@ +151,3 @@ + g_signal_connect (channel, "invalidated", + G_CALLBACK (on_channel_closed), self); + self->priv->channels = g_list_prepend (self->priv->channels, I'm not 100% sure it can actually happen, but checking if the channel is not already invalidated sounds safer to me. @@ +154,3 @@ + g_object_ref (channel)); + } + else How can this happen? find_main_channel should have returned NULL in that case. @@ +189,3 @@ + } + +return retval; alignement is wrong. @@ +197,3 @@ + EmpathyObserver *self = EMPATHY_OBSERVER (object); + + tp_clear_object (&self->priv->observer); priv->channels seem to be leaked here, and the ref on the channels are leaked as well. @@ +231,3 @@ + + self->priv->observer = tp_simple_observer_new (dbus, FALSE, + "Empathy.Observer", FALSE, Empathy.Observer should be changed to fit the new name. @@ +234,3 @@ + observe_channels, self, NULL); + + factory = empathy_channel_factory_new (); don't we have a _dup_singleton()? @@ +239,3 @@ + g_object_unref (factory); + + /* Calls */ Not a very useful comment. /* Observe Call and StreamedMedia channels */
(In reply to comment #5) Thanks for the review. I've everything, but the following: > @@ +67,3 @@ > + if (error != NULL) > + { > + DEBUG ("Could not claim CDO"); > > display the error message. As a popup window? Why? This is an error when trying to autoreject the call, so if we don't autoreject it, the call will show up, and displaying an error together with it sounds pretty confusing. > @@ +139,3 @@ > + if (type == TPY_IFACE_QUARK_CHANNEL_TYPE_CALL && > + tpy_call_channel_get_state (TPY_CALL_CHANNEL (chan), NULL, NULL) > + == TPY_CALL_STATE_ENDED) > > SM channels are automatically closed when the call is terminated? If they aren't, I don't see an State property or something... So I assumed they are. > @@ +141,3 @@ > + == TPY_CALL_STATE_ENDED) > + continue; > + > > This deserve at least a DEBUG message explaining what we are doing to this poor > channel and why. A DEBUG or a comment? We're not doing anything to the channel (just seeing its state, but not modifying it). I don't think a DEBUG is appropriate here. > @@ +149,3 @@ > + } > + > + g_signal_connect (channel, "invalidated", > > I think you should use tp_g_signal_connect_object() to avoid to be called after > the object has been destroyed. That can't happen as I ref the channel (and unref it when it's invalidated). > @@ +234,3 @@ > + observe_channels, self, NULL); > + > + factory = empathy_channel_factory_new (); > > don't we have a _dup_singleton()? It's _dup(), I've used it. (Maybe it should be made a proper singleton by overriding ->constructor). I've pushed everything in a new commit for easier reviewing. I'll squash all into a single commit when you give a +1 since this split doesn't make a lot of sense other than for reviewing purposes. We should also set the DelayApprovers property when that's merged.
(In reply to comment #6) > > @@ +67,3 @@ > > + if (error != NULL) > > + { > > + DEBUG ("Could not claim CDO"); > > > > display the error message. > > As a popup window? Why? This is an error when trying to autoreject the call, so > if we don't autoreject it, the call will show up, and displaying an error > together with it sounds pretty confusing. No no, just in the DEBUG(). > > @@ +139,3 @@ > > + if (type == TPY_IFACE_QUARK_CHANNEL_TYPE_CALL && > > + tpy_call_channel_get_state (TPY_CALL_CHANNEL (chan), NULL, NULL) > > + == TPY_CALL_STATE_ENDED) > > > > SM channels are automatically closed when the call is terminated? > > If they aren't, I don't see an State property or something... So I assumed they > are. Please check to be sure. > > @@ +141,3 @@ > > + == TPY_CALL_STATE_ENDED) > > + continue; > > + > > > > This deserve at least a DEBUG message explaining what we are doing to this poor > > channel and why. > > A DEBUG or a comment? We're not doing anything to the channel (just seeing its > state, but not modifying it). I don't think a DEBUG is appropriate here. I meant a DEBUG when we are rejecting the channel. > > @@ +149,3 @@ > > + } > > + > > + g_signal_connect (channel, "invalidated", > > > > I think you should use tp_g_signal_connect_object() to avoid to be called after > > the object has been destroyed. > > That can't happen as I ref the channel (and unref it when it's invalidated). How so? If for any reason EmpathyObserver is destroyed it could survive some TpChannels. > We should also set the DelayApprovers property when that's merged. Yep, hopefully I should release it today.
All fixed. (In reply to comment #7) > > > @@ +139,3 @@ > > > + if (type == TPY_IFACE_QUARK_CHANNEL_TYPE_CALL && > > > + tpy_call_channel_get_state (TPY_CALL_CHANNEL (chan), NULL, NULL) > > > + == TPY_CALL_STATE_ENDED) > > > > > > SM channels are automatically closed when the call is terminated? > > > > If they aren't, I don't see an State property or something... So I assumed they > > are. > > Please check to be sure. I'm not sure how to check if the StreamedMedia channel is finished or not... Perhaps checking if the channel handle is TP_HANDLE_TYPE_NONE. Any idea?
(In reply to comment #8) > I'm not sure how to check if the StreamedMedia channel is finished or not... > Perhaps checking if the channel handle is TP_HANDLE_TYPE_NONE. Any idea? channel handles are immutables, so it never changes once the channel created. You could check with this scenario: - A calls B (streamed media) - A close the call - Is B's channel automatically closed? But actually as StreamedMedia implements Group, isn't checking if the self handle is in the members of the channel good enough? You should probably check with a StreamedMedia expert tbh (Sjoerd, Olivier, etc).
+ /* Ongoing calls, reffed */ Please use the name of the type stored in the list. +/* The Call Observer looks at incoming and ongoing calls, and s/ongoing/outgoing ? + DEBUG ("Could not claim CDO: %s", error->message); You leak the GError here.
All fixed. I'm now also setting delay-approvers, and I've bumped the tp-glib min version (but we need a release for that).
Your commit bumping the tp-glib dep will conflict will mine. You can drop it, I'd merge my branch as soon as the tp-glib release is out. Looks good, feel free to merge to 2.34 and master once tp-glib has been released.
Thanks, merged. This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.