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.

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

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

Merged
merged 1 commit into from Sep 28, 2017

Conversation

binary1248
Copy link
Member

Fixes #1223.
Closes #1230.

@eXpl0it3r
Copy link
Member

@naezith @texus Can you give this a spin?

@texus
Copy link
Contributor

texus commented Sep 25, 2017

This solves the issues on both my linux computers. 👍

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

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 😸

@eXpl0it3r
Copy link
Member

Tested on Debian x64 Gnome. Was able to reproduce the issue and this PR fixed it.

Copy link
Member

@eXpl0it3r eXpl0it3r left a comment

Choose a reason for hiding this comment

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

Works as intended and makes sense to me in the context.

@eXpl0it3r eXpl0it3r moved this from Review & Testing to Merged / Superseded in SFML 2.5.0 Sep 25, 2017
@eXpl0it3r eXpl0it3r moved this from Merged / Superseded to Ready in SFML 2.5.0 Sep 25, 2017
@naezith
Copy link

naezith commented Sep 26, 2017

Sorry for testing late, I just did. Absolutely nothing changed for me. #1223 is still there. @eXpl0it3r @binary1248

Linux (Ubuntu 16.04)

@eXpl0it3r
Copy link
Member

eXpl0it3r commented Sep 26, 2017

Having tested and reviewed the code myself, I don't trust your result. Did you make sure that the correct library is linked and not an old version.

If the new code is actually running, then the only reason it fails must be because the timestamps reported by X don't match, which I can't think is happening as three independent testers report it to work fine.

@naezith
Copy link

naezith commented Sep 26, 2017

@eXpl0it3r I manually went into Commits of this, did "Browse the repository at this point in the history", then downloaded .zip of whole repository from that page. Checked if the changes are there. Renamed my original SFML and build folders. Put this new one there and changed the name to SFML, generated files with cmake, then built it with "make" into the build folder. Then ran this script I wrote which copies the libs to the game path:

#!/bin/bash

game_path="/home/naezith/.steam/steam/steamapps/common/Remnants of Naezith Beta"
linux_content="/media/naezith/SSD/steamworks_sdk/tools/ContentBuilder/content/linux_content"
TARGETS=("$linux_content/lib/" "$game_path/lib/") 

for TARGET_PATH in "${TARGETS[@]}"; do
	echo " "
	echo ">> NEW TARGET PATH:""$TARGET_PATH"

	# Copy SFML
	SFML_PATH=/home/naezith/sfml-build/lib/
	SFML_LIBS=(audio graphics window network system)

	for f in "${SFML_LIBS[@]}"; do
		path=$SFML_PATH"libsfml-"$f".so.2.4"
		echo $path
		cp $path "$TARGET_PATH"
	done 
done

Then I opened the SFML project and compiled it, and tested. Is there any suspicious part in this? Maybe I am doing this testing thing wrong.

@naezith
Copy link

naezith commented Sep 26, 2017

My bad, there was SFML installed directly to /usr/local/lib kind of folders. They were prioritized I guess. I cleaned them now. I did the same stuff as I explained and now it works! Thanks a lot.

@eXpl0it3r eXpl0it3r merged commit 4494498 into master Sep 28, 2017
@eXpl0it3r eXpl0it3r moved this from Ready to Merged / Superseded in SFML 2.5.0 Sep 28, 2017
@eXpl0it3r eXpl0it3r deleted the bugfix/unix_keyrepeat branch September 28, 2017 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

5 participants