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

Parse xterm-keys for motion directly #2609

Merged
merged 5 commits into from
Dec 9, 2018
Merged

Conversation

jeapostrophe
Copy link
Contributor

Ideally, something better should be done (re #2554) but this is a decent
intermediate step for some useful keys.

Note: NCurses supports parsing these keys when shifted (KEY_SR,
_SLEFT, S_RIGHT, etc), but it does not do the same thing for the other
modifiers.

@jeapostrophe
Copy link
Contributor Author

jeapostrophe commented Nov 28, 2018

I am particularly interested in Alt+Arrows, because I am building something that matches my muscle memory from other editors.

@Screwtapello
Copy link
Contributor

The "proper" way to handle these keys (according to user_caps(5)) is:

  • start with the list of modifiable keys: kDC, kDN, kEND, kHOM, kLFT, kNXT, kPRV, kRIT, kUP
  • and the list of modifier codes: 2 for Shift, 3 for Alt, 4 for Shift+Alt, 5 for Control, etc.
  • for each possible combination:
    • concatenate the key and the modifier to produce a potential property name like kLFT4
    • call tigetstr(property_name) to obtain the byte-string that the terminal will send to signify that key and modifier (on my terminal, kLFT4 is ^[[1;4D)
    • call key_defined(byte_string) to return the dynamically-allocated key-code ncurses uses to signify that key
    • add the resulting key-code to some dynamically-allocated equivalent of the switch statement at line 584 of ncurses_ui.cc

Compared to this patch, such an algorithm would work for all modifiable keys, not just the arrow keys, Home, and End. Also, it would work for any terminal in the terminfo database, not just ones that exactly copy xterm's encoding.

On the other hand, such an algorithm would be a much more invasive change, and nobody's actually done the work to implement it, while this patch is already here and ready.

src/keys.hh Show resolved Hide resolved
src/ncurses_ui.cc Outdated Show resolved Hide resolved
if ( f == NULL )
{
ungetch(c3); ungetch(c2); ungetch(c1); break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if the functor (from above) has a valid target, instead of checking for the NULL value (in C++, you should rather use std::nullptr, but it won't matter with a functor).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've switched to nullptr. I didn't embed this check into the switch, because we don't want the break on the inner one, we want it on the outer one.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullptr is a built in, so no need for std:: in front. but you are correct, we use nullptr in Kakoune codebase.

src/ncurses_ui.cc Outdated Show resolved Hide resolved
@jeapostrophe
Copy link
Contributor Author

@lenormf I've updated based on your feedback.

@Screwtapello Good to know. An efficient way to implement the dynamic switch would be a trie from key sequences to key encodings and have the reading code explore that trie and have a default case of letting the bytes through. This is essentially just a tokenization/scanner (lex) problem, so there's lots of efficient techniques to do it well if you know the mapping between strings and values you want. The problem is that doing it smart would be out-of-scope for Kakoune and you don't want to depend on other libraries. Just using std::map as a trie is likely to be much less efficient, but doable.

src/ncurses_ui.cc Outdated Show resolved Hide resolved
src/ncurses_ui.cc Outdated Show resolved Hide resolved
@jeapostrophe
Copy link
Contributor Author

Updated again re: @lenormf

@jeapostrophe
Copy link
Contributor Author

@mawww Anything else you'd like changed before merging?

@jeapostrophe
Copy link
Contributor Author

I added my copyright waiver.

src/ncurses_ui.cc Outdated Show resolved Hide resolved
Ideally, something better should be done (re mawww#2554) but this is a decent
intermediate step for some useful keys.

Note: NCurses supports parsing these keys when shifted (KEY_SR,
_SLEFT, S_RIGHT, etc), but it does not do the same thing for the other
modifiers.
  I dedicate any and all copyright interest in this software to the
  public domain.  I make this dedication for the benefit of the public at
  large and to the detriment of my heirs and successors.  I intend this
  dedication to be an overt act of relinquishment in perpetuity of all
  present and future rights to this software under copyright law.
@mawww
Copy link
Owner

mawww commented Dec 9, 2018

Hi, I tried to test that PR locally, and I could not get it to work proprerly, what settings do you put xterm in ? I see that putting it in 8-bit mode generates those keys, except that the CSI sequence is sent as 8-bits as well, which is not parsed correctly. I have local changes to handle all that, but I was wondering what settings you were using.

@jeapostrophe
Copy link
Contributor Author

I don't use xterm. I am using iterm2/tmux and kitty on OS X. kitty has default settings. iterm2 has the "xterm defaults" and tmux has the xterm keys on.

@Screwtapello
Copy link
Contributor

Screwtapello commented Dec 9, 2018

If I start a genuine xterm and run:

tput smxk; cat

...and then press Ctrl+Up, I see the output:

^[[1;5A

...which would be parsed as <c-up> by this code. smkx enables application keypad mode, which Kakoune does in ncurses_ui.cc line 507. The only configuration option I've messed with is metaSendsEscape: true, but that shouldn't be relevant to here since I'm not pressing Meta.

@mawww
Copy link
Owner

mawww commented Dec 9, 2018

It seems here ncurses parses some keys, such as , for which I get immediatly '553' out of wgetch. I assume this is an internal value ncurses uses, and it seems disabling the 'keypad' call fixes, however I then get ^]OA for up, which seems to be "Single Shift Select G3 Character set" then the code for up (A)...

@Screwtapello
Copy link
Contributor

The keypad() call determines whether ncurses interprets the key encodings defined in terminfo, or passes them through to the application. It seems like mawww's terminal emulator properly generates the special key encodings for modified cursor keys, and his terminfo database describes those encodings, so ncurses recognises them and wgetch() returns a dynamically-allocated key number, as described in my comment above.

jeapostrophe's situation is that while their terminal sends those encodings, their terminfo database doesn't describe them, so ncurses doesn't recognise them and passes them straight through to the application uninterpreted.

@mawww mawww merged commit c0be01a into mawww:master Dec 9, 2018
@mawww
Copy link
Owner

mawww commented Dec 9, 2018

I merged this and used it as a base to refactor key parsing slightly, I am still unsure what direction we should take from here, at lease this adds more support for key combinations while preserving what we had, I am tempted to evolve that in the direction Screwtape described, but maybe we can hope for a common, forward compatible key encoding scheme that we can implement (maybe the one Leonerd suggested, or the kitty one...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants