Rename prompter concepts

[aadcg]

Description

Fixes #2601 #2527

Checklist:

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

• 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:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • Google Common Lisp Style Guide
• 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.
• 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.)
  • New and existing unit tests pass locally with my changes.

[aadcg]

I can only change the status of this PR by the time I finish a draft on the prompt buffer's manual section.

[aadcg]

I've added a draft of the prompt buffer manual section. It's in org, since I can't imagine myself editing big chunks of text directly with spinneret.

Too early to take a look.

[aadcg]

First acceptable draft concluded.

[aadcg]

@aartaka has convinced me to keep the concept of marks, instead of renaming it to selection as I've proposed.

I remind that we can only be sure if the concepts make sense globally when judging how easy it is to write a manual section on the topic of prompt buffer.

@aartaka take a first look at the draft org file please.

[aartaka]

Only reviewed the .org file :)

[aartaka]

I don't like the name of the section, but I don't like the non-descriptive and unintuitive name of the slot either. So meybe we can find some other name? StumpWM uses :password t, which is overly narrow, I believe.

[aadcg]

Regarding the section name, we're on the same page.

Regarding the slot, that's a fair point. A suggestion that comes to mind is conceal-input-p. It's better but not much better perhaps.

[aadcg]

Generally speaking, me and @aartaka have agreed on most topics.

To move forward, I need input from @jmercouris and @Ambrevar.

Let me summarize all of it in a table.

The last table features a third column. If the row is non-empty, it means @aartaka has a different suggestion.

I'm not happy about mark-current-suggestion-forward/backwards. Oddly, @aartaka didn't raise the topic.

| old concept       | new concept                  |
|-------------------+------------------------------|
| return-actions    | action-on-return             |
| marks-actions     | action-on-marks              |
| selection-actions | action-on-current-suggestion |
| selection         | current-suggestion           |



| old slot               | new slot                          |
|------------------------+-----------------------------------|
| multi-selection-p      | enable-marks-p                    |



| old command            | new command (aadcg)               | new command (aartaka) |
|------------------------+-----------------------------------+-----------------------|
| select-previous        | previous-suggestion               |                       |
| select-next            | next-suggestion                   |                       |
| select-first           | first-suggestion                  |                       |
| select-last            | last-suggestion                   |                       |
| select-previous-page   | pagedown-suggestions              | next-page             |
| select-next-page       | pageup-suggestions                | previous-page         |
| select-previous-source | previous-source                   |                       |
| select-next-source     | next-source                       |                       |
|------------------------+-----------------------------------+-----------------------|
| mark-all               | mark-all-suggestions              | mark-all              |
| unmark-all             | unmark-all-suggestions            | unmark-all            |
| toggle-mark            | mark-current-suggestion-forward   |                       |
| toggle-mark-backwards  | mark-current-suggestion-backwards |                       |
| toggle-mark-all        | mark-complementary-suggestions    |                       |
|------------------------+-----------------------------------+-----------------------|
| cancel-input           | exit-prompt-buffer                |                       |
|------------------------+-----------------------------------+-----------------------|
| return-selection       | run-return-action-over-marks      |                       |
| return-marks-action    | set-return-action                 |                       |

[aadcg]

Well, I'll take the silence as approval and continue the work :p

[aartaka]

I'm not happy about mark-current-suggestion-forward/backwards. Oddly, @aartaka didn't raise the topic.

mark-forward and mark-backwards does sound better (and more consistent) to me, yes. But even the wordy version is okay, given that it's still discoverable enough :)

[Ambrevar]

Sorry for the delay, I'll come back to this as soon as possible.

[aadcg]

The renaming was done as per the table below.

I now need to go back to the manual to see if everything makes sense.

| old concept       | new concept                   |
|-------------------+-------------------------------|
| return-action     | actions-on-return             |
| marks-actions     | actions-on-marks              |
| selection-actions | actions-on-current-suggestion |
| selection         | current-suggestion            |

| old slot               | new slot                          |
|------------------------+-----------------------------------|
| multi-selection-p      | enable-marks-p                    |

| old command             | new command                     |
|-------------------------+---------------------------------|
| select-previous         | previous-suggestion             |
| select-next             | next-suggestion                 |
| select-first            | first-suggestion                |
| select-last             | last-suggestion                 |
| select-previous-page    | previous-page                   |
| select-next-page        | next-page                       |
| select-previous-source  | previous-source                 |
| select-next-source      | next-source                     |
|-------------------------+---------------------------------|
| toggle-mark             | mark-forward                    |
| toggle-mark-backwards   | mark-backwards                  |
| toggle-mark-all         | mark-complement                 |
|-------------------------+---------------------------------|
| cancel-input            | exit-prompt-buffer              |
|-------------------------+---------------------------------|
| return-selection        | run-action-on-return-over-marks |
| return-marks-action     | set-action-on-return            |

