Initial X11 scancode implementation #1400

eliasdaler · 2018-03-29T20:44:48Z

SFML Tasks

Agree on code structure (see below)
Adjust code according to the review comments
Test (use this code for it and switch your keyboard layout to non-US for harder cases)
Merge into the feature/scancode branch (see Scancode #1235)

Hello everyone! This is my first PR and it was pretty hard to get this working! Keycodes, KeySyms, scancodes, SFML's scancodes - it's all different and sometimes it's hard to convert one to another.
A lot of the code was based on GLFW implementation because it deals with very hard cases.

I've introduced X11InputManager singleton, because someone needs to hold mappings between keycodes and SFML's scancodes. If someone comes up with a better alternative - I'll be happy to remake it. :)

getDescription was pretty hard to implement because there's no easy way to convert keycode or keysym to unicode. One way of doing it is using huge lookup tables like this one. But thankfully, we have Xutf8LookupString now. But we need to create a fake event for it to work... which works, but isn't pretty.
And one more thing: some keys like Tab, Enter and Space produce unwanted results - they return " ", "\n" or "\t", so they have to be accounted for. It's even worse for Delete or Backspace, but it's the same thing.

There'll probably be a lot of things which need to be fixed with my code - I haven't done much X11 programming, so maybe I've made some mistakes - I'll be happy to fix them.

…epresentation.

…sely

eliasdaler · 2018-03-29T20:47:14Z

Oh, and one more thing - maybe it's worth putting KeySym <-> sf::Keyboard::Key mappings to a separate header? They make X11InputManager.cpp much longer than it could be.

mantognini · 2018-03-30T07:14:27Z

Fantastic! Looking forward testers feedback.

Feel free to split files in a meaningful manner. I'll probably end up doing something similar for macOS. ;-)

binary1248 · 2018-03-30T18:11:33Z

Is the singleton really necessary? We already have a reference counted Display in Display.cpp. You can initialize the stuff in OpenDisplay, deinitialize the stuff in CloseDisplay and convert all those methods to static methods. I would also give the XInputManager its own XIC. Something tells me that sharing it with some random Window (and even changing it whenever new Windows are created) won't work out so well...

eliasdaler · 2018-03-30T18:59:00Z

Fair enough. But where should I store the mapping between the keycodes and scancodes?

binary1248 · 2018-03-30T19:00:56Z

In an anonymous namespace at the top of XInputManager.cpp like is done in all the other files.

…nts in getDescription now

(also made better names for keycode, sf::Scancode and sf::Key conversion functions)

eliasdaler · 2018-03-31T00:06:32Z

So, I did tons of work on this:

X11InputManager is now KeyboardImpl as it deals with keyboard input only, and we already have JoysticImpl, so it's similar.
We don't need a singleton anymore - everything is static.
I've made a script which generated a giant switch-case which maps KeySym to Unicode from this file. Sadly, XLib doesn't provide a way to do it without such mappings. I can share the script I made to generate the function if anyone is interested.

Okay, some of the namings might not be perfect, so feel free to give feedback about how to improve them.
And I'll see if I can make less conversions between KeySyms, Keycodes, sf::Keyboard::Key and sf::Keyboard::Scancode. But right now the implementation looks a lot better!

… Keyboard Enter)

eliasdaler · 2018-04-07T19:59:20Z

Thanks a lot for testing!

At first I made a commit to change ScanEnter and ScanReturn, but then I realized that I've made a mistake. Right now ScanEnter = keyboard enter and ScanReturn = numpad enter. That's how @mantognini wrote it for now.

As for the naming - getDescription now returns "Enter" and "Enter (Numpad)". The names of scancodes should be discussed in #1235 and it seems like we may agree to have ScanEnter for keyboard "Enter" and ScanNumpadEnter for numpad "Enter". It's still not 100%, so you should probably post your point there!

Also see #1395. We agreed to have sf::Keyboard::Enter, so it should be ScanEnter for main Enter, not ScanReturn, imo.

P. S. "Fn" is keyboard specific and it's impossible to handle it.

binary1248 · 2018-04-08T23:49:09Z

src/SFML/Window/CMakeLists.txt

