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

feat: add support for custom keybindings and editor actions in the REPL #2739

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

Snehil-Shah
Copy link
Member

@Snehil-Shah Snehil-Shah commented Aug 3, 2024

Towards #2647.

Description

What is the purpose of this pull request?

This pull request:

  • adds editor actions
  • adds support for custom keybindings
  • make existing actions configurable
  • adds settings, APIs and update docs

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

For the sake of user experience, I decided the key object can have 4 properties: name, ctrl, shift, and meta. In actuality, a readline key object also has properties like sequence and code, which mainly contain the escape sequence of the keypress.

In most keypresses, the name, ctrl, shift, and meta properties are correctly emitted and are enough to determine the action we have to trigger. But in some cases, for instance, CTRL+/, instead of emitting an event with { 'name': '/', 'ctrl': true }, it emits a sequence { 'sequence': '\x1f' }. So if a user has keybindings with CTRL+/ as the key, it won't work, as such a keypress is never emitted.

Now such cases might be limited, as readline only uses the sequence prop to identify two keypress actions, namely undo and redo.
Maybe we can handle these two special cases manually in code? Not sure if a robust solution exists.

Ofcourse, we can also punt the entire responsibility of giving the readline-ready key object, onto the user, but that's not user friendly IMO.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@Snehil-Shah
Copy link
Member Author

Another solution to the problem in OP is, we can expect the user to give us the ASCII sequence of the keypress instead.

image

Every keypress will always have a sequence prop unlike name, ctrl, shift and meta.

Now this is not user-friendly as the user is expected to know the control sequence of the keypress they are wanting to assign.

So, another approach would be to maintain a table mapping all possible keybindings to their corresponding emitted control sequences. Something like:

const CONTROL_SEQUENCES = {
    '\x01': 'Ctrl+A',
    '\x02': 'Ctrl+B',
    '\x03': 'Ctrl+C',
    '\x04': 'Ctrl+D',
    '\x05': 'Ctrl+E',
    '\x06': 'Ctrl+F',
    '\x07': 'Ctrl+G',
    '\x08': 'Ctrl+H',
    '\x09': 'Ctrl+I',
    '\x0A': 'Ctrl+J',
    '\x0B': 'Ctrl+K',
    '\x0C': 'Ctrl+L',
    '\x0D': 'Ctrl+M',
    '\x0E': 'Ctrl+N',
    '\x0F': 'Ctrl+O',
    '\x10': 'Ctrl+P',
    '\x11': 'Ctrl+Q',
    '\x12': 'Ctrl+R',
    '\x13': 'Ctrl+S',
    '\x14': 'Ctrl+T',
    '\x15': 'Ctrl+U',
    '\x16': 'Ctrl+V',
    '\x17': 'Ctrl+W',
    '\x18': 'Ctrl+X',
    '\x19': 'Ctrl+Y',
    '\x1A': 'Ctrl+Z'
};

But not sure if control sequences and their corresponding keys are standardized across emulators..?
How should we proceed?

@Snehil-Shah
Copy link
Member Author

https://github.com/nodejs/node/blob/main/lib/internal/readline/utils.js#L59

readline parses some keys based on xterm and gnome emulator behaviors (I guess). Maybe we can enforce xterm standards to parse keys? I suppose most combinations might emit standardized sequences..(?)

@Planeshifter
Copy link
Member

@Snehil-Shah Not requiring providing a sequence was the right call, I think, given that asking users to know or look up the control sequences for keybindings they want is undesirable. The current approach of a key object with Boolean modifier properties seems good; we could consider renaming meta to alt, as that the latter be understood by more people maybe?

Enforcing xterm standards for key parsing could be feasible. xterm is widely used and AFAIK many terminal emulators aim for compatibility with xterm, so this sounds worth investigating.

I also do think that it would be a pragmatic choice to handle the undo and redo special cases separately.

@Snehil-Shah
Copy link
Member Author

Now if we decide to go ahead and enforce xterm control sequences, we also have the flexibility to change the input format the user can give. So we can have something like the string "CTRL-E" as input, and map this to the corresponding xterm control sequence..

@Snehil-Shah
Copy link
Member Author

Went ahead and added parsing of unrecognized keypresses. Fortunately, there were not many cases to cover, the above problems were mainly around keys involving symbols.

@Planeshifter I think it would be better to stick to meta as the internal readline key object, also uses it. This makes it easier to map keybindings. And julia also uses meta instead of alt in their naming so we're not alone.

@Snehil-Shah Snehil-Shah marked this pull request as ready for review August 11, 2024 13:52
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Looks good to me!

One small comment: I believe we can safely assume that _rli.line is a string, correct? Hence, we could use the base package equivalents, so e.g. @stdlib/string/base/uppercase instead of @stdlib/string/uppercase, to bypass runtime argument validation of these functions.

@Planeshifter Planeshifter added the Ready To Merge A pull request which is ready to be merged. label Aug 14, 2024
@Snehil-Shah
Copy link
Member Author

@Planeshifter Makes sense..

@kgryte kgryte removed the Ready To Merge A pull request which is ready to be merged. label Aug 15, 2024
@kgryte
Copy link
Member

