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

[RFC] Fix function keys in embedded terminal #5014

Merged
merged 5 commits into from Sep 6, 2017

Conversation

Projects
None yet
4 participants
@rjmill
Contributor

rjmill commented Jul 6, 2016

fix #4343

F5-F12 seem to be working perfectly. F1-F4 work as expected in htop and bash, but output extra characters in zsh.

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 6, 2016

Can you combine the two PRs? Any other K_ codes that might need to be handled?

@marvim marvim added the WIP label Jul 6, 2016

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 6, 2016

Sure thing!

I imagine pretty much all the K_S_* codes will need to be handled, but they should be a pretty straightforward fix. I'll add them to the PR too.

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from 621ecf7 to b30162b Jul 9, 2016

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 9, 2016

I wonder if ctrl-space (#3101) could be fixed in this way as well.
Here's a report about ctrl-left/right also: #5024

@@ -793,6 +794,57 @@ static VTermKey convert_key(int key, VTermModifier *statep)
case K_KMULTIPLY: return VTERM_KEY_KP_MULT;
case K_KDIVIDE: return VTERM_KEY_KP_DIVIDE;

case K_S_F1: *statep |= VTERM_MOD_SHIFT;

This comment has been minimized.

@justinmk

justinmk Jul 9, 2016

Member

perhaps all of these K_S_* variants should be grouped in a separate switch before this one.

switch(key) {
    case K_S_F2:
    case K_S_F3:
    case K_S_F4:
    ...
      *statep |= VTERM_MOD_SHIFT;
}

This comment has been minimized.

@justinmk

justinmk Jul 9, 2016

Member

and that switch should live in convert_modifiers() ?

This comment has been minimized.

@rjmill

rjmill Jul 9, 2016

Contributor

I thought about that, but if we took that approach, we'd still need to check all the K_S_* keys individually in the big switch since the K_S_* keys count as different keys from the K_* ones.

If I understand your suggestion correctly, such a function would only let us go from

case K_S_F1:      *statep |= VTERM_MOD_SHIFT;
case K_F1:        return VTERM_KEY_FUNCTION(1);

to

case K_S_F1:
case K_F1:        return VTERM_KEY_FUNCTION(1);

This comment has been minimized.

@justinmk

justinmk Jul 9, 2016

Member

Good point, but I think that's still preferable:

  • the *statep |= VTERM_MOD_SHIFT; logic is a performed in a single place instead of repeated
  • modification of statep continues to be driven by convert_modifiers

This comment has been minimized.

@rjmill

rjmill Jul 9, 2016

Contributor

That's a good point. I'll make the change.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 9, 2016

I'll look into it. Should be doable. Any key combination that vim can recognize should be able to be passed into the terminal fairly easily this way.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 9, 2016

ctrl-left/right seem pretty plug and play for this fix. ctrl+space might be a little trickier, but I think I can figure something out for it.

@justinmk do you know of any good way to test to make sure these keys are working correctly? so far, I've just been finding programs/commands that expect them and using those, but that's not scaling very well, haha.
calling up vim from the embedded terminal with a special vimrc works pretty well for testing
showkey works better

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from 547c415 to 022a9d5 Jul 10, 2016

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 10, 2016

As of right now, the shifted function keys only work "in theory". It seems like nvim isn't able to recognize the codes for them consistently. If there are any platforms that nvim knows how to handle the shifted function keys on, or if someone were to implement that in nvim, the shifted function keys should just work.

If you'd rather me take the shifted function key logic out, just let me know.

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 10, 2016

@rjmill Ctrl_AT should correspond to ctrl-space. (^<space> is sent to terminal as ^@).

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 10, 2016

If you'd rather me take the shifted function key logic out, just let me know.

The logic looks fine to me.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 10, 2016

Ctrl_AT should correspond to ctrl-space. (^@ is sent to terminal as ^).

The problem with ctrl-space and ctrl+@ is that there doesn't seem to be a VTermKey enum value that corresponds to K_ZERO (which is what both key combos are by the time they get to the terminal logic.)

I've got some ideas on how to handle that, but they're all either dirty hacks or more significant/risky/clunky refactors that should probably get their own separate PR. It sounds like upstream changes aren't much of an option either.

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 10, 2016

@leonerd can you say whether vterm has a ctrl-space key or some other way to send that key?

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from 022a9d5 to 3a3c3d9 Jul 11, 2016

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 11, 2016

Ctrl-space and ctrl-@ work now!

Side note, ctrl-2 was also broken, but is now "fixed." Not sure why, but gnome-terminal and urxvt both interpret ctrl-2 the same as ctrl-space and ctrl-@ . So if anyone had been complaining about ctrl-2 outputting weird characters, that's taken care of now.

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 11, 2016

Nice! Need to think about why that (much appreciated) hack is needed.

We should add some test coverage to this PR (could just plop it in a new describe() block in tui_spec.lua. Use thelpers.feed_data() to send the keys.). That way, we can move forward with this, and if some indirectly related problem is found later, we can fix it while being sure we didn't regress the progress made here.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 11, 2016

Need to think about why that (much appreciated) hack is needed.

It's because there's already an ASCII representation for the character (0), but vim converts it to its own code, just like it does for all the other special keys. But since it's ASCII, vterm(quite sensibly) doesn't have a special enum for it. Hence why it needs to be handled differently.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 11, 2016

It might be feasible to convert it to ASCII earlier in the key handling process in keymap.c, but I imagine it will break some stuff, given that 0 is falsey in C. Lots of room for strange behavior there.

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 11, 2016

It's because there's already an ASCII representation for the character (0), but vim converts it to its own code, just like it does for all the other special keys. But since it's ASCII, vterm(quite sensibly) doesn't have a special enum for it. Hence why it needs to be handled differently.

Sure, but I wondered why ASCII 0 was special, and ctrl-a (ASCII 1) is not. In gdb ctrl-a reaches terminal_send_key() as c=1. However, the answer is in this comment:

*  NUL cannot be in the input string, therefore it is replaced by
*  K_SPECIAL   KS_ZERO>KE_FILLER
*/
#define KS_ZERO                 255
@justinmk

This comment has been minimized.

Member

justinmk commented Jul 11, 2016

It looks like this block in get_special_key_name is a more robust way to extract the modifiers from a K_S_ special key int. That block could be factored out to a function, then then modifiers there could be checked to set equivalent vterm modifiers. Then I think KEY2TERMCAP1(c) - 48 could be passed to VTERM_KEY_FUNCTION(...).

It's less readable, for little gain I suppose. Though it could fill in whatever remaining modifier+special keys we're missing support for.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jul 12, 2016

I've got your suggestion mostly working, but there are still edge cases that aren't handled properly (including Ctrl_AT). I think it's a much cleaner solution than those switch statements, but it looks like we're still going to have to resort to hacks to get this one working fully.

For what it's worth, if we go with the switch statements, any special keys/combinations we miss will be pretty trivial to tack on as we find them. Whereas if we miss stuff with the cleaner solution, it will most likely be an ordeal to support any keys we miss.

I'll keep searching for a clean solution, if only to satisfy my curiosity, but I think there's a good chance that any practical solution to this is going to involve some kind of hack for the edge cases.

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 12, 2016

but there are still edge cases that aren't handled properly (including Ctrl_AT)

ASCII 0 is not in the table, so yes it will be a special case. But anything else that's not in the table is just not supported by Vim 's input model which maps these keys to single integers. Right?

Anything else that comes through would not be mapped to a K_* key I believe. If there's any hope of supporting those missing combos, we depend on the global modifier flags.

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 4, 2016

Note: also see if shift+tab can be handled.

@justinmk justinmk referenced this pull request Oct 5, 2016

Open

:terminal improvements #5431

5 of 17 tasks complete
@rjmill

This comment has been minimized.

Contributor

rjmill commented Oct 15, 2016

This fix will handle shift+tab. (Shift+tab is actually what originally brought this issue to my attention, haha.)

I'll try to get this wrapped up over the next week.

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 15, 2016

Great!

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from 3a3c3d9 to 9dec8ba Nov 13, 2016

@rjmill

This comment has been minimized.

Contributor

rjmill commented Nov 13, 2016

After dusting this PR off, it looks like special keys are working as expecting in the terminal with the current implementation by putting it all into the big switch statement. I have a local branch where I implemented your suggestion to extract the block in get_special_key_name to its own function and use that to handle special key codes.

It looks like I left off trying to get the branch with the extracted function to the same point of functionality as the branch with the switch statement. It seems that it mostly just doesn't handle ASCII 0. I remember having concerns that there might be other edge cases besides ASCII 0, but I don't remember why, nor do I see why there would be other edge cases besides ASCII 0. As long as those concerns don't turn out to be valid, I should be able to merge those changes into this PR tonight.

c = Ctrl_AT;
}

