[PATCH 1/4] Port Wayland clients to new xkbcommon API

[Kristian Høgsberg]

On Tue, May 8, 2012 at 12:26 PM, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi,
>
> On 7 May 2012 21:01, Kristian Høgsberg <hoegsberg at gmail.com> wrote:
>> On Tue, May 01, 2012 at 08:37:09PM +0100, Daniel Stone wrote:
>>> +     if (xkb_state_mod_name_is_active(xkb_state, "Mod1",
>>> +                                      (XKB_STATE_DEPRESSED | XKB_STATE_LATCHED)))
>>> +             modifiers |= MOD_ALT;
>>> +
>>> +     if ((modifiers & MOD_CTRL) && (modifiers & MOD_SHIFT) &&
>>
>> I'm still not convinced this API is practical.  Testing a specific
>> combination of modifiers is such a common operation and it just can't
>> be this awkward.  The original code tests that shift and control are
>> pressed, but it really should test modifers == SHIFT | CONTROL, that
>> is, that those and *only* those modifiers are down.  To do that we
>> need to get the modifier masks up front (in display_create for
>> window.c), and then create the modifier mask we want to test against:
>>
>> [...]
>>
>> or we can define MOD_CTRL in window.h and then translate the
>> serialized mask to those values:
>> [...]
>>
>> which is what we had before.  Plus a MOD_OTHER if something else we
>> don't know about is down.  So maybe it's ok if we just wrap it in the
>> toolkits...  At the end of the day, we'll just hard code shift, ctrl
>> and alt again, just a layer up in the toolkits.
>
> Thanks, this is actually really useful feedback.  I'm going to wedge
> something like this in:
>    enum xkb_state_match_type {
>        XKB_STATE_MATCH_ALL_NON_EXCL, /* all are down */
>        XKB_STATE_MATCH_ALL_EXCL, /* all are down & nothing else */
>        XKB_STATE_MATCH_ANY, /* at least one is down */
>    };
>    if (xkb_state_mod_indices_are_down(state,
> XKB_STATE_MATCH_ALL_EXCL, ctrl_index, alt_index, XKB_MOD_INVALID))
>        fprintf(stderr, "Ctrl + Alt and nothing else are down! Hooray!\n");
>
> And the same for mod names rather than indices.  Would that solve your concerns?

I think that I'd be fine with just get_mod_index (from name) and
serialize_mods.  If you want to keep the mod_name_is_active variants
and want to add the more complex matching function above, that's fine.
 I just think xkb_state_serialise_mods() promoted along with mod
indexes and bit math is much simpler and would like to see it promoted
to something that not discouraged ("This function should not be used
in regular clients;").  And spelled with a z, pretty please!

Kristian

> Also I'm going to add a bunch of predefined names so there's no
> opportunity to screw that up, e.g.
> #define XKB_MOD_NAME_CTRL "Control"
> #define XKB_MOD_NAME_ALT "Mod1"
> etc.