Fix to issue #286

[TobiasWallner]

Fixed issue #286 Increased the userexperience of keys and metakeys:

Key::A + MetaKey::Ctrl

is now possible.

Additionally for C++17 and following the key functions are also constexpr so that you can use them in a const context or even a switch statement.

switch(key){
  case MetaKey::Alt + Key::A: ... 

is now possible.

Additionally I cleaned up in the repo and fixed a lot of implicit downward casts and removed unnecessary constructions of std::string in a function that returns such but the return value was std::string::c_str() which is an unnecessary conversion to const char* and back to std::string.

[TobiasWallner]

@flagarde Can you help me? All tests pass except for this one:

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 128 == 128 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 129 == 129 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 130 == 130 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 131 == 131 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 132 == 132 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 133 == 133 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 134 == 134 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 135 == 135 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 136 == 136 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 137 == 137 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 138 == 138 )

/__w/cpp-terminal/cpp-terminal/tests/key.test.cpp:78: ERROR: CHECK( Term::Key(Term::Key(i)) == i ) is NOT correct!
  values: CHECK( 139 == 139 )

And as far as I can see, the tess shoule be correct, right? all the numbers are the same. Is the test wrong?

[flagarde]

Strange indeed, let me have a look

[TobiasWallner]

maybe I trapped somehow into undefined-behaviour, because for some it is not failing

[TobiasWallner]

I think it is UB, because with the enum can have intager values that don't correspond to an enum entry. I will change it partly back but will keep the C++17 and larger constexpr so that adding a meta key and a key can be used in an switch case statement for example

[TobiasWallner]

Ok ... Finally !!!! juhu, I managed that gcc 4.7 compiles. It is not 100% like I wanted it to be but now you can write something like the following in a switch statement:

switch(keyEvent){
    case Term::MetaKey::Value::Ctrl + Term::Key::Value::q : this->quit(); break;

[TobiasWallner]

@flagarde So now everything seems to work and this is the only thing that is failing me: 418: Wrong amount of left-padding spaces(want multiple of 2) and it won't go away. Everytime I try to set the left-padding spaces of that line to an even number and push it the pre-commit bot will change it to 13 and then complain that it is not a multiple of 2???

[flagarde]

Hi, I had a quick look on your PR and some changes are nice additions but there is other I'm a bit more reluctant to apply. One of them is the move of the functions out of the class, can you give me a reason of such change ? The other one is the conversion from Key to char and vice et versa. The reason is maybe we are to european languages centered and so most of our keyboard is close to the ASCII that is encoded in char. But some languages have other alphabet (cyrillic) or even not alphabet at all (chinese...), or have many (japanese). Most of them use IME and for example in japanese you can change alphabet so pressing the Key "A" will not generate a "A" but on of their "letters". While what you did could be done I would prefer not to allow automatic convertion from Key to char, as it is really not the same things. I would even say char is an old stuffs C++ should have be purged of to directly move to u8string by defaut (I know utf8 is newer than c++ so one of the reason...). As wstring etc this is quite a remanent from the past that create more trouble than people would confess. Most of the time we are used to use only ASCII even if our language need more (mine need "ç" "à" etc and even some that are difficult and sometime impossible to obtain like "À, É, È, Ç") resulting to wrong sound even wrong grammar for purists.

I think the best way to avoid such confusion is to not allow this convertion by the library and if the user need/want to do it force him to static_cast or bypass this doing some own-cooked tricks. Before this library allowed direct comparaisons between char and Key, and it took time to change this.

[TobiasWallner]

One of them is the move of the functions out of the class, can you give me a reason of such change ?

Result of the way I tackeled the problem, earlier I did not had an enclosing class that could have members. I will put it back in the class as member functions.

The other one is the conversion from Key to char and vice et versa

Ther is only one conversion from char to Key but that is explicit. https://github.com/TobiasWallner/cpp-terminal/blob/854217a6e160fd4b5bfe2daf0044fe0ca88ffefc/cpp-terminal/key.hpp#L218C3-L218C78

But some languages have other alphabet (cyrillic) or even not alphabet at all (chinese...), or have many (japanese). Most of them use IME and for example in japanese you can change alphabet so pressing the Key "A" will not generate a "A" but on of their "letters". While what you did could be done I would prefer not to allow automatic convertion from Key to char.

As far as I understand there is no implicit conversion from Key to char, only an explicit one from char to Key -> so no loss of data. If I am wrong, please show me the code line?

Most of the time we are used to use only ASCII even if our language need more (mine need "ç" "à" etc and even some that are difficult and sometime impossible to obtain like "À, É, È, Ç") resulting to wrong sound even wrong grammar for purists.

I am speaking german and have the same struggle (ü, ä, ö, ß), I feel you. That is also why I have created my own utf8::Char and utf8::String for my texteditor. Main reason of utf8::Char (4 bytes) and using a utf32 character is, that I believe that the conversion between the two is unnecessary, because you need 4Bytes either way and it allows an easy conversion from utf8::Char to std::string_view

[TobiasWallner]

@flagarde as you wanted all ctype like functions are member functions again

[flagarde]

@TobiasWallner Thx a lot, it seems really good now, I will cross-check it one more time and fix the merge comflics with the branch I'm working on. Will merge it soon