[aadcg]

I've reviewed the renaming. It seems to make sense conceptually, and it passed all of my inspection routines to ensure that I'm not introducing bugs.

If it looks good then I will merge, but only after:

• extracting the .org manual file
• rebase into master
• squash commits

Waiting for review. @jmercouris @aartaka @Ambrevar

[aartaka]

Maybe mention that it's a keyword argument to prompt(1)?

[aadcg]

Actually, according to my words, since this is not a user option, I should not refer to it!

[aadcg]

I didn't explain myself very well so let me make it clearer. I drafted the manual together with the renaming to ensure that the renaming actually makes sense. Imagine if I had made the renaming first only to realize later, while writing the manual section, that I needed to re-do some renaming.

When I mentioned the bit below, I was expecting a review of the renaming itself.

If it looks good then I will merge, but only after:

• extracting the .org manual file
• rebase into master
• squash commits

My plan is to get feedback on the renaming. If it's ok then rebase, delete .org file, squash and merge. Later, in another PR, we will continue to shape the manual section.

Since @aartaka reviewed this incomplete draft of the manual (sorry for the incomplete sections, I wasn't expecting a review there), I will answer all of his comments in this PR but the changes will be reflected in the PR I will open later. Is that ok @aartaka?

[aartaka]

Ohhh, sorry about that (^_^;) Yeah, the renaming is fine by me!

[aadcg]

Your valuable comments will not be forgotten @aartaka! Will get back to them shortly.

I'd like to wait for @Ambrevar to have a final word about the renaming.

Also, do the changes in the changelog look good?

[aadcg]

@Ambrevar could I ask you for your feedback?

Please read both of my comments above:

• comment 1
• comment 2

[Ambrevar]

Thank you @aadcg for this amazing work, and very sorry for taking so long to get back to it! :/

Here are the few names I would like to work on:

• mark-complement: "complement" is not used much in our code base. I like "toggle" better.
• exit-prompt-buffer: we actually use "quit" more often.
• mark-forwards and mark-backwards: the previous name highlighted that the command is a toggler, which is kind of cool. If you didn't like the asymmetry, what about toggle-mark-forwards and toggle-mark-backwards? Obviously your approach has the benefit of being more concise. So no strong opinion here.
• run-action-on-return-over-marks : Why not just run-action-on-return?

[aadcg]

I agree with all of your comments and made the changes accordingly @Ambrevar.

Now, I need to review this huge diff from top to bottom with extra care (I've already spotted issues)! I'll come back to this in some days.

[aadcg]

I've reviewed it and it looks good to me. Would you like to have a final look @aartaka?

[aartaka]

All in all, my comments are rather requests for future work. One significant thing: get return back. Rationale:

• We have actions-on-return, but we have no return that these happen on. Which is quite a gap in our prompter concepts.
• run-action-on-on-return is concerned with much more than just running the actions. Which is a bad sign. There should be some other infrastructural function to do channel and history management, like return. Action runner should not be concerned with anything beyond actions.
• return is just so good of a name and API point in general, while run-actions-on-return feels like quite a specialized thing. No amount of documentation can fix the confusing API structure, so we better have a single point of exit that's clearly named—return.

On the subject on naming, you seem to have renamed cancel-input to quit-prompt-buffer, but left the prompt-buffer-canceled error with its old name. Why so? Maybe rename it too? What I'd personally do is:

• Retain the meaningful cancel in the name of the command, so it's something like cancel-prompt-buffer. cancel-input is not perfect, but it conveys a clear message of bailing out, instead of awfully neutral quit-*.
• Don't rename the condition, leave it as is. It's clear enough.

[aartaka]

The whole thing with %current-suggestion business feels wrong. What if we stored the actual suggestion object inside current-suggestion slot? This way we can get rid of all the indexing and math, and simply get to list search.

This is not perfect design either, especially on the side of performance and needing to store the reference to source inside every suggestion. But it's much cleaner conceptually, I believe.

Obviously, not the change to do in this PR. @Ambrevar, does that sound like a plausible idea moving forward with prompter?

[aartaka]

I liked the actionability that return-selection had. It's quite an obvious part of the API that was beneficial for REPL-exploration. And run-action-on-return seems to do a lot of stuff that actually is not about actions on return at all..

How about resurrecting return-selection as, say, return, encapsulating all the channel management and the actual run-action-on-return inside it?

[aadcg]

Thanks!

@aartaka I suggest opening issues regarding the architecture of the library.

I'll soon continue to work on the prompt buffer's manual section, while not forgetting the review that @aartaka did on that.

[aadcg]

On the subject on naming, you seem to have renamed cancel-input to quit-prompt-buffer, but left the prompt-buffer-canceled error with its old name. Why so? Maybe rename it too? What I'd personally do is:

• Retain the meaningful cancel in the name of the command, so it's something like cancel-prompt-buffer. cancel-input is not perfect, but it conveys a clear message of bailing out, instead of awfully neutral quit-*.
• Don't rename the condition, leave it as is. It's clear enough.

This was discussed above when @Ambrevar mentioned:

exit-prompt-buffer: we actually use "quit" more often.

I don't see much a problem in the fact that we have a command called quit-prompt-buffer (that is user-facing) and a condition called prompt-buffer-canceled (that is developer-facing).

[Ambrevar]

@aadcg @aartaka Was an issue opened about the return issue?

Note that I was planning (Soon Enough©) to replace Calispel with Lparallel in the Prompter library, which should simplify a bunch of things, in particular remove lots of channels.

[aartaka]

On the subject on naming, you seem to have renamed cancel-input to quit-prompt-buffer, but left the prompt-buffer-canceled error with its old name. Why so? Maybe rename it too? What I'd personally do is:

• Retain the meaningful cancel in the name of the command, so it's something like cancel-prompt-buffer. cancel-input is not perfect, but it conveys a clear message of bailing out, instead of awfully neutral quit-*.
• Don't rename the condition, leave it as is. It's clear enough.

This was discussed above when @Ambrevar mentioned:

exit-prompt-buffer: we actually use "quit" more often.

I don't see much a problem in the fact that we have a command called quit-prompt-buffer (that is user-facing) and a condition called prompt-buffer-canceled (that is developer-facing).

This was discussed in the context of you renaming cancel-input to exit-prompt-buffer, but only as a matter of framing exit-prompt-buffer in a more conventional way. So it's a different layer of the arguement:

• Pierre was saying about the framing of the new name, not discussing the meaning of the new name itself (@Ambrevar, correct me if I'm wrong),
• I'm talking about the meaning of the new name, and that this meaning is slightly misleading.

To exemplify it with a more down-to-earth example:

• When I review a PR and leave a comment like "Here a loop would suit better than do*", this does not immediately mark PR as approved. It's just a stylistic comment.
• When I click "Approve" checkbox on GitHub or write something like "Looks good to me!", then I unambiguosly approve a given PR.

[aartaka]

@aadcg @aartaka Was an issue opened about the return issue?

Now there is: atlas-engineer/prompter#26

Note that I was planning (Soon Enough©) to replace Calispel with Lparallel in the Prompter library, which should simplify a bunch of things, in particular remove lots of channels.

Yay!

[aadcg]

@aartaka, since December 2 I have suggested and renamed cancel-input to exit-prompt-buffer. On that topic, the only feedback I had came from @Ambrevar that advocated for quit-prompt-buffer. I think there was plenty of time for everyone to chime in.

If there's any topic that needs to be further discussed, please open an issue.

[aartaka]

@aartaka, since December 2 I have suggested and renamed cancel-input to exit-prompt-buffer. On that topic, the only feedback I had came from @Ambrevar that advocated for quit-prompt-buffer. I think there was plenty of time for everyone to chime in.

Yes, that I agree with—I was not attentive enough when reviewing this PR, and was unable to spot this naming thing earlier. Sorry about that. Still, I noteced it now, and, as they say, it's better late than never.

I've been in a rush, so sorry if I sounded off. I didn't intend to downplay your work on this PR—it's huge and it made a lot of things much more consistent ٩(๑ơలơ)۶♡

If there's any topic that needs to be further discussed, please open an issue.

Yes, see atlas-engineer/prompter#27.

[aadcg]

Thanks for the manual review @aartaka. I'll make a new version of it. I'll probably open a PR with the org file so that we continue to go through it. At the last minute, I'll integrate it in manual.lisp. Notice that by using (:nxref :command 'foo-command), the keybinding will be mentioned so let's not worry about that while reviewing the .org file.