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

Add REPL tab-completion functionality. #2964

Merged
merged 3 commits into from
May 26, 2023
Merged

Add REPL tab-completion functionality. #2964

merged 3 commits into from
May 26, 2023

Conversation

aartaka
Copy link
Contributor

@aartaka aartaka commented May 14, 2023

Description

Y'ALL ON ATLAS TEAM, WE'RE ALL ON VACATION, DON'T REVIEW THIS PR, GET SOME TIME FOR YOURSELF ヽ(o`皿′o)ノ

This allows us to tab-complete things in REPL (at least for lisp-cells), which I myself found a painfully lacking feature.

Discussion

  • I'm not yet sure which shape the API should have. Should complete return
    • A list of string?
    • A list of symbols?
    • A single most intuitive symbol?
    • Most intuitive string (as it does right now)?
    • Should it maybe return nothing and do all the completion work itself, so that all this ffi-buffer-paste thing is more customizable per cell?

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • Git hygiene:
    • I have pulled from master before submitting this PR
    • There are no merge conflicts.
  • I've added the new dependencies as:
    • ASDF dependencies,
    • Git submodules,
      cd /path/to/nyxt/checkout
      git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    • and Guix dependencies.
  • My code follows the style guidelines for Common Lisp code. See:
  • I have performed a self-review of my own code.
  • My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
  • Documentation:
    • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
    • I have updated the existing documentation to match my changes.
    • I have commented my code in hard-to-understand areas.
    • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
    • I have added a migration.lisp entry for all compatibility-breaking changes.
    • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
  • Compilation and tests:
    • My changes generate no new warnings.
    • I have added tests that prove my fix is effective or that my feature works. (If possible.)
    • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

Copy link
Member

@aadcg aadcg left a comment

Choose a reason for hiding this comment

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

I'm not yet sure which shape the API should have. Should complete return

I think it should return the top suggestion as a string. This suggestion may be set interactively when the user uses the prompt buffer.


Issue #1: When completing, the symbol around the cursor doesn't get pasted onto the prompt buffer.

2023-05-22_23:07:42


Issue #2: If I type a prefix of the symbol I'm trying to complete, it won't appear at the top of the suggestions. This may be a limitation of the prompt buffer. Perhaps it doesn't sort sources, I didn't check it.

2023-05-22_23:15:06

@Ambrevar
Copy link
Member

Looks good to me other than André's remarks.

@aartaka
Copy link
Contributor Author

aartaka commented May 23, 2023

I think it should return the top suggestion as a string. This suggestion may be set interactively when the user uses the prompt buffer.

Good, then no change to the current API :D

Issue #1: When completing, the symbol around the cursor doesn't get pasted onto the prompt buffer.

Yes, doesn't. That deliberate: instead of parsing the contents of the cell (compiler-complete problem) and somehow replacing it (requiring JS), we just list all the suggestions and paste the selected one verbatim.

This is not a perfect API, but:

  • It's reliable.
  • And it's an improvement over the previous state.

So, unless we come up with something better, this one would be fine :)

But then, should we call it tab-completion if it's not actually completing? Maybe call it suggest or something?

Issue #2: If I type a prefix of the symbol I'm trying to complete, it won't appear at the top of the suggestions. This may be a limitation of the prompt buffer. Perhaps it doesn't sort sources, I didn't check it.

Yeah, it doesn't. I've reordered the sources to be more intuitive in 8266378, but there's not much to do beyond that :)

@aadcg
Copy link
Member

aadcg commented May 25, 2023

But then, should we call it tab-completion if it's not actually completing? Maybe call it suggest or something?

Yes, this is not completion. Suggest sounds good to me!

Also, I'd strongly advice not including a button for this feature (see below).

The feature is easily discoverable by commonplace keybindings (TAB).

2023-05-22_23:15:06


Personally, I find it odd to integrate this feature at such a premature stage. I think that the bare minimum should do as follows. Let | denote the cursor. If the user has typed prin| and he's looking for print, then prin should be replaced by print when they select print from the prompt buffer and press RET.

This needs to be extensively tested to ensure that the text replacement is done right. Handling whitespace, non-alpha symbols, etc.

Although I didn't think about this long enough, it strikes me that the suggested solution does it at the level of repl-mode. It seems more natural to do it at the level of nyxt/mode/input-edit.

@aartaka
Copy link
Contributor Author

aartaka commented May 25, 2023

So, putting it on a backburner? Or delete a button and ship :D I mean, yeah, something like general tab-completion won't hurt, but this REPL-local hack is okay for the use-case (((o(*゚▽゚*)o)))

@aartaka
Copy link
Contributor Author

aartaka commented May 25, 2023

All addressed:

  • Button removed.
  • complete and friends are renamed to suggest etc.

Rebasing, squashing, merging?

@jmercouris
Copy link
Member

Hello Artyom, thank you very much for this! It does in fact make the REPL infinitely more useful. I find myself struggling to remember symbols, and this really helps.

With regards to this PR, it is I believe an improvement. Ideally, we need to consider a completion framework at the level of input-edit as Andre suggested.

That said, I think this should be merged in because it begins the development of a framework. When the next time happens that we want to add tab-completion we'll have a basis of which to build off. A way in order to see how the code could be generalized for reuse. We'll discover the API.

I am thinking of the approach as delineated in Practical Common Lisp of writing code and then replacing things with Macros as the correct solution emerges (https://gigamonkeys.com/book/practical-a-simple-database.html).

@aadcg
Copy link
Member

aadcg commented May 25, 2023

@aartaka I'd like to emphasize that this is a fantastic improvement!

The remarks I made only make sense when reviewing the PR based on the title. I think it would be premature to jump at solving a big and general problem when we can have this simple and elegant solution that enhances the REPL experience.

Thank you for working on this.

For my side, it's ready to be merged. I'm not sold on the name of the command, suggest-into-cell, but I don't have an amazing alternative. The best I can come up with is suggest-at-point. Please decide as you find most adequate, it's not very important.

@aartaka
Copy link
Contributor Author

aartaka commented May 26, 2023

For my side, it's ready to be merged. I'm not sold on the name of the command, suggest-into-cell, but I don't have an amazing alternative. The best I can come up with is suggest-at-point. Please decide as you find most adequate, it's not very important.

I mean, the *-at-point Emacs convention is exactly the content-based actions. Which the current state not necessarily is. So I'd leave *-into-cell name, because the only thing it implies is adding something to the content.

@aartaka aartaka force-pushed the repl-tab-completion branch from 19081f6 to 4c4c048 Compare May 26, 2023 12:03
@aartaka aartaka force-pushed the repl-tab-completion branch from 4c4c048 to 2c96778 Compare May 26, 2023 12:10
@aartaka aartaka merged commit 492eb5f into master May 26, 2023
@aartaka aartaka deleted the repl-tab-completion branch May 26, 2023 12:23
@aadcg
Copy link
Member

aadcg commented May 26, 2023

I mean, the *-at-point Emacs convention is exactly the content-based actions.

This is not correct. See face-at-point. At point simply means "where the cursor is". Nonetheless, as I've said, I don't think it's very important :)

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

Successfully merging this pull request may close these issues.

4 participants