We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3472

Summary: Holding key down sends keyup events with 2.0.5
Product: SDL Reporter: Jari Ronkainen <ronchaine>
Component: eventsAssignee: Alex Baines <alex>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: alex, kratz00, sezeroz
Version: 2.0.5   
Hardware: x86_64   
OS: Linux   
Attachments: Patch to not forward filtered events when compiled without fcitx/ibus
fix double events & key repeat flag when built without fcitx/ibus headers

Description Jari Ronkainen 2016-10-26 00:39:32 UTC
When holding a key down, keyup events are received with SDL2.0.5

Wrote a simple program to illustrate the problem, since I suck at explaining:

#include <SDL.h>
#include <iostream>
#include <unordered_map>

int main()
{
    SDL_Init(SDL_INIT_VIDEO);
    
    SDL_Window* window;
    
    window = SDL_CreateWindow("SDL2 2.0.5 keyboard bug",
                              SDL_WINDOWPOS_CENTERED,
                              SDL_WINDOWPOS_CENTERED,
                              64, 64,
                              SDL_WINDOW_SHOWN);
    SDL_Event sdlevent;
    
    std::unordered_map<uint16_t, bool> keys_down;

    while(1)
    {
        SDL_PollEvent(&sdlevent);

        if (sdlevent.type == SDL_KEYUP) {
            if (keys_down[sdlevent.key.keysym.scancode] == true)
            {
                std::cout << "keyup: scancode: "
                          << sdlevent.key.keysym.scancode
                          << "\n";

                keys_down[sdlevent.key.keysym.scancode] = false;
            }
        } else if (sdlevent.type == SDL_KEYDOWN) {
            if (keys_down[sdlevent.key.keysym.scancode] == false)
            {
                std::cout << "keydown: scancode: "
                          << sdlevent.key.keysym.scancode
                          << "\n";

                keys_down[sdlevent.key.keysym.scancode] = true;
            }

            if (sdlevent.key.keysym.scancode == 41) // quit on esc
                break;
        }
    }
    SDL_Quit();
}

Holding key down causes following output:
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
keydown: scancode: 81
keyup: scancode: 81
...
etc.

Equivalent output on 2.0.4
keydown: scancode: 81
Comment 1 Ozkan Sezer 2016-10-27 09:13:37 UTC
Possibly related: https://forums.libsdl.org/viewtopic.php?t=12168
which points to https://hg.libsdl.org/SDL/rev/c3874aa1f2d1 as the
culprit.  Installing ibus-devel (e.g.: yum install ibus-devel) fixed
the test case for me.
Comment 2 Alex Baines 2016-10-27 16:55:07 UTC
I tried the code, but get the 2.0.4 style output here. Could you provide this info, so I can try and track it down?

1) Distro + version etc.

2) Did you build SDL-2.0.5 yourself or use a pre-built version.

3) If you built it yourself, was it using cmake or autotools, and was anything printed about IME when building?

4) The output of 'xprop -root | grep XKB'

5) The output of 'echo $XMODIFIERS'

6) The output of 'ps ax | grep ibus'

7) The output of 'ibus-daemon --version'

Thanks
Comment 3 Jari Ronkainen 2016-10-27 17:21:21 UTC
It seems that Ozkan is right, I now built this twice, both with and without ibus headers.  Seems that it works when built with the ibus headers available.

Here's requested info, anyways:

> 1) Distro + version etc.
Arch Linux

> 
> 2) Did you build SDL-2.0.5 yourself or use a pre-built version.
Used pre-built

> 4) The output of 'xprop -root | grep XKB'
_XKB_RULES_NAMES(STRING) = "evdev", "pc105", "fi", "", ""
 
> 5) The output of 'echo $XMODIFIERS'
@im=ibus

 
> 6) The output of 'ps ax | grep ibus'
1417 ?        Ssl    1:19 ibus-daemon -dxr
1420 ?        Sl     0:00 /usr/lib/ibus/ibus-dconf
1422 ?        Sl     0:17 /usr/lib/ibus/ibus-ui-gtk3
1425 ?        Sl     0:45 /usr/lib/ibus/ibus-x11 --kill-daemon
1460 ?        Sl     0:24 /usr/lib/ibus/ibus-engine-simple
2186 ?        Sl     0:00 /usr/lib/ibus/ibus-engine-m17n --ibus

