Skip to content

Commit

Permalink
Fixed bug 3249 - keysym.mod is incorrect when mod keys are pressed fo…
Browse files Browse the repository at this point in the history
…r SDL_KEYDOWN events

Adam M.

The keysym.mod field does not reflect the state of the modified keys when processing key down events for the modifier keys themselves. The documentation says that it returns the current key modifiers, but they are not current for key down events involving modifier keys. I interpret "current" to mean "equal to SDL_GetModState() at the instant the event is processed/enqueued".

For example, if you depress the Shift key you get a key down event with .mod == 0. However, .mod should not be 0 because a shift key is down. If you then release the Shift key, you get a key up event with .mod == 0. Neither event reports the modifier key.

If you press Shift and then A, .mod is incorrect (== 0) when Shift is pressed, but is correct later when A is pressed (== KMOD_LSHIFT).

You might say this behavior is deliberate, i.e. keysym.mod is the value /before/ the event, not the current value as documented, but that explanation is incorrect because only key down events behave that way. Key up events correctly give the current value, not the value before the event.

Not only is it inconsistent with itself, I think it makes keyboard processing harder.

The problem is near line 740 in SDL_keyboard.c:

if (SDL_KEYDOWN == type) {
    modstate = keyboard->modstate; // SHOULD THIS BE MOVED DOWN?
    switch (keycode) {
    case SDLK_NUMLOCKCLEAR:
        keyboard->modstate ^= KMOD_NUM;
        break;
    case SDLK_CAPSLOCK:
        keyboard->modstate ^= KMOD_CAPS;
        break;
    default:
        keyboard->modstate |= modifier;
        break;
    }
} else {
    keyboard->modstate &= ~modifier;
    modstate = keyboard->modstate;
}

In the key down path, modstate (and thus keysym.mod) ends up being the modifier state /before/ the event, but in the key up path modstate ends up being the modifier state /after/ the event. Personally I think the "modstate = keyboard->modstate" line should just be moved after the entire if/else statement, so that keysym.mod always reflects the current state.
  • Loading branch information
slouken committed Aug 12, 2017
1 parent c086251 commit 195b8bd
Showing 1 changed file with 1 addition and 4 deletions.
5 changes: 1 addition & 4 deletions src/events/SDL_keyboard.c
Expand Up @@ -672,7 +672,6 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
int posted;
SDL_Keymod modifier;
SDL_Keycode keycode;
Uint16 modstate;
Uint32 type;
Uint8 repeat;

Expand Down Expand Up @@ -745,7 +744,6 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
break;
}
if (SDL_KEYDOWN == type) {
modstate = keyboard->modstate;
switch (keycode) {
case SDLK_NUMLOCKCLEAR:
keyboard->modstate ^= KMOD_NUM;
Expand All @@ -759,7 +757,6 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
}
} else {
keyboard->modstate &= ~modifier;
modstate = keyboard->modstate;
}

/* Post the event, if desired */
Expand All @@ -771,7 +768,7 @@ SDL_SendKeyboardKey(Uint8 state, SDL_Scancode scancode)
event.key.repeat = repeat;
event.key.keysym.scancode = scancode;
event.key.keysym.sym = keycode;
event.key.keysym.mod = modstate;
event.key.keysym.mod = keyboard->modstate;
event.key.windowID = keyboard->focus ? keyboard->focus->id : 0;
posted = (SDL_PushEvent(&event) > 0);
}
Expand Down

0 comments on commit 195b8bd

Please sign in to comment.