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 UX to cycle through completions in the REPL #2463

Merged
merged 27 commits into from
Jul 1, 2024

Conversation

Snehil-Shah
Copy link
Member

Subtask of #1845

Description

What is the purpose of this pull request?

This pull request:

  • replaces readline's built-in completer with a new UX that allows highlighting and navigating completions.

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

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

Recording of new behavior:

completer.mp4

Checklist

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


@stdlib-js/reviewers

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Snehil-Shah added Enhancement Issue or pull request for enhancing existing functionality. REPL Issue or pull request specific to the project REPL. labels Jun 26, 2024
kgryte and others added 2 commits June 26, 2024 16:41
Signed-off-by: Athan <kgryte@gmail.com>
Co-authored-by: Athan <kgryte@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
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.

Very slick and works great, thanks so much @Snehil-Shah for your work on this!

One small issue: The current behavior of the tab completions is a little unintuitive when there are too many completions to be rendered in the terminal. Those won't be visible but one can then still navigate to them with the right arrow keys.

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

@Planeshifter I updated the code, now one can only traverse what they see. And as we also support interactive terminal window sizes, they can always resize. Here's a video demonstrating this:

resize.mp4

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

kgryte commented Jun 29, 2024

@Snehil-Shah Pulled down the latest and played around. Works nicely. Thanks for working on this.

One comment: currently, when I enter co and then TAB, I get a list of potential matches. When I enter the next letter, such as n, the list of potential matches disappears and I need to hit TAB again to see the updated list. I then need to repeat this sequence for each new character until I've completed an identifier.

What I wonder is whether we should eliminate the need for repeatedly hitting TAB in order to see the list of potential matches. In terms of UX, one thing I do when seeing completion possibilities is use the list to help guide my spelling, having immediate feedback that I've misspelled something when the list disappears due to a spelling mistake.

In which case, I wonder if hitting TAB and triggering a completion list should trigger a "completion mode" in which the list of completions is continually displayed and updated as I enter new characters, without needing to explicitly hit TAB after every entered character.

Not sure how feasible this suggestion is.

@kgryte
Copy link
Member

kgryte commented Jun 29, 2024

@Snehil-Shah Yes, this is what I was thinking. Thanks for the updates. The only other small niggle is that, when I type con and hit TAB, the completions display. When I continue typing const, the completions disappear due to an exact match with const; however, const may not be what I want, but I am no longer able to see the possibilities until I hit the next character consta. I wonder if it would make sense to keep the completion options still visible even when an exact match.

@Snehil-Shah
Copy link
Member Author

@kgryte The new UX doesn't make the completions disappear due to an exact match. Turns out it's a bug in the completer callback and not the completerEngine, and it is only specific to const. For example, typing conj while in tab completions view, correctly displays both conj and conjf.

image

I will investigate this and fix it in a separate patch PR.

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

Also, wrote the test for this and the specific case you mentioned of an exact match.

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.

Did another look through the PR and played around with the latest changes; looks all good to me!

Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
@Snehil-Shah Snehil-Shah requested a review from kgryte July 1, 2024 12:07
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
Signed-off-by: Athan <kgryte@gmail.com>
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.

LGTM. Let's go ahead and get this in. Any remaining bug fixes can be addressed in follow-up PRs. Thanks, @Snehil-Shah, for contributing, and @Planeshifter for reviewing!

@kgryte kgryte merged commit 1d49bc6 into stdlib-js:develop Jul 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Issue or pull request for enhancing existing functionality. REPL Issue or pull request specific to the project REPL.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants