Review of wip/carlosg/event-delivery

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Review of wip/carlosg/event-delivery

Alexander Larsson
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
       [hidden email]            [hidden email]
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!
_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of wip/carlosg/event-delivery

Carlos Garnacho
Hi Alex,

Thanks for the review!

On Tue, May 16, 2017 at 12:54 PM, Alexander Larsson <[hidden email]> wrote:

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

Some of those (the warning, panes not working yet) were already on my
list. The implicit grab issue is a nice gotcha...

>
> gtk4-demo:
>   - After startup, if I click on last row ("Menus") it doesn't properly
>     repaint the selected row

Besides getting some i/o chld windows out of the way, I'm not sure how
could the stuff in the branch have affected rendering. When I started
the branch, I was also seeing artifacts on master, and removing i/o
windows kinda helped. Now after a recent rebase they got worse in my
branch, but seem better in master...

>   - combo boxes: mouse press + move on combo does not work, must click
>     and then move

Hmm, it seems now combobox pops up on button release, in master too.
Seems to have changed compared to gtk3-demo indeed.

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

Hmm, the demo however sets the overlay as passthrough?
https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/overlay.c#n60

>   - popovers: The icons in the entry doesn't accept clicks, nor do the
>     calendar

Yeah, both GtkEntry icons and calendars are b0rk so far. These bits
rely too badly on properly laid out GdkWindows and event->any.window
checks, so need a bit of a refactor besides input handling.

>   - tree view/list store - size allocation is broken, and "Fixed?"
>     checkbutton is not working

Right, events on cell renderers in general are broken, didn't look too
deep into this yet.

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

Indeed, ::pick should check some invariants beforehand. And right, my
next most fundamental change in the todo is tackling event compression
differently, so GTK is given all received events, and performs
frame-sync motion compression by default.

In this picture, repicking should also be a delayed operation, so all
generated crossing events are propagated in the same clock stage.

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

I think we essentially agree :).

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

You're right. gtk_widget_real_pick() is still a bit simplistic wrt the
input shape, should be easy to fix. Making it dependent on the CSS box
sounds cool, however popover is still a separate usecase since its
pointy area is not quite CSS-y.

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

Sure, will add some refcounting so the struct is good while operating
on it. As the theory goes, each pointer enter or touch down will
generate a brand new GtkPointerFocus as seen by
gtk_window_lookup_pointer_focus().

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

Oops, you are indeed correct, quite an oversight.

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

Not a strong opinion. Probably map is useless now for (most, yet)
child widgets, but unmap not so much (eg. to stop timeouts that apply
while mapped), so might be nice to preserve for parity.

Cheers,
  Carlos
_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of wip/carlosg/event-delivery

Matthias Clasen-2
On Tue, May 16, 2017 at 1:25 PM, Carlos Garnacho <[hidden email]> wrote:

>   - interactive overlay: buttons over entry get prelights and clicks
>     instead of the entry stealing them

Hmm, the demo however sets the overlay as passthrough?
https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/overlay.c#n60

There's two overlay. One is passthrough, the other isn't: the "Numbers" are supposed to be passthrough, while the entry is supposed to get input.

_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of wip/carlosg/event-delivery

Carlos Garnacho
Hey,

On Tue, May 16, 2017 at 8:24 PM, Matthias Clasen
<[hidden email]> wrote:

> On Tue, May 16, 2017 at 1:25 PM, Carlos Garnacho <[hidden email]> wrote:
>>
>>
>> >   - interactive overlay: buttons over entry get prelights and clicks
>> >     instead of the entry stealing them
>>
>> Hmm, the demo however sets the overlay as passthrough?
>> https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/overlay.c#n60
>
>
> There's two overlay. One is passthrough, the other isn't: the "Numbers" are
> supposed to be passthrough, while the entry is supposed to get input.

Not what I see in code :). AFAICS both the entry and the label are
children of a vbox, which is the only, pass-through overlay. The
inspector seems to confirm this hierarchy.

