New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disabling key repeat broken on Linux if window recreated #564

Closed
abodelot opened this Issue Apr 4, 2014 · 9 comments

Comments

Projects
None yet
4 participants
@abodelot
Contributor

abodelot commented Apr 4, 2014

On Linux, if the SFML window is created more than once, then it is impossible to disable key repeat, calling the method setKeyRepeatEnabled(false) has no effect.

This bug can be reproduced with this snippet:

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

int main()
{
    sf::Window app;
    app.create(sf::VideoMode(480, 320), "sfml window 1");
    app.create(sf::VideoMode(480, 320), "sfml window 2");
    app.setKeyRepeatEnabled(false);

    while (app.isOpen())
    {
        sf::Event event;
        while (app.pollEvent(event))
        {
            if (event.type == sf::Event::KeyPressed)
                std::cout << "key pressed: " << event.key.code << std::endl;
            else if (event.type == sf::Event::Closed)
                app.close();
        }
        app.display();
    }
    return 0;
}

When a key is pressed once, the message is printed as long as the key is hold down.
Commenting the window recreation line app.create(sf::VideoMode(480, 320), "sfml window 2")) makes the faulty behaviour disappear.

Tested with the current SFML git snapshot.

Bug still happens if I close the first window before creating the 2nd one, or if I disable key repeat on the first window as well.

The bug doesn't occur on Windows. I don't know about Mac OS.

@LaurentGomila LaurentGomila added this to the 2.x milestone Apr 4, 2014

@LaurentGomila LaurentGomila self-assigned this Apr 4, 2014

@Kojirion

This comment has been minimized.

Show comment
Hide comment
@Kojirion

Kojirion Apr 6, 2014

Can reproduce this on Kubuntu 12.04.

Kojirion commented Apr 6, 2014

Can reproduce this on Kubuntu 12.04.

@abodelot

This comment has been minimized.

Show comment
Hide comment
@abodelot

abodelot Apr 7, 2014

Contributor

Here's what I've investigated so far:

Disabling key repeat mechanism in X11 is done by checking if the event following KeyRelease is a KeyPress with same keycode and timestamp. If so, the next KeyPress event in the queue is discarded. This test is located here:

src/SFML/Window/Unix/WindowImplX11.cpp#L639

But when the window is created more than once, this test fails because the KeyRelease event isn't followed by a KeyPress event anymore, a ReparentNotify (XReparentEvent) is fired instead (nextEvent.type == ReparentNotify).

Xlib documentation says:

The X server generates this event whenever a client application calls XReparentWindow() and the window is actually reparented.

But there isn't any occurrence of XReparentWindow() in SFML source code, while this has obviously something to do with recreating the window implementation.

Contributor

abodelot commented Apr 7, 2014

Here's what I've investigated so far:

Disabling key repeat mechanism in X11 is done by checking if the event following KeyRelease is a KeyPress with same keycode and timestamp. If so, the next KeyPress event in the queue is discarded. This test is located here:

src/SFML/Window/Unix/WindowImplX11.cpp#L639

But when the window is created more than once, this test fails because the KeyRelease event isn't followed by a KeyPress event anymore, a ReparentNotify (XReparentEvent) is fired instead (nextEvent.type == ReparentNotify).

Xlib documentation says:

The X server generates this event whenever a client application calls XReparentWindow() and the window is actually reparented.

But there isn't any occurrence of XReparentWindow() in SFML source code, while this has obviously something to do with recreating the window implementation.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 7, 2014

Member

XReparentEvent is triggered only when the window is created again, isn't it? So how can it have an effect on events that are triggered later when you press a key?

Member

LaurentGomila commented Apr 7, 2014

XReparentEvent is triggered only when the window is created again, isn't it? So how can it have an effect on events that are triggered later when you press a key?

@Kojirion

This comment has been minimized.

Show comment
Hide comment
@Kojirion

Kojirion Apr 7, 2014

My spontaneous answer would be, because it isn't consumed and sticks around instead? (but I have no idea if it's really the case)

Kojirion commented Apr 7, 2014

My spontaneous answer would be, because it isn't consumed and sticks around instead? (but I have no idea if it's really the case)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 7, 2014

Member

All X11 events are consumed, I can't let the internal event queue grow forever ;)

Member

LaurentGomila commented Apr 7, 2014

All X11 events are consumed, I can't let the internal event queue grow forever ;)

@abodelot

This comment has been minimized.

Show comment
Hide comment
@abodelot

abodelot Apr 7, 2014

Contributor

@LaurentGomila:

