[RFC] libxkbcommon: the xkbcomp killer

[Dan Nicholson]

2009/4/23 Kristian Høgsberg <krh at bitplanet.net>:
> On Thu, Apr 23, 2009 at 9:31 AM, Dan Nicholson <dbn.lists at gmail.com> wrote:
>> Right now there are a couple difficult XKB tasks that the server forks
>> xkbcomp to accomplish. The main reason for this is that xkbcomp contains a
>> parser for XKB files and code to do intelligent things with the definitions
>> in them. However, these interactions are awful and are an endless source of
>> headaches for users and developers.
>
> Awesome, this is so cool.  I'd like to use this in Wayland, so the
> comments below are with that in mind, but there's nothing Wayland
> specific to the comments, they're generally just pushing for a bit
> stronger separation of the modules.
>
>> Since a discussion with Daniel a few months back, I've been working on
>> moving the guts of xkbcomp into a library, libxkbcommon, which exposes
>> appropriate interfaces to the server (blame Daniel if you hate the name
>> :). A secondary goal of libxkbcommon is to move XKB code duplicated in
>> the server and client libraries into a reusable location. A few repos
>> encompass the work I've done.
>>
>> http://cgit.freedesktop.org/~dbn/libxkbcommon/
>>
>> The library itself. New interfaces are in <X11/extensions/XKBcommon.h>.
>> Right now all that's really exposed are compiling keymaps and listing
>> components (i.e., satisfy the server's requirements).
>
> Looking at the configure.ac here, it looks like it requires xproto and
> kbproto?  What's the dependency there?  Could we move anything it
> needs from kbproto to libxkbcommon headers and move the Xlib specifics
> from kbproto into Xlib or libXKB?  Of course older Xlibs won't be able
> to build against the newer kbproto, but that's why we have version
> numbers.  I see you pulled in the keyname->keysym map to avoid the
> libX11 dependency, nice.

Well, I really didn't want have another set of duplicated xkb
datatypes. Take a look at XkbDescPtr in XKBstr.h. Not only does it
pull in a ton of other structures from XKBstr.h, but there's a
different definition in the server. I'd be more than happy to separate
the datatypes more appropriately, but it's gonna break the XKB API to
move things around or redefine them.

There's far more XKB code duplicated in libxkbcommon than just the
keyname<->keysym mappings. The xkbcomp code uses a lot of XKB API
currently in Xlib and libxkbfile. Moving all the datatypes around to
appropriate locations is definitely something I think would be good,
but I'm pretty sure things will break. I was trying to avoid API
breakage for now.

>> http://cgit.freedesktop.org/~dbn/kbproto/
>>
>> Refactored XKB headers. I needed to split the generic datatypes away from
>> the client side datatypes to avoid pulling in Xlib.h and friends. Should
>> remain API compatible as the headers just go from XKBstr.h to XKBstr.h +
>> XKBstrcommon.h (generic types). It's not pretty, but the XKB headers never
>> were, and I didn't want to have to make a full internal copy of them like
>> in the server.
>
> When the server was modularized a lot of implementation headers ended
> up in proto modules and we're still dragging that around.  I think
> it'd be great to see that cleaned up to, even if we have to bump the
> kbproto major.  In some cases it wasn't possible to split the headers
> properly, since they had structs that were used in the implementation
> on both the client and server side.  But libxkbcommon fills that gap,
> and as I see it, with kbproto (protocol tokens and structs),
> libxkbcommon (server and client shared implementation and headers),
> libXCB/libX11 (client side) and xserver (server side), we can now move
> things around to where they should be.

Yeah. Same comments as above. I think this is the way to go, but I'm
sure some things will be broken in all the code movement. It might
require a bump to the XKB extension version to do all the API cleanup.

>> This is just scratching the surface, though. There is currently a full XKB
>> stack in the server and a lot of it could just be reused from xkbcommon. A
>> ton of code is there to support writing a XKB keymap file. Sadly, I can't
>> kill this yet as it's what makes XkbGetKeyboardByName possible.
>>
>> On the positive side, the server is now fork-free and xkbcomp is a thing of
>> the past.
>>
>> http://cgit.freedesktop.org/~dbn/libxkbfile/
>>
>> Not really needed, but I moved a couple generic headers to kbproto. This
>> version just requires a newer kbproto so there aren't header conflicts.
>>
>> Things still to do:
>>  - There's a bug somewhere in the keymap compiling that exposed a floating
>>   point exception in the server xkb code, but my keyboard seems to be
>>   working correctly
>>  - Figure out how to do all the "merge new components into current keymap"
>>   thing required by X_kbGetKbdByName without resorting to generating a
>>   whole custom keymap file.
>>  - Refactor more XKB code into xkbcommon and actually start using it in
>>   xserver, xlib and xkbfile. This work increases the number of full XKB
>>   stacks to 3. :)
>
> To this last point, what can libxkbcommon do now and what's currently missing?

The big thing is the second TODO. I haven't looked into it in great
detail, but I think that most of the XKB code in the server will go
away if this can be done in xkbcommon. See the big comment I added in
this commit:

http://cgit.freedesktop.org/~dbn/xserver/commit/?h=xkbcommon&id=4353c5bebe8eecbc76e056f5457ac88ea566bf01

I think much of the remaining duplicated code is there to support
xkb/xkbfmisc.c:XkbWriteXKBKeymapForNames. I think this could be done
better and without writing files in xkbcommon using the xkbcomp
internal API. Unfortunately, I just don't understand exactly what's
going on yet.

After that, it should be pretty mechanical to change the server to use
xkbcommon functions for allocation and the like and leave truly
server-only API. On the client side (Xlib and libxkbfile), I only
copied the code that was needed in xkbcomp. Those functions in Xlib
and xkbfile could then just be wrappers over their xkbcommon
counterparts to remain API compatible. I haven't done any of this yet,
but things should work with this code. It just has a ton of code
duplication. To get an idea how much duplication, here's what the
XKBcommon.h header looked like at one point.

http://cgit.freedesktop.org/~dbn/libxkbcommon/commit/?id=6a84a34d8689fb76b202f583452b97d57c68673d

Oh, a couple more TODOs:
- Leaks like crazy. xkbcomp was not designed to clean up after itself.
- Further down the road, figure out how sanely expose the xkbcomp
internals to clients so that you could actually have a reasonable xkb
tool to inform a user what's in the XKB files.

>> Let me know what you think. I'm open to changing names or interfaces or
>> whatever, but I think the major direction of the code is a good thing.
>
> I think this is really great work, getting that fork out of the server
> is excellent.  Again, my pipe dream is that I can eventally compile
> libxkbcommon with as few X11 dependencies as possible and then 1)
> parse an xkb keymap, 2) feed it an evdev keycode, 3) get the resulting
> keysym.  I can dream ;)

My major dream is just to remove all the duplication and get clean
separation. For wayland, I think you'll have to get more familiar with
the in-server XKB code to see just how to achieve what you want. I'm
still pretty ignorant about what the server actually does internally.

--
Dan