GNOME Bugzilla – Bug 633051
Use correct signature for signals
Last modified: 2011-02-14 17:29:59 UTC
Signal handlers should pay attention whether they need to use g_signal_connect or g_signal_connect_swapped, and whether they need a gpointer parameter at the end (all handlers that are not class handlers need one). The particular case I found out is the draw signal in ui/fixedtip.c, which was connected with a NULL data and g_signal_connect_swapped, but an handler which assumed g_signal_connect. Patch will follow.
Created attachment 173133 [details] [review] Use correct signature and connection function for ::draw In ui/fixedtip.c, use g_signal_connect instead of g_signal_connect_swapped so the first parameter is the emitter and not a NULL pointer. At the same time, ensure that both the GTK2 and the GTK3 code paths have the correct signature for the handler (including the data pointer).
Review of attachment 173133 [details] [review]: Looks like a correct fix, but some comments about the details ::: src/ui/fixedtip.c @@ +60,2 @@ { + g_return_val_if_fail (GTK_IS_WIDGET (tooltips), FALSE); Generally g_return_if_fail() should only ever be used on public or semi-public API ... it doesn't make sense to add one just to check that we didn't misconnect an internal signal. @@ +61,3 @@ + g_return_val_if_fail (GTK_IS_WIDGET (tooltips), FALSE); + + gtk_paint_flat_box (gtk_widget_get_style (tooltips), For consistency with the expose_handler path, it would be better to make the gtk_widget_get_allocated_width/height calls use 'tip' @@ +74,3 @@ static gint +expose_handler (GtkWidget *tooltips, + gpointer user_data) This is the wrong signature for GtkWidget::expose-event
Created attachment 173180 [details] [review] Use correct signature and connection function for ::draw In ui/fixedtip.c, use g_signal_connect instead of g_signal_connect_swapped so the first parameter is the emitter and not a NULL pointer. At the same time, ensure that both the GTK2 and the GTK3 code paths have the correct signature for the handler (including the data pointer).
Review of attachment 173180 [details] [review]: Code looks fine, except for one bit of indentation. Some suggestions about the commit message: In ui/fixedtip.c, use g_signal_connect instead of g_signal_connect_swapped so the first parameter is the emitter and not a NULL pointer. Actually with this version, it doesn't matter, so it might be better to say "use g_signal_connect() instead of g_signal_connect_swapped() for clarity and since we aren't actually passing user data at all" [ I'd like to see g_signal_connect_swapped() go away - it's a misfeature that it even exists. But that's a different story :-) ] At the same time, ensure that both the GTK2 and the GTK3 code paths have the correct signature for the handler (including the data pointer). In our conventions you don't need to put the data pointer on your callback functions if you don't need it; either way is "correct" - passing extra ignored parameters is fine in all reasonable implementations of the C language. It doesn't hurt though, so I'd just leave off the "(including the data pointer)" here. ::: src/ui/fixedtip.c @@ +121,1 @@ G_CALLBACK (draw_handler), NULL); This looks like you probably need to fix the indentation
Comment on attachment 173180 [details] [review] Use correct signature and connection function for ::draw Committed with fixes
This was resolved a long ago, it seems.