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 <[email protected]> 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