XReparentEvent is triggered only when the window is created again, isn't it?

It seems that's not the case. As long as the pressed key is hold down, the event following the KeyRelease is always a ReparentNotify. If you're positive that all X11 events are consumed, that would mean the ReparentNotify keeps being fired during key repeat.

Another odd behaviour is that when the key is hold down, I can only detect this repeated ReparentNotify by peeking the next event while processing the current KeyRelease event:

if (windowEvent.type == KeyRelease)
{
    // Check if there's a matching KeyPress event in the queue
    XEvent nextEvent;
    if (XPending(m_display))
    {
        // Grab it but don't remove it from the queue, it still needs to be processed :)
        XPeekEvent(m_display, &nextEvent);
        if (nextEvent.type == ReparentNotify)
            puts("1. KeyRelease followed by ReparentNotify");

        if (nextEvent.type == KeyPress)
        {   
            // Check if it is a duplicated event (same timestamp as the KeyRelease event)
            /* ... snip ...  */
        }
    }
}

// Convert the X11 event to a sf::Event
switch (windowEvent.type)
{
    case ReparentNotify:
        puts("2. processing ReparentNotify");
        break;
    case ....

Message 2. is printed only once when window is created for the 2nd time (which is expected behavior).
Message 1. (and only this one) keeps being printed as long as the key is hold down (which causes the bug).

Contributor

abodelot commented Apr 7, 2014

@LaurentGomila:

XReparentEvent is triggered only when the window is created again, isn't it?

It seems that's not the case. As long as the pressed key is hold down, the event following the KeyRelease is always a ReparentNotify. If you're positive that all X11 events are consumed, that would mean the ReparentNotify keeps being fired during key repeat.

Another odd behaviour is that when the key is hold down, I can only detect this repeated ReparentNotify by peeking the next event while processing the current KeyRelease event:

if (windowEvent.type == KeyRelease)
{
    // Check if there's a matching KeyPress event in the queue
    XEvent nextEvent;
    if (XPending(m_display))
    {
        // Grab it but don't remove it from the queue, it still needs to be processed :)
        XPeekEvent(m_display, &nextEvent);
        if (nextEvent.type == ReparentNotify)
            puts("1. KeyRelease followed by ReparentNotify");

        if (nextEvent.type == KeyPress)
        {   
            // Check if it is a duplicated event (same timestamp as the KeyRelease event)
            /* ... snip ...  */
        }
    }
}

// Convert the X11 event to a sf::Event
switch (windowEvent.type)
{
    case ReparentNotify:
        puts("2. processing ReparentNotify");
        break;
    case ....

Message 2. is printed only once when window is created for the 2nd time (which is expected behavior).
Message 1. (and only this one) keeps being printed as long as the key is hold down (which causes the bug).

@abodelot

This comment has been minimized.

Show comment
Hide comment
@abodelot

abodelot Apr 7, 2014

Contributor

Okay, some good news.
So there are a lot of other events which get stuck between KeyRelease and KeyPress.

Until the XReparentEvent is properly discarded, we've got a cycling sequence of (KeyRelease, ConfigureNotify, MapNotify, FocusIn, UnmapNotify, FocusOut, DestroyNotify, and finally KeyPress) yield as long as the pressed key is hold down.

Discarding remaining events upon the window reparenting successfully fixes the problem. PR incoming!

Contributor

abodelot commented Apr 7, 2014

Okay, some good news.
So there are a lot of other events which get stuck between KeyRelease and KeyPress.

Until the XReparentEvent is properly discarded, we've got a cycling sequence of (KeyRelease, ConfigureNotify, MapNotify, FocusIn, UnmapNotify, FocusOut, DestroyNotify, and finally KeyPress) yield as long as the pressed key is hold down.

Discarding remaining events upon the window reparenting successfully fixes the problem. PR incoming!

LaurentGomila added a commit that referenced this issue Apr 7, 2014

Merge pull request #567 from abodelot/x11keyrepeat
Fixed disabling key repeat on Linux (#564)

@LaurentGomila LaurentGomila modified the milestones: 2.2, 2.x Apr 7, 2014

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 7, 2014

Member

Thank you for your quick and efficient contribution :)

Member

LaurentGomila commented Apr 7, 2014

Thank you for your quick and efficient contribution :)

@Bromeon Bromeon added the resolved label Apr 7, 2014

@abodelot

This comment has been minimized.

Show comment
Hide comment
@abodelot

abodelot Apr 7, 2014

Contributor

I'm glad I could help!

Contributor

abodelot commented Apr 7, 2014

I'm glad I could help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment