Created attachment 305039 [details] [review]
window: Attach popup menus to their parent widgets

Review of attachment 305039 [details] [review]:

Thanks for the patch, Jonas. I haven't tested it on Wayland, yet. A few initial comments after quickly scanning the code.

::: src/terminal-window.c
@@ +2051,3 @@
+    gtk_menu_attach_to_widget (GTK_MENU (popup_menu),
+			       GTK_WIDGET (window),
+			       NULL);

Won't it be more precise to attach it to the TerminalScreen instead?

@@ +3198,3 @@
   menu = gtk_ui_manager_get_widget (priv->ui_manager, "/NotebookPopup");
+  if (!gtk_menu_get_attach_widget (GTK_MENU (menu)))
+    gtk_menu_attach_to_widget (GTK_MENU (menu), widget, NULL);

Don't you want to do the same in notebook_popup_menu_cb below?

I see no reason for the if () check before attaching; just always attach the menu to the widget it's going to be popped up from.

(In reply to Debarshi Ray from comment #2)
> Review of attachment 305039 [details] [review] [review]:
> 
> Thanks for the patch, Jonas. I haven't tested it on Wayland, yet. A few
> initial comments after quickly scanning the code.
> 
> ::: src/terminal-window.c
> @@ +2051,3 @@
> +    gtk_menu_attach_to_widget (GTK_MENU (popup_menu),
> +			       GTK_WIDGET (window),
> +			       NULL);
> 
> Won't it be more precise to attach it to the TerminalScreen instead?

Sure. I changed to attach to that.

> 
> @@ +3198,3 @@
>    menu = gtk_ui_manager_get_widget (priv->ui_manager, "/NotebookPopup");
> +  if (!gtk_menu_get_attach_widget (GTK_MENU (menu)))
> +    gtk_menu_attach_to_widget (GTK_MENU (menu), widget, NULL);
> 
> Don't you want to do the same in notebook_popup_menu_cb below?

Ah, missed that one. New patch with the two fixes coming.

(In reply to Christian Persch from comment #3)
> I see no reason for the if () check before attaching; just always attach the
> menu to the widget it's going to be popped up from.

Without the if (), we get warnings logged that we are attaching when the menu was already attached. Had the menu been recreated every time, we could just attach it every time, but that is not the case here.

Created attachment 305073 [details] [review]
window: Attach popup menus to their parent widgets

When running on Wayland, popup menus must have a parent widget to attach
to. If a parent widget is not explicitly set, the GDK backend tries to
guess what parent widget seems appropriate. In order to have more
reliable popups, change the popups in the terminal window to set their
parent widgets explicitly.

Hmm. IMHO gtk_menu_attach_to_widget is overzealous in its warning; it should only warn when trying to attach to a *different* widget…

However, I think it would be simplest then to attach all the menus in terminal_window_init() just after the UI manager has loaded its UI file.

Created attachment 305110 [details] [review]
window: Attach popup menus to their parent widgets

When running on Wayland, popup menus must have a parent widget to attach
to. If a parent widget is not explicitly set, the GDK backend tries to
guess what parent widget seems appropriate. In order to have more
reliable popups, change the popups in the terminal window to set their
parent widgets explicitly.

(In reply to Christian Persch from comment #6)
> Hmm. IMHO gtk_menu_attach_to_widget is overzealous in its warning; it should
> only warn when trying to attach to a *different* widget…
> 
> However, I think it would be simplest then to attach all the menus in
> terminal_window_init() just after the UI manager has loaded its UI file.

I noticed that the same menu is popuped up above different widgets (when using multiple tabs), so this wouldn't be possible. I uploaded a new version of the patch that simply reattaches everytime, handling the cases where a reused menu opened on different widgets gets attached to the correct one.

In other words, doing anything on init is not possible, given that the widget the menu should be atteched to changes over time.

Comment on attachment 305110 [details] [review]
window: Attach popup menus to their parent widgets

Ok.

Attachment 305110 [details] pushed as 67afb95 - window: Attach popup menus to their parent widgets

It appears this causes a crash: bug 752208.

Should we keep this close maybe? While it introduces the crash, it is probably a good idea to keep using only bug 752208 for that issue.

Looks like bug 752208 is now fixed.