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 729820 - GMenuModel submenu-action set 'false' before item activation
GMenuModel submenu-action set 'false' before item activation
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkMenu
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2014-05-08 16:41 UTC by fakey
Modified: 2014-06-16 20:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test that crashes because activation happens after hide (1.69 KB, text/plain)
2014-05-08 16:41 UTC, fakey
  Details
Test that crashes because activation happens after hide (1.74 KB, text/plain)
2014-05-08 19:19 UTC, fakey
  Details
Activate menu item before closing its parent shell (1021 bytes, patch)
2014-05-08 19:21 UTC, fakey
rejected Details | Review
Defer submenu-action state change to an idle (2.42 KB, patch)
2014-05-09 18:07 UTC, fakey
none Details | Review
Change submenu-action state after activate (1.58 KB, patch)
2014-05-14 16:33 UTC, fakey
none Details | Review
Change submenu-action state in an idle (2.67 KB, patch)
2014-05-14 20:00 UTC, fakey
none Details | Review
Change submenu-action state after activate (7.44 KB, patch)
2014-05-15 21:37 UTC, fakey
none Details | Review
Change submenu-action state after activate (7.56 KB, patch)
2014-05-16 16:02 UTC, fakey
needs-work Details | Review
Change submenu-action state after activate (7.65 KB, patch)
2014-05-20 17:39 UTC, fakey
committed Details | Review
menu binding: emit submenu close after activate (3.82 KB, patch)
2014-06-16 18:30 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description fakey 2014-05-08 16:41:01 UTC
Created attachment 276176 [details]
Test that crashes because activation happens after hide

GtkMenuItem "activate" is emitted after its parent shell's "hide", which is an issue if the application makes changes to that shell in response to a "hide". For example, an application might build its menus lazily, adding items when about to show, and removing them on hide. In this case, the menu item would be finalized before activation occurs.

The activation should occur while the parent menu is still in a shown state, before the menu is hidden.
Comment 1 fakey 2014-05-08 19:19:27 UTC
Created attachment 276193 [details]
Test that crashes because activation happens after hide
Comment 2 fakey 2014-05-08 19:21:46 UTC
Created attachment 276194 [details] [review]
Activate menu item before closing its parent shell
Comment 3 Allison Karlitskaya (desrt) 2014-05-08 19:24:02 UTC
A bit of background to this issue:  we see this problem with GtkMenuTracker and people who dynamically generate menu context.  By the time the action gets invoked, the menu has already been destroyed.

gedit was having trouble with this and their stack-switcher menu, for example.  They were able to solve it by handling the hide via idle (which meant that the menu item was still around at the time of the activation).

It gets trickier with D-Bus gets involved with submenu actions, however.  In this case it's not possible to use the idle trick because the second D-Bus message may still be on the wire.  The thought comes to mind to use a timeout, but that's obviously a pretty nasty hack.

There are three ways that this problem can be fixed:

 - fix GtkMenu directly

 - fix the menu tracker to attach the activate signal in such a way that the
   action gets activated before the menu close signal fires

 - fix the submenu-action handling in the tracker so that the to-close
   transition is always handled from an idle


I think it's probably best to fix GtkMenu directly, unless we find that it introduces substantial compatibility issues with existing code.
Comment 4 fakey 2014-05-09 14:56:30 UTC
So... doing the activate before the hide can be a bad thing in the case that the activate handler does something like gtk_dialog_run () on a modal GtkDialog. In that case, the menu remains open until losing focus. But even worse is the case where the activate handler does anything blocking or resource intensive, because the menu retains the grab on the keyboard and pointer.
Comment 5 fakey 2014-05-09 15:17:29 UTC
Review of attachment 276194 [details] [review]:

We can't do this because an activate handler might block and the menu retains a grab on the keyboard and pointer until hidden.
Comment 6 fakey 2014-05-09 18:07:11 UTC
Created attachment 276258 [details] [review]
Defer submenu-action state change to an idle
Comment 7 Matthias Clasen 2014-05-11 13:26:26 UTC
I agree: changing the order of activate and hide is out of the question, because the hide takes down all the grabs, and you'd break all the worlds activate handlers that don't expect the grab to be around.
Comment 8 Matthias Clasen 2014-05-11 13:29:12 UTC
yet another alternative might be to add another signal to use for the tracker, instead of hide - and guarantee that it always gets emitted after the hide and the activate, and after the grab is gone.
Comment 9 fakey 2014-05-14 16:33:32 UTC
Created attachment 276545 [details] [review]
Change submenu-action state after activate
Comment 10 fakey 2014-05-14 19:49:39 UTC
Comment on attachment 276258 [details] [review]
Defer submenu-action state change to an idle

This patch is working much better than the one using the selection-done signal.
Comment 11 fakey 2014-05-14 20:00:28 UTC
Created attachment 276558 [details] [review]
Change submenu-action state in an idle

