X11: fix KeyPressed duplicated with IBus

[Edgaru089]

Description

This PR is a small patch to not send KeyPressed events when the underlying X11 event is taken by the input method. It essentially only involves moving a if statement to include the part sending KeyPressed events, and indenting.

This fixes a bug in #1850 not discovered until @kimci86 tested it on GNOME with the bundled IM framework IBus. (here)

How to test this PR?

Dump KeyPressed events from a window.

#include <SFML/Graphics.hpp>

int main()
{
    sf::RenderWindow window(sf::VideoMode(100, 100), "Keys");
    window.setFramerateLimit(60);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
            else if (event.type == sf::Event::KeyPressed)
                printf("KeyPressed, Key=%d\n", (int)event.key.code);
            else if (event.type == sf::Event::TextEntered)
                printf("TextEntered, Text=%d\n", (int)event.text.unicode);
        }

        window.clear();
        window.display();
    }
}

[kimci86]

I tested it with the following code to see KeyReleased events as well.
Apparently, framerate has an impact on the behavior so I added a command line argument to choose framerate.

#include <SFML/Graphics.hpp>

int main(int argc, char* argv[])
{
    sf::RenderWindow window(sf::VideoMode(100, 100), "Keys");
    window.setFramerateLimit(1 < argc ? std::atoi(argv[1]) : 60);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
            else if (event.type == sf::Event::KeyPressed)
                printf("KeyPressed, Key=%d\n", (int)event.key.code);
            else if (event.type == sf::Event::KeyReleased)
                printf("KeyReleased, Key=%d\n", (int)event.key.code);
            else if (event.type == sf::Event::TextEntered)
                printf("TextEntered, Text=%d\n", (int)event.text.unicode);
        }

        window.clear();
        window.display();
    }
}

cmake_minimum_required(VERSION 3.22)
project(test)
find_package(SFML 2.5 COMPONENTS graphics REQUIRED)
add_executable(test main.cpp)
target_link_libraries(test sfml-graphics)
install(TARGETS test DESTINATION .)

It ran it against those versions of SFML:

• e458f4651e3cbfd168b9be0cfa328a401dccf895 (One commit before 2.6.x)
  • All good!
• 256755146990aee69c77103797dca140744f6c3a (2.6.x)
  • The first KeyPressed event is duplicated
  • Several KeyPressed events after release when the key was pressed for some time and especially when FPS is low (e.g. 15).
• d073a9b06217d411d9534d78ba4c6359c39ced05 (Edgaru089:2.6.x)
  • I suspect the first KeyPressed event is filtered, and the second is made available, but it comes after a small delay. It can come after the KeyReleased event.
  • Several KeyPressed events after release when the key was pressed for some time and especially when FPS is low (e.g. 15).

So it does not seem to fix the issue. 🤔

As noticed in the previous PR, the environment variable XMODIFIERS is set to @im=ibus by default on my system. There are no issues with the test program when XMODIFIERS=@im=none.

[Edgaru089]

Oh no. Key and text events should be two different kinds of things...

Maybe we can keep track of which kinds of Keyboard::Key have been filtered away, and, for them:

• If this event is filtered by the IME: Send KeyPressed only
• If this event is not filtered: Send TextEntered only

This should not affect code discarding repeating key events.

[Edgaru089]

Went digging through SDL1.2 source code. What they're doing is like:

• If a event is filtered by the IME: Send KeyPressed only
• If a event is not filtered:
• • If X(utf8)LookupString() returned both a key press and some text: send TextEntered only
• • If X(utf8)LookupString() returned only a key press: send KeyPressed only (there's no text anyway)

It basically assumes the IME filters every text event before re-sending it, which makes some sense.

EDIT: No it does not make sense. Keys without text attached like the arrow keys are filtered but return only key presses on XLookupString(). Looks like we have to keep a std::set of some sort...

[eXpl0it3r]

Any updates on this?

[Edgaru089]

Any updates on this?

@eXpl0it3r Um, maybe I can use a std::bitset<Keyboard::KeyCount> to keep track on key event filtering? Or maybe scancodes are a better index so this should come after the scancode commits... Any suggestions?

[Edgaru089]

@kimci86 The updated code now uses a global std::bitset<Keyboard::KeyCount> to keep track of which keys have been filtered. This is (I guess) probably a good way to fix the KeyPressed/TextEntered mess.

Perhaps in the future we should use Keyboard::Scancode instead of Keyboard::Key to keep track of the keys.

As for the bug under low framerates, well I suppose that's a (or maybe, one of many) design flaw of the entire X input stack and it makes (somewhat) sense for the input method to go crazy when it receives a repeated event and the previous filtered one is not even processed. The entire reasoning behind it is, to be frank, beyond me.

[kimci86]

Thank you for you commit, but it does not seem to be a good fix.

With the test program at 25 fps (which is not extremely low in my opinion):

• A short press on A: good

  KeyPressed, Key=0
  TextEntered, Text=97
  KeyReleased, Key=0
  

• A short press on ² (a key on my AZERTY keyboard not in the sf::Keyboard::Key enum): repeated KeyPressed event.

  KeyPressed, Key=-1
  KeyPressed, Key=-1
  TextEntered, Text=178
  KeyReleased, Key=-1
  

• A long press on A: events come after a small delay. Some KeyPressed and TextEntered events arrive after KeyReleased.

  KeyPressed, Key=0
  TextEntered, Text=97
  KeyPressed, Key=0
  TextEntered, Text=97
  KeyPressed, Key=0
  KeyPressed, Key=0
  TextEntered, Text=97
  KeyPressed, Key=0
  TextEntered, Text=97
  KeyPressed, Key=0
  TextEntered, Text=97
  KeyPressed, Key=0
  KeyPressed, Key=0
  TextEntered, Text=97
  KeyPressed, Key=0
  TextEntered, Text=97
  KeyReleased, Key=0
  KeyPressed, Key=0
  TextEntered, Text=97
  TextEntered, Text=97
  TextEntered, Text=97
  

I don't know how it can be fixed. All I can think of for now is to check if IBus is used somehow (maybe environment variables) and fallback to previous implementation if so, but that is not really a fix and it makes the code harder to test with more variability. But for now, key events are not reliable 🤕

[Edgaru089]

A short press on ² (a key on my AZERTY keyboard not in the sf::Keyboard::Key enum): repeated KeyPressed event.

This should be resolved if we use scancodes instead of key codes, see the review.

A long press on A: events come after a small delay. Some KeyPressed and TextEntered events arrive after KeyReleased.

I can't repeat that behaviour on my standard QWERTY, even with my keyboard layout set to AZERTY (pressing Q types A), but again that might be a IBus problem. Maybe you can add printf()s to the 2 places where KeyPresseds are emitted (L1929 and L1962) and see what happens. With a text key, only the first branch should be triggered.

[kimci86]

I am trying to understand and debug this. IBus seems to need some special handling. See libsdl-org/SDL#1659

[Edgaru089]

I am trying to understand and debug this. IBus seems to need some special handling. See libsdl-org/SDL#1659

That's not the same thing. SDL2 uses DBus to connect to the local IBus/Fcitx daemon, and send/receive events through the DBus connection, thus bypassing the entire X/XIM input stack. We're not doing that.

If you want to look at some sample code, grab the SDL1.2 code.

[Edgaru089]

Just went through the hassle of installing a brand new Ubuntu 20.04 in a virtual machine, with GNOME and IBus bundled.

A long press on A: events come after a small delay. Some KeyPressed and TextEntered events arrive after KeyReleased.

Well I set my keyboard layout to AZERTY, pressed and held the key on the right of Tab, and did not see any KeyPressed arriving before KeyReleased (and I guess that's the whole point). Plus, the number of KeyPressed I received matches the number of TextEntered.

TextEntered, yes, sometimes comes after KeyReleased and we might need to document that somehow, but they are text input events and they, by their very nature, should not be related to key presses in any way, nor should the user ever assume the TextEntered events are generated by KeyPressed (or at least by the ones the application actually receives).

[eXpl0it3r]

IMHO doing some best-effort known key filtering, isn't really an acceptable solution. As soon as the best-effort isn't covered anymore, SFML shouldn't just send whatever combinations of pressed and released events.

Why do we get with this change get weird released/pressed event behavior?

TextEntered, yes, sometimes comes after KeyReleased and we might need to document that somehow, but they are text input events and they, by their very nature, should not be related to key presses in any way, nor should the user ever assume the TextEntered events are generated by KeyPressed (or at least by the ones the application actually receives).

We don't guarantee the order of these events, as such this is fine and doesn't really need to be documented.

[Edgaru089]

IMHO doing some best-effort known key filtering, isn't really an acceptable solution. As soon as the best-effort isn't covered anymore, SFML shouldn't just send whatever combinations of pressed and released events.

The commit now uses a std::bitset to keep track of which sf::Keyboard::Keys are filtered at runtime, it's not known key filtering.

This does not work under keyboard layouts other than QWERTY, but once the scancode PR gets merged we can instead keep track of which scancodes are filtered, and things should work for any key pressed by a user as long as the scancodes are correct.

---

Why do we get with this change get weird released/pressed event behavior?

Because X is bad. The client have to manually feed the events to the IM, and the IM gives text by inserting (KeyPress!) events into the event queue.

Without XIM, text inputs look like:

XNextEvent() -> event, If event.type == KeyPress?

XLookupString() -> text input

Simple. But with XIM, things become messy:

XNextEvent() -> event, event.type == KeyPress
XFilterEvent(event) -> bool (manually present the event to the IM)

• True: Key pressed, but the IM filtered it away.
• False: The IM did not filter it away. Either the IM is not interested in this type of keypress (modifiers etc., should send sf::KeyPressed), or this event is sent by the IM itself as text input (should not send sf::KeyPressed). And no, XLookupString() returns a status of XLookupBoth if the text input has a keysym (A...Z, numbers etc.) so we cannot simply tell which one is which.

We need to send sf::KeyPresseds only on the original events not sent by the IM (to keep them before sf::KeyReleaseds), so the best thing we can do is to send sf::KeyPressed when

• This event is filtered away, or
• This event is not filtered away, and has never been filtered before.

@binary1248 I really need your help on this.

[eXpl0it3r]

Superseded by #2242