After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 580794 - creates redundant call windows
creates redundant call windows
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: VoIP
unspecified
Other Linux
: Normal normal
: 3.4
Assigned To: empathy-maint
: 661186 (view as bug list)
Depends on:
Blocks: 629902
 
 
Reported: 2009-04-29 18:57 UTC by Dafydd Harries
Modified: 2011-10-12 14:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
omg windows (55.15 KB, image/png)
2009-04-29 18:58 UTC, Dafydd Harries
  Details
Branch containing the fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/reuse-call-window (24.56 KB, patch)
2009-06-29 20:01 UTC, Jonathan Tellier
none Details | Review
I had forgotten about en_GB resourses... (24.90 KB, patch)
2009-06-29 20:23 UTC, Jonathan Tellier
none Details | Review
Removed the modifications to po files. I'll let the l10n team take care of this (24.28 KB, patch)
2009-07-17 13:48 UTC, Jonathan Tellier
none 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. (24.35 KB, patch)
2009-07-17 15:40 UTC, Jonathan Tellier
reviewed Details | Review
Fixed patch (29.45 KB, patch)
2009-08-07 16:26 UTC, Jonathan Tellier
none Details | Review
Reuse Call windows when possible (19.38 KB, patch)
2011-07-08 16:07 UTC, Guillaume Desmottes
rejected Details | Review
CallFactory: make it a TpBaseClient subclass (7.72 KB, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
Make EmpathyCallFactory an approver (4.34 KB, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
Reuse Call windows when possible (7.77 KB, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
Let the existing call window know about new incoming calls (4.96 KB, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
CallWindow: display a dialog to accept or reject incoming calls (4.48 KB, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
Resize the hangup button in ::realize (1.48 KB, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
CallWindow: explain call states (965 bytes, patch)
2011-09-07 15:20 UTC, Emilio Pozuelo Monfort
committed Details | Review
Make EmpathyCallFactory an approver (4.44 KB, patch)
2011-09-08 11:29 UTC, Emilio Pozuelo Monfort
committed Details | Review

Description Dafydd Harries 2009-04-29 18:57:17 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.
Comment 1 Dafydd Harries 2009-04-29 18:58:14 UTC
Created attachment 133599 [details]
omg windows
Comment 2 Jonathan Tellier 2009-06-29 20:01:54 UTC
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(-)
Comment 3 Jonathan Tellier 2009-06-29 20:23:11 UTC
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(-)
Comment 4 Jonathan Tellier 2009-07-17 13:48:55 UTC
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 5 Danielle Madeley 2009-07-17 14:21:37 UTC
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.
Comment 6 Jonathan Tellier 2009-07-17 15:40:33 UTC
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(-)
Comment 7 Cosimo Cecchi 2009-08-06 17:26:45 UTC
Marking old patches as obsolete.
Comment 8 Guillaume Desmottes 2009-08-07 12:19:14 UTC
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?
Comment 9 Cosimo Cecchi 2009-08-07 12:28:28 UTC
(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.
Comment 10 Guillaume Desmottes 2009-08-07 12:55:51 UTC
Humm, I see... That's a bit scary :)
Comment 11 Guillaume Desmottes 2009-08-07 16:04:53 UTC
(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.
Comment 12 Jonathan Tellier 2009-08-07 16:26:15 UTC
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(-)
Comment 13 Guillaume Desmottes 2009-10-02 14:38:23 UTC
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.
Comment 14 Guillaume Desmottes 2010-09-20 14:14:36 UTC
Let's do this with the new call UI.
Comment 15 Emilio Pozuelo Monfort 2011-07-08 13:18:24 UTC
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.
Comment 16 Guillaume Desmottes 2011-07-08 16:07:40 UTC
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.
Comment 17 Guillaume Desmottes 2011-07-08 16:26:17 UTC
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.
Comment 18 Guillaume Desmottes 2011-07-08 16:26:43 UTC
Review of attachment 191527 [details] [review]:

Oh it did attach it after all...
Comment 19 Emilio Pozuelo Monfort 2011-07-11 11:53:18 UTC
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.
Comment 20 Guillaume Desmottes 2011-07-12 10:01:55 UTC
+  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).
Comment 21 Emilio Pozuelo Monfort 2011-09-07 15:20:17 UTC
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
Comment 22 Emilio Pozuelo Monfort 2011-09-07 15:20:24 UTC
Created attachment 195887 [details] [review]
Make EmpathyCallFactory an approver
Comment 23 Emilio Pozuelo Monfort 2011-09-07 15:20:28 UTC
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.
Comment 24 Emilio Pozuelo Monfort 2011-09-07 15:20:32 UTC
Created attachment 195889 [details] [review]
Let the existing call window know about new incoming calls
Comment 25 Emilio Pozuelo Monfort 2011-09-07 15:20:35 UTC
Created attachment 195890 [details] [review]
CallWindow: display a dialog to accept or reject incoming calls
Comment 26 Emilio Pozuelo Monfort 2011-09-07 15:20:39 UTC
Created attachment 195891 [details] [review]
Resize the hangup button in ::realize

So that it's not resized many times.
Comment 27 Emilio Pozuelo Monfort 2011-09-07 15:20:43 UTC
Created attachment 195892 [details] [review]
CallWindow: explain call states
Comment 28 Emilio Pozuelo Monfort 2011-09-07 15:22:03 UTC
New branch on top of master with comments addressed and following the design from bug #629902
Comment 29 Guillaume Desmottes 2011-09-07 15:43:14 UTC
Review of attachment 195886 [details] [review]:

++

I guess you should rewrite the "Conflicts" part from the commit msg.
Comment 30 Guillaume Desmottes 2011-09-07 15:47:01 UTC
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).
Comment 31 Guillaume Desmottes 2011-09-07 15:49:39 UTC
Review of attachment 195888 [details] [review]:

++
Comment 32 Guillaume Desmottes 2011-09-07 15:51:52 UTC
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.
Comment 33 Guillaume Desmottes 2011-09-07 15:52:39 UTC
Review of attachment 195889 [details] [review]:

looks good
Comment 34 Guillaume Desmottes 2011-09-07 15:57:15 UTC
Review of attachment 195890 [details] [review]:

++
Comment 35 Guillaume Desmottes 2011-09-07 15:57:54 UTC
Review of attachment 195891 [details] [review]:

++
Comment 36 Guillaume Desmottes 2011-09-07 15:58:11 UTC
Review of attachment 195892 [details] [review]:

++
Comment 37 Emilio Pozuelo Monfort 2011-09-08 11:29:37 UTC
Created attachment 195974 [details] [review]
Make EmpathyCallFactory an approver
Comment 38 Guillaume Desmottes 2011-09-15 09:53:23 UTC
I just checked and this branch doesn't affect translated strings so I'm going to merge it now.
Comment 39 Guillaume Desmottes 2011-09-15 09:54:21 UTC
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
Comment 40 Guillaume Desmottes 2011-10-12 14:32:38 UTC
*** Bug 661186 has been marked as a duplicate of this bug. ***