c = unmodify_key(c, &mod_mask);

This comment has been minimized.

@justinmk

justinmk Nov 14, 2016

Member

this is modifying the global mod_mask, is that what we want?

@rjmill

This comment has been minimized.

Contributor

rjmill commented Nov 14, 2016

@justinmk I think we were planning on reusing that block of code to help ensure that we didn't miss any special keys with the switch statement.

I agree that we should put the unmodify_key branch in its own PR (if we use it at all.) Putting everything into the switch statement feels a bit less elegant, but it's a lot more readable and straightforward to fix if we ever need to revisit it.

I'll go ahead and roll this back to the commit before the unmodify_key one.

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from 61874e8 to ca716f3 Nov 14, 2016

@rjmill rjmill changed the title from [WIP] Fix function keys in embedded terminal to [RFC] Fix function keys in embedded terminal Nov 14, 2016

@marvim marvim added RFC and removed WIP labels Nov 15, 2016

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from ca716f3 to 27a1e8f Nov 20, 2016

@junegunn junegunn referenced this pull request Nov 23, 2016

Closed

function key binds no longer work? #741

4 of 15 tasks complete

@rjmill rjmill force-pushed the rjmill:fix-function-keys branch from 27a1e8f to a1d545f Jan 10, 2017

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jan 11, 2017

@justinmk What else do I need to do to wrap up this PR?

@justinmk justinmk added this to the 0.2 milestone Jan 11, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Jan 11, 2017

Some tests would be nice, but it's probably too complicated. I've added this to 0.2 milestone so it will get a look soonish.

@rjmill

This comment has been minimized.

Contributor

rjmill commented Jan 11, 2017

Thanks! I'd be happy to write some tests, but I'm not really sure where to start testing something like this. Is it something I can just figure out by poking around in the test suite?

@justinmk justinmk modified the milestones: 0.2, 0.2.1 Apr 24, 2017

@bohrshaw

This comment has been minimized.

Contributor

bohrshaw commented Sep 5, 2017

I've recently made extensive use of the embedded terminal.
This is probably the most anticipated PR, personally.

justinmk added a commit to justinmk/neovim that referenced this pull request Sep 5, 2017

justinmk added a commit to justinmk/neovim that referenced this pull request Sep 5, 2017

@justinmk justinmk merged commit a1d545f into neovim:master Sep 6, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
QuickBuild Build pr-5014 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@justinmk justinmk removed the RFC label Sep 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment