Merge lp:~compiz-team/compiz-core/compiz-core.fix_864330 into lp:compiz-core/0.9.5

[Sam Spilsbury]

Rework the event tracking system slightly to cover more cases where events would
not be recorded properly:

1. When determining restack requests relative to another window which is pending
   a restack, check to see if we already requested to restack relative to it
   after it was pending the restack, rather than unconditionally submitting
   the restack and risking that being a no-op
2. When a window gets closed, its frame window is marked "None" immediately
   so we must discard tracking of any remaining pending configure events
   as they wont come to the frame window as we know it
3. Remove the static timeout and replace it with a timer object that clears up
   any stale configure requests which didn't get matched to events (note that
   this will be removed, but exists only to safeguard against performance regressions
   with the current API)
4. Make the move plugin lock down server geometry updates whenever it receives the
   "lock_position" signal on either the frame or the client window
5. Fix a problem with maximizing undecorated windows (resized to height = 0)

[Jason Smith (jassmith)]

code looks good, testing hasn't yet revealed any issues

review: Approve

=== modified file 'plugins/move/src/move.cpp'
--- plugins/move/src/move.cpp	2011-09-29 03:29:41 +0000
+++ plugins/move/src/move.cpp	2011-10-11 09:53:25 +0000
@@ -525,7 +525,7 @@
 		Window xid = CompOption::getIntOptionNamed (options, "window", 0);
 		int    lock = CompOption::getIntOptionNamed (options, "active", 0);
 
-		if (xid == w->id ())
+		if (xid == ROOTPARENT (w))
 		    MoveWindow::get (w)->mLocked = lock ? true : false;
 	    }
 	}

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2011-10-04 11:28:15 +0000
+++ src/privatescreen.h	2011-10-11 09:53:25 +0000
@@ -111,6 +111,8 @@
     virtual ~PendingEvent ();
 
     virtual bool match (XEvent *);
+    unsigned int serial () { return mSerial; } // HACK: will be removed
+    virtual void dump ();
 
     typedef boost::shared_ptr<PendingEvent> Ptr;
 
@@ -131,6 +133,8 @@
 
     virtual bool match (XEvent *);
     bool matchVM (unsigned int valueMask);
+    bool matchRequest (XWindowChanges &xwc, unsigned int);
+    virtual void dump ();
 
     typedef boost::shared_ptr<PendingConfigureEvent> Ptr;
 
@@ -154,6 +158,8 @@
     bool match (XEvent *);
     bool pending ();
     bool forEachIf (boost::function <bool (compiz::X11::PendingEvent::Ptr)>);
+    void clear () { mEvents.clear (); } // HACK will be removed
+    void dump ();
 
 protected:
     bool removeIfMatching (const PendingEvent::Ptr &p, XEvent *);

=== modified file 'src/privatewindow.h'
--- src/privatewindow.h	2011-10-04 11:28:15 +0000
+++ src/privatewindow.h	2011-10-11 09:53:25 +0000
@@ -211,7 +211,7 @@
 
 	void readIconHint ();
 
-	void addPendingConfigure (XWindowChanges &, unsigned int);
+	bool checkClear ();
 
     public:
 
@@ -293,6 +293,7 @@
 	typedef std::pair <XWindowChanges, unsigned int> XWCValueMask;
 
 	compiz::X11::PendingEventQueue pendingConfigures;
+	CompTimer                     mClearCheckTimeout;
 	bool pendingPositionUpdates;
 
 	char *startupId;

=== modified file 'src/window.cpp'
--- src/window.cpp	2011-10-08 11:38:21 +0000
+++ src/window.cpp	2011-10-11 09:53:25 +0000
@@ -897,13 +897,16 @@
 
 	    }
 
-	    gettimeofday (&lastConfigureRequest, NULL);
 	    compiz::X11::PendingEvent::Ptr pc =
 		    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 									      new compiz::X11::PendingConfigureEvent (
 										  screen->dpy (), serverFrame, valueMask, &xwc)));
 
 	    pendingConfigures.add (pc);
+	    if (priv->mClearCheckTimeout.active ())
+		priv->mClearCheckTimeout.stop ();
+	    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+					    2000, 2500);
 
 	    XSendEvent (screen->dpy (), screen->root (), false,
 			SubstructureNotifyMask, (XEvent *) &xev);
