jupyter-xeus / cpp-terminal

C++ library for writing multiplatform terminal applications
https://jupyter-xeus.github.io/cpp-terminal/
Other
490 stars 53 forks source link

Bug in key + metakey arithmetic #288

Open TobiasWallner opened 1 year ago

TobiasWallner commented 1 year ago

While inspecting and changeing the files: https://github.com/jupyter-xeus/cpp-terminal/blob/master/cpp-terminal/key.cpp https://github.com/jupyter-xeus/cpp-terminal/blob/master/cpp-terminal/key.hpp I noticed the following possible error:

Key::Value::q + MetaKey::Value::Ctrl == Key::Value::Ctrl_Q; // returns false

The addition adds a flag bit at bit position 23 becaulse: https://github.com/jupyter-xeus/cpp-terminal/blob/83b7300a5bdd118ff2d99fed6c3e1c2f91e9a1d4/cpp-terminal/key.hpp#L249 and: https://github.com/jupyter-xeus/cpp-terminal/blob/83b7300a5bdd118ff2d99fed6c3e1c2f91e9a1d4/cpp-terminal/key.cpp#L22

so the result will not be Key::Value::Ctrl_Q https://github.com/jupyter-xeus/cpp-terminal/blob/83b7300a5bdd118ff2d99fed6c3e1c2f91e9a1d4/cpp-terminal/key.hpp#L35

When I enter Ctrl+Q on my keyboard, I would like to intuitively use the library and use Key::Value::q + MetaKey::Value::Ctrl to check against.

Currently we employ two different schools of thought and mixed and messed them up.

1) all controll characters are from 0 to 31 2) all controll characters have the 22nd bit set high

and both are being though of as being a controll character. We should choose one of the two. I understand the motivation of the second because it allows for the future feature of seperatelly reading the Ctrl key and storing it in a flag. However, like this the feature of adding metakeys to normal keys is pretty much useless, or am I wrong and missing something? @flagarde

flagarde commented 1 year ago

This need to be fixed indeed. The reason why the first is like this is historical, we cannot change this. The method 2 is to deal this CTL+ALT+F5 for example (not yet all implemented but needed...). So we need to deal with the 2. The CTL key is a mess :). I think I have seen this probem that why I put the comment.

TobiasWallner commented 12 months ago

should we solve this by

1) convertign all constructions of key directly to the Metakey + Key alternative 2) on comparison between keys make Metakey::Ctrl + Key and CTRL_ return true?

Personally I would do the first, because then it would already work for all the ctype.h like functions.

Basically where should we have the extra if?

I tried to find some way to get the key status in linux without sudo rights but to be honest. Linux seems to be really lacking in that regard and I could not find any useful resources except for X11.

Furthermore I would suggest to rename the current Key::Value::Ctrl_<letter> to Key::Value::Legacy_Ctrl_<letter> and add additionally Key::Value::Ctrl_<letter> that correspond to the Metakey + Key alternative

flagarde commented 12 months ago

I fear we cannot do the first solution because the char we receive it's C0 1to aroud 30 (ANSII) so if we use alternative 2 we cannot match this value and conversion to char would be more difficult (I don't like the convertion and would like not to promote that convertion in the library for the user) but I would like the user to cast it if he want without effort.

Your solution 1 is not possible if you want to respect ctype and if you want to assign same valut to the Ctrl variable. ctype is old and mean mainly C0 C1 characters but there is more like Ctrl+F1 etc. Unless we convert back on forthe the C0 C1 keys.

Solution 2 seems the best solution but I'm still thinking about it. I like the Ctrl_A because it really says what it is it maps a combination keys to a value. What I don't like it more the name of the class Key but I couldn't find a better name. That why I would like to add the option for user to be able to use Ctrl + A that is more closer to the reality.

flagarde commented 12 months ago

should we solve this by

1) convertign all constructions of key directly to the Metakey + Key alternative 2) on comparison between keys make Metakey::Ctrl + Key and CTRL_ return true?

Personally I would do the first, because then it would already work for all the ctype.h like functions.

Basically where should we have the extra if?

I tried to find some way to get the key status in linux without sudo rights but to be honest. Linux seems to be really lacking in that regard and I could not find any useful resources except for X11.

Furthermore I would suggest to rename the current Key::Value::Ctrl_<letter> to Key::Value::Legacy_Ctrl_<letter> and add additionally Key::Value::Ctrl_<letter> that correspond to the Metakey + Key alternative

There is maybe some other alternative than using X11 but it will depend on the terminal capabilities, that why I have not focused on it for now. I tried to solve a big problem I found with the key press that PR thread try to solve. My idea is to be out of the OS magics strange sometimes bugged studs as quick as possible and wrap/change all these mess using standard library stufs and make it cross plarforms. Then we can try to use the terminal capability to have better informations on key presses.

But for now most of the keys are/could be detected. What I would like then to do before this better key detect is mouse move detection. This feature is missing for console game or Graphical Interface into console

TobiasWallner commented 11 months ago

Just wondering, is Ctrl_A correct or would Ctrl_a be more correct?

flagarde commented 11 months ago

both are ok I think but i'm not ssure it is good to add both.

flagarde commented 11 months ago

@TobiasWallner After some consideration I think the Ctrl_A is closer to the keyboard configuration. Most if not all the keyboard have A not a