[Xcb] Next Release?

[Daniel Martin]

On Fri, Oct 11, 2013 at 06:39:27PM +0900, Arnaud Fontaine wrote:
> Peter Harris <pharris at opentext.com> writes:
> > On 2013-10-04 06:47, Arnaud Fontaine wrote:
> >> Peter Harris <pharris at opentext.com> writes:
> >> 
> >>> My short-term plan, if I ever get a free minute, is to convert the
> >>> events into structs (deleting the type field) and add one event that
> >>> contains the type and a union with all the structs.
> >> 
> >> Done by Daniel Martin but not merged yet:
> >> http://lists.freedesktop.org/archives/xcb/2013-August/008541.html
> >> 
> >> Should it be merged?
> >
> > Yes, plesae.
> >
> > It appears I forgot to review Daniel's patch at the time. I'll do so now:
> >
> > Reviewed-by: Peter Harris <pharris at opentext.com>
> >
> >> * [Bug 68387] New: xinput2 XIQueryDevice reply not read correctly
> >>   https://bugs.freedesktop.org/show_bug.cgi?id=68387
> >
> > Yes, please. Looks like Ran Benita both reviewed and tested this patch.
> 
> Concerning this patch and the ones above, Daniel Martin asked me to hold
> off a week to see if can finish his parser rewrite to handle
> <switch>/<case> in XML description. As this rewrite would break the API,
> I think it's better to wait a week and see how it goes...

Due to (personal) distractions I'm unable to make it usable anytime
soon. Sorry for that.

But, I sat down and had a look at that patches and had a look if we
can get rid of "xkb: Comment out Doodads" with the patch attached to bug
#68387 "xinput2 XIQueryDevice reply not read correctly".
While doing so I've found more issues in the generated xkb code. Those
issues seem to be a real show stoper, which we can't comment out that
easily. More on this at the bottom.

> Therefore I have not merged the following patches:
> 
> * [Bug 68387] New: xinput2 XIQueryDevice reply not read correctly
>   https://bugs.freedesktop.org/show_bug.cgi?id=68387

This seem to be the right thing. As Ran mentioned in the bug the size of
lists is correct afterwards. Unit tests for other lists might be nice.
Anyone knows of simple to run applications using xcb and heavily using
lists out of the head?

> * [PATCH proto 1/3] xkb: Comment out Doodads
>   http://lists.freedesktop.org/archives/xcb/2013-August/008539.html
>   => Reviewed-By: Ran Benita <ran234 at gmail.com>
> 
>   * [PATCH proto 3/3] xkb: Unify events into single event
>     http://lists.freedesktop.org/archives/xcb/2013-August/008541.html
>     => Tested-By: Ran Benita <ran234 at gmail.com>

I don't like the patch. If we add it know we'll have an API break when
replacing the union with a switch/case. Which we can't do atm. due to my
fault.

Would it be an option to have a planned "break a lot of things - 2.0"
release? There're other things that could be changed even atm., i.e. the
valueparams in xproto usually could've been switch/bitcases.

> * [PATCH 1/1] XKB: Fix values of AXFBOpt enum
>   http://lists.freedesktop.org/archives/xcb/2013-September/008668.html
>   => Reviewed-By: Ran Benita <ran234 at gmail.com>
> 
>   * [Xcb] [PATCH 2/1] XKB: Rewrite AXOption
>     http://lists.freedesktop.org/archives/xcb/2013-September/008669.html
>     => Reviewed-By: Ran Benita <ran234 at gmail.com>

Those 2 patches can be merged too.

> Please correct me if I have misunderstood something.
> 
> Moreover, As per Daniel's request, I have also merged these patches:
> https://github.com/bartsch/xcb-proto/tree/next-for-master
> 
> >> * [Bug 69118] New: Unix spool is located at /var/spool/sockets/X11/... or /usr/spool/sockets/X11/... under HPUX
> >>   https://bugs.freedesktop.org/show_bug.cgi?id=69118
> >
> > Yes, please. The "Simpler patch." version clearly can't hurt anybody not
> > using HPUX. And anybody still using HPUX is used to hurt by now.
> 
> Merged, thanks.

Thanks for merging.

Some more problems with xkb:

First, the summary if you don't want to read the whole story: It looks
like _every_ structure having a fixed size field after a variadic
field/list is broken! Most other extensions are not affected as they've
the variadic stuff at the end of a structure - not intermixed with fixed
size fields.

Most and easy example - xkb/struct[name=Property]:
    http://cgit.freedesktop.org/xcb/proto/tree/src/xkb.xml#n558

    This struct has a
    - fixed size field (nameLength)
    - variadic list (name)
    - fixed size field (valueLength)
    - variadic list (value).

    Now, look at the generated structure:
    typedef struct xcb_xkb_property_t {
        uint16_t nameLength;
        uint16_t valueLength; <-- doesn't belong here
    } xcb_xkb_property_t;

    valueLength is not supposed to show up in the generated structure!
    The generator should've created an accessor for this field (taking
    the prior 'name' list into account. Event worse in this case, the
    default naming scheme within the code generator would choose a name
    for the accessor which conflicts with length function for the
    'value' list. Both would be called xcb_xkb_property_value_length.
    (Yes, valueLength and nameLength need to be renamed, but imho the
    generator doesn't detect such conflicts.)

Another example I just briefly point at - xkb/request[name=GetKbdByName]:
    http://cgit.freedesktop.org/xcb/proto/tree/src/xkb.xml#n2032

    typedef struct xcb_xkb_get_kbd_by_name_request_t {
        uint8_t               major_opcode;
        uint8_t               minor_opcode;
        uint16_t              length;
        xcb_xkb_device_spec_t deviceSpec;
        uint16_t              need;
        uint16_t              want;
        uint8_t               load;
        uint8_t               pad0;
        uint8_t               keymapsSpecLen;

        keymapsSpecLen is the last fixed size field and the last field
        having a fixed position in the request. Everything below is
        wrong.

        uint8_t               keycodesSpecLen;
        uint8_t               typesSpecLen;
        uint8_t               compatMapSpecLen;
        uint8_t               symbolsSpecLen;
        uint8_t               geometrySpecLen;
    } xcb_xkb_get_kbd_by_name_request_t;

    I haven't tried to send the request. It could work until you specify
    some spec field/name. But, the problem remains.


Atm. I'm looking at c_client.py how to fix that. (And to compensate my
lack of a rewrite so far.)

Cheers,
    Daniel