GNOME Bugzilla – Bug 769287
GtkMenuToolButton:show-menu is emitted twice and breaks dynamic menus
Last modified: 2016-08-04 21:32:30 UTC
Created attachment 332318 [details] example program https://git.gnome.org/browse/gtk+/commit/?id=0796d7b6ff9393746d90841c8db2ba9a2f7e6df8 (found by bisecting) broke dynamic population of GtkMenuToolButton's menu by making the :show-menu signal be emitted twice. This is because the new gtk_menu_button_clicked() calls gtk_toggle_button_set_active() after having emitted the signal, which calls gtk_button_clicked() if the state changed, causing a second emission of the GtkButton:clicked signal. This is very problematic, because it has 2 side effects: * if the :show-menu handler creates a new menu and uses gtk_menu_tool_button_set_menu() to assign it, the menu will be poped up twice. * if the :show-menu handler clears the existing menu and re-fills it, no signal will be emitted when one of its items is activated. Attached demo show the problem: * If you click on the "Replace menu" arrow, the menu pops up. If you click on the "Hello" item, it emits the :activate signal, but then re-shows the menu as if you didn't really click. * If you click on the "Refill menu" arrow, the menu pops up. If you click on the "Hello" item, the menu closes but no signal is emitted. --- Additionally to that bug, the patch in question makes the menu pop on button *release*. It's expected if I believe the commit message, but it's inconsistent with every other menus. Menubars, contextual menus: they all popup the menu on button *press*, which actually allows to click on the menu, keep the button pressed, and release it on an item, which will activate it. So, my suggestion would be reverting that patch altogether. But well, any other fix for the actual issue would be good anyway. Also, please backport the chosen fix to every supported previous releases (all of 3.16, 3.18 and 3.20 are affected) as it breaks certain applications that worked just fine until 3.16.
https://git.gnome.org/browse/gtk+/commit/?id=65f7fb04ad3cd59ac87ac13a3a59ac18c65c610c could have helped, but it actually doesn't fix it for some reason. Still reproducible on master (3.21.4).
Created attachment 332320 [details] [review] GtkMenuButton: use :toggled instead of :clicked And I must say that I don't get why this isn't simply done in :toggled like it used to, as :clicked emits :toggled properly -- and it fixes the bug I'm describing. And AFAICT popovers still work just fine (popover demo does). Attached patches fixes it that way.
Thanks for the patch, it looks like a valid approach, and the "popup on button release" behavior is preserved, which is desirable (eg. menubuttons on headerbars or toolbars with window-dragging enabled). I'm going to push it as-is. (In reply to Colomban Wendling from comment #1) > https://git.gnome.org/browse/gtk+/commit/ > ?id=65f7fb04ad3cd59ac87ac13a3a59ac18c65c610c could have helped, but it > actually doesn't fix it for some reason. Still reproducible on master > (3.21.4). It didn't help because this is not a reentrancy issue, it's just that popping down the menu sets the toggle button back to !active, which is implemented by emitting ::clicked and letting the toggle button toggle itself, this ::clicked emission popped up the menu again here. This means that ::clicked is still emitted when the GtkMenu is popped down, which I don't see quite coherent, although I guess that if every other GtkToggleButton subclass can live with it, so can GtkMenuButton.
Attachment 332320 [details] pushed as 6452134 - GtkMenuButton: use :toggled instead of :clicked