Reparenting cleanup

[Timo Korvola iki fi]

Hello everybody.

I've been using Sawfish with KDE for some time now.  Most things work
but the system tray has a lot of problems.  One of these was that the
tray and Sawfish sometimes appeared to fight over a tray icon,
simultaneously trying to reparent it.  So I set out to find when
Sawfish reparents windows.  I found the details rather confusing.  The
following patch is an attempt to clarify the behaviour: Windows are
framed at map_request, never at map_notify.  They are reparented back
to the root at unmap_notify unless Sawfish detects that the unmapping
was caused by someone else - such as the system tray - reparenting the
window (this check I took from Fluxbox).

After these changes I do not see reparenting fights anymore.  Although
improved, the system tray unfortunately remains quite unreliable:
sometimes icons just don't appear at all, occasionally (rarely) they
appear in the root window.  Also, windows that are initially withdrawn,
then mapped by clicking a tray icon suffer from this bug:
<URL:http://bugzilla.gnome.org/show_bug.cgi?id=104928>.  In addition
they appear alone in some kind of a bogus virtual desktop.  Windows that
are initially mapped, then unmapped and mapped again by clicking the
tray icon twice work fine.

-- 
	Timo Korvola		<URL:http://www.iki.fi/tkorvola>

--- sawfish-1.3+cvs20061004/src/events.c.orig	2006-11-16 00:23:22.000000000 +0200
+++ sawfish-1.3+cvs20061004/src/events.c	2006-11-16 00:48:48.000000000 +0200
@@ -687,31 +687,40 @@
     DEFSTRING (iconify_mod, "sawfish.wm.state.iconify");
     Window id = ev->xmaprequest.window;
     Lisp_Window *w = find_window_by_id (id);
+    int fresh = 0;
+
     if (w == 0)
     {
+	fresh = 1;
 	w = add_window (id);
-	if (w == 0)
+	if (w == 0) {
+	    fprintf (stderr, "warning: failed to allocate a window\n");
 	    return;
-
-	if (w->wmhints && w->wmhints->flags & StateHint
-	    && w->wmhints->initial_state == IconicState)
-	{
-	    rep_call_lisp1 (module_symbol_value (rep_VAL (&iconify_mod),
-						 Qiconify_window), rep_VAL(w));
 	}
     }
+
+    /* Reparent now and don't worry about it later. -- thk */
+    if (!w->reparented)
+    {
+	Fgrab_server ();
+	create_window_frame (w);
+	install_window_frame (w);
+	Fungrab_server ();
+    }
+
+    if (fresh && w->wmhints && w->wmhints->flags & StateHint
+	&& w->wmhints->initial_state == IconicState)
+    {
+	rep_call_lisp1 (module_symbol_value (rep_VAL (&iconify_mod),
+					     Qiconify_window), rep_VAL(w));
+    }
     else
     {
-	if (!w->reparented)
-	{
-	    Fgrab_server ();
-	    create_window_frame (w);
-	    install_window_frame (w);
-	    Fungrab_server ();
-	}
 	w->mapped = TRUE;
-	rep_call_lisp1 (module_symbol_value (rep_VAL (&iconify_mod),
-					     Quniconify_window), rep_VAL(w));
+	if (!fresh)
+	    rep_call_lisp1 (module_symbol_value (rep_VAL (&iconify_mod),
+						 Quniconify_window),
+			    rep_VAL(w));
     }
 
     if (!w->client_unmapped)
@@ -736,13 +745,18 @@
 	if (ev->xreparent.parent != root_window
 	    && ev->xreparent.parent != w->frame)
 	{
-	    /* Not us doing the reparenting.. */
+	    /* The window is no longer on our turf and we must not
+	       reparent it to the root. -- thk */
+	    w->reparented = FALSE;
+	    XRemoveFromSaveSet (dpy, w->id);
+
+	    /* Not us doing the reparenting. */
 	    remove_window (w, FALSE, FALSE);
-	    XReparentWindow (dpy, ev->xreparent.window, ev->xreparent.parent,
-			     ev->xreparent.x, ev->xreparent.y);
 	}
 	Fcall_window_hook (Qreparent_notify_hook, rep_VAL(w), Qnil, Qnil);
     }
