[PATCH weston] editor: unify key handling

[Daniel Stone]

Hi Weng,
Thanks for the patch, and sorry for the horrendous delay in giving you feedback.

In general, your patch looks very good and makes a lot of sense. However:

On 1 February 2017 at 03:22, Weng Xuetian <wengxt at gmail.com> wrote:
> @@ -395,81 +402,8 @@ text_input_keysym(void *data,
>                   uint32_t modifiers)
>  {
>         struct text_entry *entry = data;
> -       [...]
> +       handle_key(entry, time, key, state, modifiers);
>  }
>
> @@ -1381,29 +1313,19 @@ handle_bound_key(struct editor *editor,
>  }
>
>  static void
> -key_handler(struct window *window,
> -           struct input *input, uint32_t time,
> -           uint32_t key, uint32_t sym, enum wl_keyboard_key_state state,
> -           void *data)
> -{
> -       struct editor *editor = data;
> -       struct text_entry *entry;
> +handle_key(struct text_entry *entry, uint32_t time,
> +          uint32_t sym, enum wl_keyboard_key_state state,
> +          uint32_t modifiers) {
> +       struct input *input = entry->input;
>         const char *new_char;
>         char text[16];
> -       uint32_t modifiers;
> -
> -       if (!editor->active_entry)
> -               return;
> -
> -       entry = editor->active_entry;
> -
> -       if (state != WL_KEYBOARD_KEY_STATE_PRESSED)
> +       if (!input || state != WL_KEYBOARD_KEY_STATE_PRESSED)
>                 return;
>
>         modifiers = input_get_modifiers(input);
>         if ((modifiers & MOD_CONTROL_MASK) &&
>             (modifiers & MOD_SHIFT_MASK) &&

This is now incorrect. It overrides the modifiers passed from the
keysym event, with those from the general keyboard state, which may be
different.

Note that the two sets of modifiers are different. The keysym
modifiers are 'raw', e.g. see how we look up shift_mask from the XKB
keymap and compare against that in the keysym state. In order to use
the same codepath, we would need to transform the modifiers from the
keysym event the way that input_get_modifiers() does, i.e. into
MOD_CONTROL_MASK/MOD_SHIFT_MASK.

Could you please submit another revision correcting this? You may want
to split this into a couple patches: one preparatory patch which
allows clients to do the input_get_modifiers() mapping, another patch
which makes the keysym handler use that (deleting shift_mask in the
process), and then one last patch which merges the two.

Cheers,
Daniel