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

Scancode #1235

Open
wants to merge 4 commits into
base: master
from

Conversation

@mantognini
Copy link
Member

commented May 26, 2017

SFML Tasks

  • Adjust key names as discussed
  • Clear requirements for getDescription
  • Agree on naming (capitalization, "proper" names)
  • Mac implementation
  • Windows implementation (see #1294)
  • Linux implementation (see #1400)
  • Review
  • Test ❗️

Following on #7, and some (old) discussion on the forum, here is an API to deal with Scancodes and an implementation for Mac. You can test this implementation, or new ones, using the scancode branch of SFML-Test-Events.

Note that there are a few TODOs that deserve a quick discussion, especially w.r.t. the upcoming 3.0 and its breaking changes.

So, send your feedback this way and if you want to implement it for Linux or Windows, let me know!

@eXpl0it3r
Copy link
Member

left a comment

As mentioned, it's still missing Windows and Linux (& iOS and Android) implementations.

sRAlt, ///< Keyboard Right Alt key
sRSystem, ///< Keyboard Right System key

sCodeCount ///< Keep last -- the total number of scancodes

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jul 18, 2017

Member

The s-prefix seems a bit non-SFML like. But I don't have a good alternative to suggest right now.

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Jul 18, 2017

Member

Yep, would strip it as well. They're in separate enums anyway?

This comment has been minimized.

Copy link
@mantognini

mantognini Jul 18, 2017

Author Member

Regular enums are not scopes, symbols clash here. See \todo above for v3.

This comment has been minimized.

Copy link
@kim366

kim366 Sep 7, 2017

namespace ScanCodes
{
    enum ScanCodes
    {
        A, B, C, ..
    }
};

That's what I do in my Code anyways, if I want implicit conversions to int, which enum class doesn't offer.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Sep 7, 2017

Member

This is still an issue: the "s" prefix doesn't comply with our coding rules. What about "Scan" as a prefix?

This comment has been minimized.

Copy link
@kim366

kim366 Sep 12, 2017

@LaurentGomila Does my solution not work? You could do Scan for the namespace and Codes for the enum, so when accessing the enum type, you'd do Scan::Codes and for individual scancodes, for example Scan::A. namespace can also be substituted with class.

/// \return The localized description of the code
///
////////////////////////////////////////////////////////////
static String localizedRepresentation(Scancode code);

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jul 18, 2017

Member

Isn't "Representation" already kind of implying "localized"? I just find the function name a bit long.

This comment has been minimized.

Copy link
@MarioLiebisch

MarioLiebisch Jul 18, 2017

Member

Why not use a verb only, localize()?

This comment has been minimized.

Copy link
@mantognini

mantognini Jul 18, 2017

Author Member

localize is already used. With representation, I think it makes it more clear that this returns not general constants across all OSes, but I agree that the name is a bit lengthy. Alternatives?

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jul 18, 2017

Member

Why not getDescription ? (don't forget that all functions in SFML should start with a verb)

And is there a reasoning behind taking a scan code and not a key code, in this function?

This comment has been minimized.

Copy link
@mantognini

mantognini Jul 18, 2017

Author Member

👍 Why not indeed!

This comment has been minimized.

Copy link
@mantognini

mantognini Jul 20, 2017

Author Member

I think description is better than name or label because the values are meant to be displayed, not used as something unique like a key in a map. With name or label, I think the idea of the values being constant is conveyed too strongly. Two calls to this functions with the same argument can yield two different strings if the keyboard layout has changed.

And is there a reasoning behind taking a scan code and not a key code, in this function?

Yes: a key code would always yield the same string. The mapping would be no different than what Thor's doing. And I don't think SFML needs this function too.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jul 20, 2017

Member

I think description is better than name or label

What kind of strings does this function return, actually? Can you show some examples (on different OSes 😜 )?

This comment has been minimized.

Copy link
@mantognini

mantognini Jul 20, 2017

Author Member

Most "special" descriptions are here: https://github.com/SFML/SFML/pull/1235/files#diff-eff73a64e30104826bef3e4337f1245eR136

This is the fallback when the OS' localisation system cannot yield something like é or }.

I don't have examples for other OSes but the (theoretical) one in the doc. But I see no reason why we cannot have something similar for Windows or Linux.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Sep 7, 2017

Member

So, are we going to change this for "getDescription"?

This comment has been minimized.

Copy link
@mantognini

mantognini Sep 8, 2017

Author Member

I unfortunately don't have time for that at the moment, sorry. Whoever wants to chip in can do the appropriate changes. ;-)

@eXpl0it3r

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

We're still looking for someone to be willing to provide an implementation for Windows and Linux!

If you want to contribute, fork SFML and provide a pull request based this branch feature/scancode.

@eXpl0it3r eXpl0it3r force-pushed the feature/scancode branch 3 times, most recently from e8e8879 to 681e0b2 Sep 8, 2017

@eXpl0it3r eXpl0it3r force-pushed the feature/scancode branch from 681e0b2 to f6d95ce Sep 8, 2017

@Hapaxia

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

I think the scancodes should be named more similar to the SFML key codes already.

For example, since Return is used in the key code enum, it should be used for the scan code on the main key set. However, the number pad's enter key should probably never be called return. The current setup seems inverted. If the pad's enter isto be called Return, it could be NumpadReturn or similar, which would mean that PadEquals should be NumpadEquals. Whether it is Numpad or just Pad, they should be the same. It's also possible that the pad's Enter key on PC is equivalent to the pad's Equals key on the Mac.

There is absolutely no need for the unnecessary pre-fix 'forwards' to be added to a standard slash. 'Reverse solidus' just means "backwards slash"; how is it different from BackSlash?

All of the above presumes the Scan prefix is included, of course.

@JonnyPtn JonnyPtn referenced this pull request Oct 4, 2017

Open

Initial Windows scancode implementation #1294

0 of 2 tasks complete
@mantognini
Copy link
Member Author

left a comment

For example, since Return is used in the key code enum, it should be used for the scan code on the main key set. However, the number pad's enter key should probably never be called return. The current setup seems inverted.

Yes, there are some inconsistencies. The names I've selected are from the USB HID specs. We might indeed want to deviate from them a bit here and there.

'Reverse solidus' just means "backwards slash"; how is it different from BackSlash?

That's a good question, to which the HID specs doesn't seem to give an answer, so I don't know.

ScanNum8, ///< Keyboard 8 and * key
ScanNum9, ///< Keyboard 9 and ) key
ScanNum0, ///< Keyboard 0 and ) key
ScanEnter, ///< Keyboard Return (ENTER) key

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

s/ScanEnter/ScanReturn.

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 25, 2018

Member

While deprecating some codes, I wonder if it wouldn't make sense to switch this to ScanEnter. But I think it's just a personal preference, so both are valid options.

This comment has been minimized.

Copy link
@mantognini

mantognini Jan 26, 2018

Author Member

No strong opinion here, I'll change it to whatever the majority wants.

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Feb 11, 2018

Contributor

+1 for ScanEnter

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

And maybe s/ENTER/Enter

ScanEnter, ///< Keyboard Return (ENTER) key
ScanEscape, ///< Keyboard Escape key
ScanBackspace, ///< Keyboard Backspace key
// TODO above it's BackSpace, but is it correct? What do we use here?

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

What do we want?

  1. A consistent typo and use ScanBackSpace, or
  2. Deprecate BackSpace, introduce Backspace, keep ScanBackspace?

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jan 24, 2018

Member

According to wikipedia it's a single word, so Backspace.

This comment has been minimized.

Copy link
@mantognini

mantognini Jan 24, 2018

Author Member

Okay, I'll create an independant PR that introduces Backspace and deprecates BackSpace (and for the backslash code as well).

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 25, 2018

Member

ScanBackspace

ScanLBracket, ///< Keyboard [ and { key
ScanRBracket, ///< Keyboard ] and } key
ScanBackslash, ///< Keyboard \ and | key
// TODO capitalisation

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

Idem.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jan 24, 2018

Member

Same answer 😀

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 25, 2018

Member

ScanBackslash

// TODO capitalisation
ScanDash, ///< Keyboard Non-US # and ~
ScanSemicolon, ///< Keyboard ; and : key
// TODO capitalisation

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

Idem.

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jan 24, 2018

Member

Seems like it can be both, si let's keep it consistent and write it as a single word (Semicolon).

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 25, 2018

Member

ScanSemicolon

ScanMultiply, ///< Keypad * key
ScanMinus, ///< Keypad - key
ScanPlus, ///< Keypad + key
ScanPadEquals, ///< keypad = key, probably Mac only

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

s/Pad/ Numpad for consistency.

ScanMinus, ///< Keypad - key
ScanPlus, ///< Keypad + key
ScanPadEquals, ///< keypad = key, probably Mac only
ScanReturn, ///< Keypad Enter (return) key

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

s/ScanReturn/ScanNumpadReturn.

ScanDown, ///< Keyboard Down Arrow key
ScanUp, ///< Keyboard Up Arrow key
ScanNumLock, ///< Keypad Num Lock and Clear key
ScanDivide, ///< Keypad / key

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

Divide (and the following codes) are on the keypad, so should use the ScanNumpad prefix?

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jan 24, 2018

Member

How is it done in the key code enumeration?

This comment has been minimized.

Copy link
@mantognini

mantognini Jan 24, 2018

Author Member

The doc about Divide doesn't mention the keypad at all, however it is most probably implicit. There is no prefix either.

Not having a prefix generates some slight inconsistencies, for example with ScanNumpadEqual (for which the prefix is used to make the difference with ScanEqual). Note that there's no NumpadEqual or equivalent in the key codes. There's also one with ScanNumpadReturn.

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 25, 2018

Member

What about just the prefix Num? But I guess, if we prefix, let's just use the full Numpad.

While one might argue that they might not be part of the numpad at all times, I think the prefix makes sense: ScanNumpadDivide.

This comment has been minimized.

Copy link
@mantognini

mantognini Jan 26, 2018

Author Member

Num is used as prefix for 0 ... 9, so it would be ambiguous.

While one might argue that they might not be part of the numpad at all times, I think the prefix makes sense: ScanNumpadDivide.

It would also make it more obvious to which key it's referring to. However, do we also update Divide in the key codes to NumpadDivide?

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 26, 2018

Member

Consistency is king, so if we change it here, we need to change there too. Let's just make sure we have one change for the deprecation and not let it trickle every now and then.

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Feb 11, 2018

Contributor

I also propose to change all "keypad" to "numpad" in comments for additional consistency.

ScanGraveAccent, ///< Keyboard ` and ~ key
ScanComma, ///< Keyboard , and < key
ScanPeriod, ///< Keyboard . and > key
ScanForwardSlash, ///< Keyboard / and ? key

This comment has been minimized.

Copy link
@mantognini

mantognini Nov 28, 2017

Author Member

As suggested by @Hapaxia, should we drop the Forward part for brevity/(internal) consistency sakes?

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Jan 24, 2018

Member

I'd say yes.

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 25, 2018

Member

ScanSlash

@mantognini

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2017

@SFML/sfml I'd like to update this but I need your feedback on the above open questions.

/// The scancodes are based on a subset of Table 12: Keyboard/Keypad Page
/// of Universal Serial Bus (USB): HID Usage Tables, v1.12.
///
/// \todo When porting this for SFML 3, remove the `s` prefix and use

This comment has been minimized.

Copy link
@mantognini

mantognini Jan 12, 2018

Author Member

s/s/Scan

This comment has been minimized.

Copy link
@eXpl0it3r

eXpl0it3r Jan 26, 2018

Member

Does this make sense on the doxygen doc? Maybe just a in code comment? I'm fine either way.

This comment has been minimized.

Copy link
@mantognini

mantognini Jan 26, 2018

Author Member

Yes, I think it makes sense because it generates a TODO page, centralising all TODOs in one place automatically. And it's easy to grep so it's a win-win situation. :)

@mantognini

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

I'd like your input on the above open questions in order to update the code & integrate @JonnyPtn's work from #1294 for Windows.

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Jan 25, 2018

@eXpl0it3r eXpl0it3r removed this from WIP in SFML 2.5.0 Jan 25, 2018

@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Jan 25, 2018

ScanRBracket, ///< Keyboard ] and } key
ScanBackslash, ///< Keyboard \ and | key
// TODO capitalisation
ScanDash, ///< Keyboard Non-US # and ~

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Feb 11, 2018

Contributor

After some research, it looks like a proper name of that key is "hyphen-minus". But it's certainly not a dash, because typing a dash (even en/em dashes) is not possible with a regular keyboard.

Scancode may be called ScanHyphen or ScanMinus (though we have ScanMinus for numpad...), so we need to discuss this as well. sf::Keyboard::Key::Dash also exists and probably should be renamed.

This comment has been minimized.

Copy link
@mantognini

mantognini Feb 12, 2018

Author Member

ScanHyphen makes more sense to me. For scancode it's not an issue to make the difference between hyphen and minus (through numpad), but for Keyboard codes it's harder (impossible?)... Maybe we should deprecate both of Dash and Subtract and always map it to a new HyphenMinus? Any opinion @LaurentGomila?

This comment has been minimized.

Copy link
@LaurentGomila

LaurentGomila Feb 12, 2018

Member

I don't know. Maybe we should just have a look at what others do.

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Feb 20, 2018

Contributor

GLFW uses "minus": GLFW_KEY_MINUS
SDL is the same: SDL_SCANCODE_MINUS
Any other examples?

Maybe we can have ScanMinus and ScanNumpadMinus?

@mantognini mantognini self-assigned this Feb 12, 2018

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

By the way, I've been working on Linux implementation for some time, and I'll make a pull request once all the functions are implemented. I think there'll be a lot of things to discuss since the current keyboard code for X11 was a bit scattered around and I had to move it around a bit to get the needed functionality. I based my implementation on GLFW's, because scancodes on X11 are not easy to deal with.
And I just want to know: what is the proper way to refer to other code bases as a source? I've seen some code that was based on SDL and it just said so in comments. Should I do the same for GLFW? :)

P.S. getDescription is giving me the most trouble as for now, it's very hard to implement properly.

@mantognini

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2018

This PR is now stalled until #1395 is merged in order to address naming.

@eliasdaler I only see your question now... I guess it's always safer to post an additional comment instead of editing your messages. ;-)

what is the proper way to refer to other code bases as a source? I've seen some code that was based on SDL and it just said so in comments. Should I do the same for GLFW? :)

That would be perfect IMO.

Did you make some progress on getDescription? Don't worry about the upcoming changes of enum values, I'll rebase/adapt your work for Linux (and @JonnyPtn's for Windows) afterwards in one go.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2018

I haven't done work on it yet (didn't have the time to do so). I'll try to get back to it as soon as possible.

@mantognini

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2018

No problem

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

Here's my X11 implementation. :)
#1400

@eliasdaler eliasdaler referenced this pull request Mar 30, 2018

Open

Initial X11 scancode implementation #1400

2 of 4 tasks complete
ScanNumpad8, ///< Keypad 8 and Up Arrow key
ScanNumpad9, ///< Keypad 9 and Page Up key
ScanNumpad0, ///< Keypad 0 and Insert key
ScanReverseSolidus, ///< Keyboard Non-US \ and | key

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Mar 31, 2018

Contributor

How does this differ from a backslash? On my QWERTY keyboard this key is the only way to type ""

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 2, 2018

Author Member

On my QWERTY keyboard this key is the only way to type ""

To type quotes?

Honestly, after ready the USB HID standard, I still don't understand what this key is...

This comment has been minimized.

Copy link
@texus

texus Apr 2, 2018

Contributor

Isn't ScanReverseSolidus just the key between the left shift and Z on the image below (there is a backslash there on my Belgian layout), while ScanBackslash is a key found near the return key?
US keyboard layout

In SDL it seems to be called SDL_SCANCODE_NONUSBACKSLASH.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 2, 2018

Author Member

This is probably it, yes.

This comment has been minimized.

Copy link
@Hapaxia

Hapaxia Apr 2, 2018

Contributor

As mentioned above, 'reverse solidus' literally just means 'backwards slash' so using both makes no sense at all.
In the layout image shown by texus, the key proposed (left of z) for reverse solidus doesn't even have that on it.
Would it not be more prudent to just name it "pipe" (i.e. ScanPipe) or something like that?

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

@mantognini, I meant \. Github just removed backslash somehow.
But looking at layout that @texus has shown, it seems pretty unusual for me. Yes, ScanPipe seems less ambigious.

https://wiki.libsdl.org/SDL_Scancode
SDL named it "Backslash". Maybe it produces the same keycode in different layouts? On my laptop keyboard this button is above enter, but on the other one it's left to it, same as on layout @texus displayed.

@texus, can you show what "xev" shows when you press different buttons with "pipe" on it?

This comment has been minimized.

Copy link
@texus

texus Apr 2, 2018

Contributor

It seems like in many cases it contains the < and > symbols, so it could be named after them, but there are keyboard (e.g. UK layout) where there is only a backslash and pipe on that key.

I'm not sure what output from xev you want exactly, on my keyboard layout the pipe key is even located at the 1 number key. Below are some xev outputs though.

Pressing the ScanReverseSolidus key: (first one is normal press, second one is with shift and third one is with AltGr)

KeyPress event, serial 37, synthetic NO, window 0x4200001,
    root 0x125, subw 0x0, time 42422739, (-320,124), root:(843,535),
    state 0x10, keycode 94 (keysym 0x3c, less), same_screen YES,
    XLookupString gives 1 bytes: (3c) "<"
    XmbLookupString gives 1 bytes: (3c) "<"
    XFilterEvent returns: False

KeyPress event, serial 37, synthetic NO, window 0x4200001,
    root 0x125, subw 0x0, time 42425023, (-320,124), root:(843,535),
    state 0x11, keycode 94 (keysym 0x3e, greater), same_screen YES,
    XLookupString gives 1 bytes: (3e) ">"
    XmbLookupString gives 1 bytes: (3e) ">"
    XFilterEvent returns: False

KeyPress event, serial 37, synthetic NO, window 0x4200001,
    root 0x125, subw 0x0, time 42426153, (-320,124), root:(843,535),
    state 0x90, keycode 94 (keysym 0x5c, backslash), same_screen YES,
    XKeysymToKeycode returns keycode: 20
    XLookupString gives 1 bytes: (5c) "\"
    XmbLookupString gives 1 bytes: (5c) "\"
    XFilterEvent returns: False

I get the following when pressing the key where the backslash would be according to the layout in the image I showed in my previous comment:

KeyPress event, serial 34, synthetic NO, window 0x6c00001,
    root 0x125, subw 0x0, time 43321539, (-263,93), root:(919,550),
    state 0x10, keycode 51 (keysym 0xb5, mu), same_screen YES,
    XLookupString gives 2 bytes: (c2 b5) "µ"
    XmbLookupString gives 2 bytes: (c2 b5) "µ"
    XFilterEvent returns: False

To get the actual pipe symbol I have to press AltGr+1:

KeyPress event, serial 37, synthetic NO, window 0x6c00001,
    root 0x125, subw 0x0, time 43010269, (-289,117), root:(890,504),
    state 0x90, keycode 10 (keysym 0x7c, bar), same_screen YES,
    XLookupString gives 1 bytes: (7c) "|"
    XmbLookupString gives 1 bytes: (7c) "|"
    XFilterEvent returns: False

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

Thanks! Very helpful. I was mostly looking for the key codes.

Take a look at this
So, there's a BKSL key for XKB where backslash is on the QWERTY layout (on my QWERTY keyboard keycode 51 is the backslash). And for keycode 94 the symbolic name is "LSGT" (less, greater). Which makes sense, because it produces "<" primarly.
And the KeySym is named "XK_less". I wonder how it's on Windows and Mac.

So, ScanLess or ScanLessThan maybe?

(Btw, this scancode is named SDL_SCANCODE_NONUSBACKSLASH in SDL. and GLFW_KEY_WORLD_1 in GLFW.)

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

I don't really care about the name but for consistency we should probably remain close to the HID documentation, i.e. non US backslash or reverse solidus (the latter is easier to capitalise though).

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 9, 2018

Contributor

Then I propose to leave it as is. It's a pretty weird key, so I don't think a lot of people will use it. And those who do probably won't complain much. :)

@@ -29,6 +29,7 @@
// Headers
////////////////////////////////////////////////////////////
#include <SFML/Window/Export.hpp>
#include <SFML/System/String.hpp>

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

It's possible to do a forward declaration here.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

I'll keep it in mind when updating the PR, thanks!

////////////////////////////////////////////////////////////
/// \brief Scancodes
///
/// The enumerators are bound to a physical key and do *not* depend

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

Maybe highlighting "not" here is a bit excessive.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

Good point.

////////////////////////////////////////////////////////////
enum Scancode
{
ScanUnknown = -1, ///< Represents any scancode not present in this enum

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

"Or unrecognized/unhandled one"?

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

It's similar to Unknown for keycode.

ScanX, ///< Keyboard x and X key
ScanY, ///< Keyboard y and Y key
ScanZ, ///< Keyboard z and Z key
ScanNum1, ///< Keyboard 1 and ! key

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

Do we really need to say "X and Y" for scancodes? On some layouts this is "1 and |". On some it's "1 and &". Maybe we need to describe scancodes by their main function.

This comment has been minimized.

Copy link
@Hapaxia

Hapaxia Apr 3, 2018

Contributor

The scan key represents the key and not what it can type so upper and lower case makes no sense here, I agree.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

The doc corresponds to the AT-101 reference keyboard. It can give the user an idea of the physical location of each key.

ScanRShift, ///< Keyboard Right Shift key
ScanRAlt, ///< Keyboard Right Alt key
ScanRSystem, ///< Keyboard Right System key

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

I'd suggest checking this out: https://github.com/torvalds/linux/blob/master/include/uapi/linux/input-event-codes.h

It has a bunch of additional scancodes like "Calculator" or "Mail".
Some of them may not be ever needed for SFML. Let's discuss it on the forum, maybe?

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

Those other codes are not part of the Keyboard/Keypad Page (0x07) but of the Consumer Page (0x0C). We can discuss adding them later on if they are requested. :)

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 4, 2018

Contributor

Fair enough. They're not important for now, I think.

/// when the given physical key is pressed by the user, when no
/// modifiers are involved.
///
/// \warning The result is OS-dependent: for example, sf::Keyboard::ScanLSystem

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

TODO: discuss what to return here: "Left Shift" or "Shift (Left)"? And update the code/comment accordingly.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

Left Shift seems more intuitive to me, but I used Shift (Left) in the macOS impl... go guess why. 🙃

Whichever people prefer.

/// and might not be the same as the US keycode in some country
/// (e.g. the keys 'Y' and 'Z' are switched on QWERTZ keyboard and
/// By 'localized' we mean keys that depend on the keyboard layout
/// and might not be the same as the US keycode for some countries

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 2, 2018

Contributor

s/countries/keyboard layouts

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 4, 2018

Author Member

This text is part of the USB HID documentation. I believe it is correct from an historical point of view, at least.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

I have a small idea: what if getDescription returns empty string if the corresponding description is not found? I think empty string is a better return that "Unknown Scancode". It's easier to test for it in code later. E.g.:

auto str = sf::Keyboard::getDescription(sf::Keyboard::ScanW);
if (str != "") { ... };

And what about lowercase vs uppercase? I think that it should be lowercase, because the function's description says:

From a high level point of view, this conversion corresponds somewhat to the string available through sf::Event::TextEvent when the given physical key is pressed by the user, when no modifiers are involved.

So, when no modifiers are involved, you get the lowercase letter.

/// modifiers are involved.
///
/// \warning The result is OS-dependent: for example, sf::Keyboard::ScanLSystem
/// is "Left Meta" on Linux, "Left Windows" on Windows and

This comment has been minimized.

Copy link
@eliasdaler

eliasdaler Apr 9, 2018

Contributor

Btw, "Meta" is Alt. System is "Super" on Linux.

This comment has been minimized.

Copy link
@mantognini

mantognini Apr 10, 2018

Author Member

Good catch!

@mantognini

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2018

I have a small idea: what if getDescription returns empty string if the corresponding description is not found?

I have no strong opinion either way. Others' opinion is welcome.

So, when no modifiers are involved, you get the lowercase letter.

Yes, that seems reasonable to me.

@eXpl0it3r

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

I have a small idea: what if getDescription returns empty string if the corresponding description is not found?

I wouldn't recommend that, as "no response" is often a bit ambiguous, while when you return "Unknown" or similar, the situation is clear.

@LaurentGomila

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

I wouldn't recommend that, as "no response" is often a bit ambiguous, while when you return "Unknown" or similar, the situation is clear.

It's true if you directly display the string, but it's the opposite if you want to do something with it in your code: an empty string is less ambiguous (ie. it's testable) than a string with arbitrary text.

I think it's more intuitive and reliable if people have to do this:

auto desc = sf::Keyboard::getDescription(...);
if (desc.isEmpty())
    desc = "Unknown";

... than that:

auto desc = sf::Keyboard::getDescription(...);
if (desc == "Unknown")
    ...;
@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

Another question about getDescription. Do we want to get translated descriptions of Shift, Alt, Space, Enter, etc. keys in non-English layouts? For example, suppose the layout is AZERTY, used in France.

In Windows, GetKeyNameText for "Home" will return "ORIGINE" on French AZERTY. Is this something that we want to have? It's very system specific. Linux doesn't have the ability to do this, AFAIK.

And don't you think that most people will just get a scancode and then have their own database of this key named in different languages? That's why I'm not sure what we want to have by default.

@LaurentGomila

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

In this case, I don't see how we could provide a getDescription function that would be useful. A function that returns some localized human-readable text is unusable, unless you can explicitly choose which language you want the output in, which SFML (and in most cases the underlying OS) doesn't support. At best, it could serve for debugging / prototyping, which is not worth it.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

Agreed. For "Press X to do Y" strings, users can make their own string dictionaries (which will be different for languages and platforms). And for reading text input, we already have TextEntered events. No reasons to do text input stuff with scancodes, I think.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

One more thing. How should implementations deal with NumLock? Should keypad "7" return "ScanHome" on keypress when NumLock is off? NumLock is not a modifier key, it kinda switches keyboard layout. And if someone maps some button to a ScanHome, it'll be impossible to press it on laptop keyboards, for example, as many of them don't have a dedicated "Home" key.

P.S. On a second thought... I think that "ScanNum7" should be sent, no matter if NumLock is on and off. After all, scancodes correspond to a physical location, not to what the key does.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented May 10, 2018

More thinking... getDescription is absolutely necessary to be useable for GUI. How else would developers be able to say "Press [ScancodeW] to move forward" and have it shown as "W" on QWERTY and "Z" on AZERTY? It'll require them to do system-specific stuff, which is pretty hard and not obtainable without reading messages from event loop!

So, what I propose is:

  1. Have getDescription only for Alpha-numeric and punctuation characters (A, B, C, etc. + ScanGraveAccent, etc. On some layouts numbers map to characters too, in AZERTY, for example)
    Return empty string for other keys
  2. Return "Shift", "Ctrl", "Tab", etc. for functional keys which should correspond to the same scancodes in most (if not all) layouts. Problems:
    • English only
    • Left Ctrl? or Ctrl (Left)?
    • Alt or Meta? Platform dependent? Not cool
    • Return? Enter?

I propose to go with 1). And let the users handle Ctrl/Shift/Tab. etc. for themselves with their own dictionaries of keyboard key names in different languages.

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

@mantognini, @eXpl0it3r, @binary1248, @LaurentGomila , any thoughts about my comment above?

@LaurentGomila

This comment has been minimized.

Copy link
Member

commented May 16, 2018

In this case, can't we just convert the scancode to a keycode (Keyboard::localize) and then get the key name from a lookup table, as many people already do?

But if it's needed, yes, a function that gives the character produced by a key identified by its scancode (or keycode), would totally be feasible; if I remember correctly, Qt does it for example (see http://doc.qt.io/qt-5/qkeyevent.html#text).

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented May 16, 2018

Hmm, yes, it seems like getting sf::Keyboard::Key from Keyboard::localize and then mapping it to some string seems to cover a lot of cases.

The problem is that on some layouts not all keys will map to sf::Keyboard::Key properly. And we'll get a lot of sf::Keyboard::Unknown from Keyboard::localize...

The easiest example is AZERTY. sf::Keyboard::localize(sf::Keyboard::ScanNum2) returns Unknown, because it's mapped to "é" on AZERTY.

@eXpl0it3r eXpl0it3r moved this from Discussion to WIP in SFML 2.6.0 Aug 13, 2018

@eliasdaler

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

I think I might be able to work on this a bit again. Can we talk about what's blocking this again?

I might be able to test it on Mac, Linux implementation is done and was tested by several people. Windows implementation seems to be lacking some things, and there's still some uncertainty about interfaces/enum members.

Let's review it?

@JonnyPtn

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2019

I can probably look at the Windows implementation as soon as these questions are resolved

@mantognini

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

Sorry for the lack of responsiveness but I should probably say that I currently (and for the foreseeable future) can't invest time in SFML. Please feel free to take over this PR. Your help if very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.