Created attachment 64660 [details]
testcase as patch for print-editor.c

I think the problem is that we set transient_for after we create a GtkComboBox and put it in the dialog. We then destroy the combobox, causing it to destroy its popup window. At that time we've set the right transient_for for the dialog, but the popup window has the old wrong transient_for.

No, I changed the order and this is still happening. I think the problem is this in gtk_menu_popdown():

  /* The X Grab, if present, will automatically be removed when we hide
   * the window */
  gtk_widget_hide (menu->toplevel);
  gtk_window_group_add_window (gtk_window_get_group (NULL), GTK_WINDOW (menu->toplevel));

This always sets the window group of the toplevel to the default group when its destroyed. However, we earlier set transient_for on the toplevel, so when that gets destroyed we do:

static void
gtk_window_destroy (GtkObject *object)
{
  GtkWindow *window = GTK_WINDOW (object);
  
  toplevel_list = g_slist_remove (toplevel_list, window);

  if (window->transient_parent)
    gtk_window_set_transient_for (window, NULL);

... leading to: 

gtk_window_unset_transient_for  (GtkWindow *window)
{
  if (window->transient_parent)
    {
      if (window->transient_parent->group)
	gtk_window_group_remove_window (window->transient_parent->group,
					window);


But the transient parent is still in the custom window group, while the menu toplevel is in the default group, which crashes on the assert in gtk_window_group_remove_window().

I'm attaching a small testcase for this.

Created attachment 64861 [details]
Minimal testcase

I think this is a menu issue. In gtk_menu_popup() we set the menu window group to the same as the parent menushells window group, and on gtk_menu_popdown we set it to the default group. But at the same time we also set transient_fo for on the menu toplevel which also sets the window group. This then causes a conflict.

Created attachment 64865 [details] [review]
Possible solution

It seems gtk_menu_popup only sets the window group if the menu is popped up from a parent menuitem (i.e. parent_menu_shell != NULL), but it always sets it back to the default group. This patch makes it only switch back if we had a parent menushell.

I don't know if this is right, but it seems to make things work at least.

Created attachment 64866 [details] [review]
Fix for gtkprintunixdialog.c part of the bug

My initial analysis is also right. The previous patch fixes the testcase, but the print dialog still crashes. This patch fixes that by making the transient-for window a construct property and creating the comboboxes (and other widgets) after the transient-for is set.

Maybe the transient-for construct property should be on GtkWindow or GtkDialog though. I'm not sure why e.g. gtk_file_chooser_dialog_new_valist don't pass the parent as a construct property like the other args.

The menu changes look good.

The gtkprintunixdialog changes look fine to me, 
modulo moving the property up the chain.

We discussed moving the transient parent property up to GtkDialog,
but it should probably be a GtkWindow property, even. 

*** Bug 340789 has been marked as a duplicate of this bug. ***

I commited this, moving the transient-for property to GtkWindow.

*** Bug 341112 has been marked as a duplicate of this bug. ***

It seems this isn't really fixed yet. This is causing a crash in evolution when closing the composer window.

So, what happens in evolution is that the window gets destroyed, and then a plug inside the window gets unparented, causing it to change its window group. Then a menu inside the plug gets the hierarchy-changed signal on its attached window ( gtkmenu.c:attach_widget_hierarchy_changed) and re-sets the transient parent, but when unsetting the transient parent we look at the window->transient_parent->group which is not the same as it was when we added the group, causing an assert.

Also, i think the handling of window groups in gtk_menu_popup/popdown isn't necessary anymore, given the new handling of transient_for in attach_widget_hierarchy_changed. We now always set the menu transient to its attached widget (i.e. combobox or parent menuitem), which handles the window group stuff.

Created attachment 65313 [details] [review]
Possible fix

Created attachment 65321 [details] [review]
New approach

This is a new approach that removes all setting of transient-for on hierarchy changes. Instead we just set it on popup, first to the parent menushell, and if that is not availbile (i.e. if the parent is a combobox) then to the attached widget (unless the screen was explicitly set, then we don't want to move it to the attached widgets screen).

Created attachment 65324 [details]
Another approach

This patch takes the approach of just using gdk_window_set_transient_for(). That should keep window groups intact. The advantage of this is that menus will then have correct transient parents if they change attach widget when they are open.

But both approaches are fine with me.

Created attachment 65326 [details]
The right patch, hopefully

Commited the approach from comment #15. The two approaches are both basically sound, but that one gets the window group from the attach widget into the menu toplevel window, which seems better.

Please take a look at #340898.