After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 757635 - Make event handling controller-driven
Make event handling controller-driven
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: .General
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-11-05 13:29 UTC by Carlos Garnacho
Modified: 2017-10-26 11:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
widget: Expose gtk_widget_event_internal in gtkwidgetprivate.h (2.73 KB, patch)
2015-11-05 13:31 UTC, Carlos Garnacho
reviewed Details | Review
gtk: Add GtkEventControllerWidget (5.96 KB, patch)
2015-11-05 13:31 UTC, Carlos Garnacho
reviewed Details | Review
widget: Invert the relationship between event handlers and controllers (4.56 KB, patch)
2015-11-05 13:31 UTC, Carlos Garnacho
none Details | Review
gtk: Add GtkEventControllerFallback (6.00 KB, patch)
2015-11-05 15:05 UTC, Carlos Garnacho
none Details | Review
widget: Invert the relationship between event handlers and controllers (5.20 KB, patch)
2015-11-05 15:05 UTC, Carlos Garnacho
none Details | Review

Description Carlos Garnacho 2015-11-05 13:29:55 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.
Comment 1 Carlos Garnacho 2015-11-05 13:31:38 UTC
Created attachment 314914 [details] [review]
widget: Expose gtk_widget_event_internal in gtkwidgetprivate.h

And respect event const-ness on the event.
Comment 2 Carlos Garnacho 2015-11-05 13:31:43 UTC
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.
Comment 3 Carlos Garnacho 2015-11-05 13:31:57 UTC
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.
Comment 4 Emmanuele Bassi (:ebassi) 2015-11-05 13:41:12 UTC
Review of attachment 314914 [details] [review]:

Looks good.
Comment 5 Emmanuele Bassi (:ebassi) 2015-11-05 13:41:57 UTC
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.
Comment 6 Emmanuele Bassi (:ebassi) 2015-11-05 13:42:57 UTC
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?
Comment 7 Benjamin Otte (Company) 2015-11-05 13:44:47 UTC
(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?
Comment 8 Carlos Garnacho 2015-11-05 13:54:30 UTC
(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.
Comment 9 Carlos Garnacho 2015-11-05 13:57:24 UTC
(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.
Comment 10 Carlos Garnacho 2015-11-05 13:58:25 UTC
(In reply to Carlos Garnacho from comment #9)
> tbf, this isn't consistent across widgets either... (GtkComboBoxEntry and

                                                       ^ GtkComboBoxText
Comment 11 Carlos Garnacho 2015-11-05 15:05:43 UTC
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.
Comment 12 Carlos Garnacho 2015-11-05 15:05:48 UTC
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.
Comment 13 Carlos Garnacho 2017-10-26 11:08:16 UTC
This is now done on master, even if with a different incarnation.