@@ -913,13 +916,16 @@
 	}
 	else
 	{
-	    gettimeofday (&lastConfigureRequest, NULL);
 	    compiz::X11::PendingEvent::Ptr pc =
 		    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 									      new compiz::X11::PendingConfigureEvent (
 										  screen->dpy (), serverFrame, valueMask, &xwc)));
 
 	    pendingConfigures.add (pc);
+	    if (priv->mClearCheckTimeout.active ())
+		priv->mClearCheckTimeout.stop ();
+	    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+					    2000, 2500);
 	    XConfigureWindow (screen->dpy (), serverFrame, valueMask, &xwc);
 	}
 
@@ -951,10 +957,7 @@
 	if (shaded)
 	    xwc.height = bw;
 	else
-	    xwc.height = bw;
-
-	if (shaded)
-	    height = 0;
+	    xwc.height = serverGeometry.height () + bw;
 
 	if (serverFrameGeometry.x () == xwc.x)
 	    valueMask &= ~(CWX);
@@ -1032,13 +1035,16 @@
 
 	    }
 
-	    gettimeofday (&lastConfigureRequest, NULL);
 	    compiz::X11::PendingEvent::Ptr pc =
 		    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 									      new compiz::X11::PendingConfigureEvent (
 										  screen->dpy (), serverFrame, valueMask, &xwc)));
 
 	    pendingConfigures.add (pc);
+	    if (priv->mClearCheckTimeout.active ())
+		priv->mClearCheckTimeout.stop ();
+	    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+					    2000, 2500);
 
 	    XSendEvent (screen->dpy (), screen->root (), false,
 			SubstructureNotifyMask, (XEvent *) &xev);
@@ -1048,13 +1054,16 @@
 	}
 	else
 	{
-	    gettimeofday (&lastConfigureRequest, NULL);
 	    compiz::X11::PendingEvent::Ptr pc =
 		    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 									      new compiz::X11::PendingConfigureEvent (
 										  screen->dpy (), serverFrame, valueMask, &xwc)));
 
 	    pendingConfigures.add (pc);
+	    if (priv->mClearCheckTimeout.active ())
+		priv->mClearCheckTimeout.stop ();
+	    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+					    2000, 2500);
 
 	    XConfigureWindow (screen->dpy (), serverFrame, valueMask, &xwc);
 	}
@@ -2088,7 +2097,7 @@
 #ifdef DEBUG
 	abort ();
 #else
-	pendingConfigures = compiz::X11::PendingEventQueue (screen->dpy ());
+	pendingConfigures.clear ();
 #endif
     }
 
@@ -2199,28 +2208,13 @@
 	{
 	    XWindowChanges xwc;
 	    unsigned int   valueMask = CWX | CWY;
-	    struct timeval tv, old;
-	    compLogMessage ("core", CompLogLevelDebug, "pending configure notifies on 0x%x,"\
+	    compLogMessage ("core", CompLogLevelDebug, "pending configure notifies on 0x%x, "\
 			    "moving window asyncrhonously!", (unsigned int) priv->serverId);
 
-	    old = priv->lastConfigureRequest;
-	    gettimeofday (&tv, NULL);
-
 	    xwc.x = priv->serverGeometry.x () + dx;
 	    xwc.y = priv->serverGeometry.y () + dy;
 
 	    configureXWindow (valueMask, &xwc);
-
-	    priv->lastConfigureRequest = old;
-
-	    /* FIXME: This is a hack to avoid performance regressions
-	     * and must be removed in 0.9.6 */
-	    if (tv.tv_usec - priv->lastConfigureRequest.tv_usec > 30000)
-	    {
-		compLogMessage ("core", CompLogLevelWarn, "failed to receive ConfigureNotify event from request at %i (now: %i)\n",
-				priv->lastConfigureRequest.tv_usec, tv.tv_usec);
-		priv->pendingConfigures = compiz::X11::PendingEventQueue (screen->dpy ());
-	    }
 	}
     }
 }
@@ -2231,16 +2225,59 @@
     return !mEvents.empty ();
 }
 
