[PATCH xserver 2/7] mi: miValidateTree based on paintable not viewable

[Keith Packard]

Adam Jackson <ajax at redhat.com> writes:

Reading through this, I don't see what this patch ends up changing --
TreatAsTransparent will always return True for unmapped Always windows,
so every place you're checking paintable && !TreatAsTransparent, you
could equivalently be checking viewable && !TreatAsTrasparent without
having changed TreatAsTransparent.

What needs changing is that miComputeClips needs to be called for
paintable windows, not just viewable windows; everything else seems like
it doesn't need changing at all?

> +    }
> +    else if (pParent->paintable)
>          newVis = VisibilityFullyObscured;
> -        break;
> +    else {
> +        newVis = VisibilityNotViewable;
>      }

This isn't right -- unmapped windows should always be NotViewable, as
seen through the protocol.

>      pParent->visibility = newVis;
> -    if (oldVis != newVis &&
> +    if (pParent->realized &&
> +        oldVis != newVis &&
>          ((pParent->
>            eventMask | wOtherEventMasks(pParent)) & VisibilityChangeMask))
>          SendVisibilityNotify(pParent);
> @@ -293,14 +303,13 @@ miComputeClips(WindowPtr pParent,
>               (oldVis == VisibilityUnobscured))) {
>              pChild = pParent;
>              while (1) {
> -                if (pChild->viewable) {
> +                if (pChild->paintable) {
>                      if (pChild->visibility != VisibilityFullyObscured) {
>                          RegionTranslate(&pChild->borderClip, dx, dy);
>                          RegionTranslate(&pChild->clipList, dx, dy);

And this is going to end up wrong -- I think we need to ignore
visibility for purposes of computing clip values here, and just check
for empty clip lists? In any case, the use of visibility to optimize
clip computations is going to serve us poorly, especially if we ever
want to fix it with composite windows.

>              for (; pChild; pChild = pChild->nextSib) {
> -                if (pChild->viewable && !TreatAsTransparent(pChild))
> +                if (pChild->paintable && !TreatAsTransparent(pChild))
>                      RegionAppend(&childUnion, &pChild->borderSize);

This should be checking viewable, in which case you don't need to change
TreatAsTransparent
>              for (pChild = pParent->lastChild; pChild; pChild = pChild->prevSib) {
> -                if (pChild->viewable && !TreatAsTransparent(pChild))
> +                if (pChild->paintable && !TreatAsTransparent(pChild))
>                      RegionAppend(&childUnion, &pChild->borderSize);

Same here.

>              }
>          }
>          RegionValidate(&childUnion, &overlap);
>  
>          for (pChild = pParent->firstChild; pChild; pChild = pChild->nextSib) {
> -            if (pChild->viewable) {
> +            if (pChild->paintable) {
>                  /*
> -                 * If the child is viewable, we want to remove its extents
> +                 * If the child is paintable, we want to remove its extents
>                   * from the current universe, but we only re-clip it if
>                   * it's been marked.

I'm not sure you need to use paintable here -- unmapped paintable
windows will never want to be removed from their parent anyways, so this
is equivalent to viewable.

>                   */
> @@ -564,7 +573,7 @@ miValidateTree(WindowPtr pParent,       /* Parent to validate */
>      ScreenPtr pScreen;
>      WindowPtr pWin;
>      Bool overlap;
> -    int viewvals;
> +    int paintables = 0;

I think viewvals is correct here; the only windows affecting their
parent clip list are those which are viewable; unmapped paintable
windows have no impact on their parents.

-- 
-keith
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20180724/f900f606/attachment.sig>