GNOME Bugzilla – Bug 769706
Add show/hide API to GtkPopover
Last modified: 2016-08-20 18:55:17 UTC
Created attachment 333069 [details] [review] popover: Add API to show/hide with transitions See patch
Review of attachment 333069 [details] [review]: Looks mostly good! ::: gtk/gtkpopover.c @@ +1519,3 @@ else if (!gtk_widget_is_ancestor (event_widget, widget)) + { + gtk_popover_popdown (popover); *mumbles something about one liners between braces*. I'll make it your call though :) @@ +2592,3 @@ + +/** + * gtk_popoverr_poopup: Glad to see I'm not the only one committing those typos :P @@ +2597,3 @@ + * Pops @popover up. This is different than a gtk_widget_show() call + * in that it shows the popover with a transition (unless they have been + * disabled with gtk_popover_set_transitions_enabled()). IMHO we could just deprecate gtk_popover_[gs]et_transitions_enabled(). If the caller is in control of whether calling show/hide or popup/popdown, it might not make sense anymore.
(In reply to Carlos Garnacho from comment #1) > Review of attachment 333069 [details] [review] [review]: > > Looks mostly good! > > ::: gtk/gtkpopover.c > @@ +1519,3 @@ > else if (!gtk_widget_is_ancestor (event_widget, widget)) > + { > + gtk_popover_popdown (popover); > > *mumbles something about one liners between braces*. I'll make it your call > though :) The rule is the else-branch should have braces if the if-branch has them (or vice-versa) as far as I know. You're right about the one higher up though :) > > @@ +2592,3 @@ > + > +/** > + * gtk_popoverr_poopup: > > Glad to see I'm not the only one committing those typos :P orrr :( > @@ +2597,3 @@ > + * Pops @popover up. This is different than a gtk_widget_show() call > + * in that it shows the popover with a transition (unless they have been > + * disabled with gtk_popover_set_transitions_enabled()). > > IMHO we could just deprecate gtk_popover_[gs]et_transitions_enabled(). If > the caller is in control of whether calling show/hide or popup/popdown, it > might not make sense anymore. Ah right, I didn't think about that.
Created attachment 333072 [details] [review] GtkPopover: Add gtk_popover_popdown/popup Since not chaining up in gtk_widget_show/gtk_widget_hide is not allowed, we can't just implicitly delay the hiding in GtkPopover's hide implementation.
Created attachment 333074 [details] [review] GtkPopover: Deprecate transitions-enabled
Created attachment 333075 [details] [review] Use gtk_popover_popdown/popup where appropriate
Review of attachment 333072 [details] [review]: And please add the new symbols to gtk3-sections.txt ::: gtk/gtkpopover.c @@ +2596,3 @@ + * in that it shows the popover with a transition. If you want to show + * the popover without a transition, use gtk_widget_show(). + */ Please add Since: 3.22 here @@ +2621,3 @@ + * in that it shows the popover with a transition. If you want to hide + * the popover without a transition, use gtk_widget_hide(). + */ And here
Review of attachment 333074 [details] [review]: ::: gtk/gtkpopover.c @@ +2327,3 @@ * Since: 3.16 + * + * Deprecated: 3.22: You can show or hide the popover without transtions Typo: 'transtions' should be 'transitions' @@ +2358,3 @@ * Since: 3.16 + * + * Deprecatd: 3.22: See gtk_popover_set_transitions_enabled(). I would just copy the deprecation message, instead of sending the reader on an easter egg hunt.
Review of attachment 333075 [details] [review]: ok
Review of attachment 333072 [details] [review]: The commit message should probably explain that this changes the behavior of show/hide for popovers and adds new api for the previous behavior. We should also add a release note for this, since app authors will want to replace show/hide with popup/popdown for their own popovers as well. Release notes go in docs/reference/gtk/migrating-3xtoy.xml nowadays.
Created attachment 333183 [details] [review] Add gtk_popover_popdown/popup
Created attachment 333184 [details] [review] GtkPopover: Deprecate transitions-enabled
Created attachment 333277 [details] [review] GtkPopover: Add gtk_popover_popup/popdown Since not chaining up in gtk_widget_show/gtk_widget_hide is not allowed, we can't just implicitly delay the hiding in GtkPopover's hide implementation. Fix this by introducing gtk_popover_popup() and gtk_popover_popdown() to show or hide a popover with transition and revert GtkPopover's show/hide implementation to apply their effect without the transition.
Created attachment 333278 [details] [review] GtkPopover: Deprecate transitions-enabled The effect of transitions-enabled=true can now be achieved using gtk_popover_popup/popdown and the effect of transitions-enabled=false can be achieved using gtk_widget_show/hide.
Created attachment 333279 [details] [review] Use gtk_popover_popdown/popup where appropriate
There was a bug in the last patch series causing the popover to be invisible if it was hidden by a broken grab and the third patch was missing the hide/show->popup/popdown conversion in GtkScaleButton.
Attachment 333278 [details] pushed as a6b9b36 - GtkPopover: Deprecate transitions-enabled Attachment 333279 [details] pushed as a985e62 - Use gtk_popover_popdown/popup where appropriate
Shouldn't that deprecation documentation mention gtk_popover_popup() and gtk_popover_popdown() too?
Could certainly do that, what do you think about Deprecated: 3.22: You can show or hide the popover without transitions using gtk_widget_show() and gtk_widget_hide() while gtk_popover_popup() and gtk_popover_popdown() will use transitions. ?
Yes, thanks, I'd think so. After all, those are the functions that seem to replace the deprecated API. I only discovered them by finding the commit message.
Pushed: https://git.gnome.org/browse/gtk+/commit/?id=5c696a7ee31bdf801471f46349fe3c2253bc35ca