After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 769706 - Add show/hide API to GtkPopover
Add show/hide API to GtkPopover
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkPopover
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-08-10 15:08 UTC by Timm Bäder
Modified: 2016-08-20 18:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
popover: Add API to show/hide with transitions (6.75 KB, patch)
2016-08-10 15:08 UTC, Timm Bäder
none Details | Review
GtkPopover: Add gtk_popover_popdown/popup (6.39 KB, patch)
2016-08-10 17:29 UTC, Timm Bäder
none Details | Review
GtkPopover: Deprecate transitions-enabled (2.31 KB, patch)
2016-08-10 17:29 UTC, Timm Bäder
none Details | Review
Use gtk_popover_popdown/popup where appropriate (8.49 KB, patch)
2016-08-10 17:30 UTC, Timm Bäder
none Details | Review
Add gtk_popover_popdown/popup (7.13 KB, patch)
2016-08-12 14:44 UTC, Timm Bäder
none Details | Review
GtkPopover: Deprecate transitions-enabled (3.63 KB, patch)
2016-08-12 14:45 UTC, Timm Bäder
none Details | Review
GtkPopover: Add gtk_popover_popup/popdown (8.10 KB, patch)
2016-08-14 10:39 UTC, Timm Bäder
committed Details | Review
GtkPopover: Deprecate transitions-enabled (4.12 KB, patch)
2016-08-14 10:40 UTC, Timm Bäder
committed Details | Review
Use gtk_popover_popdown/popup where appropriate (9.21 KB, patch)
2016-08-14 10:41 UTC, Timm Bäder
committed Details | Review

Description Timm Bäder 2016-08-10 15:08:37 UTC
Created attachment 333069 [details] [review]
popover: Add API to show/hide with transitions

See patch
Comment 1 Carlos Garnacho 2016-08-10 15:44:28 UTC
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.
Comment 2 Timm Bäder 2016-08-10 16:14:26 UTC
(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.
Comment 3 Timm Bäder 2016-08-10 17:29:45 UTC
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.
Comment 4 Timm Bäder 2016-08-10 17:29:56 UTC
Created attachment 333074 [details] [review]
GtkPopover: Deprecate transitions-enabled
Comment 5 Timm Bäder 2016-08-10 17:30:04 UTC
Created attachment 333075 [details] [review]
Use gtk_popover_popdown/popup where appropriate
Comment 6 Matthias Clasen 2016-08-12 05:19:30 UTC
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
Comment 7 Matthias Clasen 2016-08-12 05:21:13 UTC
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.
Comment 8 Matthias Clasen 2016-08-12 05:22:06 UTC
Review of attachment 333075 [details] [review]:

ok
Comment 9 Matthias Clasen 2016-08-12 05:25:12 UTC
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.
Comment 10 Timm Bäder 2016-08-12 14:44:25 UTC
Created attachment 333183 [details] [review]
Add gtk_popover_popdown/popup
Comment 11 Timm Bäder 2016-08-12 14:45:02 UTC
Created attachment 333184 [details] [review]
GtkPopover: Deprecate transitions-enabled
Comment 12 Timm Bäder 2016-08-14 10:39:56 UTC
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.
Comment 13 Timm Bäder 2016-08-14 10:40:31 UTC
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.
Comment 14 Timm Bäder 2016-08-14 10:41:04 UTC
Created attachment 333279 [details] [review]
Use gtk_popover_popdown/popup where appropriate
Comment 15 Timm Bäder 2016-08-14 10:42:07 UTC
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.
Comment 16 Matthias Clasen 2016-08-16 16:02:38 UTC
Attachment 333278 [details] pushed as a6b9b36 - GtkPopover: Deprecate transitions-enabled
Attachment 333279 [details] pushed as a985e62 - Use gtk_popover_popdown/popup where appropriate
Comment 17 Murray Cumming 2016-08-19 16:20:28 UTC
Shouldn't that deprecation documentation mention gtk_popover_popup() and gtk_popover_popdown() too?
Comment 18 Timm Bäder 2016-08-20 07:40:32 UTC
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.

?
Comment 19 Murray Cumming 2016-08-20 18:19:44 UTC
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.