GNOME Bugzilla – Bug 757635
Make event handling controller-driven
Last modified: 2017-10-26 11:09:22 UTC
With an eye kept on gtk4, I'm attaching some patches that I think will ease the transition towards a purely controller-driven event handling. There, I've added a GtkEventControllerWidget that takes away the event signal emission duties from its GtkWidget, and makes it so _gtk_widget_run_controllers() ends up calling gtk_widget_event_internal(), and not the opposite. This object has been kept private so far, and created by default for every widget. At some point in the gtk4 transition it could be made public and/or optional, setting a target for removal for the widgets that want to adapt to the new world order. I've also been very tempted to making: gtk_event_controller_set_event_mask (controller, GDK_ALL_EVENTS_MASK); in this controller, bringing us as close to "no event masks" as we can currently be. There shouldn't be any side effects, although leaned on the cautious side in the end.
Created attachment 314914 [details] [review] widget: Expose gtk_widget_event_internal in gtkwidgetprivate.h And respect event const-ness on the event.
Created attachment 314915 [details] [review] gtk: Add GtkEventControllerWidget This event controller will take away the responsibility from GtkWidget of calling GtkWidgetClass event handlers and signals.
Created attachment 314916 [details] [review] widget: Invert the relationship between event handlers and controllers Event handling has been made controller-centric, the emission of widget signals has been delegated to a GtkEventControllerWidget that's created for every widget and attached to the bubbling phase. This marks a transition path towards purely controller-driven event handling and event mask removal. When the time is right, we'll be able to remove the GtkEventControllerWidget or making it optional without fundamental changes to the event propagation logic.
Review of attachment 314914 [details] [review]: Looks good.
Review of attachment 314915 [details] [review]: ::: gtk/gtkeventcontrollerwidget.h @@ +34,3 @@ +G_BEGIN_DECLS + +#define GTK_TYPE_EVENT_CONTROLLER_WIDGET (gtk_event_controller_widget_get_type ()) Should probably use G_DECLARE_FINAL_TYPE here.
Review of attachment 314916 [details] [review]: ::: gtk/gtkwidget.c @@ +4378,3 @@ + /* Set up the legacy event controller */ + controller = gtk_event_controller_widget_new (widget); + gtk_event_controller_set_propagation_phase (controller, GTK_PHASE_BUBBLE); You're leaking controller, here. The ownwership is a bit weird — is the controller owned by the widget, or is the widget owned by the controller?
(In reply to Carlos Garnacho from comment #2) > Created attachment 314915 [details] [review] [review] > gtk: Add GtkEventControllerWidget > Drive-by comment: "GtkEventControllerWidget" to me sounds like a name for a widget, not a name for an event controller. I guess that's what we get for using 2 different naming schemes (prefix vs suffix) for widgets and event controllers. But I'm wondering if there's a better name. GtkEventControllerFallback? GtkEventControllerCompat? Something like that?
(In reply to Emmanuele Bassi (:ebassi) from comment #6) > Review of attachment 314916 [details] [review] [review]: > > ::: gtk/gtkwidget.c > @@ +4378,3 @@ > + /* Set up the legacy event controller */ > + controller = gtk_event_controller_widget_new (widget); > + gtk_event_controller_set_propagation_phase (controller, GTK_PHASE_BUBBLE); > > You're leaking controller, here. > > The ownwership is a bit weird — is the controller owned by the widget, or is > the widget owned by the controller? Oops, right. I was thinking that _gtk_widget_add_controller() is called on gtk_event_controller_constructed(), but we only do take weak refs there. I actually need to keep the ref.
(In reply to Benjamin Otte (Company) from comment #7) > (In reply to Carlos Garnacho from comment #2) > > Created attachment 314915 [details] [review] [review] [review] > > gtk: Add GtkEventControllerWidget > > > Drive-by comment: "GtkEventControllerWidget" to me sounds like a name for a > widget, not a name for an event controller. I guess that's what we get for > using 2 different naming schemes (prefix vs suffix) for widgets and event > controllers. tbf, this isn't consistent across widgets either... (GtkComboBoxEntry and GtkFileChooser* come to mind). But I see your point. > > But I'm wondering if there's a better name. GtkEventControllerFallback? > GtkEventControllerCompat? Something like that? I guess "fallback" applies well, can't think of a better name either.
(In reply to Carlos Garnacho from comment #9) > tbf, this isn't consistent across widgets either... (GtkComboBoxEntry and ^ GtkComboBoxText
Created attachment 314924 [details] [review] gtk: Add GtkEventControllerFallback This event controller will take away the responsibility from GtkWidget of calling GtkWidgetClass event handlers and signals.
Created attachment 314925 [details] [review] widget: Invert the relationship between event handlers and controllers Event handling has been made controller-centric, the emission of widget signals has been delegated to a GtkEventControllerFallback that's created for every widget and attached to the bubbling phase. This marks a transition path towards purely controller-driven event handling and event mask removal. When the time is right, we'll be able to remove the GtkEventControllerFallback or making it optional without fundamental changes to the event propagation logic.
This is now done on master, even if with a different incarnation.