Created attachment 167048 [details] [review]
http://git.Collabora.co.uk/?p=user/cassidy/empathy;a=shortlog;h=refs/heads/av-candidate-624344

 libempathy/empathy-call-handler.c |  161 +++++++++++++++++++++++++
 libempathy/empathy-call-handler.h |   12 ++
 src/empathy-call-window.c         |  129 ++++++++++++++++++++
 src/empathy-call-window.ui        |  241 +++++++++++++++++++++++++++++++++++++
 4 files changed, 543 insertions(+), 0 deletions(-)

Review of attachment 167048 [details] [review]:

::: libempathy/empathy-call-handler.c
@@ +480,3 @@
+      if (remote_candidate != NULL)
+        {
+          priv->audio_remote_candidate = fs_candidate_copy (remote_candidate);

Free the candidate before overwriting it if it already exists maybe ?

::: src/empathy-call-window.c
@@ +1465,3 @@
+    {
+      case FS_CANDIDATE_TYPE_HOST:
+        return "host";

Shouldn't these strings be translatable?

Also, I would add some explanations as tooltip or something, proposed text:

host: The IP address as seen by the machine
srflx: The IP address as seen by a server on the Internet
prflx: The IP address of the peer as seen by the other side
relay: The IP address of a relay server
multicast: The IP address of the multicast group

@@ +1526,3 @@
+  g_assert (candidate != NULL);
+  str = g_strdup_printf ("%s:%u (%s)", candidate->ip,
+      candidate->port, candidate_type_to_str (candidate));

If one of the candidates is of type relayed, you can probably put an extra message to make it clear to the user.

The IP could be IPv6, so using ":" as a ip/port separator may not be  a good idea.

@@ +1617,3 @@
+      G_CALLBACK (video_remote_candidate_notify_cb), self, 0);
+  tp_g_signal_connect_object (priv->handler, "notify::video-local-candidate",
+      G_CALLBACK (video_local_candidate_notify_cb), self, 0);

Maybe you want to hook up to only local or remote and update both at the same time or something...

Review of attachment 167048 [details] [review]:

::: libempathy/empathy-call-handler.c
@@ +480,3 @@
+      if (remote_candidate != NULL)
+        {
+          priv->audio_remote_candidate = fs_candidate_copy (remote_candidate);

done.

::: src/empathy-call-window.c
@@ +1465,3 @@
+    {
+      case FS_CANDIDATE_TYPE_HOST:
+        return "host";

I'm not convinced It's worth translating as it's mainly for debugging purpose. If a user tell us on IRC the Chinese version of this string that won't help us much. :)

But the tooltip is a really good idea; I've added it (and this one is translated).

@@ +1526,3 @@
+  g_assert (candidate != NULL);
+  str = g_strdup_printf ("%s:%u (%s)", candidate->ip,
+      candidate->port, candidate_type_to_str (candidate));

What kind of message would you display and how?

I now use a space to separate the IP from the port.

@@ +1617,3 @@
+      G_CALLBACK (video_remote_candidate_notify_cb), self, 0);
+  tp_g_signal_connect_object (priv->handler, "notify::video-local-candidate",
+      G_CALLBACK (video_local_candidate_notify_cb), self, 0);

I group them by media type as they are changed at the same time.

> What kind of message would you display and how?

I'm not sure exactly what messages to write.. Maybe its not a great idea (since SIP calls could be relayed without us knowing anyway).

Created attachment 167173 [details]
screenshot

The tooltip is now displayed on an "info" image so it easily discoverable.

Great, ship it !

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.