[Xcb] Request for Review: XKB GSoC merge

[Josh Triplett]

On Thu, Sep 23, 2010 at 03:22:05PM -0400, Peter Harris wrote:
> I've been working on merging the XKB GSoC work (thanks Christoph). I've
> found and fixed a few issues, but it could use another set of eyes.
> 
> http://cgit.freedesktop.org/~peterh/xcbproto/
> http://cgit.freedesktop.org/~peterh/libxcb/
> 
> Please review, and let us know what you think. I plan to commit these
> changes in the next week or so, unless somebody can come up with a
> reason not to.

Do these changes add additional API to libxcb or the extension libraries
other than libxcb-xkb, or do they only affect extensions which use the
new constructs such as switch?  If the latter, then that seems to
mitigate potential issues with the API, since if absolutely necessary we
can always rev libxcb-xkb's API without affecting other users of libxcb.
Perhaps we shold announce XKB support in the next release as somewhat
experimental, and get a wider audience to provide feedback on it when
they go to use it?

Some comments below, but don't take any of these as objections, just
thoughts for discussion.

The way the new API reorders all fixed-size fields before all
variable-size fields makes me a bit uncomfortable, since normally XCB's
structures match the wire protocol exactly, which among other things
lets us just claim the protocol documentation as 1:1 documentation for
our protocol layer.  It still represents a fairly systematic difference
that people could learn, but I wonder how much it helps over just adding
accessor functions for fixed-length fields (treating them like
variable-length fields).  Then again, *that*, and the handling of
variable-length fields, already represents a systematic difference
between XCB and the wire protocol...

I don't quite understand the _aux variants of the request functions, and
I hesitate to add to the combinatorial explosion we started with
_checked/_unchecked.  Can you give an example of a request and the
corresponding functions, and why _aux proves sufficiently simpler to
warrant an extra pair of functions for each request?

I also don't fully understand the difference between _unserialize and
_unpack functions.

doc/xkb_issues mentions issues with lists of variable-length types
distinguished by a common "type" field, and describes those as
unsupported, apparently because the definitions for the "type" field
don't exist in the wire protocol spec?  Does any technical reason exist
why the current protocol spec couldn't support that, or does this just
need documentation (possibly by inspecting the existing XKB
implementation)?  Should this get done before merging?  Would it affect
the API or just augment the API to have this done?

docs/xkb_issues also mentions that "There are still some bugs in
xkb.xml: Either certain fields are missing that are required by the
protocol, or Xlib simply has another understanding of the protocol."  It
sounds like the protocol specification just never matched what the
actual code did (not that shocking).  Do we know the set of
discrepancies, or do we still need to discover them?  Does this issue
still apply, or did it get fixed?  If it still applies, should we fix it
before merging?

- Josh Triplett