@@ -89,6 +89,10 @@ elseif(SFML_OS_LINUX OR SFML_OS_FREEBSD)
        ${SRCROOT}/Unix/Display.hpp
        ${SRCROOT}/Unix/InputImpl.cpp
        ${SRCROOT}/Unix/InputImpl.hpp
+        ${SRCROOT}/Unix/KeyboardImpl.hpp
+        ${SRCROOT}/Unix/KeyboardImpl.cpp
+        ${SRCROOT}/Unix/KeySymToSFKeyMapping.hpp


If there is no name clash, I would just drop the SF.

binary1248 · 2018-04-08T23:49:43Z

src/SFML/Window/Unix/InputImpl.cpp

 #include <SFML/System/Err.hpp>
 #include <X11/Xlib.h>
 #include <X11/keysym.h>
+#include <cstring>


Is this still needed? I don't see any new line of code that would need this included.

binary1248 · 2018-04-08T23:50:02Z

src/SFML/Window/Unix/KeySymToSFKeyMapping.hpp

+////////////////////////////////////////////////////////////
+//
+// SFML - Simple and Fast Multimedia Library
+// Copyright (C) 2007-2017 Laurent Gomila (laurent@sfml-dev.org)


binary1248 · 2018-04-08T23:50:33Z

src/SFML/Window/Unix/KeySymToSFKeyMapping.hpp

+/// \return The corresponding sf::Keyboard::Key
+///
+////////////////////////////////////////////////////////////
+inline Keyboard::Key keySymToSFKey(KeySym symbol)


If no name clash, I would just drop the SF.

I don't think any sane compiler is going to actually inline this function. The resulting jump table is going to be so huge that it would completely wreck the instruction cache. I would rather just move the implementation into a .cpp file. It shouldn't make any noticeable difference performance-wise.

binary1248 · 2018-04-08T23:51:52Z

src/SFML/Window/Unix/KeySymToSFKeyMapping.hpp

+/// \return The corresponding X11 KeySym
+///
+////////////////////////////////////////////////////////////
+inline KeySym SFKeyToKeySym(Keyboard::Key key)


Same here, I would drop SF if I could.

I don't think any sane compiler is going to actually inline this function. The resulting jump table is going to be so huge that it would completely wreck the instruction cache. I would rather just move the implementation into a .cpp file. It shouldn't make any noticeable difference performance-wise.

binary1248 · 2018-04-09T00:13:25Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+        case Keyboard::ScanRShift:      return "Shift (Right)";
+        case Keyboard::ScanRAlt:        return "Meta (Right)";
+        case Keyboard::ScanRSystem:     return "Super (Right)";
+    }


I would move "Unknown Scancode" into a default case.

binary1248 · 2018-04-09T00:14:03Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+        if (key != Keyboard::Unknown)
+            break;
+    }
+    return key;


Aesthetics, would insert an empty line before this return.

binary1248 · 2018-04-09T00:14:51Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+}
+
+} // namespace priv
+} // namespace sf


Missing empty line before this line.

binary1248 · 2018-04-09T00:15:14Z

src/SFML/Window/Unix/KeyboardImpl.hpp

+////////////////////////////////////////////////////////////
+/// \brief sf::priv::InputImpl helper
+///
+/// This class manage as a singleton instance the keyboard state.


This isn't a singleton any more. 😉

binary1248 · 2018-04-09T00:15:39Z

src/SFML/Window/Unix/KeyboardImpl.hpp

+#include <X11/X.h> // Keycode
+#include <X11/XKBlib.h>
+
+namespace sf {


namespace curly braces on new lines.

eliasdaler · 2018-04-09T20:58:53Z

That's a lot of feedback, thanks a lot!
And I also found a bug, "Escape" wasn't shown correctly. Fixed it in the same commit.

For now, I don't know what to do with fallthrough cases. Should I just place return statements here?

binary1248 · 2018-04-09T22:42:02Z

For now, I don't know what to do with fallthrough cases. Should I just place return statements here?

Yes.

eliasdaler · 2018-04-09T22:53:45Z

Done!

binary1248

Just a few more aesthetic touch ups but otherwise this is good with me. 👍

binary1248 · 2018-05-10T15:20:46Z

src/SFML/Window/Unix/KeySymToUnicodeMapping.cpp

+        case 0x04b4: return 0x30a8; // kana_E
+        case 0x04ab: return 0x30a9; // kana_o
+        case 0x04b5: return 0x30aa; // kana_O
+        case 0x04b6: return 


GitHub says there is no newline at the end of this file.

Is there a reason, why 'magic numbers' are used instead of constants?
At first glance most of them are defined in X11/keysym.h.

Hmm, indeed. I must have only seen X11/keysym.h, but didn't take a look at keysymdef.h, where all these codes are.
Will redo this giant switch later. :)

