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 keypad keys not being detected on Linux #910

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@mickelson
Contributor

mickelson commented Jun 13, 2015

This pull request fixes keypad keys not being detected on Linux.

Tested on Debian 8.0.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jun 13, 2015

Member

Thanks for the fix. Your patch however, is based on an outdated revision. The current codebase looks quite different (albeit still containing the bug), so it would be nice if you could modify your commit to apply to the current master instead.

Also, you shouldn't bother keeping the old lines. SFML should only look up a single keysym that maps to an sf::Keyboard::Key value. If multiple keysyms that map to the same sf::Keyboard::Key value are looked up then it is another bug that has to be fixed.

Member

binary1248 commented Jun 13, 2015

Thanks for the fix. Your patch however, is based on an outdated revision. The current codebase looks quite different (albeit still containing the bug), so it would be nice if you could modify your commit to apply to the current master instead.

Also, you shouldn't bother keeping the old lines. SFML should only look up a single keysym that maps to an sf::Keyboard::Key value. If multiple keysyms that map to the same sf::Keyboard::Key value are looked up then it is another bug that has to be fixed.

@mickelson

This comment has been minimized.

Show comment
Hide comment
@mickelson

mickelson Jun 13, 2015

Contributor

My apologies, I appear to have completely failed at updating this to master

Please ignore for now, I will fix this

Contributor

mickelson commented Jun 13, 2015

My apologies, I appear to have completely failed at updating this to master

Please ignore for now, I will fix this

@mickelson

This comment has been minimized.

Show comment
Hide comment
@mickelson

mickelson Jun 13, 2015

Contributor

Ok, I believe I've got it sorted out now: I've updated to master and removed the old lines as requested.

I also made the fix in InputImpl.cpp, so that sf::Keyboard::isKeyPressed() will pick up the keypad as well. This might be a bad idea though, the status of the numlock key affects what gets returned by isKeyPressed.

Contributor

mickelson commented Jun 13, 2015

Ok, I believe I've got it sorted out now: I've updated to master and removed the old lines as requested.

I also made the fix in InputImpl.cpp, so that sf::Keyboard::isKeyPressed() will pick up the keypad as well. This might be a bad idea though, the status of the numlock key affects what gets returned by isKeyPressed.

@binary1248 binary1248 added this to the 2.3.2 milestone Jun 27, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 4, 2015

Member

I confirm this works on Ubuntu 15.04 with ch-fr keyboard. Ready to be merged for me. 👍

Member

mantognini commented Jul 4, 2015

I confirm this works on Ubuntu 15.04 with ch-fr keyboard. Ready to be merged for me. 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 11, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon into the 2.3.x branch, unless someone raises any concerns.

Member

eXpl0it3r commented Jul 11, 2015

This PR has been added to my merge list, meaning it will be merged soon into the 2.3.x branch, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jul 13, 2015

Member

Merged in da8a325 on branch 2.3.x

Member

eXpl0it3r commented Jul 13, 2015

Merged in da8a325 on branch 2.3.x

@eXpl0it3r eXpl0it3r closed this Jul 13, 2015

@eXpl0it3r eXpl0it3r self-assigned this Jul 13, 2015

@Hapaxia

This comment has been minimized.

Show comment
Hide comment
@Hapaxia

Hapaxia Aug 10, 2015

Contributor

It looks like the merged patch does not match the one by mickelson.
For some reason, the keys that are 'patched' in InputImpl in the merged version are not the number pad keys.

Contributor

Hapaxia commented Aug 10, 2015

It looks like the merged patch does not match the one by mickelson.
For some reason, the keys that are 'patched' in InputImpl in the merged version are not the number pad keys.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 10, 2015

Member

Not sure what you're looking at but this and this are the same expect for spacing. The commit ID won't be the same since I had to rebase the patch onto the 2.3.x branch.

So the question is, how do you come to your conclusion?

Member

eXpl0it3r commented Aug 10, 2015

Not sure what you're looking at but this and this are the same expect for spacing. The commit ID won't be the same since I had to rebase the patch onto the 2.3.x branch.

So the question is, how do you come to your conclusion?

@Hapaxia

This comment has been minimized.

Show comment
Hide comment
@Hapaxia

Hapaxia Aug 10, 2015

Contributor

They aren't the same.
First one:
keycodeMap[sf::Keyboard::Numpad0] = getKeycode(XK_KP_0);
Second one:
keycodeMap[sf::Keyboard::Num0] = getKeycode(XK_0);

Contributor

Hapaxia commented Aug 10, 2015

They aren't the same.
First one:
keycodeMap[sf::Keyboard::Numpad0] = getKeycode(XK_KP_0);
Second one:
keycodeMap[sf::Keyboard::Num0] = getKeycode(XK_0);

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 10, 2015

Member

Oh, now I see it. This must have been a mistake on my side, I'll make sure to fix it asap. Sorry for the inconvenience.

Member

eXpl0it3r commented Aug 10, 2015

Oh, now I see it. This must have been a mistake on my side, I'll make sure to fix it asap. Sorry for the inconvenience.

eXpl0it3r added a commit that referenced this pull request Aug 12, 2015

eXpl0it3r added a commit that referenced this pull request Aug 12, 2015

eXpl0it3r added a commit that referenced this pull request Aug 12, 2015

@mickelson mickelson deleted the mickelson:keypad_fix branch Sep 9, 2015

@silverweed

This comment has been minimized.

Show comment
Hide comment
@silverweed

silverweed Sep 12, 2015

Sorry for asking, but is this fix working in the downloadable 2.3.1 version or should I get the repository version to make it work?

silverweed commented Sep 12, 2015

Sorry for asking, but is this fix working in the downloadable 2.3.1 version or should I get the repository version to make it work?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Sep 12, 2015

Member

It will be available in the 2.3.2 version (see here), which you can already get by compiling SFML yourself. 😉

Member

mantognini commented Sep 12, 2015

It will be available in the 2.3.2 version (see here), which you can already get by compiling SFML yourself. 😉

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 12, 2015

Member

And if you use Linux, you should anyways build from source, to remove any chances of mismatching dependencies versions.

Member

eXpl0it3r commented Sep 12, 2015

And if you use Linux, you should anyways build from source, to remove any chances of mismatching dependencies versions.

@silverweed

This comment has been minimized.

Show comment
Hide comment
@silverweed

silverweed Sep 12, 2015

Thank you very much, will do. :)

silverweed commented Sep 12, 2015

Thank you very much, will do. :)

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