> 7) The output of 'ibus-daemon --version'
ibus-daemon - Version 1.5.14
Comment 4 Ozkan Sezer 2016-10-27 18:00:58 UTC
(In reply to Alex Baines from comment #2)
> 1) Distro + version etc.
> 

Fedora 20 - x86_64

> 2) Did you build SDL-2.0.5 yourself or use a pre-built version.
> 

Built myself, first without ibus-devel and dbus-devel installed
which displayed the issue, and second time with ibus-devel and
dbus-devel installed which removed the issue.

> 3) If you built it yourself, was it using cmake or autotools, and was
> anything printed about IME when building?
> 
> 4) The output of 'xprop -root | grep XKB'
> 
> 5) The output of 'echo $XMODIFIERS'
> 
> 6) The output of 'ps ax | grep ibus'
> 
> 7) The output of 'ibus-daemon --version'

Will post answers to 3-7 tomorrow if you want them, when I have access
to the same system again.
Comment 5 Alex Baines 2016-10-27 18:39:21 UTC
Ok, I think I understand the issue now.

If neither the ibus nor fcitx headers are present, then XSetLocaleModifiers gets called with "", instead of the usual "@im=none". This means for many people they'll get ibus via XIM.

This means that XFilterEvent will return true for some key events, indicating that no futher processing should happen. However for SDL it makes sense to send key events even if they are filtered, otherwise dead keys will not generate an SDL keypress / release.

The problem is that ibus via XIM has a bad habit of returning true from XFilterEvents for everything, and then resending another event later if it didn't really need to filter it.

In SDL 2.0.4, to avoid this case the extra processing didn't happen after XFilterEvent, so dead keys would not generate any events. For 2.0.5 I added the dead key sending back in, along with setting the locale modifiers to "@im=none" if XMODIFIERS was "@im=ibus" to avoid the resending issue.

However with the addition of fcitx, some of the IME stuff was reworked so if SDL is not built with the ibus headers, then the "@im=none" setting doesn't happen.


So, to fix it, we could:

1) Go back to not generating key presses after XFilterEvent returns true when built without the ibus headers. (2.0.4 behaviour)

2) Force "@im=none" when XMODIFIERS is "@im=ibus" even if the dbus path won't be used. (should generate events for dead keys but might break some expectations for people who really want to use ibus via xim)

3) Find another workaround for ibus' resending, maybe by looking at the timestamps / serials of key events to determine if they're resent ones.
Comment 6 Alex Baines 2016-10-28 00:33:08 UTC
Created attachment 2592 [details]
Patch to not forward filtered events when compiled without fcitx/ibus

Here is a patch that implements the 1) solution, since that is the simplest.

The drawback is if SDL is compiled without the dbus fcitx / ibus implementation then dead keys won't generate key events. But that is how 2.0.4 worked.
Comment 7 Ozkan Sezer 2016-10-28 05:24:43 UTC
(In reply to Alex Baines from comment #6)
> Created attachment 2592 [details]
> Patch to not forward filtered events when compiled without fcitx/ibus
> 
> Here is a patch that implements the 1) solution, since that is the simplest.
> 
> The drawback is if SDL is compiled without the dbus fcitx / ibus
> implementation then dead keys won't generate key events. But that is how
> 2.0.4 worked.

Did a 'yum remove dbus-devel ibus-devel', applied the proposed patch, and
built 2.0.5: Doesn't fix the issue for me.
Comment 8 Alex Baines 2016-10-28 16:43:08 UTC
Yeah, my bad. 

That patch fixes the double events, but the key repeat logic is still broken with it. I'll investigate further.
Comment 9 Alex Baines 2016-10-28 18:01:59 UTC
Created attachment 2594 [details]
fix double events & key repeat flag when built without fcitx/ibus headers

Ok, Here's a better patch. I think I tested all the combinations properly this time.

In addition to the last fix, this one uses XkbSetDetectableAutoRepeat to make the key repeat detection work even if ibus is used via xim. If this call fails, then it forces @im=none.
Comment 10 Ozkan Sezer 2016-10-28 18:38:06 UTC
With the new patch applied, 2.0.5 built both in the absence and in the presence
of dbus-devel/ibus-devel seems to work properly with the testcase for me. Thanks.
Comment 11 Sam Lantinga 2016-10-29 00:17:20 UTC
Got it, thanks!
https://hg.libsdl.org/SDL/rev/b48d8a98e261