Created attachment 218176 [details] [review]
dialpad-widget: store buttons in a hash table

Created attachment 218177 [details] [review]
add empathy_dialpad_widget_press_key()

Created attachment 218178 [details] [review]
call-window: allow user to enter dtmf events using his keyboard

Ideally we should stop sending the DTMF event when the key is released but we
can't easily do that in Gtk+; we'll have to write our own widget.
I'm going to give it a shot but this is already a nice improvement so we
shouldn't block on it.

Created attachment 218180 [details] [review]
add empathy-dialpad-button

Created attachment 218181 [details] [review]
dialpad-widget: use EmpathyDialpadButton

This will be needed if we want to stop using GtkButton in order to keep the
key pressed while the keyboard key is.

Anyway, it's good to have regardeless as that makes the code cleaner.

Review of attachment 218177 [details] [review]:

::: libempathy-gtk/empathy-dialpad-widget.c
@@ +232,3 @@
+  dtmf_dialpad_button_pressed_cb (button, NULL, self);
+  gtk_widget_activate (button);
+  dtmf_dialpad_button_released_cb (button, NULL, self);

gtk_button_clicked() maybe?

There is a difference between press-hold-release and click, in phones the first would play a "tuuuuuuut" tone, and the 2nd just a "tut" or even out of band event depending on the protocol, AFAIK.

Actually in tp-glib we have high-level API only for the 2nd case tp_call_channel_send_tones_async(); we don't have API for the StartTone/StopTone which are in the spec. See http://telepathy.freedesktop.org/spec/Channel_Interface_DTMF.html

So I would suggest replacing pressed_cb and released_cb by a single clicked_cb.

Review of attachment 218178 [details] [review]:

::: src/empathy-call-window.c
@@ +4108,3 @@
+        key = '#';
+        break;
+    }

That big swich is sad, maybe can be simplified with gdk_keyval_to_unicode(event->keyval); but then you need to verify the unicode is actually an ascii... g_unichar_digit_value() would already cover the 0-9 case.

If nobody knows, the big switch works as well... :)

For the 2 last commits, as said above, we don't currently have API for doing long-press DTMF tones anyway, so probably not worth doing a custom widget for now...

(In reply to comment #6)
> Review of attachment 218177 [details] [review]:
> 
> ::: libempathy-gtk/empathy-dialpad-widget.c
> @@ +232,3 @@
> +  dtmf_dialpad_button_pressed_cb (button, NULL, self);
> +  gtk_widget_activate (button);
> +  dtmf_dialpad_button_released_cb (button, NULL, self);
> 
> gtk_button_clicked() maybe?

Nope because we catch press and released, not clicked.

> There is a difference between press-hold-release and click, in phones the first
> would play a "tuuuuuuut" tone, and the 2nd just a "tut" or even out of band
> event depending on the protocol, AFAIK.
> 
> Actually in tp-glib we have high-level API only for the 2nd case
> tp_call_channel_send_tones_async(); we don't have API for the
> StartTone/StopTone which are in the spec. See
> http://telepathy.freedesktop.org/spec/Channel_Interface_DTMF.html
> 
> So I would suggest replacing pressed_cb and released_cb by a single clicked_cb.

You're right, but this widget has API to support the first case as well which
is why I kept it that way. But as we just ignore the 'stop-tone' signal atm, I
won't bother rewriting the widget for now, indeed.

Created attachment 218317 [details] [review]
call-window: use gdk_keyval_to_unicode()

(In reply to comment #7)
> Review of attachment 218178 [details] [review]:
> 
> ::: src/empathy-call-window.c
> @@ +4108,3 @@
> +        key = '#';
> +        break;
> +    }
> 
> That big swich is sad, maybe can be simplified with
> gdk_keyval_to_unicode(event->keyval); but then you need to verify the unicode
> is actually an ascii... g_unichar_digit_value() would already cover the 0-9
> case.
> 
> If nobody knows, the big switch works as well... :)

Oh, I didn't know about this function thanks!

Here here is a new patch using it, we still have the switch/case (to know if
the dialpad has to be displayed) but are least it's much simpler now.

ok, +1

Attachment 218176 [details] pushed as 7a2248d - dialpad-widget: store buttons in a hash table
Attachment 218177 [details] pushed as 761ef25 - add empathy_dialpad_widget_press_key()
Attachment 218178 [details] pushed as ada7b68 - call-window: allow user to enter dtmf events using his keyboard
Attachment 218180 [details] pushed as 086cd42 - add empathy-dialpad-button
Attachment 218181 [details] pushed as cc26255 - dialpad-widget: use EmpathyDialpadButton
Attachment 218317 [details] pushed as 20c864a - call-window: use gdk_keyval_to_unicode()