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

Fixed modifiers causing sf::Keyboard::Unknown being returned #1022

Merged
merged 1 commit into from May 16, 2016

Conversation

Projects
None yet
3 participants
@binary1248
Member

binary1248 commented Dec 24, 2015

Fixes #1012.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 3, 2016

Member

Can someone else on Linux test this as well?

Member

eXpl0it3r commented Jan 3, 2016

Can someone else on Linux test this as well?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 15, 2016

Member

Bump.

Member

binary1248 commented Jan 15, 2016

Bump.

Keyboard::Key key = Keyboard::Unknown;
// Try each KeySym index (modifier group) until we get a match
for (int i = 0; i < 4; ++i)

This comment has been minimized.

@TankOs

TankOs Jan 20, 2016

Member

Where does the 4 (or 0-3) come from? I looked up XLookupKeysym() but as always, X11 documentation is a pain in the *ss.

@TankOs

TankOs Jan 20, 2016

Member

Where does the 4 (or 0-3) come from? I looked up XLookupKeysym() but as always, X11 documentation is a pain in the *ss.

This comment has been minimized.

@binary1248

binary1248 Jan 20, 2016

Member

There are up to 4 modifier groups per cap, e.g. on this layout. One for each symbol in the 4 corners of a cap. The API probably doesn't state a maximum value for this because there might be keyboards with even more. 😁

@binary1248

binary1248 Jan 20, 2016

Member

There are up to 4 modifier groups per cap, e.g. on this layout. One for each symbol in the 4 corners of a cap. The API probably doesn't state a maximum value for this because there might be keyboards with even more. 😁

This comment has been minimized.

@TankOs

TankOs Jan 21, 2016

Member

I see, so on Linux, this can basically be Shift, Control, Super, Meta, Hyper, Mod1, Mod2, Mod3, Mod4, Mod5. :P Thanks for clarification.

@TankOs

TankOs Jan 21, 2016

Member

I see, so on Linux, this can basically be Shift, Control, Super, Meta, Hyper, Mod1, Mod2, Mod3, Mod4, Mod5. :P Thanks for clarification.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 20, 2016

Member

Works for me: 👍 I added another patch on top of this to fix RAlt detection. Might want to merge: bugfix/unix_keyevents_ralt

Member

TankOs commented Jan 20, 2016

Works for me: 👍 I added another patch on top of this to fix RAlt detection. Might want to merge: bugfix/unix_keyevents_ralt

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 20, 2016

Member

I already considered this in #922, but came to the conclusion that this is a user configuration option.

Member

binary1248 commented Jan 20, 2016

I already considered this in #922, but came to the conclusion that this is a user configuration option.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 21, 2016

Member

It's indeed a configuration option, but we should really go with the defaults here. I asked several people on IRC to show their xmodmap output, and it has been consistent with this:

% xmodmap 
xmodmap:  up to 4 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x32),  Shift_R (0x3e)
lock        Caps_Lock (0x42)
control     Control_L (0x25),  Control_R (0x69)
mod1        Alt_L (0x40),  Meta_L (0xcd)
mod2        Num_Lock (0x4d)
mod3      
mod4        Super_L (0x85),  Super_R (0x86),  Super_L (0xce),  Hyper_L (0xcf)
mod5        ISO_Level3_Shift (0x5c),  Mode_switch (0xcb)

Alt_R is never mapped, so that key doesn't work at all. Asking the user to configure X.Org just to have a working AltGr key is ridiculous.

Member

TankOs commented Jan 21, 2016

It's indeed a configuration option, but we should really go with the defaults here. I asked several people on IRC to show their xmodmap output, and it has been consistent with this:

% xmodmap 
xmodmap:  up to 4 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x32),  Shift_R (0x3e)
lock        Caps_Lock (0x42)
control     Control_L (0x25),  Control_R (0x69)
mod1        Alt_L (0x40),  Meta_L (0xcd)
mod2        Num_Lock (0x4d)
mod3      
mod4        Super_L (0x85),  Super_R (0x86),  Super_L (0xce),  Hyper_L (0xcf)
mod5        ISO_Level3_Shift (0x5c),  Mode_switch (0xcb)

Alt_R is never mapped, so that key doesn't work at all. Asking the user to configure X.Org just to have a working AltGr key is ridiculous.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 21, 2016

Member

My xmodmap output looks like this:

xmodmap:  up to 4 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x32),  Shift_R (0x3e)
lock        Caps_Lock (0x42)
control     Control_L (0x25),  Control_R (0x69)
mod1        Alt_L (0x40),  Alt_R (0x6c),  Meta_L (0xcd)
mod2        Num_Lock (0x4d)
mod3      
mod4        Super_L (0x85),  Super_R (0x86),  Super_L (0xce),  Hyper_L (0xcf)
mod5        ISO_Level3_Shift (0x5c),  Mode_switch (0xcb)

