[Xcb] [PATCH libxkbcommon 1/2 v2] x11: add XKB protocol keymap and state creation support

[Ran Benita]

On Thu, Jan 16, 2014 at 05:57:41PM +0100, Uli Schlachter wrote:
> On 15.01.2014 23:35, Ran Benita wrote:
> [...]
> > diff --git a/src/x11/util.c b/src/x11/util.c
> > new file mode 100644
> > index 0000000..58f2024
> > --- /dev/null
> > +++ b/src/x11/util.c
> > @@ -0,0 +1,210 @@
> > +/*
> > + * Copyright © 2013 Ran Benita
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include "x11-priv.h"
> > +
> > +XKB_EXPORT int
> > +xkb_x11_setup_xkb_extension(xcb_connection_t *conn,
> > +                            uint16_t major_xkb_version,
> > +                            uint16_t minor_xkb_version,
> > +                            enum xkb_x11_setup_xkb_extension_flags flags,
> > +                            uint16_t *major_xkb_version_out,
> > +                            uint16_t *minor_xkb_version_out,
> > +                            uint8_t *base_event_out,
> > +                            uint8_t *base_error_out)
> > +{
> > +    uint8_t base_event, base_error;
> > +    uint16_t server_major, server_minor;
> > +
> > +    if (flags & ~(XKB_X11_SETUP_XKB_EXTENSION_NO_FLAGS)) {
> > +        /* log_err_func(ctx, "unrecognized flags: %#x\n", flags); */
> > +        return 0;
> > +    }
> > +
> > +    {
> > +        const xcb_query_extension_reply_t *reply =
> > +            xcb_get_extension_data(conn, &xcb_xkb_id);
> > +        if (!reply) {
> 
> This is missing a "|| !reply->is_present". Otherwise, the xcb connection goes
> into an error state when xcb fails at figuring out XKB's major number for
> sending the UseExtension request.

Right, fixed.

> > +            /* log_err_func(ctx, "failed to query for XKB extension\n"); */
> > +            return 0;
> > +        }
> > +
> > +        base_event = reply->first_event;
> > +        base_error = reply->first_error;
> > +    }
> > +
> > +    {
> > +        xcb_generic_error_t *error = NULL;
> > +        xcb_xkb_use_extension_cookie_t cookie =
> > +            xcb_xkb_use_extension(conn, major_xkb_version, minor_xkb_version);
> > +        xcb_xkb_use_extension_reply_t *reply =
> > +            xcb_xkb_use_extension_reply(conn, cookie, &error);
> > +
> > +        if (!reply) {
> > +            /* log_err_func(ctx, */
> > +            /*              "failed to start using XKB extension: error code %d\n", */
> > +            /*              error ? error->error_code : -1); */
> > +            free(error);
> > +            return -1;
> [...]
> 
> I think you meant "return 0" here.
> 
> I have a patched version of xtrace laying around that answers all QueryExtension
> requests negatively. With this I found the above two issues:
> 
> lt-x11: test/x11.c:59: main: Assertion `device_id != -1' failed.

That's useful, I didn't really think to test it (and I still haven't,
though I will :)
But with your suggestions the x11 test will be skipped.

> 
> This makes me wonder about one bit of the commit message:
> 
> > This opens up the possibility for X clients to use
> > xcb + xcb-xkb + xkbcommon as a proper replacement for Xlib + xkbfile for
> > keyboard support.
> 
> I am not entirely sure, but doesn't Xlib transparently make things work even if
> the XKB extension is missing?
> 
> If yes: I have no clue about the details, but could something like this be
> possible with this code as well? Otherwise, I doubt a lot that any application
> using this new code will work on servers that lack the XKB extension (and I
> doubt many of them will provide helpful error messages). At least I would like
> to see the error case of xkb_x11_setup_xkb_extension() to be documented more
> clearly. To make it clear that its result really must be checked and that it
> only works if the server supports XKB.

Yes, you are right. If XKB is unavailable, Xlib falls back to the core
protocol. We don't do any of that. I am tempted to say we don't care
about that, legacy, etc. But it might be possible to do something
reasonable (maybe borrow code from xcb-keysym). I will have to look
this over. However in the mean time I've added a note to the
documentation, though I've nothing much to say other than "find some
other solution if you care about this".

> Uli
> who has no clue about keyboards

Thanks a lot for you comments.
Ran

> -- 
> "In the beginning the Universe was created. This has made a lot of
>  people very angry and has been widely regarded as a bad move."