PolyText/PolyPoint fixes, mi cleanups, ChangeGC rework

Jamey Sharp jamey at minilop.net
Sat May 8 16:39:15 PDT 2010


Wow, the rabbit hole just keeps getting deeper. Here are five new
cleanup patches, two new bug fixes, and an improved version of my
previous ChangeGC rework patch series.

On Fri, May 7, 2010 at 9:33 PM, Keith Packard <keithp at keithp.com> wrote:
> I'm liking the general direction where the basic GC changing function
> takes the union instead of either union or XID list. However, I'd
> suggest that only DIX ever needs to use the XID version of this
> function, and in fact only in a very few places. The only places in the
> next patch that I see where it should be using the XID version is in
> ProcChangeGC and doPolyText.

Looking at this more closely, as far as I can tell, doPolyText doesn't
need the XID variant because it already calls dixLookupResourceByType.
It just needs to use pFont instead of fid.

So that leaves ProcChangeGC and CreateGC.

> I'd suggest that open-coding the XID lookup in those two functions
> would be more appropriate than exposing an XID-using ChangeGC
> function.
> ...
> What do you think? Crazy talk?

Not at all. :-) But I think it makes the most sense to let ProcChangeGC
and CreateGC share the guts of processing the wire-level value-list, so
I've kept an exposed ChangeGCXIDs in this reworked patch series. It's
just used only by those two functions.

Can I drop the _X_EXPORT from its prototype, and maybe move it to
dix/dispatch.h or something? If I did, would you feel better about
leaving it in?

> Further, now that we're back to a single function which changes a GC,
> how about using the venerable 'ChangeGC' name again? Nice and short.

Much better. I didn't much like the names I'd picked. :-)

By the way, I think all the ChangeGC callers pass effectively constant
value-masks. I was considering splitting ChangeGC into a separate setter
function for each field, plus a FinishChangeGC function to call
CreateDefaultTile and pGC->funcs->ChangeGC. ChangeGCXIDs would then be
the only function that needs to test all the mask bits to choose which
setters to call. I think I may be too lazy to actually try it, but does
it sound like a sensible change?

Jamey


More information about the xorg-devel mailing list