Like I said, this depends on the user's mapping. Using English mappings will also map Alt_R because the key isn't used as a modifier. Because AltGr is a key modifier on e.g. German mappings and is used to shift keysyms, it isn't mapped on those, with reason. Switching between mappings will also add/remove the Alt_R mapping depending on the target layout. If a user really wanted to use a key that is intended to be used as a keysym shift as a "normal" non-modifier key, they would be better off configuring it to behave as such. This keeps SFML consistent with other libraries who also decided that handling Alt_R is up to the user if they have more than 1 option. Applications that aim to be "international friendly" should not endorse use of Alt_R anyway.

Member

binary1248 commented Jan 21, 2016

My xmodmap output looks like this:

xmodmap:  up to 4 keys per modifier, (keycodes in parentheses):

shift       Shift_L (0x32),  Shift_R (0x3e)
lock        Caps_Lock (0x42)
control     Control_L (0x25),  Control_R (0x69)
mod1        Alt_L (0x40),  Alt_R (0x6c),  Meta_L (0xcd)
mod2        Num_Lock (0x4d)
mod3      
mod4        Super_L (0x85),  Super_R (0x86),  Super_L (0xce),  Hyper_L (0xcf)
mod5        ISO_Level3_Shift (0x5c),  Mode_switch (0xcb)

Like I said, this depends on the user's mapping. Using English mappings will also map Alt_R because the key isn't used as a modifier. Because AltGr is a key modifier on e.g. German mappings and is used to shift keysyms, it isn't mapped on those, with reason. Switching between mappings will also add/remove the Alt_R mapping depending on the target layout. If a user really wanted to use a key that is intended to be used as a keysym shift as a "normal" non-modifier key, they would be better off configuring it to behave as such. This keeps SFML consistent with other libraries who also decided that handling Alt_R is up to the user if they have more than 1 option. Applications that aim to be "international friendly" should not endorse use of Alt_R anyway.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

US keyboards don't have AltGr, but they still have an Alt key there, which is also mapped by SFML, to sf::Keyboard::Key::RAlt. On my system, it's mapped to ::Unknown, because in the same place I have AltGr, which is not mapped by SFML.

The simple question is: Why is Alt_R mapped, but ISO_Shift_Level_3 and Mode_Switch are not? All 3 symbols are triggered by the same key on the proper keyboard layouts. Even if AltGr acts as a modifier key on non-US layouts, what's the problem? Shift, Control and (L)Alt are also modifier keys, yet we map them (for good reasons).

Member

TankOs commented Jan 22, 2016

US keyboards don't have AltGr, but they still have an Alt key there, which is also mapped by SFML, to sf::Keyboard::Key::RAlt. On my system, it's mapped to ::Unknown, because in the same place I have AltGr, which is not mapped by SFML.

The simple question is: Why is Alt_R mapped, but ISO_Shift_Level_3 and Mode_Switch are not? All 3 symbols are triggered by the same key on the proper keyboard layouts. Even if AltGr acts as a modifier key on non-US layouts, what's the problem? Shift, Control and (L)Alt are also modifier keys, yet we map them (for good reasons).

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jan 22, 2016

Member

Simple... because Shift, Control and LAlt are also present on all keyboards. If SFML were designed from the beginning to be e.g. German-centric, I could imagine there also being an sf::Keyboard::Key::Oe and sf::Keyboard::Key::Ae or similar keys, because those have to be made accessible to people who want to check for those keystrokes. However, besides being of use to people who use German keyboards (or other keyboards where they are present) they are pretty useless. If you are using a German keyboard, there is no direct, single keystroke way of triggering a e.g. sf::Keyboard::Key::LBracket keystroke either, because there is no unmodified way to get this.

The reason why you don't get RAlt using a German layout is well.... because Germans don't have an RAlt key. 😛 They have an AltGr key, and that is not the same as RAlt even though some might consider it as such (if it would have been named something completely different like "Right Shift" or something there would probably be less confusion). That is one of the disadvantages of having an English-centric API, but like I said, other libraries don't do much different either.

Member

binary1248 commented Jan 22, 2016

Simple... because Shift, Control and LAlt are also present on all keyboards. If SFML were designed from the beginning to be e.g. German-centric, I could imagine there also being an sf::Keyboard::Key::Oe and sf::Keyboard::Key::Ae or similar keys, because those have to be made accessible to people who want to check for those keystrokes. However, besides being of use to people who use German keyboards (or other keyboards where they are present) they are pretty useless. If you are using a German keyboard, there is no direct, single keystroke way of triggering a e.g. sf::Keyboard::Key::LBracket keystroke either, because there is no unmodified way to get this.

