Created attachment 311900 [details] [review]
GtkPopver: Remove _get_pointed_to_coords

It's only used one and removing it only adds a single line there.

Created attachment 311901 [details] [review]
GtkWindow: Don't needlessly resize popovers

Check whether the given popover even changed size in
_gtk_window_set_popover_position. If not, just move its GdkWindow
without calling gtk_widget_queue_resize. Using popover_get_rect here is
still relatively costly, but popover_size_allocate would be doing that
anyway.

Created attachment 311902 [details] [review]
GtkPopover: Don't resize during the transition

Call gtk_popover_update_position instead which will pick up the new
transition_diff value and pass it on to
_gtk_window_set_popover_position, which in turn will move the window
correctly.

Review of attachment 311900 [details] [review]:

Looks good! "GtkPopover" and "once" on the commit log, being super picky

Review of attachment 311901 [details] [review]:

Looks mostly good, there's a few extra newlines around though, the code doesn't usually use more than 1 to separate functions or code blocks.

::: gtk/gtkwindow.c
@@ +11960,3 @@
+      cairo_rectangle_int_t new_size;
+      popover_get_rect (data, window, &new_size);
+      move_popover_window (data, &new_size);

Knowing it all goes down to a same move_resize() vfunc, wouln't it be clearer here to gdk_window_move()? I guess that means you could also remove move_popover_window() as a result.

I also notice we now have immediate vs delayed effect here, I guess it's mostly harmless as those will be pretty much mutually exclusive in practice, and the least appealing behavior is what we currently have for every case...

Review of attachment 311902 [details] [review]:

This one looks fine to me, setting as a-c-n, although it'll depend on the changes to the second patch.

(In reply to Carlos Garnacho from comment #5)
> Review of attachment 311901 [details] [review] [review]:
> 
> Looks mostly good, there's a few extra newlines around though, the code
> doesn't usually use more than 1 to separate functions or code blocks.
> 
> ::: gtk/gtkwindow.c
> @@ +11960,3 @@
> +      cairo_rectangle_int_t new_size;
> +      popover_get_rect (data, window, &new_size);
> +      move_popover_window (data, &new_size);
> 
> Knowing it all goes down to a same move_resize() vfunc, wouln't it be
> clearer here to gdk_window_move()? I guess that means you could also remove
> move_popover_window() as a result.

Hmm yes I guess that could also work. Just didn't come to my mind when writing it.

> I also notice we now have immediate vs delayed effect here, I guess it's
> mostly harmless as those will be pretty much mutually exclusive in practice,
> and the least appealing behavior is what we currently have for every case...

Yes, but I guess that's the effect you get everytime you use an input_output window to move widgets? In theory, not resizing when moving could be wrong I guess, e.g. when you open a second popover directly after the first one gets shown, so the second one should move down during the transition(?). Sounds more like a nightmare of course, but that makes it even more likely to exist in reality :p


(In reply to Carlos Garnacho from comment #6)
> Review of attachment 311902 [details] [review] [review]:
> 
> This one looks fine to me, setting as a-c-n, although it'll depend on the
> changes to the second patch.

What depends on the changes from teh second patch?


I'll make the changes and quickly test them with animations disabled (which I forgot now) tomorrow morning, thanks for the quick review!

(In reply to Timm Bäder from comment #7)
> > I also notice we now have immediate vs delayed effect here, I guess it's
> > mostly harmless as those will be pretty much mutually exclusive in practice,
> > and the least appealing behavior is what we currently have for every case...
> 
> Yes, but I guess that's the effect you get everytime you use an input_output
> window to move widgets? In theory, not resizing when moving could be wrong I
> guess, e.g. when you open a second popover directly after the first one gets
> shown, so the second one should move down during the transition(?). Sounds
> more like a nightmare of course, but that makes it even more likely to exist
> in reality :p

Showing/sliding in several popovers shouldn't be an issue, if all are shown at the same time, the animation would run for the same times and distances, so it'd look coordinated. 

I was more concerned about mid-animation changes to needs_resize/needs_move, which I guess you can trigger entering a popover submenu fast enough after showing. Probably the animation is short and fast enough to notice the frame "skip" if that happens.

> 
> 
> (In reply to Carlos Garnacho from comment #6)
> > Review of attachment 311902 [details] [review] [review] [review]:
> > 
> > This one looks fine to me, setting as a-c-n, although it'll depend on the
> > changes to the second patch.
> 
> What depends on the changes from teh second patch?

I meant that it should wait to be pushed with the second patch despite a-c-n, the first one can be pushed now :)

Created attachment 311923 [details] [review]
GtkPopover: Remove _get_pointed_to_coords

It's only used once and removing it only adds a single line there.

Created attachment 311924 [details] [review]
GtkWindow: Don't needlessly resize popovers

Check whether the given popover even changed size in
_gtk_window_set_popover_position. If not, just move its GdkWindow
without calling gtk_widget_queue_resize. Using popover_get_rect here is
still relatively costly, but popover_size_allocate would be doing that
anyway.

Comment on attachment 311923 [details] [review]
GtkPopover: Remove _get_pointed_to_coords

Looks great

Comment on attachment 311924 [details] [review]
GtkWindow: Don't needlessly resize popovers

This one looks good too

Okay pushed, thanks!