[Xcb] Re: very first attempt of the keybinding

[Vincent Torri]

Hey,

thank you for your answer. I'm doing the changes.

One question, though:

in atoms.c, there's following code in InternAtomFast:

	InternAtomFastCookie cookie;

	if((cookie.u.atom = InternAtomPredefined(name_len, name)).xid !=
None)
	{
		cookie.tag = TAG_VALUE;
		return cookie;
	}

I don't understand why you check cookie.u.atom, as you've just declared
cookie. Its values should be random, shouldn't they ?

Vincent

On Thu, 5 May 2005, Jamey Sharp wrote:

> 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
>
> _______________________________________________
> xcb mailing list
> xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
>