Updating the commit message to provide detail.
Comment 12 Matthias Clasen 2014-05-15 00:10:35 UTC
I don't like the idle approach. I'd be happier if we could avoid it
Comment 13 Allison Karlitskaya (desrt) 2014-05-15 03:54:21 UTC
What is specifically bad about it?  I'm kinda fond of it because it involves less digging inside the guts of GtkMenu... it's also only used for menuitems with a submenu-action... which is not a lot of them.
Comment 14 Matthias Clasen 2014-05-15 13:33:33 UTC
But we are inside gtk - what I hate is workarounds in one side of the code base for issues in the other side. We had and still have far too much of that in the a11y code, and it generally makes debugging hard, breaks in corner cases, or has unintended side effect.
Comment 15 fakey 2014-05-15 21:37:41 UTC
Created attachment 276629 [details] [review]
Change submenu-action state after activate

We must add a new signal since selection-done doesn't cover all cases of hide sufficiently...
Comment 16 Matthias Clasen 2014-05-16 15:19:42 UTC
Review of attachment 276629 [details] [review]:

::: gtk/gtkmenushell.c
@@ +610,3 @@
+  menu_shell = GTK_MENU_SHELL (widget);
+  menu_shell->priv->should_close = TRUE;
+}

How is should_close different from !visible ?

@@ +613,3 @@
+
+void
+_gtk_menu_shell_check_can_close (GtkMenuShell *menu_shell)

I'd rather call this _gtk_menu_shell_maybe_emit_close or something like that, to indicate that this may emit a signal - check in the name makes me expect a boolean return
Comment 17 fakey 2014-05-16 15:34:09 UTC
Review of attachment 276629 [details] [review]:

::: gtk/gtkmenushell.c
@@ +610,3 @@
+  menu_shell = GTK_MENU_SHELL (widget);
+  menu_shell->priv->should_close = TRUE;
+}

You're right that these should probably be called should_emit_close and can_emit_close instead. We're using these to prevent any possibility of emitting the close signal multiple times for a single hide. Once we emit the close, we reset should_close back to the default state of FALSE.

Renaming all of these things will hopefully make it clearer...
Comment 18 fakey 2014-05-16 16:02:42 UTC
Created attachment 276678 [details] [review]
Change submenu-action state after activate

Improve the naming for clarity
Comment 19 Allison Karlitskaya (desrt) 2014-05-20 15:57:16 UTC
Review of attachment 276678 [details] [review]:

Didn't check the validity of the approach in general, but one very large problem noticed:

::: gtk/gtkmenushell.h
@@ +64,2 @@
   void     (*deactivate)       (GtkMenuShell *menu_shell);
+  void     (*close)            (GtkMenuShell *menu_shell);

you can't add new functions to the vtable like this -- it changes the ABI.

you need to add this to the end, and remove one of the "reserved" slots.
Comment 20 fakey 2014-05-20 17:39:47 UTC
Created attachment 276891 [details] [review]
Change submenu-action state after activate

Preserve ABI by using one of the reserved vfunc slots.
Comment 21 Matthias Clasen 2014-05-26 15:33:24 UTC
going back to the original bug description, I don't understand why the menuitem or its shell would get finalized prematurely - gtk_menu_shell_activate_item is careful to take references on both for the duration.
Comment 22 Allison Karlitskaya (desrt) 2014-05-26 15:49:15 UTC
The problem is when people dynamically construct menus in response to menus being popped up and destroy them again when the menu is torn down.
Comment 23 Allison Karlitskaya (desrt) 2014-06-16 18:30:09 UTC
Created attachment 278550 [details] [review]
menu binding: emit submenu close after activate

We want to make sure that the submenu action is changed back to FALSE
_after_ the menu item has been activated.  This prevents the menu
teardown handler from deleting the menu item before it can be activated.

Unfortunately, GtkMenuShell emits "hide" before the item activation.
This is probably done to prevent the application from doing things like
showing dialogs when the menu is still holding the grab.

In the case where we are doing an activate, set a boolean flag on each
of the open menus (following the parent stack) indicating that we'll be
emitting another signal soon (selection done).  If that flag is set, we
defer the setting of the submenu action until we receive the second
signal.
Comment 24 Matthias Clasen 2014-06-16 18:50:13 UTC
Review of attachment 278550 [details] [review]:

If it solves your problem, I'm fine with it.

::: gtk/gtkmenushellprivate.h
@@ +66,3 @@
+                                     * signal is coming soon (when checked
+                                     * from inside of a "hide" handler).
+                                     */

The name is a bit misleading to me - it has to be read as an instruction to the menutracker, right ? ('defer hiding this menu to the selection-done signal handler'). Might be clearer to make it a bit more descriptive, like the comment. Maybe selection_done_coming_soon ?

Anyway, nitpicks
Comment 25 Matthias Clasen 2014-06-16 18:50:24 UTC
Review of attachment 278550 [details] [review]:

If it solves your problem, I'm fine with it.

::: gtk/gtkmenushellprivate.h
@@ +66,3 @@
+                                     * signal is coming soon (when checked
+                                     * from inside of a "hide" handler).
+                                     */

The name is a bit misleading to me - it has to be read as an instruction to the menutracker, right ? ('defer hiding this menu to the selection-done signal handler'). Might be clearer to make it a bit more descriptive, like the comment. Maybe selection_done_coming_soon ?

Anyway, nitpicks
Comment 26 Allison Karlitskaya (desrt) 2014-06-16 19:36:07 UTC
Attachment 278550 [details] pushed as b532e1f - menu binding: emit submenu close after activate
Comment 27 fakey 2014-06-16 19:38:34 UTC
Thanks, this does fix the problem for me!