Review of wip/carlosg/event-delivery

[Alexander Larsson <alexl redhat com>]

I took a quick look at the wip/carlosg/event-delivery branch.

Here are some things in the tests that seems broken from interactive
testing:

testgtk
 - click on close button gives:
    Gtk-CRITICAL **: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed
   This also happens in many other cases
  - button box: 
     Click on button, hold down, mouse to other button => it 
     prelights. Should not have gotten enter notify due to implicit 
     grab.
  - eventbox: 
     above child is not working
  - expander: 
     Press on expander, mouse outside window, release, now the 
     expander expands when you enter the window
  - panes: 
     Resizing the pane doesn't work
  - spinbutton: 
     clicking on the buttons in the spinbutton shows really weird
     prelight behaviour
  - spinbutton: 
     Can't hold down button on spinbutton button to step
  - test scroll: 
     Can't keep scrolling when cursor is outside toplevel (implicit 
     grab not working)

gtk4-demo:
  - After startup, if I click on last row ("Menus") it doesn't properly
    repaint the selected row
  - combo boxes: mouse press + move on combo does not work, must click 
    and then move
  - icon view basics: Scrolling the icon view verticaly doesn't 
    repaint correctly
  - list box: click on expand button, then you can use enter to 
    activate it, then mouse to expand button on row below => hover,
    now hit enter. the button scrolls away from the mouse, but
    it doesn't unhover
  - interactive overlay: buttons over entry get prelights and clicks 
    instead of the entry stealing them
  - popovers: The icons in the entry doesn't accept clicks, nor do the
    calendar
  - tree view/list store - size allocation is broken, and "Fixed?" 
    checkbutton is not working

Some code review:

I worry about the GtkWidget::pick vfunc and the consistency of the
widget geometry. The implementation of gtk_widget_real_pick relies on
the widget hieararhy being correctly allocated, which is only true
directly after a layout pass. For instance, the current code will
re-pick immediately whenever the focus target is destroyed, which will
typically happen in the middle of some random code that changes
the widget hierarchy. You can't rely on the widget allocation at
this point. It may be uninitialized for new widgets, or just wrong
if some other widget got removed which will cause the widget to shift.

In the current framework our events are based on GdkWindow positions,
and these are set at size allocation time and are valid until the next
size allocation, so it doesn't have this problem.

I think picking fundamentally has to be tied to the layout cycle, we
can't just do it whenever we feel like it. For more thoughts about
this, see my previous review on this:
https://mail.gnome.org/archives/gtk-devel-list/2017-April/msg00000.html

The default picking system always treats widgets as rectangular. This
is a regression at least for GtkPopover, which currently uses input
shapes to get correct event handling for the rounded corners. However,
it strikes me that if we make it a slightly bit smarter we could
handle CSS corners in general vs input. For instance, we could have
"mouse enter" on a button with rounded corners only when the pointer
is actually inside the css box (i.e. not when its on the rounded part
of the corner).

GtkPointerFocus is not refcounted, and is seemingly freed in various
callbacks. I can't swear this is an issue, but it seems likely that we
can run into cases where some code acesses it and then it gets freed
due to some state change in a function call and when we get back
we continue to access the freed object.

The notify_types for the crossing events seem wrong:
https://git.gnome.org/browse/gtk+/tree/gtk/gtkmain.c?h=wip/carlosg/event-delivery&id=c43ce71c21da075b53f1c00bd286e2ccfd1312aa#n1376
This sends the same notify type to all the widgets involved in the
crossign events, which is not right. The first and last widget
to get events should get different details. See this for the details:
https://tronche.com/gui/x/xlib/events/input-focus/normal-and-grabbed.html

This seems to cause issues like the one worked around by:
https://git.gnome.org/browse/gtk+/commit/?h=wip/carlosg/event-delivery&id=0b569fd6827e1728c8dbf67440535dc87694983b

I wonder if at some point we could move the map vfunc to the toplevel?
Should any child widget ever need to do anything on map?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
       alexl redhat com            alexander larsson gmail com 
He's a one-legged day-dreaming farmboy who hides his scarred face behind 
a mask. She's a vivacious cigar-chomping politician with the power to see 
death. They fight crime!