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 641836 - Capture/bubble event handling
Capture/bubble event handling
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
2.91.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2011-02-08 14:10 UTC by Carlos Garcia Campos
Modified: 2014-05-23 21:28 UTC
See Also:
GNOME target: 3.14
GNOME version: ---


Attachments
Add GtkWidget::captured-event signal (8.53 KB, patch)
2011-02-08 14:10 UTC, Carlos Garcia Campos
none Details | Review
Add GtkWidget::captured-event signal (13.36 KB, patch)
2011-06-20 16:33 UTC, Carlos Garcia Campos
needs-work Details | Review
Updated patch (14.38 KB, patch)
2011-07-20 12:36 UTC, Carlos Garcia Campos
none Details | Review
Updated patch (14.59 KB, patch)
2011-07-28 16:42 UTC, Carlos Garcia Campos
none Details | Review
Patch #1: emit captured events from the widget holding the GTK+ grab, or the toplevel (4.86 KB, patch)
2011-10-24 09:03 UTC, Carlos Garnacho
none Details | Review
patch #2: propagate captured-event for crossing events (1.59 KB, patch)
2011-10-24 09:04 UTC, Carlos Garnacho
none Details | Review

Description Carlos Garcia Campos 2011-02-08 14:10:27 UTC
Created attachment 180383 [details] [review]
Add GtkWidget::captured-event signal

In gtk+ events are always propagated from child widget to the parent. There are cases where the opposite approach is desired, see bug #462208 for an example. We could implement it without breaking backwards compatibility with a similar way used by clutter. The idea is to add a new captured-event signal to GtkWidget that is emitted before the event signal. This new signal is propagated from parent to child. 
Attached patch is just a first attempt to implement it, if we agree on this approach I'll cleanup and improve the patch.
Comment 1 Dan Winship 2011-06-20 14:02:00 UTC
Comment on attachment 180383 [details] [review]
Add GtkWidget::captured-event signal

>+static gboolean
>+_gtk_propagate_captured_event (GtkWidget *widget,
>+                               GdkEvent  *event)

rather than duplicating all of this, it seems like it would be better to have a lower-level method that takes a "gboolean captured" and have gtk_propagate_event() and _gtk_propagate_captured_event() both call it.