+bool
+PrivateWindow::checkClear ()
+{
+    if (pendingConfigures.pending ())
+    {
+	/* FIXME: This is a hack to avoid performance regressions
+	 * and must be removed in 0.9.6 */
+	compLogMessage ("core", CompLogLevelWarn, "failed to receive ConfigureNotify event on 0x%x\n",
+			id);
+	pendingConfigures.dump ();
+	pendingConfigures.clear ();
+    }
+
+    return false;
+}
+
 void
 compiz::X11::PendingEventQueue::add (PendingEvent::Ptr p)
 {
+    compLogMessage ("core", CompLogLevelDebug, "pending request:");
+    p->dump ();
+
     mEvents.push_back (p);
 }
 
 bool
 compiz::X11::PendingEventQueue::removeIfMatching (const PendingEvent::Ptr &p, XEvent *event)
 {
-    return p->match (event);
+    if (p->match (event))
+    {
+	compLogMessage ("core", CompLogLevelDebug, "received event:");
+	p->dump ();
+	return true;
+    }
+
+    return false;
+}
+
+void
+compiz::X11::PendingEvent::dump ()
+{
+    compLogMessage ("core", CompLogLevelDebug, "- event serial: %i", mSerial);
+    compLogMessage ("core", CompLogLevelDebug,  "- event window 0x%x", mWindow);
+}
+
+void
+compiz::X11::PendingConfigureEvent::dump ()
+{
+    compiz::X11::PendingEvent::dump ();
+
+    compLogMessage ("core", CompLogLevelDebug,  "- x: %i y: %i width: %i height: %i "\
+						 "border: %i, sibling: 0x%x",
+						 mXwc.x, mXwc.y, mXwc.width, mXwc.height, mXwc.border_width, mXwc.sibling);
 }
 
 bool
@@ -2266,8 +2303,21 @@
     return false;
 }
 
+void
+compiz::X11::PendingEventQueue::dump ()
+{
+    foreach (compiz::X11::PendingEvent::Ptr p, mEvents)
+	p->dump ();
+}
+
 compiz::X11::PendingEventQueue::PendingEventQueue (Display *d)
 {
+    /* mClearCheckTimeout.setTimes (0, 0)
+     *
+     * XXX: For whatever reason, calling setTimes (0, 0) here causes
+     * the destructor of the timer object to be called twice later on
+     * in execution and the stack gets smashed. This could be a
+     * compiler bug, but requires further investigation */
 }
 
 compiz::X11::PendingEventQueue::~PendingEventQueue ()
@@ -2310,7 +2360,44 @@
 bool
 compiz::X11::PendingConfigureEvent::matchVM (unsigned int valueMask)
 {
-    return valueMask & mValueMask;
+    unsigned int result = mValueMask != 0 ? valueMask & mValueMask : 1;
+
+    return result != 0;
+}
+
+bool
+compiz::X11::PendingConfigureEvent::matchRequest (XWindowChanges &xwc, unsigned int valueMask)
+{
+    if (matchVM (valueMask))
+    {
+	if (valueMask & CWX)
+	    if (xwc.x != mXwc.x)
+		return false;
+
+	if (valueMask & CWY)
+	    if (xwc.y != mXwc.y)
+		return false;
+
+	if (valueMask & CWWidth)
+	    if (xwc.width != mXwc.width)
+		return false;
+
+	if (valueMask & CWHeight)
+	    if (xwc.height != mXwc.height)
+		return false;
+
+	if (valueMask & CWBorderWidth)
+	    if (xwc.border_width != mXwc.border_width)
+		return false;
+
+	if (valueMask & (CWStackMode | CWSibling))
+	    if (xwc.sibling != mXwc.sibling)
+		return false;
+
+	return true;
+    }
+
+    return false;
 }
 
 bool
@@ -2322,29 +2409,16 @@
     if (!compiz::X11::PendingEvent::match (event))
 	return false;
 
-    if (mValueMask & CWX)
-	if (ce->x != mXwc.x)
-	    matched = false;
-
-    if (mValueMask & CWY)
-	if (ce->y != mXwc.y)
-	    matched = false;
-
-    if (mValueMask & CWWidth)
-	if (ce->width != mXwc.width)
-	    matched = false;
-
-    if (mValueMask & CWHeight)
-	if (ce->height != mXwc.height)
-	    matched = false;
-
-    if (mValueMask & CWBorderWidth)
-	if (ce->border_width != mXwc.border_width)
-	    matched = false;
-
-    if (mValueMask & (CWStackMode | CWSibling))
-	if (ce->above != mXwc.sibling)
-	    matched = false;
+    XWindowChanges xwc;
+
+    xwc.x = ce->x;
+    xwc.y = ce->y;
+    xwc.width = ce->width;
+    xwc.height = ce->height;
+    xwc.border_width = ce->border_width;
+    xwc.sibling = ce->above;
+
+    matched = matchRequest (xwc, mValueMask);
 
     /* Remove events from the queue
      * even if they didn't match what
@@ -2440,7 +2514,6 @@
 	    xwc.x = priv->serverFrameGeometry.x ();
 	    xwc.y = priv->serverFrameGeometry.y ();
 
-	    gettimeofday (&priv->lastConfigureRequest, NULL);
 	    compiz::X11::PendingEvent::Ptr pc =
 		    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 									      new compiz::X11::PendingConfigureEvent (
@@ -2448,6 +2521,11 @@
 
 	    priv->pendingConfigures.add (pc);
 
+	    /* Got 3 seconds to get its stuff together */
+	    if (priv->mClearCheckTimeout.active ())
+		priv->mClearCheckTimeout.stop ();
+	    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+					    2000, 2500);
 	    XConfigureWindow (screen->dpy (), ROOTPARENT (this), valueMask, &xwc);
 
 	    if (priv->serverFrame)
