[PATCH 2/2] compositor: add a way to change the keyboard leds

[Giulio Camuffo]

2014-08-21 17:12 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Thu, 21 Aug 2014 08:37:17 +0100
> Daniel Stone <daniel at fooishbar.org> wrote:
>
>> Hi,
>> Last nitpick, sorry ...
>>
>> On Wednesday, August 20, 2014, Giulio Camuffo <giuliocamuffo at gmail.com>
>> wrote:
>> >
>> > +       mods_depressed =
>> > xkb_state_serialize_mods(keyboard->xkb_state.state,
>> > +                                               XKB_STATE_DEPRESSED);
>> > +       mods_latched = xkb_state_serialize_mods(keyboard->xkb_state.state,
>> > +                                               XKB_STATE_LATCHED);
>> > +       mods_locked = xkb_state_serialize_mods(keyboard->xkb_state.state,
>> > +                                               XKB_STATE_LOCKED);
>> > +       group = xkb_state_serialize_group(keyboard->xkb_state.state,
>> > +                                      XKB_STATE_EFFECTIVE);
>> > +
>> > +       num = (1 << keyboard->xkb_info->mod2_mod);
>> > +       caps = (1 << keyboard->xkb_info->caps_mod);
>>
>>
>> I still don't really believe the non-xkbcommon build should exist, but
>> shouldn't all this be #ifdef'ed?
>
> Looks like that to me, too, we have #ifdef ENABLE_XKBCOMMON it seems,
> and input.c is full of it. So a simple ifdef around this function would
> be needed.

Rather than ifdeffing away the function maybe it's better to just
return -1 if ENABLE_XKBCOMMON is not defined? That way the API remains
the same for both cases.

>
> FWIW, I think the --disable-xkbcommon build is broken anyway,
> compositor.h uses e.g. xkb_mod_index_t and unconditionally includes
> xkbcommon.h, so if xkbcommon is not in the standard system path, weston
> won't build at all. But apparently it works for runtime to not link to
> libxkbcommon.so, so let's keep it that way.
>
> I just wonder...
>
> weston_keyboard_set_leds()... but that also changes the XKB state to
> match the leds, right?
>
> Should it be called weston_keyboard_set_locks() or something?

That would need adding a new enum weston_keyboard_locks { NUM_LOCK, ..
}, but yeah, i suppose it makes more sense.


Thanks,
Giulio


>
> The "compositor: add an option to set the default numlock value" is
> fine by me, and the "evdev/libinput: sync the leds of keyboards with
> the xkb state" you both already seemed to agree to leave for later.
>
>
> Thanks,
> pq