>+  if ((event->type == GDK_KEY_PRESS) ||
>+      (event->type == GDK_KEY_RELEASE))
>+    {
>+      /* Only send key events within Window widgets to the Window
>+       * The Window widget will in turn pass the
>+       * key event on to the currently focused widget
>+       * for that window.
>+       */

This is just a workaround for the lack of event capture, and can be reimplemented now as a captured_event override in GtkWindow.
Comment 2 Carlos Garcia Campos 2011-06-20 16:33:57 UTC
Created attachment 190287 [details] [review]
Add GtkWidget::captured-event signal

Thanks for the review, I have updated the patch to use a common function. I've left the key events hack for now, I think we can fix it in a following patch once captured-event is supported.
Comment 3 Dan Winship 2011-07-19 16:11:39 UTC
Comment on attachment 190287 [details] [review]
Add GtkWidget::captured-event signal

>+_popagate_event_up (GtkWidget *widget,

_propagate_event_up and _propagate_event_down are both missing an "r"

It is also very strange that they both eat a ref on @widget. The code all seems to handle that correctly, but... it's really weird. It makes _propagate_event_up/_down() look like they're causing a double-free, and makes _propagate_event() look like it's leaking a ref.

>+      /* Scroll events are special cased here because it
>+       * feels wrong when scrolling a GtkViewport, say,
>+       * to have children of the viewport eat the scroll
>+       * event
>+       */

(This is another place where the current code is hacking around the lack of captured-event, which can now be rewritten.)

>   /**
>+   * GtkWidget::captured-event:
>+   * @widget: the object which received the signal.
>+   * @event: the #GdkEvent which triggered this signal
>+   *
>+   * TODO
>+   *
>+   * Since: FIXME
>+   */

obviously needs to be filled in, and the GtkWidget::event docs need to cross-reference it.
Comment 4 Carlos Garcia Campos 2011-07-20 12:12:22 UTC
(In reply to comment #3)
> (From update of attachment 190287 [details] [review])
> >+_popagate_event_up (GtkWidget *widget,
> 
> _propagate_event_up and _propagate_event_down are both missing an "r"

oops. 

> It is also very strange that they both eat a ref on @widget. The code all seems
> to handle that correctly, but... it's really weird. It makes
> _propagate_event_up/_down() look like they're causing a double-free, and makes
> _propagate_event() look like it's leaking a ref.

Currently gtk_propagate_event() does the ref/unref, so I did the same.
Comment 5 Carlos Garcia Campos 2011-07-20 12:36:49 UTC
Created attachment 192299 [details] [review]
Updated patch

Updated patch according to review, I haven't fixed the workarounds for key and scroll events for now to make sure the patch doesn't break current event handling. I think we can fix both in a following separate patch.
Comment 6 Dan Winship 2011-07-26 15:44:25 UTC
Comment on attachment 192299 [details] [review]
Updated patch

>+_propagate_event (GtkWidget *widget,
>+                  GdkEvent  *event,
>+                  gboolean   captured)
>+{
>+  gboolean handled_event;

need to initialize that to FALSE. There's a path through the GDK_KEY_PRESS || GDK_KEY_RELEASE section that never sets it either way, and then ends up returning stack garbage, messing up key events.

>+gtk_propagate_event (GtkWidget *widget,
>+                     GdkEvent  *event)
>+{
>+  gint handled_event;

that's unused now

(In reply to comment #4)
> > It is also very strange that they both eat a ref on @widget. The code all seems
> > to handle that correctly, but... it's really weird. It makes
> > _propagate_event_up/_down() look like they're causing a double-free, and makes
> > _propagate_event() look like it's leaking a ref.
> 
> Currently gtk_propagate_event() does the ref/unref, so I did the same.

It's not the ref/unref I was objecting to, it's the fact that you ref in one function and then unref in a different function, and in a particularly confusing fashion. (Eg, _propagate_event_down() refs every element of widgets *except* the first one, but then later unrefs all of them, which happens to be correct, because _propagate_event() had leaked a ref on that one.)

Oh, also, gtk doesn't prefix the names of static functions with "_" anywhere else.
Comment 7 Carlos Garcia Campos 2011-07-28 16:42:29 UTC
Created attachment 192823 [details] [review]
Updated patch

Patch updated according to review.
Comment 8 Dan Winship 2011-08-03 17:45:28 UTC
started trying to update gtk documentation, ran into some oddities

1) When there is a grab_widget, we propagate captured-events downward from its toplevel ancestor. That seems wrong (and likely to cause bugs if grab_widget's ancestors don't know that it takes grabs). Only the grab_widget itself should get a captured-event. (FWIW, that corresponds to Clutter's behavior. Although in clutter, the bubbling event emission also only goes to the grab actor, while in gtk, the event will bubble up to the grab_widget's ancestors if the grab_widget doesn't eat it. Which is weird and possibly a bug.)

2) Enter and leave events are not bubbled upward in gtk, and the patch makes them also not capturable downward. I suspect probably they don't bubble upward just to make people's event handlers simpler, because dealing with a child enter/leave is totally different from dealing with your own enter/leave. But on the captured side, the event handlers will mostly expect to be dealing with child events anyway, so I think it makes sense to make enters and leaves capturable even if they don't bubble.
Comment 9 Matthias Clasen 2011-10-23 06:20:20 UTC
Carlos, are you going to update the patch to incorporate these ideas ?

> Only the grab_widget itself should get a captured-event. 

I think what should actually happen is that we should emit ::captured-event starting at the grab_widget, down to the event_widget (in the case that event_widget is a descendent of grab_widget).
Comment 10 Carlos Garcia Campos 2011-10-23 07:23:55 UTC
Yes, I've been busy with other stuff, I'll try to find time this week for this and kinetic scrolling.
Comment 11 Carlos Garnacho 2011-10-24 09:03:46 UTC
Created attachment 199800 [details] [review]
Patch #1: emit captured events from the widget holding the GTK+ grab, or the toplevel
Comment 12 Carlos Garnacho 2011-10-24 09:04:44 UTC
Created attachment 199801 [details] [review]
patch #2: propagate captured-event for crossing events
Comment 13 Carlos Garnacho 2011-10-24 09:14:39 UTC
I'm not the Carlos requested, but been checking these patches over the weekend, just attached the patches to fix both suggestions from comment #8.

Overall, and seeing the kinetic scrolling usecase (which I gather is a common one), I wonder if ::captured-event couldn't be more useful if it returned a flag set and GTK+ maintained itself the event queue that could be possibly replayed down, something like:

typedef enum {
  GTK_CAPTURED_EVENT_FLAG_HANDLED = 1 << 0, /* event was handled */
  GTK_CAPTURED_EVENT_FLAG_QUEUE = 1 << 0 /* queue event for possible later processing */
} GtkCapturedEventFlags;

struct GtkWidgetClass {
  ...
  GtkCapturedEventFlags (* captured_event) (GtkWidget *widget, GdkEvent *event);
  ...
};

static void gtk_widget_release_captured_events (GtkWidget *widget, gboolean replay);

This still puts in implementors the reponsibility to handle event pairedness and such, but as widgets may in the end decide that the captured events were not meant for it, this could simplify what'd seem a common pattern in the handling of this event.

If this sounds like a good idea, I may go ahead and implement this
Comment 14 Carlos Garnacho 2011-11-26 19:45:22 UTC
(In reply to comment #13)
> Overall, and seeing the kinetic scrolling usecase (which I gather is a common
> one), I wonder if ::captured-event couldn't be more useful if it returned a
> flag set and GTK+ maintained itself the event queue that could be possibly
> replayed down

btw, this is implemented in the touchscreens branch

Another thing I think could be useful for capturing widgets is something like an upwards GTK+ grab, which prevents children beneath a widget to receive events, receiving ::grab-notify too if needed. it could make easier captured-event handling with arbitrary child widgets (eg, not having to care about crossing events while capturing happens, like treeview column headers which remain static)
Comment 15 Matthias Clasen 2014-05-12 00:35:49 UTC
we'll get this when the gesture branch gets merged
Comment 16 Matthias Clasen 2014-05-23 21:28:29 UTC
branch has been merged