GNOME Bugzilla – Bug 787444
The color-selection dialog which is opened by a GtkColorButton should not be destroyed by the "delete-event" signal.
Last modified: 2017-09-12 20:18:14 UTC
Created attachment 359402 [details] [review] a patch to improve the behavior. When I add custom palette by using the function gtk_color_chooser_add_palette, if I close the dialog by escape key then click the color button again, a color-selection dialog with the default palette is shown. To avoid this behavior, the color selection dialog should not be destroyed by the "delete-event" signal.
But if it's not destroyed, then every call to ensure_dialog() will stack up redundant signal connections, as it's called from button_clicked() and add_palette(). So clearly this patch is not sufficient. (Also, you missed a space before the function arguments.)
(In reply to Daniel Boles from comment #1) > But if it's not destroyed, then every call to ensure_dialog() will stack up > redundant signal connections Wrong, of course :) it exits early if a dialo already exists
Created attachment 359430 [details] [review] a patch to improve the behavior (rev-1)
(In reply to Daniel Boles from comment #1) > (Also, you missed a space before the function arguments.) I have updated the patch. I think with this patch, the escape key will be work almost same as the cancel button.
Review of attachment 359430 [details] [review]: Indeed, it will then do the same on both Cancel and Esc, which makes perfect sense. The lack of this was presumably just an oversight. Thanks for noticing it! I don't know, but it may be considered better to handle this as e.g. FileChooserButton does: connect ::delete-event to a callback that then emits ::response with GTK_RESPONSE_DELETE_EVENT. That way, if any cleanup logic is ever needed in the 'cancel' case, it can be kept in a single place. So I'll wait to see if anyone would prefer it to be written that way. * https://git.gnome.org/browse/gtk+/tree/gtk/gtkfilechooserbutton.c?h=gtk-3-22#n2909
(In reply to Daniel Boles from comment #5) > connect ::delete-event to a callback that then emits > ::response with GTK_RESPONSE_DELETE_EVENT. That way, if any cleanup logic is > ever needed in the 'cancel' case, it can be kept in a single place. Of course, instead if adding RESPONSE_DELETE_EVENT to the condition in the ::response handler, you could instead make the ::delete-event handler emit with RESPONSE_CANCEL. So many ways to do it, and they'll all achieve the same thing in this simple case, but it'd be nice to use the preferred way, if there is any.
Created attachment 359520 [details] [review] a patch to improve the behavior (rev-2)
(In reply to Daniel Boles from comment #6) > > connect ::delete-event to a callback that then emits > > ::response with GTK_RESPONSE_DELETE_EVENT. That way, if any cleanup logic is > > ever needed in the 'cancel' case, it can be kept in a single place. > > Of course, instead if adding RESPONSE_DELETE_EVENT to the condition in the > ::response handler, you could instead make the ::delete-event handler emit > with RESPONSE_CANCEL. I agree with you. I have updated the patch that following your advice.
Review of attachment 359520 [details] [review]: Thanks! I have a final nitpick, though: ::: gtk/gtkcolorbutton.c @@ +500,3 @@ + gpointer user_data) +{ + g_signal_emit_by_name (dialog, "response", GTK_RESPONSE_CANCEL); dialog_response() expects to receive a ColorButton as its user_data. You don't pass one here. Sure, in this case, now, response() will never access that argument - so there's no practical problem. But I'd prefer to see the ColorButton* passed through g_signal_connect() and _emit_by_name(). This prevents it possibly needing added later - and avoids requiring that anyone altering response() must realise they need to expect/dodge an undefined pointer from ::delete-event.
Created attachment 359615 [details] [review] a patch to improve the behavior (rev-3)
(In reply to Daniel Boles from comment #9) > dialog_response() expects to receive a ColorButton as its user_data. You > don't pass one here. Sure, in this case, now, response() will never access How about the rev3 patch? This modification is not for dialog_response() but dialog_delete_event() because even if adopt rev2 patch, dialog_response() receive a ColorButton as its user_data. In addition, if someone want to do something related to delete-event, he/she can do that in dialog_response() because if a dialog receives a delete event, the “response” signal will be emitted with a response ID of GTK_RESPONSE_DELETE_EVENT. signal propagation: Esc -> (response: GTK_RESPONSE_DELETE_EVENT) -> dialog_response -> (delete-event) -> dialog_delete_event -> (response: GTK_RESPONSE_CANCEL) -> dialog_response So, I think the modification of rev3 patch is just an insurance.
(In reply to Hiroyuki Ito from comment #11) > even if adopt rev2 patch, > dialog_response() receive a ColorButton as its user_data. My bad - of course, you're right: the user data set by g_signal_connect() for any given handler is automatically passed to that handler when emitting, and it needn't/shouldn't be explicitly passed. So, rev2 was fine after all. :) Still, it does seem nicer to pass the ColorButton pointer here, even if it's not currently used; maybe it could be later. (Otherwise when passing NULL I would've omitted the user_data argument at the handler side.)
Review of attachment 359615 [details] [review]: I doubt there can be any argument against this one. Thanks!
tested and works great; thanks for the report and patch! one thing: please submit as a git-formatted patch in the future, so that your name/email/etc are already there, rather than requiring someone to add them in.