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

[Uli Schlachter]

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.

> +            /* 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.


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.

Uli
who has no clue about keyboards
-- 
"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."