@@ -3259,11 +3337,18 @@
     return pc->matchVM (CWStackMode | CWSibling);
 }
 
+static bool isExistingRequest (compiz::X11::PendingEvent::Ptr p, XWindowChanges &xwc, unsigned int valueMask)
+{
+    compiz::X11::PendingConfigureEvent::Ptr pc = boost::shared_static_cast <compiz::X11::PendingConfigureEvent> (p);
+
+    return pc->matchRequest (xwc, valueMask);
+}
+
 void
 PrivateWindow::reconfigureXWindow (unsigned int   valueMask,
 				   XWindowChanges *xwc)
 {
-    unsigned int frameValueMask = valueMask;
+    unsigned int frameValueMask = 0;
 
     /* Immediately sync window position
      * if plugins were updating w->geometry () directly
@@ -3273,28 +3358,37 @@
 
     /* Remove redundant bits */
 
-    if (serverGeometry.x () == xwc->x)
+    if (valueMask & CWX && serverGeometry.x () == xwc->x)
 	valueMask &= ~(CWX);
 
-    if (serverGeometry.y () == xwc->y)
+    if (valueMask & CWY && serverGeometry.y () == xwc->y)
 	valueMask &= ~(CWY);
 
-    if (serverGeometry.width () == xwc->width)
+    if (valueMask & CWWidth && serverGeometry.width () == xwc->width)
 	valueMask &= ~(CWWidth);
 
-    if (serverGeometry.height () == xwc->height)
+    if (valueMask & CWHeight && serverGeometry.height () == xwc->height)
 	valueMask &= ~(CWHeight);
 
-    if (serverGeometry.border () == xwc->border_width)
+    if (valueMask & CWBorderWidth && serverGeometry.border () == xwc->border_width)
 	valueMask &= ~(CWBorderWidth);
 
-    if (window->serverPrev && ROOTPARENT (window->serverPrev) == xwc->sibling)
+    if (valueMask & CWSibling && window->serverPrev)
     {
 	/* check if the sibling is also pending a restack,
 	 * if not, then setting this bit is useless */
-
-	if (window->serverPrev->priv->pendingConfigures.forEachIf (boost::bind (isPendingRestack, _1)))
-	    valueMask &= ~(CWSibling | CWStackMode);
+	if (ROOTPARENT (window->serverPrev) == xwc->sibling)
+	{
+	    bool matchingRequest = priv->pendingConfigures.forEachIf (boost::bind (isExistingRequest, _1, *xwc, valueMask));
+	    bool restackPending = window->serverPrev->priv->pendingConfigures.forEachIf (boost::bind (isPendingRestack, _1));
+	    bool remove = matchingRequest;
+
+	    if (!remove)
+		remove = !restackPending;
+
+	    if (remove)
+		valueMask &= ~(CWSibling | CWStackMode);
+	}
     }
 
     if (valueMask & CWBorderWidth)
@@ -3330,13 +3424,18 @@
 	    compLogMessage ("core", CompLogLevelWarn, "restack_mode not Above");
     }
 
-    if (serverFrameGeometry.x () == xwc->x - serverGeometry.border () - serverInput.left)
+    frameValueMask = valueMask;
+
+    if (frameValueMask & CWX &&
+	serverFrameGeometry.x () == xwc->x - serverGeometry.border () - serverInput.left)
 	frameValueMask &= ~(CWX);
 
-    if (serverFrameGeometry.y () == xwc->y - serverGeometry.border () - serverInput.top)
+    if (frameValueMask & CWY &&
+	serverFrameGeometry.y () == xwc->y - serverGeometry.border () - serverInput.top)
 	frameValueMask &= ~(CWY);
 
