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 auto-closing brackets/quotations in the REPL #1680

Merged
merged 95 commits into from
Apr 1, 2024

Conversation

Snehil-Shah
Copy link
Member

@Snehil-Shah Snehil-Shah commented Mar 4, 2024

Resolves #1672

Description

What is the purpose of this pull request?

This pull request:

  • Implements auto-closing of brackets and quotations in the REPL.

demo

  • Adds option to toggle this behavior.

settings

  • Adds support for modifier key to add newline for multi-line editing

Related Issues

Does this pull request have any related issues?

This pull request:

Other

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

For Multi-line editing, I have used ALT+ENTER as the modifier key.

This might seem odd given SHIFT+ENTER is the conventional key combination to add a newline instead of executing a command, but it is done because the terminal or the readline module cannot differentiate between ENTER & SHIFT+ENTER or even CTRL+ENTER.

values of keypress events

ALT+ENTER generates a different keypress value than the others.

Questions

Any questions for reviewers of this pull request?

I am utilizing readline module's private methods (shown in image) to mess with the input stream. Public apis like write() or cursorTo() aren't working as expected.

image

I am not entirely sure if it's okay to use these private methods, but they definitely work as desired.

UPDATE: Solved!

Checklist

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


@stdlib-js/reviewers

Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
@Planeshifter Planeshifter 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. labels Mar 4, 2024
@Snehil-Shah
Copy link
Member Author

@kgryte maybe we can also add a keyboard shortcut to toggle this behaviour?

@kgryte
Copy link
Member

kgryte commented Mar 4, 2024

@Snehil-Shah Atm, we don't have any support for keyboard shortcuts in the REPL, so I'd like to avoid adding anything like that here. Adding keyboard shortcuts is a future concern, but needs to be addressed more holistically (e.g., vim, Sublime Text, etc, keybindings).

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.

Left some initial comments.

Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
@Snehil-Shah Snehil-Shah requested a review from kgryte March 5, 2024 09:16
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
@Snehil-Shah
Copy link
Member Author

@kgryte IPython uses CTRL+O to manually add multi-line, maybe we can add that too on top of ALT+ENTER?

@kgryte
Copy link
Member

kgryte commented Mar 6, 2024

Hmm...CTRL+O is not obvious to me. I think people are more likely to be used to chat apps which use CMD or SHIFT as the modifier with ENTER.

Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
@Snehil-Shah
Copy link
Member Author

@kgryte improved the manual multi-line editing by making it cursor-aware.

manual

when manually adding a newline (using alt+enter), it adds a newline from the cursor position just like a text editor would!

is this okay?

@Snehil-Shah Snehil-Shah changed the title feat: add auto-closing brackets and quotations in REPL. feat: add auto-closing brackets/quotations and manual multi-line editing in REPL. Mar 8, 2024
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
Signed-off-by: Snehil Shah <130062020+Snehil-Shah@users.noreply.github.com>
@Snehil-Shah
Copy link
Member Author

@kgryte Made auto-closing symbols more ergonomic by handling the case where the user instinctively adds a closing bracket when typing with flow. The extra closing symbol is discarded similar to IDEs.

instinct

@kgryte
Copy link
Member

kgryte commented Mar 31, 2024

@Snehil-Shah Thanks for adding auto-delete tests!

@kgryte
Copy link
Member

kgryte commented Mar 31, 2024

Another thing is do we need two separate settings for auto-deletion and auto-closing? Aren't both generally considered part of the same feature?

They are certainly related, but, while often they are implemented together, IMO they are still distinct. As a user, I may want auto-deletion, but not auto-closing because I may find auto-insertion annoying or want more fine-grained control as to when characters are inserted. And vice versa. For auto-deletion in particular, if I typed [] and then only want to delete the [, I need to resort to odd little workarounds (e.g., first inserting a space between the characters or deleting and then retyping ], etc). That may be enough for users to prefer one behavior but not the other.

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@kgryte
Copy link
Member

kgryte commented Mar 31, 2024

We can just use regex in this case to check the character just before/after the symbol like I did before.

Ah, okay, that was why you had that logic in there; I wasn't able to figure out from the comments alone what the intent was. But also, the regexp approach was too limited, as string content can be more than just ASCII. I think I have a solution for this and can push up a fix later today.

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@kgryte
Copy link
Member

kgryte commented Apr 1, 2024

@Snehil-Shah Okay. I think this is in a pretty good state. You mind doing a final review to check that we didn't miss anything?

@Snehil-Shah
Copy link
Member Author

@kgryte Checked everything, looks good to go👍

@kgryte kgryte removed the Needs Changes Pull request which needs changes before being merged. label Apr 1, 2024
@kgryte
Copy link
Member

kgryte commented Apr 1, 2024

@Snehil-Shah Thanks for checking! Let's go ahead and get this in. Any subsequent work can be addressed in follow-up PRs. Thanks for your work on this; this is really nice functionality to have.

@kgryte kgryte merged commit 8314237 into stdlib-js:develop Apr 1, 2024
7 checks passed
@Snehil-Shah Snehil-Shah deleted the repl branch April 26, 2024 12:29
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. 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 auto-closing brackets and quotations in REPL
3 participants