binary1248 · 2018-05-10T15:22:55Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+{
+
+const KeyCode NullKeyCode = 0;
+KeyCode scancodeToKeycode[sf::Keyboard::ScanCodeCount];     ///< Mapping of SFML scancode to X11 KeyCode


Is the space before the comment meant for alignment with the line below? I can't tell what is the intention here.

binary1248 · 2018-05-10T15:24:27Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+bool isValidKeycode(KeyCode keycode)
+{
+    // Valid key code range is [8,255], according to the Xlib manual
+    return keycode >= 8 || keycode <= 255;


Might not be necessary, but brackets around the comparisons makes it safer and maybe even a bit more readable.

binary1248 · 2018-05-10T15:25:43Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+        std::memcpy(name, desc->names->keys[keycode].name, XkbKeyNameLength);
+        name[XkbKeyNameLength] = '\0';
+
+        if (strcmp(name, "TLDE") == 0)      sc = sf::Keyboard::ScanGraveAccent;


I would align the (strcmp(name, "TLDE") == 0) with the lines below just for that extra touch.

binary1248 · 2018-05-10T15:26:04Z

src/SFML/Window/Unix/KeyboardImpl.cpp

+        else if (strcmp(name, "AE10") == 0) sc = sf::Keyboard::ScanNum0;
+        else if (strcmp(name, "AE11") == 0) sc = sf::Keyboard::ScanDash;
+        else if (strcmp(name, "AE12") == 0) sc = sf::Keyboard::ScanEquals;
+        else if (strcmp(name, "TAB") == 0)  sc = sf::Keyboard::ScanTab;


I would align the ) == 0) with the other lines.

eliasdaler · 2018-05-16T06:32:19Z

Agreed with everything. Done! 👍

eliasdaler · 2019-10-10T17:10:26Z

@HiImJulien

Looking back at why I didn't use the mapping already present in <X11/keysym.h> - it doesn't really correspond to correct and actual Unicode code points! See

... So this mapping is really necessary.

eXpl0it3r · 2020-04-15T19:08:38Z

I've rebased, fixed my bad rebase mistakes and merged your branch onto feature/scancode, hope you don't mind. 😉
Let's iron out the rest on one branch, so refactorings are a bit easier.

mantognini and others added 8 commits September 8, 2017 23:53

Add new API for scancodes

397d589

Add support of scancodes for macOS

86fe234

Handle layout changes on macOS

10e2062

Use Scan prefix instead of s and getDescription instead of localizedR…

f6d95ce

…epresentation.

Initial scancode implementation for Linux

fa16402

Store pointer to Display in X11InputManager

c45c008

Add comments for functions and follow SFML's coding standard more clo…

9886400

…sely

getDescription implementation for X11

f9cb4c7

eliasdaler mentioned this pull request Mar 29, 2018

Scancode #1235

Merged

41 tasks

eXpl0it3r added feature m:sfml-window s:undecided p:linux labels Mar 29, 2018

eXpl0it3r assigned mantognini Mar 29, 2018

mantognini added this to the 2.6 milestone Mar 30, 2018

mantognini requested a review from binary1248 March 30, 2018 07:14

eliasdaler added 7 commits March 30, 2018 23:44

Fixed broken code

10621ed

Generate KeySym -> Unicode mapping, so we don't need to make fake eve…

c21005b

…nts in getDescription now

KeySym to sf::Keyboard::Key mapping

48a6390

(also made better names for keycode, sf::Scancode and sf::Key conversion functions)

X11InputManager doesn't need m_display now