The reason why you don't get RAlt using a German layout is well.... because Germans don't have an RAlt key. 😛 They have an AltGr key, and that is not the same as RAlt even though some might consider it as such (if it would have been named something completely different like "Right Shift" or something there would probably be less confusion). That is one of the disadvantages of having an English-centric API, but like I said, other libraries don't do much different either.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

Ä/Ö/Ü etc. are mapped to their US counterparts. The implementation is not ideal, because there are indeed many differences (this is also what's being discussed...more or less ;)), but at least I can detect key presses.

While there's no Key::Ue, I can still check for the English counterpart, and that's good enough. I can just get an image of the default US layout and can work with that. There's one key that I can't simply map, and that's less-than, because on the US keyboard they have a larger shift key there.

However, the point is: I can use the keys. But I can't use AltGr, it's completely dead. And that makes no sense, because it's at the exact same location as on the US keyboard, just without "Gr". I really don't see a practical reason why this can't/shouldn't be supported. Not being able to map keys makes me go crazy when it's technically possible, even without a keyboard layout rewrite in SFML.

Member

TankOs commented Jan 22, 2016

Ä/Ö/Ü etc. are mapped to their US counterparts. The implementation is not ideal, because there are indeed many differences (this is also what's being discussed...more or less ;)), but at least I can detect key presses.

While there's no Key::Ue, I can still check for the English counterpart, and that's good enough. I can just get an image of the default US layout and can work with that. There's one key that I can't simply map, and that's less-than, because on the US keyboard they have a larger shift key there.

However, the point is: I can use the keys. But I can't use AltGr, it's completely dead. And that makes no sense, because it's at the exact same location as on the US keyboard, just without "Gr". I really don't see a practical reason why this can't/shouldn't be supported. Not being able to map keys makes me go crazy when it's technically possible, even without a keyboard layout rewrite in SFML.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 22, 2016

Member

I personally don't really mind if AltGr gets mapped or not, the only reason as I already mentioned why I closed my initial PR was because it diverted from what other libraries did. If consistency with other libraries in this regard is not a concern I don't have anything against your change.

@TankOs Feel free to open a PR for bugfix/unix_keyevents_ralt and/or feature/more_keys and close this PR as superseded.

Member

binary1248 commented Feb 22, 2016

I personally don't really mind if AltGr gets mapped or not, the only reason as I already mentioned why I closed my initial PR was because it diverted from what other libraries did. If consistency with other libraries in this regard is not a concern I don't have anything against your change.

@TankOs Feel free to open a PR for bugfix/unix_keyevents_ralt and/or feature/more_keys and close this PR as superseded.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 29, 2016

Member

So is this PR obsolete (once @TankOs implements the other bits)?

Member

eXpl0it3r commented Mar 29, 2016

So is this PR obsolete (once @TankOs implements the other bits)?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 29, 2016

Member

Yes.

Member

binary1248 commented Mar 29, 2016

Yes.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 May 8, 2016

Member

Until the Windows and OS X implementations for #1045 are ready, this will have to take its place in order to fix #1012 for 2.4. #1045 will be rebased onto master in 2.5.

Member

binary1248 commented May 8, 2016

Until the Windows and OS X implementations for #1045 are ready, this will have to take its place in order to fix #1012 for 2.4. #1045 will be rebased onto master in 2.5.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r May 8, 2016

Member

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

Member

eXpl0it3r commented May 8, 2016

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

Fixed modifiers causing sf::Keyboard::Unknown being returned in key e…
…vents on Unix (#1012). On Unix, SFML now tries harder to create proper key events on keyboards that shift keys which are typically unshifted on QWERTY layouts (this makes the numeric codes usable even on AZERTY layouts).

@eXpl0it3r eXpl0it3r merged commit 556371e into master May 16, 2016

14 checks passed

debian-gcc-64 Build #138 done.
Details
freebsd-gcc-64 Build #138 done.
Details
osx-clang-el-capitan Build #18 done.
Details
static-analysis Build #138 done.
Details
windows-gcc-492-tdm-32 Build #23 done.
Details
windows-gcc-492-tdm-64 Build #23 done.
Details
windows-gcc-530-mingw-32 Build #23 done.
Details
windows-gcc-530-mingw-64 Build #23 done.
Details
windows-vc11-32 Build #139 done.
Details
windows-vc11-64 Build #139 done.
Details
windows-vc12-32 Build #139 done.
Details
windows-vc12-64 Build #137 done.
Details
windows-vc14-32 Build #138 done.
Details
windows-vc14-64 Build #140 done.
Details

@eXpl0it3r eXpl0it3r deleted the bugfix/unix_keyevents branch May 16, 2016

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