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

[Daniel Stone]

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?

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.

>> -     if (key == KEY_F5 && input->modifiers == Mod4Mask) {
>> +     if (num_syms == 0 && syms[0] == XK_F5 &&
>
> Should this num_syms == 1 here?

Er, yeah.

>> @@ -1511,17 +1515,10 @@ input_handle_keyboard_enter(void *data,
>>  {
>>       struct input *input = data;
>>       struct window *window;
>> -     struct display *d = input->display;
>> -     uint32_t *k, *end;
>>
>>       input->display->serial = serial;
>>       input->keyboard_focus = wl_surface_get_user_data(surface);
>>
>> -     end = keys->data + keys->size;
>> -     input->modifiers = 0;
>> -     for (k = keys->data; k < end; k++)
>> -             input->modifiers |= d->xkb->map->modmap[*k];
>> -
>
> This part break setting modifier state on keyboard enter, but the
> planned keyboard.modifiers events will restore that and give us
> modifier state for pointer enter as well.

Rather, which especially helps when nesting too.  I should have that
done in very short order once I'm done with this.

>> +     d->xkb_info.names.rules = strdup("evdev");
>> +     d->xkb_info.names.model = strdup("pc105");
>> +     d->xkb_info.names.layout = strdup(option_xkb_layout);
>> +     d->xkb_info.names.variant = strdup(option_xkb_variant);
>> +     d->xkb_info.names.options = strdup(option_xkb_options);
>>
>> -     names.rules = "evdev";
>> -     names.model = "pc105";
>> -     names.layout = option_xkb_layout;
>> -     names.variant = option_xkb_variant;
>> -     names.options = option_xkb_options;
>
> Is there anything in the libxkbcommon API changes that requires these
> strings to be strdup'ed?

Not anymore.

v2 coming soon I guess, sorry everyone who's been trying to compile
Weston in the meantime. ;) I've pushed a for-weston branch of
libxkbcommon which will still build with master.

Cheers,
Daniel