GNOME Bugzilla – Bug 755435
Optimize GtkPopover positioning
Last modified: 2015-09-29 13:30:32 UTC
Updating popovers currently causes a resize in any case, even if they just changed position and not size. This is e.g. happening in during the show/hide transition, at 60fps. The transition does not seem to cause any resize anymore, and I couldn't find other issues with this, granted that I didn't test it for a long time. See the attached patches for a (hopeful) improvement and a small cleanup.
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!