[PATCH libxkbcommon] Add xkb_key_get_action API

[Ran Benita]

On Mon, Mar 19, 2012 at 04:10:10PM +0000, Daniel Stone wrote:
> Hi Ran,
> 
> On 16 March 2012 17:04, Ran Benita <ran234 at gmail.com> wrote:
> > By which one can tell when to switch groups, etc.
> >
> > Signed-off-by: Ran Benita <ran234 at gmail.com>
> > ---
> > This would allow me to delete some duplicate code in the application
> > and would make me a happy camper for a while.
> 
> It makes sense now, but the API I was hoping for (and am mostly done with) is:
> +/**
> + * Updates a state object to reflect the given key being pressed or released.
> + */
> +_X_EXPORT void
> +xkb_state_update_key(struct xkb_state *state, xkb_keycode_t key, enum
> xkb_key_direction direction);
> 
> That way users can remain ignorant of the actions completely, which
> would make the flow:
> {
>     struct xkb_keymap *xkb = (...);
>     struct xkb_state *state;
>     xkb_keycode_t lctrl, ralt, q;
> 
>     state = xkb_state_new(xkb);
>     /* state is empty */
>     xkb_state_update_for_key(state, lctrl, XKB_KEY_PRESSED);
>     /* state now reflects Control mod, LCtrl vmod */
>     xkb_state_update_for_key(state, ralt, XKB_KEY_RELEASED);
>     /* Control, Mod1, LCtrl, RAlt */
>     xkb_state_update_for_key(state, q, XKB_KEY_PRESSED);
>     /* as above */
>     xkb_state_update_for_key(state, q, XKB_KEY_RELEASED);
>     /* same again */
>     xkb_state_update_for_key(state, lctrl, XKB_KEY_RELEASED);
>     /* Mod1, RAlt */
>     xkb_state_update_for_key(state, ralt, XKB_KEY_RELEASED);
>     /* empty again */
> }
> 
> Does that sound OK? Any thoughts? Hopefully that'll lead to even less
> code in your app (and everyone else's).

I like the API, it's clear and extensible, and the filter-pipeline
implementation is also nice (I'd never think of something like that..).
I've ported some of my code to use it, and it mostly worked. The few
things I have remaining are:
- Handling of group lock actions (I tried a simple patch for that, see
  the following pull request).
- Some way to update the state according to a given led state, i.e. say
  the app is starting with the Caps Lock led on, I'd like to update the
  modifier state according to the indicator map. Right now there are
  only getters in the new API.
- Some help with handling key repeat events. I'm not sure what the scope
  of this (if any) should be, but there is some stuff like the key
  repeat control and per key repeat. Should any of this be exposed?
- Related, maybe the "down" parameter can be changed to be a little more
  descriptive? Maybe an enum or even a flag mask. I think boolean
  parameters usually don't go well, and are also pretty meaningless on
  the caller's side, e.g:
        xkb_state_update_key(state, keycode, 1);
  versus
        xkb_state_update_key(state, keycode, XKB_KEY_PRESSED);
  or something more suitable.

Again, it's pretty nice and helpful; thanks for doing it :)

> > Wasn't sure about a couple of things:
> > - Should the by_level API be exposed (the _syms one seems to be non-static
> >  but not exported).
> 
> I'm thinking that will need to be exported to allow clients to
> interrogate the keymap, but want to encourage people to use the state
> API as much as possible.
> 
> > - Should the API allow for returning multiple actions per keypress (as the
> >  _syms API seems to suggest will be done for keysyms)?
> 
> At the moment, I can't see a use for multiple actions per keypress,
> whereas there's a definite need for symbols, as Unicode has been
> pushing back on adding precomposed symbols, so some keymaps have
> symbols which cannot be represented in a single symbol; at the moment,
> they're working around it by having a placeholder symbol which
> triggers an entry in the Compose map with multiple symbols.

Interesting. I think many wrapping APIs work by only sending one event
per key press; those will need to be adapted to respond with multiple
events.

> Cheers,
> Daniel