GNOME Bugzilla – Bug 580794
creates redundant call windows
Last modified: 2011-10-12 14:32:38 UTC
If Elliot calls me and then hangs up and then calls me again, Empathy creates a second call window on the second call. It should just reuse the first one.
Created attachment 133599 [details] omg windows
Created attachment 137580 [details] [review] Branch containing the fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/reuse-call-window po/en_CA.po | 3 + src/empathy-call-window.c | 285 ++++++++++++++++++++++++++++++++++--------- src/empathy-call-window.h | 1 + src/empathy-call-window.ui | 21 +++ src/empathy-event-manager.c | 100 +++++++++++++--- src/empathy-event-manager.h | 6 + src/empathy.c | 5 +- 7 files changed, 345 insertions(+), 76 deletions(-)
Created attachment 137582 [details] [review] I had forgotten about en_GB resourses... po/en_CA.po | 3 + po/en_GB.po | 4 + src/empathy-call-window.c | 285 ++++++++++++++++++++++++++++++++++--------- src/empathy-call-window.h | 1 + src/empathy-call-window.ui | 21 +++ src/empathy-event-manager.c | 100 +++++++++++++--- src/empathy-event-manager.h | 6 + src/empathy.c | 5 +- 8 files changed, 349 insertions(+), 76 deletions(-)
Created attachment 138588 [details] [review] Removed the modifications to po files. I'll let the l10n team take care of this src/empathy-call-window.c | 285 +++++++++---------------------------------- src/empathy-call-window.h | 1 - src/empathy-call-window.ui | 21 --- src/empathy-event-manager.c | 100 +++------------- src/empathy-event-manager.h | 6 - src/empathy.c | 5 +- 6 files changed, 76 insertions(+), 342 deletions(-)
Comment on attachment 138588 [details] [review] Removed the modifications to po files. I'll let the l10n team take care of this >+ g_object_get (priv->handler, "contact", &contact, NULL); You seem to be leaking this ref. There are two instances of this I think. Should you also be referring contacts before they go into the hash table? I'm not sure.
Created attachment 138605 [details] [review] Fixed the leaks. As for reffing the contact before adding it to the hash table, my tests tell me that I don't have to. I would appreciate a definitive answer on that matter though. src/empathy-call-window.c | 289 ++++++++++++++++++++++++++++++++++--------- src/empathy-call-window.h | 1 + src/empathy-call-window.ui | 21 +++ src/empathy-event-manager.c | 100 +++++++++++++--- src/empathy-event-manager.h | 6 + src/empathy.c | 5 +- 6 files changed, 346 insertions(+), 76 deletions(-)
Marking old patches as obsolete.
Your branch conflict with the new master. Could you rebase it please? In your comment describing call_windows, please make it in the form: (key_type *) => (value_type *) and say if the ref/memory are owned or borrowed. +empathy_call_window_event_added_cb (EmpathyEventManager *manager, + EmpathyEvent *event, EmpathyCallWindow *self) style is wrong; one arg per line + if (g_strcmp0 (event->header, header_if_current_contact) == 0) uses tp_strdiff empathy_call_window_event_added_cb seems bong to me. You should have a better way to check if the event is a VoIP one. If needed add information to EmpathyEvent. empathy_call_window_event_removed_cb: Why are you doing that? Please comment. empathy_call_window_dispose: you can group these 2 calls: g_object_get (priv->handler, "tp-call", &call, NULL); g_object_get (priv->handler, "contact", &contact, NULL); You don't release your reference on priv->event_manager. You should do it before disconnecting its signals. empathy_call_window_present_window: the call_windows hash table should be created in _class_init. call_windows = g_hash_table_new (NULL, NULL); uses g_direct_hash and g_direct_equal if ((window = g_hash_table_lookup (call_windows, contact)) == NULL) split this line It's not clear to me who owns the ref of the EmpathyCallWindow. empathy_event_manager_get_existing_event_from_public: I'd rename it find_event_priv_from_event No need of the event_found boolean, just return in the for loop empathy_events_are_equals use tp_strdiff There is something I don't understand. The "event-added" and "event-removed" are fired with an EventPriv as argument but you get an EmpathyEvent in the callback. Did I misunderstand something?
(In reply to comment #8) > There is something I don't understand. The "event-added" and "event-removed" > are fired with an EventPriv as argument but you get an EmpathyEvent in the > callback. Did I misunderstand something? This should be right, as struct EventPriv has an EmpathyEvent as first member, so you can safely cast between them.
Humm, I see... That's a bit scary :)
(In reply to comment #8) > empathy_call_window_event_added_cb seems bong to me. > You should have a better way to check if the event is a VoIP one. If needed add > information to EmpathyEvent. Fixing #589409 should allow us to do that.
Created attachment 140130 [details] [review] Fixed patch > empathy_call_window_event_removed_cb: Why are you doing that? Please comment. Without this, if the remote side redials and hangup before the local side answered, the call window keeps ringing indefinitely. > empathy_call_window_present_window: > the call_windows hash table should be created in _class_init. Actually, _present_window is the first function to be called (before _class_init). Since it uses the hash table, if it has not already been initialized, it needs to create it. > It's not clear to me who owns the ref of the EmpathyCallWindow. The hash table? src/empathy-call-window.c | 298 +++++++++++++++++++++++++++++++++++-------- src/empathy-call-window.h | 1 + src/empathy-call-window.ui | 21 +++ src/empathy-event-manager.c | 129 ++++++++++++++----- src/empathy-event-manager.h | 26 +++- src/empathy.c | 5 +- 6 files changed, 384 insertions(+), 96 deletions(-)
I fixed bug #589409 using an implementation similar to yours but slightly different (I fixed the name of the enum and the coding style). As your branch wasn't properly recorded I had to make a new one. That means this branch should be rebased to master to fix conflicts. Sorry for that.
Let's do this with the new call UI.
http://cgit.collabora.com/git/user/pochu/empathy.git/log/?h=call-reuse-windows-580794 I've taken Jonathan's patch and ported it to master, and only then I've found some issues: - We can't use EmpathyEventManager anymore as empathy-call is now a separate process (at least not without heavily modifying EmpathyEventManager, which is not worth it as we can avoid using it). - Having the hash table of EmpathyContact -> EmpathyCallWindow in EmpathyCallWindow doesn't make sense. What my patch does is: empathy-call.c keeps track of the existing CallWindows. It also is a Call approver, and when a new Call channel appears, if one of the existing CallWindows is with the same contact, it lets the CallWindow start ringing and accept or reject the call. If the CallWindow accepts the call (i.e. the user clicks on "Answer"), then a handler will be created normally, empathy-call.c will know about it and it will look (again) for an existing CallWindow with the same contact. If it finds it, it tells the window to handle the call, otherwise it creates a new window.
Created attachment 191527 [details] [review] Reuse Call windows when possible If we have a call window opened for a contact and we get an incoming call from the same person, use the existing window instead of creating another one. Based on a patch from Jonathan Tellier.
Bugzilla refuses to attach the patch for some reason so I'll have to do it old school style. :( + RINGING, Is that incoming ringing (you have to pick up) or outgoing rining (peer has to pick up)? I think we should make this clearer. + TpyCallChannel *pending_channel; I'd add a comment explaining when those are used. + empathy_call_window_status_message (self, _("Ringing")); Same comment as above; what's the exact semantic of this Ringing? + empathy_call_window_status_message (self, _("Call refused")); Refused by who? empathy_call_window_start_ringing: we should either assert than self->priv->pending_* == NULL or use tp_clear_object() before to be sure to not introduce leaks. empathy_call_window_present same here for priv->handler. + tp_add_dispatch_operation_context_delay (context); Why are you delaying the context? You are supposed to accept as soon as you displayed the UI letting user to accept/decline the channel. /* Ignore. */ + tp_add_dispatch_operation_context_accept (context); This is wrong. If we are not going to allow user to accept/reject the call, we should _fail the context. If not and if we are the only approver, MC will block handling the channel forever, waiting for us to accept/approve.
Review of attachment 191527 [details] [review]: Oh it did attach it after all...
Thanks for the review (In reply to comment #17) > + RINGING, > Is that incoming ringing (you have to pick up) or outgoing rining (peer has to > pick up)? I think we should make this clearer. That's incoming ringing, outgoing is CONNECTING. I've added a commit that explains all the different states. > + TpyCallChannel *pending_channel; > I'd add a comment explaining when those are used. Done > + empathy_call_window_status_message (self, _("Ringing")); > Same comment as above; what's the exact semantic of this Ringing? Changed to "Incoming call" which seems more obvious. > + empathy_call_window_status_message (self, _("Call refused")); > Refused by who? Changed to disconnected. We should leave "Call refused" for when the remote contact refuses the call. > empathy_call_window_start_ringing: > we should either assert than self->priv->pending_* == NULL or use > tp_clear_object() before to be sure to not introduce leaks. > > empathy_call_window_present > same here for priv->handler. Both fixed. > + tp_add_dispatch_operation_context_delay (context); > Why are you delaying the context? You are supposed to accept as soon as you > displayed the UI letting user to accept/decline the channel. Ah OK, this wasn't very clear to me. I thought I was supposed to call _delay until I actually accepted/rejected the incoming channels (which is when the user does that). Fixed. > /* Ignore. */ > + tp_add_dispatch_operation_context_accept (context); > This is wrong. If we are not going to allow user to accept/reject the call, we > should _fail the context. If not and if we are the only approver, MC will block > handling > the channel forever, waiting for us to accept/approve. I see. Fixed. See the new comments, I'll amend before merging. Regarding the new "Answer" button and our discussion on IRC, Nick is going to look at a good UI for that in bug #629902. I'll change the UI at some point to reflect that UI, but leaving an Accept button for now.
+ GError error = { TP_ERRORS, TP_ERROR_INVALID_ARGUMENT, "Unknown channel" }; + You added an extra \n I think the approving code should be in empathy-call-factory rather than empathy-call. That would allow us to use the same TpBaseClient for the Approver and the Handler which makes more sense as, really, those are the same client. It would also have the nice side effect of not introducing yet another Empathy.* in the debug window. self->priv->pending_cdo = g_object_ref (dispatch_operation); g_signal_connect (self->priv->pending_channel, "invalidated", G_CALLBACK (empathy_call_window_incoming_channel_invalidated_cb), self); empathy_call_window_set_state_ringing (self); tp_add_dispatch_operation_context_accept (context); Add some \n. empathy_call_window_set_state_ringing I'd add an assertion checking we are in an ok state (ie, not in a call).
Created attachment 195886 [details] [review] CallFactory: make it a TpBaseClient subclass So that we can make it both a handler and an observer at the same time. Conflicts: src/empathy-call-factory.c
Created attachment 195887 [details] [review] Make EmpathyCallFactory an approver
Created attachment 195888 [details] [review] Reuse Call windows when possible If we have a call window opened for a contact and we get an incoming call from the same person, use the existing window instead of creating another one. Based on a patch from Jonathan Tellier.
Created attachment 195889 [details] [review] Let the existing call window know about new incoming calls
Created attachment 195890 [details] [review] CallWindow: display a dialog to accept or reject incoming calls
Created attachment 195891 [details] [review] Resize the hangup button in ::realize So that it's not resized many times.
Created attachment 195892 [details] [review] CallWindow: explain call states
New branch on top of master with comments addressed and following the design from bug #629902
Review of attachment 195886 [details] [review]: ++ I guess you should rewrite the "Conflicts" part from the commit msg.
Review of attachment 195887 [details] [review]: ::: src/empathy-call-factory.c @@ +350,3 @@ + { + DEBUG ("Call with a contact for which there's no existing " + TpChannelDispatchOperation *dispatch_operation, Ideally you should use that as error message (not the end of the world but would make logs clearer to read).
Review of attachment 195888 [details] [review]: ++
Review of attachment 195887 [details] [review]: ::: src/empathy-call-factory.c @@ +342,3 @@ + if (handle != 0) + { +approve_channels (TpBaseClient *client, This comment should be after the if {} block.
Review of attachment 195889 [details] [review]: looks good
Review of attachment 195890 [details] [review]: ++
Review of attachment 195891 [details] [review]: ++
Review of attachment 195892 [details] [review]: ++
Created attachment 195974 [details] [review] Make EmpathyCallFactory an approver
I just checked and this branch doesn't affect translated strings so I'm going to merge it now.
Attachment 195886 [details] pushed as 391bad3 - CallFactory: make it a TpBaseClient subclass Attachment 195887 [details] pushed as 44731eb - Make EmpathyCallFactory an approver Attachment 195888 [details] pushed as d5136f9 - Reuse Call windows when possible Attachment 195889 [details] pushed as ee02c5c - Let the existing call window know about new incoming calls Attachment 195890 [details] pushed as 3f30f0a - CallWindow: display a dialog to accept or reject incoming calls Attachment 195891 [details] pushed as 309ca04 - Resize the hangup button in ::realize Attachment 195892 [details] pushed as 42ea425 - CallWindow: explain call states Attachment 195974 [details] pushed as 44731eb - Make EmpathyCallFactory an approver
*** Bug 661186 has been marked as a duplicate of this bug. ***