GNOME Bugzilla – Bug 741405
Add GtkPopover show/hide transitions
Last modified: 2015-02-20 15:31:08 UTC
Created attachment 292553 [details] [review] Initial patch Just adding this since it was requested and to get some feedback. This adds show/hide transitions (similar to those currently implemented in gnome-shell) to GtkPopover. Current problems (I can think about): - I'm not sure if the transition is *always* wanted. - Currently, when the transition is finished, I call gtk_widget_hide on it, i.e. after 0.33 seconds (which is arbitrary of course). This means that if the popover is connected to a GtkMenuButton, the button would get depressed only after the transition is over. This is also weird because you can't click e.g. the close button of a window during the transition. - This currently only works because TRANSITION_DIFF is smaller than the arrow size (?). Choosing bigger values here causes the popover to get clipped at the bottom/top. Not sure how relevant this is but I haven't been able to fix this propertly IIRC. I think that's it, tips and improvements are always welcome of course.
Thanks Timm for doing this! A few initial comments below, I'm not getting too much into code yet. (In reply to comment #0) > Created an attachment (id=292553) [details] [review] > Initial patch > > Just adding this since it was requested and to get some feedback. > > This adds show/hide transitions (similar to those currently implemented in > gnome-shell) to GtkPopover. > > Current problems (I can think about): > > - I'm not sure if the transition is *always* wanted. Yeah I don't think so... it should at least respect the "gtk-enable-animations" GtkSetting. It also looks a bit of place in the magnifier popover as it has other movements involved, so we might want a boolean property for this. Ideally, this should be hooked into CSS, so eg animating the margin would bring the desired result, but popover currently abuses the margin in order to fit the tail in all possible directions. Maybe we should separate margins from the tail gap in order to make this possible. This also involves abusing some widget state in order to get css transitions for it, and possibly negative margins, so it might not be as nice as it sounds... > - Currently, when the transition is finished, I call gtk_widget_hide on it, > i.e. > after 0.33 seconds (which is arbitrary of course). This means that if the > popover is connected to a GtkMenuButton, the button would get depressed only > after the transition is over. This is also weird because you can't click > e.g. > the close button of a window during the transition. The emission of ::closed should probably be moved out of unmap(), into the point where the fade out transition is started. > - This currently only works because TRANSITION_DIFF is smaller than the arrow > size (?). Choosing bigger values here causes the popover to get clipped at > the > bottom/top. Not sure how relevant this is but I haven't been able to fix > this > propertly IIRC. IMO the transition should be affecting the position calculated on gtk_popover_update_position(), that way you move the popover window itself instead of its contents, and avoid clipping during the animation, unless the popover is of course too close to a window border, etc... > > > I think that's it, tips and improvements are always welcome of course.
(In reply to comment #1) > > - Currently, when the transition is finished, I call gtk_widget_hide on it, > > i.e. > > after 0.33 seconds (which is arbitrary of course). This means that if the > > popover is connected to a GtkMenuButton, the button would get depressed only > > after the transition is over. This is also weird because you can't click > > e.g. > > the close button of a window during the transition. > > The emission of ::closed should probably be moved out of unmap(), into the > point where the fade out transition is started. > Forgot to tell, also apply_modality(popover, FALSE) should happen on transition start. We might want to ignore events mid-transition though, so you can't refocus inside or trigger anything on dismissal. This should apply to the popover window and all child windows, so we'd be down to controlling sensitivity, or capturing events in order to ignore them...
Created attachment 292671 [details] [review] Second try Ok, I tried to address all the issues. Please ignore unnecessary changes for now (like adding {}, etc.) they are just for debugging purposes. I think the only thing I couldn't quite figure out is how to ignore events during the transitions. I'm unsure about the property name and its default value. And of course, documentation is missing, and I'd consider renaming some of the variables but I can still add/change that for the final version.
(In reply to comment #3) > Created an attachment (id=292671) [details] [review] > Second try > > Ok, I tried to address all the issues. Please ignore unnecessary changes for > now (like adding {}, etc.) they are just for debugging purposes. I think the patch goes in the right direction, just some thoughts: - is the hide() vfunc really necessary? I *think* this is doable purely through map/unmap, and would like to avoid chaining up from weird places. If this is unavoidable, I'd prefer it to have the animation callback call gtk_widget_hide() again, and detect the case where the animation is simply finished and chain up as usual. - IMO it'd be better to call _transition_enabled() within start_transition(), and have that function return a gboolean > > I think the only thing I couldn't quite figure out is how to ignore events > during the transitions. I think you could: 1) Set a temporary input shape, so pointer events go through 2) ensure keyboard focus is outside the popover at the time of starting the "popover hides" animation, should be done already 3) make gtk_popover_*focus() ignore taking the focus again inside while the animation is running. > > I'm unsure about the property name and its default value. And of course, > documentation is missing, and I'd consider renaming some of the variables but I > can still add/change that for the final version. I can't think of a better name either, let's stick with this so far.
(In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=292671) [details] [review] [details] [review] > > Second try > > > > Ok, I tried to address all the issues. Please ignore unnecessary changes for > > now (like adding {}, etc.) they are just for debugging purposes. > > I think the patch goes in the right direction, just some thoughts: > - is the hide() vfunc really necessary? I *think* this is doable purely through > map/unmap, and would like to avoid chaining up from weird places. If this is > unavoidable, I'd prefer it to have the animation callback call > gtk_widget_hide() again, and detect the case where the animation is simply > finished and chain up as usual. GtkMenuButton calls gtk_widget_set_visible on the popover, so I guess it's not avoidable? Or, people would generally call gtk_widget_hide on the popover and expect the transition I guess, but I'm not 100% sure if that makes it completely unavoidable. I'll look into the other issues, thanks for the review.
(In reply to comment #5) > > I think the patch goes in the right direction, just some thoughts: > > - is the hide() vfunc really necessary? I *think* this is doable purely through > > map/unmap, and would like to avoid chaining up from weird places. If this is > > unavoidable, I'd prefer it to have the animation callback call > > gtk_widget_hide() again, and detect the case where the animation is simply > > finished and chain up as usual. > > GtkMenuButton calls gtk_widget_set_visible on the popover, so I guess it's not > avoidable? Or, people would generally call gtk_widget_hide on the popover and > expect the transition I guess, but I'm not 100% sure if that makes it > completely unavoidable. What I mean is, the call stack is something like gtk_widget_set_visible->gtk_widget_hide->gtk_widget_unmap->gdk_window_hide , just delaying the last gdk_window_hide() step until after the animation finishes seems more sensible to me. The only impediment I can see is https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n5920https://git.gnome.org/browse/gtk+/tree/gtk/gtkwindow.c#n5920 , the GtkWindow-side popover window should be hidden after the animation finishes there. I guess a new GtkPopover signal might be nice to have there.
Created attachment 295468 [details] [review] popover: Add show/hide transitions These have the same visual effect and timing than the gnome-shell ones. During the hide animation, the popover has been made to take focus elsewhere, and refuse to take any pointer/keyboard input until the popover is shown again. This has been based on work from Timm Bäder.
2 things I noticed: - clicks are not ignored during SHOWING, only when HIDING. Is that correct? - The modality doesn't seem to be disabled when HIDING (or rather, before). I don't really care about the former as the animation is so short anyway, but the latter is a problem when a popover is open and you want to close the windwo via its close button. The first click just dismisses the popover and then you have to wait for it to really vanish before you are actually able to close the window.
Another thing: I have this code where I call gtk_widget_hide on a popover in that popover's CLOSED callback, it's a GtkMenuButton with a custom popover and I essentially do that to sync the popover's visibility to the GtkMenuButton's toggled state. That call to gtk_widget_hide in there now crashes (I guess that's a hide->closed->hide->closed->... loop, judging from a backtrace).
Created attachment 295939 [details] [review] popover: Add show/hide transitions These have the same visual effect and timing than the gnome-shell ones. During the hide animation, the popover has been made to take focus elsewhere, and refuse to take any pointer/keyboard input until the popover is shown again. This has been based on work from Timm Bäder.
I think this last patch fixes most issues here, and does work fine with gnome-calendar popovers. (In reply to comment #8) > 2 things I noticed: > > - clicks are not ignored during SHOWING, only when HIDING. Is that correct? This is partly on purpose, otherwise it seems quite easy to just hide it again if the popover input mask is empty, so everywhere is "outside" of it. Whether these should have an effect or not is more debatable, keyboard events should work on SHOWING nonetheless.
I already said on IRC that all the problems I found were fixed by the last iteration, but I found this today: open the inspector, select some widget, then try to change one of its properties, close that popover again and it'll crash (with some infinite loop).
Created attachment 297387 [details] [review] popover: Add show/hide transitions These have the same visual effect and timing than the gnome-shell ones. During the hide animation, the popover has been made to take focus elsewhere, and refuse to take any pointer/keyboard input until the popover is shown again. This has been based on work from Timm Bäder.
Review of attachment 297387 [details] [review]: ::: gtk/gtkpopover.h @@ +99,3 @@ +GDK_AVAILABLE_IN_3_16 +void gtk_popover_set_transitions_enabled (GtkPopover *popover, I'm not really on board with an explicit property for toggling the transitions on top of gtk-enable-animations. it means another public entry point that we need to maintain once we hand over control on transitions to CSS. why isn't gtk-enable-animations enough? comment #1 and #2 do not make it really clear…
(In reply to Emmanuele Bassi (:ebassi) from comment #14) > Review of attachment 297387 [details] [review] [review]: > > ::: gtk/gtkpopover.h > @@ +99,3 @@ > > +GDK_AVAILABLE_IN_3_16 > +void gtk_popover_set_transitions_enabled (GtkPopover *popover, > > I'm not really on board with an explicit property for toggling the > transitions on top of gtk-enable-animations. it means another public entry > point that we need to maintain once we hand over control on transitions to > CSS. > > why isn't gtk-enable-animations enough? comment #1 and #2 do not make it > really clear… I admit this is a bit of a kludge, IMO this is currently mainly of interest for clutter apps on x11, there the transition looks weird as contents fade, but the popover shape remains at full alpha.
Oops, I realize I forgot to close the bug. The last patch was pushed with just doc updates and minor polishing.