kgryte commented Aug 15, 2024

we could use the base package equivalents

We should go ahead and make those changes. Too easy to forget to do them later.

For instance, the right arrow key should also complete the completion.
It would be a better idea to clean up this logic in the PR where we add support for configuring keybindings for existing actions.

Signed-off-by: Snehil Shah <[email protected]>
@kgryte kgryte added Feature Issue or pull request for adding a new feature. Needs Review A pull request which needs code review. REPL Issue or pull request specific to the project REPL. Needs Changes Pull request which needs changes before being merged. labels Aug 22, 2024
}
for ( j = 0; j < actionKeys.length; j++ ) {
for ( k = 0; k < possibleKeys.length; k++ ) {
if ( deepEqual( possibleKeys[ k ], actionKeys[ j ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Deep equality could be an expensive check. Is there a potential lighter weight means?

Copy link
Member

Choose a reason for hiding this comment

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

Given that we know the keys, can we not write a small custom function which explicitly compares known keys?

Copy link
Member Author

Choose a reason for hiding this comment

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

For two keys to be equal, their four properties should be equal (name, ctrl, meta, shift). Wouldn't deep equality do just that?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but it is the difference between comparing values having an unknown schema and those having a known schema. For the former, you need to dynamically resolve the list of keys and then iterate. For the latter, you can skip resolution and directly compare without iteration. It is something of a micro-optimization, but my main concern is preventing lag, especially as we continue to add various keystroke listeners.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I'll make a helper function, or better, a module like containsKeys which will take an array of possible keybindings (as one action has multiple keybindings set) and the user input key.

Copy link
Member

Choose a reason for hiding this comment

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

Seems reasonable.

Comment on lines 224 to 225
* @param {string} key - readline keypress object
* @returns {(Array<Object>|boolean)} list of possible key objects
Copy link
Member

Choose a reason for hiding this comment

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

These types seem off. Is key an object or a string? And it is not obvious that the function can return a boolean.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. parseMetaSymbolSequence can return a boolean. Would it make more sense to make the types consistent? For example, return an empty array? Alternatively, rather than a boolean, it is more common for us to return null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to update the jsdoc, sorry for the confusion. Yes, the main parse_key function doesn't return a boolean.

I updated parseMetaSymbolSequence to return null instead. If it returns null means it's not a meta symbol sequence. So we just return the original key object.

Should parseMetaSymbolSequence return an empty array? The only problem would be, we won't be able to check directly if it returned a sequence or not (using if ( out )) but would have to check if the array is empty. null should be fine(?).

Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue with checking out.length > 0? I suppose I am not sure why null would be preferred over checking the length of the returned array.

Comment on lines +122 to +148
'indentLineRight': [
{
'name': 'right',
'ctrl': false,
'shift': false,
'meta': true
},
{
'name': 'i',
'ctrl': true,
'shift': false,
'meta': false
}
],
'indentLineLeft': [
{
'name': 'left',
'ctrl': false,
'shift': false,
'meta': true
},
{
'name': 'i',
'ctrl': false,
'shift': false,
'meta': true
}
Copy link
Member

Choose a reason for hiding this comment

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

@Planeshifter and I were not able to get indentation to work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tried and it's highly likely it's clashing with your terminal's internal keybindings (that's the case with me, ALT+RIGHT is vscode terminal's internal binding). Just change the keybinding to something unoccupied and it should work.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder how we should publicly document these keybindings. As @Planeshifter and I experienced, not all the default keybindings work (e.g., due to a terminal's internal keybindings). I can already imagine getting bug reports for when keybindings don't work, but this is because a user is unaware that some other application has already laid claim to a particular keybinding.

Comment on lines +180 to +196
'name': 'backspace',
'ctrl': true,
'shift': false,
'meta': false
},
{
'name': 'backspace',
'ctrl': false,
'shift': false,
'meta': true
},
{
'name': 'w',
'ctrl': true,
'shift': false,
'meta': false
}
Copy link
Member

Choose a reason for hiding this comment

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

Should all of these keybindings work or just one of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

All of them should work. The user can set multiple keybindings to trigger a single action.

Copy link
Member

Choose a reason for hiding this comment

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

It was often the case that only one of the keybindings worked. I suppose it is possible that there are conflicts happening somewhere so that not able to see intended changes.

'meta': true
}
],
'indentLineRight': [
Copy link
Member

Choose a reason for hiding this comment

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

This also doesn't work for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this comment also for the indentation?

Copy link
Member

@kgryte kgryte Oct 26, 2024

Choose a reason for hiding this comment

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

Yes, meaning same as indentLineLeft for me, as neither worked.

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

Thanks, @Snehil-Shah, for working this. I think this PR is pretty close to merge. Just left a few small questions, and @Planeshifter and I were not able to get indentation to work.

@kgryte
Copy link
Member

kgryte commented Sep 20, 2024

Maybe we can handle these two special cases manually in code? Not sure if a robust solution exists

@Snehil-Shah I'll defer to your judgement on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature. Needs Changes Pull request which needs changes before being merged. Needs Review A pull request which needs code review. REPL Issue or pull request specific to the project REPL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC]: add support for custom key bindings in the REPL
3 participants