[PATCH resend 2/3] Make GC clientClip always be a region.

[Aaron Plattner]

On 09/28/2011 12:58 PM, Jamey Sharp wrote:
> On Wed, Sep 28, 2011 at 11:48:38AM -0700, Aaron Plattner wrote:
>> On 09/26/2011 10:53 PM, Jamey Sharp wrote:
>>> -      for (i = nRects; i-->   0; ) {
>>> ...
>>> +    for (i = nRects; i-->   0; ) {
>>
>> Whitespace error: "i-->   0"
>
> The patch got mangled somewhere in transit. In my git tree, and in
> Patchwork, it is correct: "i-->  0".
>
> http://patchwork.freedesktop.org/patch/7362/
>
> Your reply has more places where whitespace mysteriously moved around
> operators, too...

Aargh, you're right.  I blame MS Exchange -- it converts incoming utf-8 
messages to base64 and breaks them in the process, so I wonder if it did 
something similar to your patch.  Did you send it with 
Content-Transfer-Encoding: quoted-printable, or is that Exchange's dirty 
work too?

I really need to subscribe from my GMail account too...

>>> +    free((char *) pRects);
>>
>> While you're at it, could you remove this cast please?
>
> I didn't really want to make unrelated changes, but I sympathize, so
> I've amended this locally.
>
>>> +  } else
>>> +    XSetClipMask(xnestDisplay, xnestGC(pGC), None);
>>
>> I'm wavering between asking you to fix the indentation since you're
>> replacing the whole function anyway, but it would make the file
>> inconsistent.  Better to change it all at one go, I guess.
>
> Yeah, I don't think that belongs in this commit. Actually, I want to
> delete Xnest entirely, along with all the other non-xfree86 DDXes. :-)

Yeah, if xf86-video-nested actually works, I'm on board with that.  (I 
haven't tested it).

>>> diff --git a/include/gcstruct.h b/include/gcstruct.h
>>> index fb9ee0d..ed598fc 100644
>>> --- a/include/gcstruct.h
>>> +++ b/include/gcstruct.h
>>> @@ -278,13 +278,12 @@ typedef struct _GC {
>>>       unsigned int       arcMode : 1;
>>>       unsigned int       subWindowMode : 1;
>>>       unsigned int       graphicsExposures : 1;
>>> -    unsigned int       clientClipType : 2; /* CT_<kind>   */
>>
>> This leaves a misleading comment in gc.h:
>>
>> /* clientClipType field in GC */
>> #define CT_NONE                 0
>> #define CT_PIXMAP               1
>> #define CT_REGION               2
>> [...]
>
> True. I've amended the following patch locally; does it help?
>
> diff --git a/include/gc.h b/include/gc.h
> index 2079cfa..a28b419 100644
> --- a/include/gc.h
> +++ b/include/gc.h
> @@ -54,7 +54,7 @@ SOFTWARE.
>   #include "screenint.h"	/* for ScreenPtr */
>   #include "pixmap.h"	/* for DrawablePtr */
>
> -/* clientClipType field in GC */
> +/* clip type argument for ChangeClip */
>   #define CT_NONE			0
>   #define CT_PIXMAP		1
>   #define CT_REGION		2

Yes, thanks.

> The defines are also used for the clientClipType field in the Picture
> structure, which might call for a similar patch someday, but I don't
> think that calls for a mention in gc.h.

Agreed.

Reviewed-by: Aaron Plattner <aplattner at nvidia.com>