Outgoing DTMF / dialpad support
Elliott Sales de Andrade
qulogic at pidgin.im
Sun Oct 20 13:54:14 EDT 2013
Hi David,
On 14 October 2013 05:43, David Woodhouse <dwmw2 at infradead.org> wrote:
> There are two open requests about DTMF support.
>
> - https://developer.pidgin.im/ticket/12617
> (Patch to add out-of-band DTMF support)
>
> This adds a generic way for protocols to send DTMF out-of-band, and
> falls back to sending in-band in the media stream. And a second patch
> adds the required protocol-specific support for Jabber. The patches
> didn't even seem to build out-of-the-box, due to typos and mismatched
> function declarations, and the ticket has been open and mostly unloved
> for three years.
>
> - https://developer.pidgin.im/ticket/15575
> (dialpad support)
>
> This is more recent, and actually adds a UI for users to dial DTMF tones
> whilst in an audio call. The patch here operates directly on a gstreamer
> pipeline to send dtmf-events, and looking at the code I'm not entirely
> sure the pipeline it selects will have *anything* to do with the media
> stream it was actually *asked* to send DTMF on.
>
> So I've generated a patch which combines the two — fixing up the generic
> part of the media_dtmf patch in ticket 12617, and combining it with the
> UI support added in ticket 15575. I am inclined to suggest we should
> leave ticket 12617 open for the Jabber support which I haven't updated
> and can't easily test, and handle the UI along with the generic part
> (i.e. my patch) in the new ticket 15575.
>
> I have some outstanding issues with my patch which I would appreciate
> some more clueful input on...
>
> Firstly, to make the generic DTMF support build cleanly I had to go
> through all the protocol plugins and add an explicit extra NULL to each
> PurplePluginProtocolInfo structure. Do we *really* still have to do it
> this way and use structure declarations from last century, or should I
> submit a separate patch which updates the code to use C99 structure
> initialisation? It's been 14 years now...
>
> Secondly, and more importantly, see the FIXME and the leak of the
> session-id string in the gtkmedia.c patch. I didn't bother to fix that
> because I'm assuming the session-id is going to be stored *somewhere* we
> can access it anyway, but I wasn't sure. Or perhaps we can even continue
> to use the one we were given, and its lifetime will exceed that of the
> UI? Suggestions welcome...
>
> Third: why wasn't pidgin_media_ready_cb() passing the sid to
> pidgin_media_add_audio_widget()? If you look carefully at the final hunk
> below, you'll see I 'fix' that too...
>
> This is just the patch of the patch which affects gtkmedia.c:
>
> diff --git a/pidgin/gtkmedia.c b/pidgin/gtkmedia.c
> index d91a951..9d5f4d8 100644
> --- a/pidgin/gtkmedia.c
> +++ b/pidgin/gtkmedia.c
> @@ -759,6 +759,150 @@ pidgin_media_add_audio_widget(PidginMedia *gtkmedia,
> }
>
> static void
> +phone_dtmf_pressed_cb(GtkButton *button, gpointer user_data)
> +{
> + PidginMedia *gtkmedia = user_data;
> + gint num;
> + gchar *sid;
> +
> + num = GPOINTER_TO_INT(g_object_get_data(G_OBJECT(button),
> "dtmf-digit"));
> + sid = g_object_get_data(G_OBJECT(button), "session-id");
> +
> + purple_media_send_dtmf(gtkmedia->priv->media, sid, num, 25, 50);
>
What are these two numbers?
> +}
> +
> +static inline GtkWidget *
> +phone_create_button(const gchar *text_hi, const gchar *text_lo)
> +{
> + GtkWidget *button;
> + GtkWidget *label_hi;
> + GtkWidget *label_lo;
> + GtkWidget *grid;
> +
> + grid = gtk_vbox_new(TRUE, 0);
> +
> + button = gtk_button_new();
> + label_hi = gtk_label_new(text_hi);
> + gtk_misc_set_alignment(GTK_MISC(label_hi), 0.5, 0.5);
> + gtk_box_pack_end(GTK_BOX(grid), label_hi, FALSE, TRUE, 0);
> + label_lo = gtk_label_new(text_lo);
> + gtk_misc_set_alignment(GTK_MISC(label_lo), 0.5, 0.5);
> + gtk_label_set_use_markup(GTK_LABEL(label_lo), TRUE);
> + gtk_box_pack_end(GTK_BOX(grid), label_lo, FALSE, TRUE, 0);
> + gtk_container_add(GTK_CONTAINER(button), grid);
> +
> + return button;
> +}
> +
> +static GtkWidget *
> +pidgin_media_add_dtmf_widget(PidginMedia *gtkmedia,
> + PurpleMediaSessionType type, const gchar *_sid)
> +{
> + GtkWidget *grid = gtk_table_new(4, 3, TRUE);
> + GtkWidget *button1;
> + GtkWidget *button2;
> + GtkWidget *button3;
> + GtkWidget *button4;
> + GtkWidget *button5;
> + GtkWidget *button6;
> + GtkWidget *button7;
> + GtkWidget *button8;
> + GtkWidget *button9;
> + GtkWidget *button_asterisk;
> + GtkWidget *button0;
> + GtkWidget *button_pound;
> + gchar *sid = g_strdup(_sid); /* FIXME: This leaks. */
> +
>
You can use g_object_set_data_full on *one* button so it autofrees.
> + /* Button 1 */
> + button1 = phone_create_button("o_o", "<b>1</b>");
>
Shouldn't one or both of these be marked for translation?
> + g_signal_connect(button1, "pressed",
> G_CALLBACK(phone_dtmf_pressed_cb), gtkmedia);
> + g_object_set_data(G_OBJECT(button1), "dtmf-digit",
> GINT_TO_POINTER('1'));
> + g_object_set_data(G_OBJECT(button1), "session-id", sid);
> + gtk_table_attach(GTK_TABLE(grid), button1, 0, 1, 0, 1, GTK_FILL |
> GTK_EXPAND, GTK_FILL | GTK_EXPAND, 2, 2);
> +
> + /* Button 2 */
>
...
> + /* Button Pound */
> +
>
All these buttons seem to be created the same way. I think this would be
simpler if you looped through a static array of the elements.
> + gtk_widget_show_all(grid);
> +
> + return grid;
> +}
> +
> +static void
> pidgin_media_ready_cb(PurpleMedia *media, PidginMedia *gtkmedia, const
> gchar *sid)
> {
> GtkWidget *send_widget = NULL, *recv_widget = NULL, *button_widget
> = NULL;
> @@ -888,7 +1032,11 @@ pidgin_media_ready_cb(PurpleMedia *media,
> PidginMedia *gtkmedia, const gchar *si
>
> gtk_box_pack_end(GTK_BOX(recv_widget),
> pidgin_media_add_audio_widget(gtkmedia,
> - PURPLE_MEDIA_SEND_AUDIO, NULL), FALSE,
> FALSE, 0);
> + PURPLE_MEDIA_SEND_AUDIO, sid), FALSE,
> FALSE, 0);
> +
> + gtk_box_pack_end(GTK_BOX(recv_widget),
> + pidgin_media_add_dtmf_widget(gtkmedia,
> + PURPLE_MEDIA_SEND_AUDIO, sid), FALSE,
> FALSE, 0);
> }
>
> if (type & PURPLE_MEDIA_AUDIO &&
>
I haven't looked into running this patch, so at a higher level, can I ask
where the buttons go?
--
Elliott aka QuLogic
Pidgin developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://pidgin.im/pipermail/devel/attachments/20131020/45608488/attachment.html>
More information about the Devel
mailing list