And this means gtk3 is broken :/, as it's essentially the same thing there.

Cheers,
  Carlos
_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of wip/carlosg/event-delivery

Alexander Larsson
On Wed, 2017-05-17 at 01:04 +0200, Carlos Garnacho wrote:

> Hey,
>
> On Tue, May 16, 2017 at 8:24 PM, Matthias Clasen
> <[hidden email]> wrote:
> > On Tue, May 16, 2017 at 1:25 PM, Carlos Garnacho <[hidden email]
> > > wrote:
> > >
> > >
> > > >   - interactive overlay: buttons over entry get prelights and
> > > > clicks
> > > >     instead of the entry stealing them
> > >
> > > Hmm, the demo however sets the overlay as passthrough?
> > > https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/overlay.c#n
> > > 60
> >
> >
> > There's two overlay. One is passthrough, the other isn't: the
> > "Numbers" are
> > supposed to be passthrough, while the entry is supposed to get
> > input.
>
> Not what I see in code :). AFAICS both the entry and the label are
> children of a vbox, which is the only, pass-through overlay. The
> inspector seems to confirm this hierarchy.
>
> And this means gtk3 is broken :/, as it's essentially the same thing
> there.

The test was added especially to test the case of a generally
transparent/shaped overlay that had some non-transparent child widget.

It works, because the entry has a GdkWindow that is not pass through.
From the gdk_window_set_pass_through docs:

 Note that a window with @pass_through %TRUE can still have a subwindow
 without pass through, so you can get events on a subset of a window.
 And in that cases you would get the in-between related events such as 
 the pointer enter/leave events on its way to the destination window.

In general, pass-through is related to a particular window, not the
entire sub-hierarchy. This matches the css pointer-events: none
behaviour, and anything else would be far less useful.

--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc
       [hidden email]            [hidden email]
He's a shy Republican ex-con with a passion for fast cars. She's a
wealthy wisecracking nun fleeing from a Satanic cult. They fight crime!
_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of wip/carlosg/event-delivery

Carlos Garnacho
Hej hej,

On Wed, May 17, 2017 at 3:27 PM, Alexander Larsson <[hidden email]> wrote:

> On Wed, 2017-05-17 at 01:04 +0200, Carlos Garnacho wrote:
>> Hey,
>>
>> On Tue, May 16, 2017 at 8:24 PM, Matthias Clasen
>> <[hidden email]> wrote:
>> > On Tue, May 16, 2017 at 1:25 PM, Carlos Garnacho <[hidden email]
>> > > wrote:
>> > >
>> > >
>> > > >   - interactive overlay: buttons over entry get prelights and
>> > > > clicks
>> > > >     instead of the entry stealing them
>> > >
>> > > Hmm, the demo however sets the overlay as passthrough?
>> > > https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/overlay.c#n
>> > > 60
>> >
>> >
>> > There's two overlay. One is passthrough, the other isn't: the
>> > "Numbers" are
>> > supposed to be passthrough, while the entry is supposed to get
>> > input.
>>
>> Not what I see in code :). AFAICS both the entry and the label are
>> children of a vbox, which is the only, pass-through overlay. The
>> inspector seems to confirm this hierarchy.
>>
>> And this means gtk3 is broken :/, as it's essentially the same thing
>> there.
>
> The test was added especially to test the case of a generally
> transparent/shaped overlay that had some non-transparent child widget.
>
> It works, because the entry has a GdkWindow that is not pass through.
> From the gdk_window_set_pass_through docs:
>
>  Note that a window with @pass_through %TRUE can still have a subwindow
>  without pass through, so you can get events on a subset of a window.
>  And in that cases you would get the in-between related events such as
>  the pointer enter/leave events on its way to the destination window.
>
> In general, pass-through is related to a particular window, not the
> entire sub-hierarchy. This matches the css pointer-events: none
> behaviour, and anything else would be far less useful.

