[Xcb] Re: very first attempt of the keybinding

[Jamey Sharp]

On Thu, 2005-05-05 at 10:07 +0200, Vincent Torri wrote:
> Hello,

Hi Vincent!

> i've modified the code. At least it compiles fine. I've not ested it yet.
> What do you think of it ? is the design good enough ?

Sorry I never commented on your first draft. Looking at this one I've
remembered that I read over that one too, but forgot to write anything
down... Oops.

I think it looks good generally. But there's at least one optimization
that this design won't allow. The MappingNotify event may tell you that
only a subset of the keys have changed, and it'd be nice to only force a
round-trip to look up the new mapping when one of those keys are
actually pressed. (I don't think this is an important optimization,
since I think keyboard mappings change very rarely, but it'd be nice to
not disallow it.)

To allow future optimizations, I'd be inclined to use a single opaque
XCBKeySymbols structure to track all per-display state. The simple
version of that struct would hold a pointer to the XCBConnection that
it's associated with, plus a tagged union of a GetKeyboardMapping cookie
and reply. On initialization of this structure the cookie would be
filled in with a query for the whole mapping; whenever a lookup is
needed the cookie would be converted to a reply; and on MappingNotify
events the reply would be discarded and the cookie reinitialized with a
new query.

There's an example of the tagged union model I'm proposing in xcb-atom:
see InternAtomFast and InternAtomFastReply. Similar code also appears in
xcb-wm, in manage.c. The reasons for using tagged unions are a little
different in those cases but the code would be roughly the same.

There are a couple little things I want to comment on in the code, too.

>   /* FIXME: is there an interest to use the iter system 
>             compared to that loop */
>   for (j = 0; j < rep->keysyms_per_keycode; j++)

No, iterators don't buy you anything here. They're only provided on
arrays for consistency, so programmers (and XCB's code generator) don't
have to remember whether a particular XCB type has an iterator or not.

In XCBKeySymbolsReply:
> ...
>       kc_to_ks->keysyms = XCBGetKeyboardMappingKeysyms (km_rep);
>     }
>   free (km_rep);

In XCBRefreshKeyboardMapping:
>       free (rep->keysyms);

You can't make either of those calls to free: all parts of an XCB reply
are in one malloc'd chunk, so you can't free just one part.
XCBGetKeyboardMappingKeysyms just returns a pointer into the reply. But
if you just store the reply itself, as I'm suggesting with the tagged
union, you won't have this problem.

Hope that helps. I'm glad you're patient: you keep writing good code
even when it takes me a while to give you feedback. :-)

--Jamey