+    /* What if someone else reparents windows to the root?
+       Shouldn't we adopt them?  Does it ever happen? -- thk */
 }
 
 static void
@@ -757,6 +771,10 @@
 	{
 	    /* arrgh, the window changed its override redirect status.. */
 	    remove_window (w, FALSE, FALSE);
+#if 0
+	    fprintf(stderr, "warning: I've had it with window %#lx\n",
+		    (long)(w->id));
+#endif
 	}
 	else
 	{
@@ -765,11 +783,11 @@
 	    w->attr.height = wa.height;
 
 	    w->mapped = TRUE;
+	    /* This should not happen.  The window should have been
+	       framed at the map request. -- thk */
 	    if (w->frame == 0)
-		create_window_frame (w);
-	    install_window_frame (w);
-	    if (w->visible)
-		XMapWindow (dpy, w->frame);
+		fprintf(stderr, "warning: window %#1x has no frame\n",
+			(long)(w->id));
 	    Fcall_window_hook (Qmap_notify_hook, rep_VAL(w), Qnil, Qnil);
 	}
     }
@@ -782,6 +800,9 @@
     if (w != 0 && ev->xunmap.window == w->id
 	&& (ev->xunmap.event == w->id || ev->xunmap.send_event))
     {
+	int being_reparented = FALSE;
+	XEvent reparent_ev;
+
 	w->mapped = FALSE;
 	if (w->reparented)
 	{
@@ -790,11 +811,30 @@
 		XUnmapWindow (dpy, w->frame);
 		reset_frame_parts (w);
 	    }
-	    /* Removing the frame reparents the client window back to
-	       the root. This means that we receive the next MapRequest
-	       for the window. */
-	    remove_window_frame (w);
-	    destroy_window_frame (w, FALSE);
+	    /* Careful now.  It is possible that the unmapping was
+	       caused by someone else reparenting the window.
+	       Removing the frame involves reparenting the window to
+	       the root.  Bad things may happen if we do that while
+	       a different reparenting is in progress. -- thk */
+	    being_reparented = XCheckTypedWindowEvent(dpy, w->id,
+						      ReparentNotify,
+						      &reparent_ev);
+	    if (!being_reparented)
+	    {
+	        /* Removing the frame reparents the client window back to
+		   the root. This means that we receive the next MapRequest
+		   for the window.
+
+		   It also means that if we die, the window stays
+		   unmapped.  This is correct for withdrawn windows
+		   while iconic windows should be mapped, which
+		   happens if they are in their frames and in the save
+		   set.  Iconic windows should therefore be kept
+		   mapped: iconification should only unmap the
+		   frame. -- thk */
+		remove_window_frame (w);
+		destroy_window_frame (w, FALSE);
+	    }
 	}
 	Fcall_window_hook (Qunmap_notify_hook, rep_VAL(w), Qnil, Qnil);
 
@@ -802,7 +842,10 @@
 
 	/* Changed the window-handling model, don't let windows exist
 	   while they're withdrawn */
-	remove_window (w, FALSE, FALSE);
+	if (being_reparented)
+	    reparent_notify(&reparent_ev);
+	else
+	    remove_window (w, FALSE, FALSE);
     }
 }
 
--- sawfish-1.3+cvs20061004/src/windows.c.orig	2006-11-16 00:23:13.000000000 +0200
+++ sawfish-1.3+cvs20061004/src/windows.c	2006-11-16 00:45:23.000000000 +0200
@@ -351,8 +351,9 @@
 	if (queued_focus_id == w->frame)
 	    queued_focus_id = w->id;
 
-	if (!w->mapped)
-	    XRemoveFromSaveSet (dpy, w->id);
+	/* Now that the window is a child of the root it does not need
+	   to be in the save set anymore.  -- thk */
+	XRemoveFromSaveSet (dpy, w->id);
     }
 }
 
@@ -514,8 +515,9 @@
     return w;
 }
 
-/* Remove W from the managed windows. If DESTROYED is nil, then the
-   window will be reparented back to the root window */
+/* Remove W from the managed windows. If DESTROYED is nil and
+   the window is currently reparented by us, it will be reparented back to
+   the root window */
 void
 remove_window (Lisp_Window *w, bool destroyed, bool from_error)
 {