From c4766dc3c6233d3353b79fbd226202e7b8e3fc46 Mon Sep 17 00:00:00 2001 From: wm4 Date: Mon, 1 Jul 2013 23:54:59 +0200 Subject: input: require VOs to send key up events, redo input key lookup Making key up events implicit was sort-of a nice idea, but it's too tricky and unreliable and makes the key lookup code (interpret_keys()) hard to reason about. See e.g. previous commit for subtle bugs and issues this caused. Make key-up events explicit instead. Add key up events to all VOs. Any time MP_KEY_STATE_DOWN is used, the matching key up event must use MP_KEY_STATE_UP. Rewrite the key lookup code. It should be simpler and more robust now. (Even though the LOC increases, because the new code is less "compact".) --- core/input/input.c | 130 +++++++++++++++++++++++++------------------------- core/input/joystick.c | 4 +- core/input/keycodes.h | 17 +++++-- core/mp_fifo.c | 5 +- 4 files changed, 80 insertions(+), 76 deletions(-) (limited to 'core') diff --git a/core/input/input.c b/core/input/input.c index 9fdd77ddd0..d7aaf54dec 100644 --- a/core/input/input.c +++ b/core/input/input.c @@ -624,7 +624,7 @@ static char *get_key_name(int key, char *ret) static char *get_key_combo_name(int *keys, int max) { char *ret = talloc_strdup(NULL, ""); - while (1) { + while (max > 0) { ret = get_key_name(*keys, ret); if (--max && *++keys) ret = talloc_asprintf_append_buffer(ret, "-"); @@ -1280,36 +1280,52 @@ static void release_down_cmd(struct input_ctx *ictx) ictx->ar_state = -1; } -static mp_cmd_t *interpret_key(struct input_ctx *ictx, int code) +static int find_key_down(struct input_ctx *ictx, int code) { - unsigned int j; - mp_cmd_t *ret; + code &= ~(MP_KEY_STATE_UP | MP_KEY_STATE_DOWN); + for (int j = 0; j < ictx->num_key_down; j++) { + if (ictx->key_down[j] == code) + return j; + } + return -1; +} + +static void remove_key_down(struct input_ctx *ictx, int code) +{ + int index = find_key_down(ictx, code); + if (index >= 0) { + memmove(&ictx->key_down[index], &ictx->key_down[index + 1], + (ictx->num_key_down - (index + 1)) * sizeof(int)); + ictx->num_key_down -= 1; + } +} +static mp_cmd_t *interpret_key(struct input_ctx *ictx, int code) +{ /* On normal keyboards shift changes the character code of non-special * keys, so don't count the modifier separately for those. In other words * we want to have "a" and "A" instead of "a" and "Shift+A"; but a separate * shift modifier is still kept for special keys like arrow keys. */ - int unmod = code & ~(MP_KEY_MODIFIER_MASK | MP_KEY_STATE_DOWN); + int unmod = code & ~MP_KEY_MODIFIER_MASK; if (unmod >= 32 && unmod < MP_KEY_BASE) code &= ~MP_KEY_MODIFIER_SHIFT; + if (!(code & MP_KEY_STATE_UP) && ictx->num_key_down >= MP_MAX_KEY_DOWN) { + mp_tmsg(MSGT_INPUT, MSGL_ERR, "Too many key down events " + "at the same time\n"); + return NULL; + } + + bool key_was_down = find_key_down(ictx, code) >= 0; + if (code & MP_KEY_STATE_DOWN) { - if (ictx->num_key_down >= MP_MAX_KEY_DOWN) { - mp_tmsg(MSGT_INPUT, MSGL_ERR, "Too many key down events " - "at the same time\n"); - return NULL; - } - code &= ~MP_KEY_STATE_DOWN; // Check if we don't already have this key as pushed - for (j = 0; j < ictx->num_key_down; j++) { - if (ictx->key_down[j] == code) - break; - } - if (j != ictx->num_key_down) + if (key_was_down) return NULL; + // Cancel current down-event (there can be only one) release_down_cmd(ictx); - ictx->key_down[ictx->num_key_down] = code; + ictx->key_down[ictx->num_key_down] = code & ~MP_KEY_STATE_DOWN; ictx->num_key_down++; ictx->last_key_down = mp_time_us(); ictx->ar_state = 0; @@ -1318,52 +1334,35 @@ static mp_cmd_t *interpret_key(struct input_ctx *ictx, int code) if (ictx->current_down_cmd && (code & MP_KEY_EMIT_ON_UP)) ictx->current_down_cmd->key_up_follows = true; return mp_cmd_clone(ictx->current_down_cmd); - } - // button released or press of key with no separate down/up events - for (j = 0; j < ictx->num_key_down; j++) { - if (ictx->key_down[j] == code) - break; - } - bool emit_key = false; - bool doubleclick = MP_KEY_IS_MOUSE_BTN_DBL(code); - if (doubleclick) { - int btn = code - MP_MOUSE_BTN0_DBL + MP_MOUSE_BTN0; - if (!ictx->num_key_down - || ictx->key_down[ictx->num_key_down - 1] != btn) - return NULL; - j = ictx->num_key_down - 1; - ictx->key_down[j] = code; - emit_key = true; - } - if (j == ictx->num_key_down) { // was not already down; add temporarily - if (ictx->num_key_down > MP_MAX_KEY_DOWN) { - mp_tmsg(MSGT_INPUT, MSGL_ERR, "Too many key down events " - "at the same time\n"); - return NULL; + } else if (code & MP_KEY_STATE_UP) { + if (key_was_down) { + remove_key_down(ictx, code); + release_down_cmd(ictx); } - ictx->key_down[ictx->num_key_down] = code; - ictx->num_key_down++; - emit_key = true; - } - // This is a key up event, but the key up command is added by - // release_down_cmd(), not by this code. - if ((code & MP_KEY_EMIT_ON_UP) && ictx->current_down_cmd) - emit_key = false; - // Interpret only maximal point of multibutton event - ret = NULL; - if (emit_key) - ret = get_cmd_from_keys(ictx, NULL, ictx->num_key_down, ictx->key_down); - if (doubleclick) { - ictx->key_down[j] = code - MP_MOUSE_BTN0_DBL + MP_MOUSE_BTN0; - return ret; - } - // Remove the key - if (j + 1 < ictx->num_key_down) - memmove(&ictx->key_down[j], &ictx->key_down[j + 1], - (ictx->num_key_down - (j + 1)) * sizeof(int)); - ictx->num_key_down--; - release_down_cmd(ictx); - return ret; + return NULL; + } else { + // Press of key with no separate down/up events + if (key_was_down) { + // Mixing press events and up/down with the same key is not allowed + mp_tmsg(MSGT_INPUT, MSGL_WARN, "Mixing key presses and up/down.\n"); + } + // Add temporarily (include ongoing down/up events) + int num_key_down = ictx->num_key_down; + int key_down[MP_MAX_KEY_DOWN]; + memcpy(key_down, ictx->key_down, num_key_down * sizeof(int)); + // Assume doubleclick events never use down/up, while button events do + if (MP_KEY_IS_MOUSE_BTN_DBL(code)) { + // Don't emit "MOUSE_BTN0+MOUSE_BTN0_DBL", just "MOUSE_BTN0_DBL" + int btn = code - MP_MOUSE_BTN0_DBL + MP_MOUSE_BTN0; + if (!num_key_down || key_down[num_key_down - 1] != btn) + return NULL; + key_down[num_key_down - 1] = code; + } else { + key_down[num_key_down] = code; + num_key_down++; + } + return get_cmd_from_keys(ictx, NULL, num_key_down, key_down); + } } static mp_cmd_t *check_autorepeat(struct input_ctx *ictx) @@ -1419,16 +1418,15 @@ static bool key_updown_ok(enum mp_command_type cmd) void mp_input_feed_key(struct input_ctx *ictx, int code) { ictx->got_new_events = true; - int unmod = code & ~(MP_KEY_MODIFIER_MASK | MP_KEY_STATE_DOWN); - if (MP_KEY_DEPENDS_ON_MOUSE_POS(unmod)) - ictx->mouse_event_counter++; if (code == MP_INPUT_RELEASE_ALL) { mp_msg(MSGT_INPUT, MSGL_V, "input: release all\n"); - memset(ictx->key_down, 0, sizeof(ictx->key_down)); ictx->num_key_down = 0; release_down_cmd(ictx); return; } + int unmod = code & ~MP_KEY_MODIFIER_MASK; + if (MP_KEY_DEPENDS_ON_MOUSE_POS(unmod)) + ictx->mouse_event_counter++; mp_msg(MSGT_INPUT, MSGL_V, "input: key code=%#x\n", code); struct mp_cmd *cmd = interpret_key(ictx, code); if (!cmd) diff --git a/core/input/joystick.c b/core/input/joystick.c index 17b4279c39..e8330ffaeb 100644 --- a/core/input/joystick.c +++ b/core/input/joystick.c @@ -139,7 +139,7 @@ int mp_input_joystick_read(void *ctx, int fd) { if(ev.value == 1) return (MP_JOY_BTN0 + ev.number) | MP_KEY_STATE_DOWN; else - return MP_JOY_BTN0 + ev.number; + return (MP_JOY_BTN0 + ev.number) | MP_KEY_STATE_UP; } else if(ev.type & JS_EVENT_AXIS) { if(ev.value < -JOY_AXIS_DELTA && axis[ev.number] != -1) { axis[ev.number] = -1; @@ -150,7 +150,7 @@ int mp_input_joystick_read(void *ctx, int fd) { } else if(ev.value <= JOY_AXIS_DELTA && ev.value >= -JOY_AXIS_DELTA && axis[ev.number] != 0) { int r = axis[ev.number] == 1 ? MP_JOY_AXIS0_PLUS+(2*ev.number) : MP_JOY_AXIS0_MINUS+(2*ev.number); axis[ev.number] = 0; - return r; + return r | MP_KEY_STATE_UP; } else return MP_INPUT_NOTHING; } else { diff --git a/core/input/keycodes.h b/core/input/keycodes.h index 80e2561514..d91465f3be 100644 --- a/core/input/keycodes.h +++ b/core/input/keycodes.h @@ -220,21 +220,28 @@ #define MP_KEY_MODIFIER_META (1<<25) #define MP_KEY_MODIFIER_MASK (MP_KEY_MODIFIER_SHIFT | MP_KEY_MODIFIER_CTRL | \ - MP_KEY_MODIFIER_ALT | MP_KEY_MODIFIER_META) + MP_KEY_MODIFIER_ALT | MP_KEY_MODIFIER_META | \ + MP_KEY_STATE_DOWN | MP_KEY_STATE_UP) // Flag for key events. Multiple down events are idempotent. Release keys by -// sending the key code without this flag, or by sending MP_INPUT_RELEASE_ALL -// as key code. +// sending the key code with KEY_STATE_UP set, or by sending +// MP_INPUT_RELEASE_ALL as key code. #define MP_KEY_STATE_DOWN (1<<26) +// Flag for key events. Releases a key previously held down with +// MP_KEY_STATE_DOWN. Do not sending redundant UP events and do not forget to +// release keys at all with UP. If input is unreliable, use MP_INPUT_RELEASE_ALL +// or don't use MP_KEY_STATE_DOWN in the first place. +#define MP_KEY_STATE_UP (1<<27) + // The following flags are not modifiers, but are part of the keycode itself. // Emit a command even on key-up (normally key-up is ignored). The command // handling code has to ignore unwanted commands specifically. -#define MP_KEY_EMIT_ON_UP (1<<27) +#define MP_KEY_EMIT_ON_UP (1<<28) // Use this when the key shouldn't be auto-repeated (like mouse buttons) // Also means both key-down key-up events produce emit bound commands. -#define MP_NO_REPEAT_KEY (1<<28) +#define MP_NO_REPEAT_KEY (1<<29) #endif /* MPLAYER_KEYCODES_H */ diff --git a/core/mp_fifo.c b/core/mp_fifo.c index 32036749d1..1c83e99dcd 100644 --- a/core/mp_fifo.c +++ b/core/mp_fifo.c @@ -53,9 +53,8 @@ void mplayer_put_key(struct mp_fifo *fifo, int code) double now = mp_time_sec(); int doubleclick_time = fifo->opts->doubleclick_time; // ignore system-doubleclick if we generate these events ourselves - if (doubleclick_time - && (code & ~MP_KEY_STATE_DOWN) >= MP_MOUSE_BTN0_DBL - && (code & ~MP_KEY_STATE_DOWN) < MP_MOUSE_BTN_DBL_END) + int unmod = code & ~MP_KEY_MODIFIER_MASK; + if (doubleclick_time && MP_KEY_IS_MOUSE_BTN_DBL(unmod)) return; mp_input_feed_key(fifo->input, code); if (code & MP_KEY_STATE_DOWN) { -- cgit v1.2.3