[Xcb] Request for Review: XKB GSoC merge

[Peter Harris]

Sorry for the latency.

I don't have as much XCB time as I'd like, and I was hoping the GSoC
Student and/or Mentor would comment on some of these issues.

Thanks for the review.

On 2010-09-23 17:11, Josh Triplett wrote:
> 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?

In terms of API, it only changes extensions that use new constructs such
as switch, and extensions that use (previously unsupported)
static/variable/static/variable interleaved encoding. From the diffs,
that's just Xprint in addition to XKB.

It changes slightly the way calculations are done in the iterators, but
my (admittedly minimal) testing indicates that the results are
equivalent. It does add *_sizeof functions, but they aren't doxygened.
I'd mark them "hidden", but some of the xproto ones are called from
outside xproto.

I'm mildly worried about adding bloat here, but not as much as I'm
worried about the lack of libxcb-xkb.

> 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?

Probably.

> 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...

Basically, we need go back in time and object to proto/xkbproto.git
before it is made official. No matter what we do with the current
protocol, it's going to hurt.

> I don't quite understand the _aux variants of the request functions

See http://cgit.freedesktop.org/xcb/util/tree/aux/xcb_aux.c#n167

I suggest that xcb/util/xcb_aux should be deprecated in favour of
auto-generating the *_aux functions from real protocol description.

...which reminds me, I was going to (temporarily) revert 9895cf56 "turn
valueparam in CreateWindow request into switch", in order to focus on
XKB. This change (along with converting the rest of the <valueparam>s
into <switch>s) can be done at a later date.

>, 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?

Unlike CreateWindow/ConfigureWindow/etc, the XKB functions take complex
nested structures. We really don't want to be writing xcb_xkb_*_aux by
hand. At least, I don't.

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

_unserialize changes fixed/variable/fixed/variable into
fixed/variable/variable. It doesn't change the length of the data.

_unpack does the opposite of the _aux functions (See above), and changes
a packed switch on the wire into a fixed size (larger) struct.

I was hoping we could avoid protocol discussion now, and just work on
bugs. I thought most of this was hashed out in June (eg.
http://lists.freedesktop.org/archives/xcb/2010-June/006143.html and
surrounding thread).

> 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?

It's because our <union>, like a C's union, is fixed size. XKB's "union"
has different lengths of data based on the 'type' field.

This should be a simple (famous last words) matter of implementing it as
a <switch> in XCB, although it will need <case> added to <bitcase> as a
possible <switch> element.

> Should this get done before merging?

Volunteer needed.

>  Would it affect
> the API or just augment the API to have this done?

I don't believe it would affect the API.

> 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?

I'm not aware of any remaining discrepancies, aside from <union
name="Doodad"> (see variable length union comment above) - I've been
checking in fixes as I find them. I'd be shocked if I've found all of
them, though.

Perhaps the GSoC Student or the GSoC Mentor could let us know if they've
found any further problems.

Peter Harris
-- 
               Open Text Connectivity Solutions Group
Peter Harris                    http://connectivity.opentext.com/
Research and Development        Phone: +1 905 762 6001
pharris at opentext.com            Toll Free: 1 877 359 4866