-   if (serverFrameGeometry.width () == xwc->width + serverGeometry.border () * 2
+   if (frameValueMask & CWWidth &&
+	serverFrameGeometry.width () == xwc->width + serverGeometry.border () * 2
 				      + serverInput.left + serverInput.right)
 	frameValueMask &= ~(CWWidth);
 
@@ -3346,13 +3445,15 @@
 
     if (shaded)
     {
-	if (serverFrameGeometry.height () == serverGeometry.border () * 2
+	if (frameValueMask & CWHeight &&
+	    serverFrameGeometry.height () == serverGeometry.border () * 2
 	    + serverInput.top + serverInput.bottom)
 	    frameValueMask &= ~(CWHeight);
     }
     else
     {
-	if (serverFrameGeometry.height () == xwc->height + serverGeometry.border () * 2
+	if (frameValueMask & CWHeight &&
+	    serverFrameGeometry.height () == xwc->height + serverGeometry.border () * 2
 	    + serverInput.top + serverInput.bottom)
 	    frameValueMask &= ~(CWHeight);
     }
@@ -3397,14 +3498,16 @@
 	    wc.width  = serverFrameGeometry.width ();
 	    wc.height = serverFrameGeometry.height ();
 
-	    gettimeofday (&lastConfigureRequest, NULL);
-
 	    compiz::X11::PendingEvent::Ptr pc =
 		    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 									      new compiz::X11::PendingConfigureEvent (
 										  screen->dpy (), priv->serverFrame, frameValueMask, &wc)));
 
 	    pendingConfigures.add (pc);
+	    if (priv->mClearCheckTimeout.active ())
+		priv->mClearCheckTimeout.stop ();
+	    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+					    2000, 2500);
 
 	    XConfigureWindow (screen->dpy (), serverFrame, frameValueMask, &wc);
 	}
@@ -4196,13 +4299,16 @@
 
 		if (serverFrame)
 		{
-		    gettimeofday (&lastConfigureRequest, NULL);
 		    compiz::X11::PendingEvent::Ptr pc =
 			    boost::shared_static_cast<compiz::X11::PendingEvent> (compiz::X11::PendingConfigureEvent::Ptr (
 										      new compiz::X11::PendingConfigureEvent (
 											  screen->dpy (), serverFrame, valueMask, &lxwc)));
 
 		    pendingConfigures.add (pc);
+		    if (priv->mClearCheckTimeout.active ())
+			priv->mClearCheckTimeout.stop ();
+		    priv->mClearCheckTimeout.start (boost::bind (&PrivateWindow::checkClear, priv),
+						    2000, 2500);
 		}
 
 		/* Below with no sibling puts the window at the bottom
@@ -4215,8 +4321,15 @@
 	    }
 	    else if (sibling)
 	    {
+		bool matchingRequest = priv->pendingConfigures.forEachIf (boost::bind (isExistingRequest, _1, *xwc, (CWStackMode | CWSibling)));
+		bool restackPending = window->serverPrev->priv->pendingConfigures.forEachIf (boost::bind (isPendingRestack, _1));
+		bool processAnyways = restackPending;
+
+		if (matchingRequest)
+		    processAnyways = false;
+
 		if (sibling->priv->id != window->serverPrev->priv->id ||
-		    window->serverPrev->priv->pendingConfigures.forEachIf (boost::bind (isPendingRestack, _1)))
+		    processAnyways)
 		{
 		    mask |= CWSibling | CWStackMode;
 
@@ -6295,8 +6408,6 @@
     if (dbg)
 	dbg->overrideRedirectRestack (priv->id, aboveId);
 
-    gettimeofday (&priv->lastConfigureRequest, NULL);
-
     priv->attrib = wa;
     priv->serverGeometry.set (priv->attrib.x, priv->attrib.y,
 			      priv->attrib.width, priv->attrib.height,
@@ -7229,6 +7340,12 @@
     XDestroyWindow (screen->dpy (), wrapper);
 
     window->windowNotify (CompWindowNotifyUnreparent);
+    /* This window is no longer "managed" in the
+     * reparenting sense so clear its pending event
+     * queue ... though maybe in future it would
+     * be better to bookeep these events too and
+     * handle the ReparentNotify */
+    pendingConfigures.clear ();
 
     frame = None;
     wrapper = None;