Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix X11 key repeat handling not filtering out events from other windows #1291

Merged
merged 1 commit into from Sep 28, 2017
Merged

Fix X11 key repeat handling not filtering out events from other windows #1291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 36 additions & 19 deletions src/SFML/Window/Unix/WindowImplX11.cpp
View file
Open in desktop
Expand Up @@ -77,6 +77,21 @@ namespace

static const unsigned int maxTrialsCount = 5;

// Predicate we use to find key repeat events in processEvent
struct KeyRepeatFinder
{
KeyRepeatFinder(unsigned int keycode, Time time) : keycode(keycode), time(time) {}

// Predicate operator that checks event type, keycode and timestamp
bool operator()(const XEvent& event)
{
return ((event.type == KeyPress) && (event.xkey.keycode == keycode) && (event.xkey.time - time < 2));
}

unsigned int keycode;
Time time;
};

// Filter the events received by windows (only allow those matching a specific window)
Bool checkEvent(::Display*, XEvent* event, XPointer userData)
{
Expand Down Expand Up @@ -752,8 +767,16 @@ WindowHandle WindowImplX11::getSystemHandle() const
void WindowImplX11::processEvents()
{
XEvent event;

// Pick out the events that are interesting for this window
while (XCheckIfEvent(m_display, &event, &checkEvent, reinterpret_cast<XPointer>(m_window)))
m_events.push_back(event);

// Handle the events for this window that we just picked out
while (!m_events.empty())
{
event = m_events.front();
m_events.pop_front();
processEvent(event);
}
}
Expand Down Expand Up @@ -1533,29 +1556,23 @@ bool WindowImplX11::processEvent(XEvent& windowEvent)
// - Discard both duplicated KeyPress and KeyRelease events when KeyRepeatEnabled is false

// Detect repeated key events
// (code shamelessly taken from SDL)
if (windowEvent.type == KeyRelease)
{
// Check if there's a matching KeyPress event in the queue
XEvent nextEvent;
if (XPending(m_display))
// Find the next KeyPress event with matching keycode and time
std::deque<XEvent>::iterator iter = std::find_if(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to do that if m_keyRepeat is false?

Copy link
Member

@eXpl0it3r eXpl0it3r Sep 25, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think so, as it has to fulfill the two tasks mentioned at the function top.

  • Discard duplicated KeyRelease events when KeyRepeatEnabled is true
  • Discard both duplicated KeyPress and KeyRelease events when KeyRepeatEnabled is false

When repeat is true it will ignore the repeated KeyRelease event that X generates.

When repeat is false it will erase the repeated KeyPress event from the queue and ignore the repeated KeyRelease event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't read the code carefully 😸

m_events.begin(),
m_events.end(),
KeyRepeatFinder(windowEvent.xkey.keycode, windowEvent.xkey.time)
);

if (iter != m_events.end())
{
// Grab it but don't remove it from the queue, it still needs to be processed :)
XPeekEvent(m_display, &nextEvent);
if (nextEvent.type == KeyPress)
{
// Check if it is a duplicated event (same timestamp as the KeyRelease event)
if ((nextEvent.xkey.keycode == windowEvent.xkey.keycode) &&
(nextEvent.xkey.time - windowEvent.xkey.time < 2))
{
// If we don't want repeated events, remove the next KeyPress from the queue
if (!m_keyRepeat)
XNextEvent(m_display, &nextEvent);
// If we don't want repeated events, remove the next KeyPress from the queue
if (!m_keyRepeat)
m_events.erase(iter);

// This KeyRelease is a repeated event and we don't want it
return false;
}
}
// This KeyRelease is a repeated event and we don't want it
return false;
}
}

Expand Down
37 changes: 19 additions & 18 deletions src/SFML/Window/Unix/WindowImplX11.hpp
View file
Open in desktop
Expand Up @@ -267,24 +267,25 @@ class WindowImplX11 : public WindowImpl
////////////////////////////////////////////////////////////
// Member data
////////////////////////////////////////////////////////////
::Window m_window; ///< X identifier defining our window
::Display* m_display; ///< Pointer to the display
int m_screen; ///< Screen identifier
XIM m_inputMethod; ///< Input method linked to the X display
XIC m_inputContext; ///< Input context used to get unicode input in our window
bool m_isExternal; ///< Tell whether the window has been created externally or by SFML
int m_oldVideoMode; ///< Video mode in use before we switch to fullscreen
::Cursor m_hiddenCursor; ///< As X11 doesn't provide cursor hiding, we must create a transparent one
::Cursor m_lastCursor; ///< Last cursor used -- this data is not owned by the window and is required to be always valid
bool m_keyRepeat; ///< Is the KeyRepeat feature enabled?
Vector2i m_previousSize; ///< Previous size of the window, to find if a ConfigureNotify event is a resize event (could be a move event only)
bool m_useSizeHints; ///< Is the size of the window fixed with size hints?
bool m_fullscreen; ///< Is the window in fullscreen?
bool m_cursorGrabbed; ///< Is the mouse cursor trapped?
bool m_windowMapped; ///< Has the window been mapped by the window manager?
Pixmap m_iconPixmap; ///< The current icon pixmap if in use
Pixmap m_iconMaskPixmap; ///< The current icon mask pixmap if in use
::Time m_lastInputTime; ///< Last time we received user input
::Window m_window; ///< X identifier defining our window
::Display* m_display; ///< Pointer to the display
int m_screen; ///< Screen identifier
XIM m_inputMethod; ///< Input method linked to the X display
XIC m_inputContext; ///< Input context used to get unicode input in our window
std::deque<XEvent> m_events; ///< Queue we use to store pending events for this window
bool m_isExternal; ///< Tell whether the window has been created externally or by SFML
int m_oldVideoMode; ///< Video mode in use before we switch to fullscreen
::Cursor m_hiddenCursor; ///< As X11 doesn't provide cursor hiding, we must create a transparent one
::Cursor m_lastCursor; ///< Last cursor used -- this data is not owned by the window and is required to be always valid
bool m_keyRepeat; ///< Is the KeyRepeat feature enabled?
Vector2i m_previousSize; ///< Previous size of the window, to find if a ConfigureNotify event is a resize event (could be a move event only)
bool m_useSizeHints; ///< Is the size of the window fixed with size hints?
bool m_fullscreen; ///< Is the window in fullscreen?
bool m_cursorGrabbed; ///< Is the mouse cursor trapped?
bool m_windowMapped; ///< Has the window been mapped by the window manager?
Pixmap m_iconPixmap; ///< The current icon pixmap if in use
Pixmap m_iconMaskPixmap; ///< The current icon mask pixmap if in use
::Time m_lastInputTime; ///< Last time we received user input
};

} // namespace priv
Expand Down