Thanks Evan, that looks great. Two questions:
On Thu, Jul 28, 2011 at 3:50 PM, Evan Broder <[email protected]> wrote:
> Previously, compiz erased its record of a window's parent frames when
> it got the DestroyNotify for that window, which could leave it with no
> way to process a stacking update relative to that frame window.
>
> Instead, when unparenting a window, set a StructureNotify on the frame
> and leave priv->frame set until the DestroyNotify comes back.
>
> The approximate race is:
>
> * compiz receives ConfigureRequest(w=A, {sibling=B, stack=Above})
> * compiz calls XConfigureWindow(w=A's-frame, {sibling=B's frame,
> stack=Above})
> * compiz receives DestroyNotify(w=B)
> * compiz calls XDestroyWindow(w=B's frame)
> * compiz unsets w->priv->frame
> * compiz receives ConfigureNotify(w=A's frame, {sibling=B's frame,
> stack=Above})
> * compiz is unable to process ConfigureNotify, because no window has
> priv->id == B's frame || priv->frame == B's frame
>
> This may have issues since priv->frame will be set and priv->destroyed
> will be false when priv->id is 0 and the window itself has been
> destroyed, but in casual usage I haven't run into any of them
>
> Signed-off-by: Evan Broder <[email protected]>
> ---
> src/event.cpp | 11 +++++++++++
> src/window.cpp | 45 ++++++++++++++++++++++++++-------------------
> 2 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/src/event.cpp b/src/event.cpp
> index c79cd2b..05ee4e8 100644
> --- a/src/event.cpp
> +++ b/src/event.cpp
> @@ -1104,6 +1104,17 @@ CompScreen::handleEvent (XEvent *event)
> w->moveInputFocusToOtherWindow ();
> w->destroy ();
> }
> + else
> + {
> + w = findTopLevelWindow (event->xdestroywindow.window);
> + if (w && event->xdestroywindow.window == w->frame())
> + {
> + w->priv->frame = None;
> + if (w->priv->id == 1)
> + w->destroy();
> + }
> + }
> +
As an additional safety measure and sanity check, it would be good to
check for w->priv->destroyRefCnt as well as w->priv->id == 1.
Perhaps CompWindow::destroyed should return something like:
return priv->destroyed || (priv->id == 1 && priv->destroyRefCnt)
I am not sure of the implications of this though, since plugins using
the API may be assuming that the window is actually no longer valid
and pending destruction, rather than having just recieved a
DestroyNotify event.
> break;
> case MapNotify:
>
> diff --git a/src/window.cpp b/src/window.cpp
> index ee77619..026ec20 100644
> --- a/src/window.cpp
> +++ b/src/window.cpp
> @@ -1203,20 +1203,22 @@ CompWindow::destroy ()
> priv->id = 1;
> priv->mapNum = 0;
>
> + if (priv->frame)
> + {
> + priv->unreparent ();
> + return;
> + }
> +
It's probably best to save the unreparenting step until the window is
actually destroyed (eg, not in use by any animations). Plugins might
need to use data that is stored on the frame window (such as the
geometry). As such, I would suggest unmapping the frame and wrapper
window and saving the unreparent step until after destroyRefCnt
reaches zero.
Also, returning after reparenting here is dangerous, if the window was
only destroyed and there is not an animation running (eg destroyRefCnt
is never incremented on CompWindowNotifyDestroy in ::windowNotify)
then PrivateScreen::pendingDestroys; will not be incremented and the
window will not be removed from the list.
> priv->destroyRefCnt--;
> if (priv->destroyRefCnt)
> return;
>
> -
> if (!priv->destroyed)
> {
> priv->destroyed = true;
> screen->priv->pendingDestroys++;
> }
>
> - if (priv->frame)
> - priv->unreparent ();
> -
> }
>
> void
> @@ -1667,23 +1669,26 @@ PrivateWindow::configureFrame (XConfigureEvent *ce)
> if (!priv->frame)
> return;
>
> - x = ce->x + priv->input.left;
> - y = ce->y + priv->input.top;
> - width = ce->width - priv->serverGeometry.border () * 2 -
> priv->input.left - priv->input.right;
> - height = ce->height - priv->serverGeometry.border () * 2 -
> priv->input.top - priv->input.bottom;
> -
> - if (priv->syncWait)
> - {
> - priv->syncGeometry.set (x, y, width, height, ce->border_width);
> - }
> - else
> + if (priv->id != 1)
> {
> - if (ce->override_redirect)
> + x = ce->x + priv->input.left;
> + y = ce->y + priv->input.top;
> + width = ce->width - priv->serverGeometry.border () * 2 -
> priv->input.left - priv->input.right;
> + height = ce->height - priv->serverGeometry.border () * 2 -
> priv->input.top - priv->input.bottom;
> +
> + if (priv->syncWait)
> {
> - priv->serverGeometry.set (x, y, width, height, ce->border_width);
> + priv->syncGeometry.set (x, y, width, height, ce->border_width);
> }
> + else
> + {
> + if (ce->override_redirect)
> + {
> + priv->serverGeometry.set (x, y, width, height,
> ce->border_width);
> + }
>
> - window->resize (x, y, width, height, ce->border_width);
> + window->resize (x, y, width, height, ce->border_width);
> + }
> }
See above with regards to checking priv->id == 1 && priv->destroyRefCnt
>
> if (priv->restack (ce->above))
> @@ -6068,9 +6073,11 @@ PrivateWindow::unreparent ()
> XFree (children);
>
> XDestroyWindow (dpy, wrapper);
> - XDestroyWindow (dpy, frame);
> wrapper = None;
> - frame = None;
> +
> + /* We want to know when the frame is actually destroyed */
> + XSelectInput (dpy, frame, StructureNotifyMask);
> + XDestroyWindow (dpy, frame);
We select for SubstructureNotifyMask on the root window, I don't think
calling XSelectInput here on the frame makes sense (unless we are not
receiving that event, which is a problem).
>
> window->windowNotify (CompWindowNotifyUnreparent);
> }
> --
> 1.7.4.3
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://lists.compiz.org/mailman/listinfo/dev
>
--
Sam Spilsbury
_______________________________________________
dev mailing list
[email protected]
http://lists.compiz.org/mailman/listinfo/dev
