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.

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 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.

(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.

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 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.

Created attachment 192823 [details] [review]
Updated patch

Patch updated according to review.

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.

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).

Yes, I've been busy with other stuff, I'll try to find time this week for this and kinetic scrolling.

Created attachment 199800 [details] [review]
Patch #1: emit captured events from the widget holding the GTK+ grab, or the toplevel

Created attachment 199801 [details] [review]
patch #2: propagate captured-event for crossing events

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

(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)

we'll get this when the gesture branch gets merged

branch has been merged