Hmm, I see, missed those bits. Which doesn't match too well to a
non-GdkWindow/GdkEventMask based world... how do we express this
selective pass-through across complex portions of a widget hierarchy?
The best I can think of is making GtkOverlay implement ::pick, make it
peek the deepmost widget of the picking operation, and check whether
any widgets on the way up to the overlay child have event controllers
attached, that seems the closest to non-passthrough child windows.

Cheers,
  Carlos

>
> --
> =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
>  Alexander Larsson                                            Red Hat, Inc
>        [hidden email]            [hidden email]
> He's a shy Republican ex-con with a passion for fast cars. She's a
> wealthy wisecracking nun fleeing from a Satanic cult. They fight crime!
_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list
Reply | Threaded
Open this post in threaded view
|

Re: Review of wip/carlosg/event-delivery

Alexander Larsson
On Wed, 2017-05-17 at 17:45 +0200, Carlos Garnacho wrote:

> Hej hej,
>
> On Wed, May 17, 2017 at 3:27 PM, Alexander Larsson <[hidden email]>
> wrote:
> > On Wed, 2017-05-17 at 01:04 +0200, Carlos Garnacho wrote:
> > > Hey,
> > >
> > > On Tue, May 16, 2017 at 8:24 PM, Matthias Clasen
> > > <[hidden email]> wrote:
> > > > On Tue, May 16, 2017 at 1:25 PM, Carlos Garnacho <carlosg@gnome
> > > > .org
> > > > > wrote:
> > > > >
> > > > >
> > > > > >   - interactive overlay: buttons over entry get prelights
> > > > > > and
> > > > > > clicks
> > > > > >     instead of the entry stealing them
> > > > >
> > > > > Hmm, the demo however sets the overlay as passthrough?
> > > > > https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/overlay
> > > > > .c#n
> > > > > 60
> > > >
> > > >
> > > > There's two overlay. One is passthrough, the other isn't: the
> > > > "Numbers" are
> > > > supposed to be passthrough, while the entry is supposed to get
> > > > input.
> > >
> > > Not what I see in code :). AFAICS both the entry and the label
> > > are
> > > children of a vbox, which is the only, pass-through overlay. The
> > > inspector seems to confirm this hierarchy.
> > >
> > > And this means gtk3 is broken :/, as it's essentially the same
> > > thing
> > > there.
> >
> > The test was added especially to test the case of a generally
> > transparent/shaped overlay that had some non-transparent child
> > widget.
> >
> > It works, because the entry has a GdkWindow that is not pass
> > through.
> > From the gdk_window_set_pass_through docs:
> >
> >  Note that a window with @pass_through %TRUE can still have a
> > subwindow
> >  without pass through, so you can get events on a subset of a
> > window.
> >  And in that cases you would get the in-between related events such
> > as
> >  the pointer enter/leave events on its way to the destination
> > window.
> >
> > In general, pass-through is related to a particular window, not the
> > entire sub-hierarchy. This matches the css pointer-events: none
> > behaviour, and anything else would be far less useful.
>
> Hmm, I see, missed those bits. Which doesn't match too well to a
> non-GdkWindow/GdkEventMask based world... how do we express this
> selective pass-through across complex portions of a widget hierarchy?
> The best I can think of is making GtkOverlay implement ::pick, make
> it
> peek the deepmost widget of the picking operation, and check whether
> any widgets on the way up to the overlay child have event controllers
> attached, that seems the closest to non-passthrough child windows.

I think we have to make GdkWindow::passthrough a feature of GtkWidget
instead, and respect it in the default pick operation.

Its a bit more work for people to set passthrough on all the widgets,
but its at least possible.

Conceptually one could have passthrough be a tri-state, with FALSE,
TRUE and SAME-AS-PARENT. I guess this is what CSS does essentially,
where you set pointer-events to "inherit".

--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc
       [hidden email]            [hidden email]
He's an all-American native American Green Beret haunted by memories of
'Nam. She's an artistic mute snake charmer from a family of eight older
brothers. They fight crime!
_______________________________________________
gtk-devel-list mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/gtk-devel-list