c2d7d15

Almost got rid of singleton (move a lot of stuff into free functions)

1af8c3d

X11InputManager now only has static members

96919e0

X11InputManager -> KeyboardImpl

aebdda6

eliasdaler added 3 commits April 7, 2018 22:45

Swap ScanEnter and ScanReturn (ScanEnter = Numpad Enter, ScanReturn =…

738b853

… Keyboard Enter)

Really fix enter keys confusion...

5eeac95

Fix formatting

03f6d3d

binary1248 requested changes Apr 9, 2018

View reviewed changes

Code review changes

266fb05

eliasdaler added 2 commits April 10, 2018 00:00

Removed duplication in a comment

b1366ba

Better descriptions for some keys (e.g. "Shift (Left)" -> "Left Shift")

d24edb3

Remove fallthrough in switch-case

0973f66

binary1248 approved these changes May 10, 2018

View reviewed changes

Code review small fixes

720e2d2

eXpl0it3r force-pushed the feature/scancode branch 5 times, most recently from 6c85444 to be8c225 Compare January 28, 2020 15:52

eXpl0it3r added s:accepted and removed s:undecided labels Feb 3, 2020

eXpl0it3r force-pushed the feature/scancode branch from 8c2de80 to fd771a5 Compare April 15, 2020 11:59

eXpl0it3r closed this Apr 15, 2020

dogunbound mentioned this pull request May 17, 2023

Pull request for 2.6.x CSFML SFML/CSFML#186

Merged

ChrisThrasher mentioned this pull request Jun 23, 2023

Implement remaining SFML 2.6.0 changes SFML/CSFML#187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial X11 scancode implementation #1400

Initial X11 scancode implementation #1400

eliasdaler commented Mar 29, 2018 •

edited

Loading

eliasdaler commented Mar 29, 2018 •

edited

Loading

mantognini commented Mar 30, 2018

binary1248 commented Mar 30, 2018

eliasdaler commented Mar 30, 2018

binary1248 commented Mar 30, 2018

eliasdaler commented Mar 31, 2018 •

edited

Loading

eliasdaler commented Apr 7, 2018 •

edited

Loading

binary1248 Apr 8, 2018

binary1248 Apr 8, 2018

binary1248 Apr 8, 2018

binary1248 Apr 8, 2018

binary1248 Apr 9, 2018

binary1248 Apr 8, 2018

binary1248 Apr 9, 2018

binary1248 Apr 9, 2018

binary1248 Apr 9, 2018

binary1248 Apr 9, 2018

binary1248 Apr 9, 2018

binary1248 Apr 9, 2018

eliasdaler commented Apr 9, 2018

binary1248 commented Apr 9, 2018

eliasdaler commented Apr 9, 2018

binary1248 left a comment

binary1248 May 10, 2018

HiImJulien May 31, 2019

eliasdaler Jun 1, 2019

binary1248 May 10, 2018

binary1248 May 10, 2018

binary1248 May 10, 2018

binary1248 May 10, 2018

eliasdaler commented May 16, 2018

eliasdaler commented Oct 10, 2019

eXpl0it3r commented Apr 15, 2020

Initial X11 scancode implementation #1400

Initial X11 scancode implementation #1400

Conversation

eliasdaler commented Mar 29, 2018 • edited Loading

eliasdaler commented Mar 29, 2018 • edited Loading

mantognini commented Mar 30, 2018

binary1248 commented Mar 30, 2018

eliasdaler commented Mar 30, 2018

binary1248 commented Mar 30, 2018

eliasdaler commented Mar 31, 2018 • edited Loading

eliasdaler commented Apr 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasdaler commented Apr 9, 2018

binary1248 commented Apr 9, 2018

eliasdaler commented Apr 9, 2018

binary1248 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasdaler commented May 16, 2018

eliasdaler commented Oct 10, 2019

eXpl0it3r commented Apr 15, 2020

eliasdaler commented Mar 29, 2018 •

edited

Loading

eliasdaler commented Mar 29, 2018 •

edited

Loading

eliasdaler commented Mar 31, 2018 •

edited

Loading

eliasdaler commented Apr 7, 2018 •

edited

Loading