vim keybinds in search

[dotcarmen]

it'd be nice to support vim keybinds in search, especially since there's already similar keybinds.

Currently, keybinds in search are:

• up and down arrow for changing active selection in the search results
• left and right arrow for changing cursor in the search query

new keybinds when keybindings: ['vim'] would include:

• <esc> to enter a "normal mode"
• j and k for up/down arrow functionality when in normal mode
• h, l, b, e for changing cursor in the search query when in normal mode
• gg and G (see #1899) also for changing active selection in the search results when in normal mode
• a, i, and c support in normal mode for interaction with search query

[SBoudrias]

The large piece of work is adding mode support with <esc> for search. If we go to that extent, then we should review the whole keybinding strategy rather than implement input modes only for the search prompt.

[dotcarmen]

i'd be happy to help with the implementation after-hours. any notes about the current keybind strategy and what you'd like to see in the new one?

[SBoudrias]

Yeah, currently it's a mix of the useKeypress() hook and a list of helper functions isUpKey().

If you can propose an implementation plan before moving into coding, it'd help making sure we're aligned on the direction.

Early thoughts, I wonder if we should roughly

1. Review useKeypress() interface to something like const { mode } = useKeypress(cb, { useVimMode: true }); (I'm thinking having mode to display which mode someone is in if they're using vim shortcuts; but maybe that UI element should be centralized 🤷🏻 )
2. Review the keypress helpers. Instead of relying on that, maybe we should normalize the keypress inside useKeypress and drop the helpers (we'd keep them for backward compat, but migrate default prompts to use that.)

[dotcarmen]

ok, having read through the implementation and considering your thoughts, i think useKeypress is fine as-is - it's a callback that gets called on raw keypress events. Users may want to use that, i think it's best to leave it available in case they want to implement their own custom features when they use the core api

that said, i do agree that the keypress helpers aren't as helpful as they could be, and the cleanest solution is to normalize navigation handling logic into a single function

with that in mind, i'd like to propose a useKeyNavigation hook which can be used like:

type KeyNavigationCallback = (
  evt: Key,
  readline: InquirerReadline,
  onInput?: () => (void | () => void),
) => (() => void);

const keyNavigation: KeyNavigationCallback = useKeyNavigation({
  up(readline) {
    /* handles up key, k with vim mode enabled & normal mode,
       ctrl-p with emacs mode enabled */
  }
  down(readline) { /* ditto but with down key, j, and ctrl-n */ }
}, { enableVim: true });

useKeypress((evt, readline) => {
  // generic user handling for keypresses
  return keyNavigation(evt, readline, () => {
    // there was no navigation command, process as regular input
  });
});

Here's a few alternatives:

const { callback, done, mode } = useKeyNavigation(/* same as above */);
// for simplicity, using the original lhs pattern below, but could also do { callback, done, mode }
const keyNavigation = useKeyNavigation(
  (command: 'up' | 'down' | '...', readline) => { /* ... */ },
  { enableVim: true },
);
const keyNavigation = useKeyNavigation({
  up(readline) {},
  down(readline) {},
  input(readline) {}, // this way, it can just be: useKeypress(keyNavigation)
}, { enableVim: true });

[dotcarmen]

also, my first PR will only be to implement whatever design we land on, and updating downstream packages to use the new API. it should be doable entirely backwards-compatibly if you like my useKeyNavigation hook

once that PR lands, i'll work on adding normal mode support and expanding commands (eg gg, G)

[SBoudrias]

I like your idea with useKeyNavigation - it also allow to remove the long if/else conditions we have right now.

I wouldn't however combine with useKeypress inside the prompts - I think it's simpler for prompt authors if we keep both hooks separated (and we keep useKeypress for backward compatibility mainly.)

Ideally, I'd want to do away with needing enableVim. Given this tends to be user preference rather than one for library authors, it'd be great to transparently allow/detect the mode switching.

And last thing we need to account for is how we get prompts to display which mode is on (like the -- INSERT -- vim displays.)

[dotcarmen]

I wouldn't however combine with useKeypress inside the prompts - I think it's simpler for prompt authors if we keep both hooks separated (and we keep useKeypress for backward compatibility mainly.)

sure. in that case, perhaps "key navigation" isn't the right phrasing, so maybe useKeypressHandler?

also i realized that inquirer already does callbacks-with-properties, so perhaps both of these will work with Object.assign magic or something:

const handleKeypress = useKeypressHandler(/* ... */);
const { done, mode, ...handleKeypress } = useKeypressHandler(/* ... */);

Ideally, I'd want to do away with needing enableVim. Given this tends to be user preference rather than one for library authors, it'd be great to transparently allow/detect the mode switching.

i'm not sure it's possible to detect emacs/vim preference unless the terminal is in emacs or vim. and given inquirer is "just" a library, there's no reason to add a config file or standard environment variables

i think it's best to leave this to the api consumer, rather than the program consumer. in my own needs, i'm writing scripts that may not necessarily be used by somebody with experience with vim, so my idea is to add an undocumented --vim flag on the CLI for my own personal usage

And last thing we need to account for is how we get prompts to display which mode is on (like the -- INSERT -- vim displays.)

I haven't looked at the ui rendering stuff yet, but i think it makes sense to lift some of that logic into useKeypressHandler - for example as a simple example based on checkbox:

const handleKeypress = useKeypressHandler(/* ... */);
const helpText = Object
    .entries(handleKeypress.keymap)
    .map(([command, keys]) => `${keys.join('')} ${command}`)
    .join(' | ');

[SBoudrias]

How do you suggest handling combined keypresses like 3j or gg with the useKeypressHandler interface?

[dotcarmen]

earlier i suggested up and down callbacks on the first argument, but it might be better to do more abstraction

in order to account for all of the commands in this issue, i propose these handlers:

• scrollRelative(offset, readline) scrolls offset (pos or neg) "lines" from the current position
• cursorRelative(offset, readline) is the cursor analogue of scrollRelative
• jumpWord(offset, readline) jumps offset words forwards or backwards
• cut(length, readline) deletes Math.abs(length) characters from the current position

i think scrollAbsolute(index, readline) and cursorAbsolute(index, readline) are the only *Relative analogues that are necessary - these accept a positive or negative index to anchor to the beginning or end of input

i also think it makes sense for there to be input(key: Key, readline) and unhandledSequence(sequence: Key[], readline). unhandledSequence will be used for unrecognized commands in vim normal mode, and for unrecognized ctrl- and alt- sequences (which also encapsulates unrecognized emacs commands - even though there's no alt- handling right now)

[SBoudrias]

Few comments:

1. Should jumpWord be abstracted away into cursorRelative?
2. input/unhandledSequence could also just be a default entry (kinda like a switch case.) Not sure if we need to differentiate both; I don't feel strongly either way if the decision doesn't carry lots of complexity.

If you're willing to, I think that's a good starting point to start a PR.

[dotcarmen]

Should jumpWord be abstracted away into cursorRelative?

that makes sense, yeah. so then i'd suggest the signature be cursorRelative(boundary: 'word' | 'char', offset, readline). boundary is always char when vim and emacs modes are disabled, so i'm tempted to say the signature should be cursorRelative({ boundary, offset }, readline), but then why not cursorRelative({ boundary, offset, readline })? that would imply making all of the signatures accept 1 options object... idk, maybe i'm spiraling, what do you think?

input/unhandledSequence could also just be a default entry (kinda like a switch case.) Not sure if we need to differentiate both; I don't feel strongly either way if the decision doesn't carry lots of complexity.

unhandledSequence as i proposed does pass multiple Key objects, which is necessary in the vim and emacs cases (maybe not? i'm not aware of any emacs sequences that need to be handled in readline interfaces...) when input always only ever needs 1 key.

maybe that's fine? default(mode: VimMode, keys: Key[], readline) (or default({ mode, keys, readline }) or some other option). in the case where the consumer doesn't have vim mode enabled, it would be implemented like this, which might be awkward:

const keypressHandler = useKeypressHandler({
  // ...
  default(_vimMode, [key], readline) { /* ... */ },
  // alternatively:
  default({ keys: [key], readline }) { /* ... */ },
});

[dotcarmen]

boundary is always char when vim and emacs modes are disabled

actually that's not true - ctrl+left/right arrow :)

[dotcarmen]

hey - haven't gotten to this yet (busy few weeks), but will start working on it this weekend :)