KeyPressed and KeyReleased events fire twice on Linux

[Nexus]

Sometimes when I release a key, 3 events are fired: KeyReleased, KeyPressed and KeyReleased in this order. I could reproduce the problem on Ubuntu 12.04 LTS, on Windows it doesn't seem to appear. I use the Git revision dd48427.

Small and complete example:

#include <SFML/Window.hpp>
#include <iostream>

int main()
{
        sf::Window window(sf::VideoMode(640, 480), "SFML Test");
        window.setKeyRepeatEnabled(false);
       
        sf::Clock clock;
       
        int i = 0;
        for (;;)
        {
                sf::Event event;
                while (window.pollEvent(event))
                {
                        switch (event.type)
                        {
                                case sf::Event::Closed:
                                        return 0;
                                       
                                case sf::Event::KeyPressed:
                                        if (event.key.code == sf::Keyboard::F)
                                                std::cout << ++i << "\tPressed \t"
                                                << clock.getElapsedTime().asMilliseconds() << std::endl;
                                        break;
                                       
                                case sf::Event::KeyReleased:
                                        if (event.key.code == sf::Keyboard::F)
                                                std::cout << "\tReleased\t"
                                                << clock.getElapsedTime().asMilliseconds() << std::endl;
                                        break;
                        }
                }
               
                window.display();
        }
}

Output (this was luck, sometimes you need really long until the bug appears). You see the problem at the 6th release event.

1       Pressed         3583
        Released        4355
2       Pressed         5262
        Released        6740
3       Pressed         7244
        Released        8220
4       Pressed         8782
        Released        9740
5       Pressed         10222
        Released        11339
6       Pressed         11781
        Released        13100
7       Pressed         13100
        Released        13100
8       Pressed         13650
        Released        14547

[Laurent]

Linux (X11) doesn't natively handle the repeated key events the same way as SFML, so I have to use a ugly hack workaround to get the expected results. I wouldn't be surprised if the corresponding code was not 100% robust, but I can't find where the mistake could be.

In case you want to give it a try, the code is in WindowImplX11.cpp, lines 556 to 593.

[Nexus]

Linux (X11) doesn't natively handle the repeated key events the same way as SFML

If you mean setKeyRepeatEnabled(), it is not the problem -- I disabled key repetition in my code.

Or should I nevertheless take a look at the lines you wrote?

[Laurent]

Yes, this piece of code handles everything; it also applies the KeyRepeatEnabled flag, since there's no native X11 function that automatically does it.

[Nexus]

Okay, I have come to a few results. First, I have realized how much I love Visual Studio, when I once have to work (and debug) with Code::Blocks. I'm really glad I don't have to develop with it...

This is the original code:

    // Detect repeated key events
    if ((windowEvent.type == KeyPress) || (windowEvent.type == KeyRelease))
    {
        if (windowEvent.xkey.keycode < 256)
        {
            // To detect if it is a repeated key event, we check the current state of the key.
            // - If the state is "down", KeyReleased events must obviously be discarded.
            // - KeyPress events are a little bit harder to handle: they depend on the EnableKeyRepeat state,
            //   and we need to properly forward the first one.
            char keys[32];
            XQueryKeymap(m_display, keys);
            if (keys[windowEvent.xkey.keycode / 8] & (1 << (windowEvent.xkey.keycode % 8)))
            {
                // KeyRelease event + key down = repeated event --> discard
                if (windowEvent.type == KeyRelease)
                {
                    m_lastKeyReleaseEvent = windowEvent;
                    return false;
                }

                // KeyPress event + key repeat disabled + matching KeyRelease event = repeated event --> discard
                if ((windowEvent.type == KeyPress) && !m_keyRepeat &&
                    (m_lastKeyReleaseEvent.xkey.keycode == windowEvent.xkey.keycode) &&
                    (m_lastKeyReleaseEvent.xkey.time == windowEvent.xkey.time))
                {
                    return false;
                }
            }
        }
    }

The debugger showed that duplicate events occur outside the 3rd if statement, that is when the key is not held down. Additionally, I think it would be correct to set m_lastKeyReleaseEvent always to the last release event, not only if it is discarded, but I might be wrong here.

I have tried to adapt your code. According to your comment, the KeyPressed condition is independent of heldDown (variable saying that the key is currently held down). In the KeyReleased condition, I discard the event if the key is down or it's a duplicate (which may occur when the key is already up again). I also assign the last key event always. The body of the 2nd if statement then looks like this:

            char keys[32];
            XQueryKeymap(m_display, keys);

            bool duplicate = (m_lastKeyReleaseEvent.xkey.keycode == windowEvent.xkey.keycode) &&
                             (m_lastKeyReleaseEvent.xkey.time == windowEvent.xkey.time);
            bool heldDown = keys[windowEvent.xkey.keycode / 8] & (1 << (windowEvent.xkey.keycode % 8));

            // KeyRelease event + key down = repeated event --> discard
            if (windowEvent.type == KeyRelease)
            {
                m_lastKeyReleaseEvent = windowEvent;
                if (heldDown || duplicate)
                    return false;
            }

            // KeyPress event + key repeat disabled + matching KeyRelease event = repeated event --> discard
            if ((windowEvent.type == KeyPress) && !m_keyRepeat && duplicate)
            {
               return false;
            }

It seems to work so far, but there is one situation where duplicates still occur: If the timestamp is minimally different. It happens sometimes that the time differs by 1 or 2 milliseconds. You could use an inequation for the duplicate expression, just to make code even uglier

By the way, you should really split your processEvent() function up
Also, the first two if statements in the original code can be merged using operator&&.

[Laurent]

Ok, I see. I'll rewrite that to include your fix (including the 2 ms lag in timestamps -- SDL does it too so it's not that bad) and clean this function a little bit

Thanks a lot for your help.

[Nexus]

No problem, I'm always glad if I can contribute to bugfixes

About the 2ms, I don't know how high the delay may be, it's just the highest one I have seen with a few tests. But SDL should know it

[Laurent]

Since you already have a test environment ready to work, could you quickly test this new code and tell me if it works as expected?

    // Detect repeated key events
    if (((windowEvent.type == KeyPress) || (windowEvent.type == KeyRelease)) && (windowEvent.xkey.keycode < 256))
    {
        // To detect if it is a repeated key event, we check the current state of the key:
        // - If the state is "down", KeyReleased events must obviously be discarded
        // - KeyPress events are a little bit harder to handle: they depend on the KeyRepeatEnabled state,
        //   and we need to properly forward the first one
       
        // Check if the key is currently down
        char keys[32];
        XQueryKeymap(m_display, keys);
        bool isDown = keys[windowEvent.xkey.keycode / 8] & (1 << (windowEvent.xkey.keycode % 8));

        // Check if it's a duplicate event
        bool isDuplicate = (windowEvent.xkey.keycode == m_lastKeyReleaseEvent.xkey.keycode) &&
                           (windowEvent.xkey.time - m_lastKeyReleaseEvent.xkey.time <= 2);

        // KeyRelease event + key down or duplicate event = repeated event --> discard
        if ((windowEvent.type == KeyRelease) && (isDown || isDuplicate))
            return false;

        // KeyPress event + matching KeyRelease event = repeated event --> discard if key repeat is disabled
        if ((windowEvent.type == KeyPress) && isDuplicate && !m_keyRepeat)
            return false;

        // Keep track of the last KeyRelease event
        if (windowEvent.type == KeyRelease)
            m_lastKeyReleaseEvent = windowEvent;
    }

I still don't get how a duplicate KeyRelease event can occur when the key is not down, but if it solves the problem, wonderful

About the 2ms, I don't know how high the delay may be, it's just the highest one I have seen with a few tests. But SDL should know it

SDL uses < 2, so they may not know better than us

[Nexus]

The code doesn't work; it fires repeated events, when m_keyRepeat is false.

You need to store the last release event also if it is discarded, i.e. reorder the if statements:

        // Keep track of the last KeyRelease event
        if (windowEvent.type == KeyRelease)
            m_lastKeyReleaseEvent = windowEvent;

        // KeyRelease event + key down or duplicate event = repeated event --> discard
        if ((windowEvent.type == KeyRelease) && (isDown || isDuplicate))
            return false;

        // KeyPress event + matching KeyRelease event = repeated event --> discard if key repeat is disabled
        if ((windowEvent.type == KeyPress) && isDuplicate && !m_keyRepeat)
            return false;

This code works as expected. I have however managed to have a delay of 3 or even 4 ms
(very rarely, and that computer is already half a decade old...)

[Laurent]

Oops! I forgot that the function returned immediately when it found an event to discard.

I have however managed to have a delay of 3 or even 4 ms

Let's use 5 ms then, it is very unlikely (impossible?) that a human can trigger two events that close.
In case you have extra free time (and I know you don't ), it would be interesting to test SDL (latest 1.3 sources) on your PC and see if you can break their code with your 4 ms delay.

[Nexus]

In case you have extra free time (and I know you don't ), it would be interesting to test SDL (latest 1.3 sources) on your PC and see if you can break their code with your 4 ms delay.

It would indeed be interesting, but at the moment I really don't have the time, already this took a lot of time (especially setting everything up and debugging on Linux).

Maybe in February