Code review comment for lp:~azzar1/compiz/fix-781931

Revision history for this message
Andrea Azzarone (azzar1) wrote :

> Ah okay, now that I've had a look into this, this comment here is misleading:
>
> /* check whether we're allowed to stack under a sibling by finding
> * the above 'sibling' and checking whether or not we're allowed
> * to stack under that - if not, then there is no valid sibling
> * underneath it */
>
> It really should say
>
> /* check whether we're allowed to stack under a sibling checking if
> * this window, or any other window above it is permitted to stack
> * above this window */
>

I agree. I will update the comment too.

>
> So I think you're definitely on the right track.
>
> My only concern is that changing pendingMaps might have some unwanted side
> effects. Code that relies on pendingMaps could assume that show () can been
> called on the window, which has a few other invariants too (eg, not hidden,
> not shaded etc).
>
> What might be better is to add a new variable to track whether or not the
> window is currently in map processing (eg, receivedMapRequestAndAwaitingMap).
> Then you can check that variable in avoidStackingRelativeTo in order to
> disregard the fact that the window is actually unmapped. Ideally
> receivedMapRequestAndAwaitingMap should only be set if we were to actually
> call show () on the window later (eg, !initiallyMinimized && !(priv->state &
> CompWindowStateHiddenMask))

Thank you for the review/suggestion. It shounds good to me and make the code more understandable.

Btw I'm working on an xorg-test.

« Back to merge proposal