Selection actions list

[aadcg]

Description

Add the ability to choose from a list of selection-actions. Simplify return-actions by ensuring it's a list.

Fixes #2198

Discussion

Also, I want to test this API with the hinting system. I can imagine it being very helpful in that context.

Checklist:

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

• [x] I have pulled from master before submitting this PR
• [x] There are no merge conflicts.
• [x] I've added the new dependencies as:
  • [x] ASDF dependencies,
  • [x] Git submodules,

    cd /path/to/nyxt/checkout
    git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    

  • [x] and Guix dependencies.
• [x] 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
• [x] 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]

@aartaka, @Ambrevar take a first quick look to make sure I'm not doing anything too silly.

[aartaka]

I don't see anything outright silly there yet ;)

[Ambrevar]

You might have misunderstood the point of C-j: it's to run a selection action manually, in case follow-mode is not enabled.

So you'd need to restore call-selection-action since otherwise the user has no way to manually call an action.

Rest looks good to me, but I haven't tested yet.

[aadcg]

You might have misunderstood the point of C-j: it's to run a selection action manually, in case follow-mode is not enabled.

For the sake of clarity, follow-mode is a deprecated concept and we now call it auto-return-p.

I don't follow. Notice that auto-return-p only concerns return-actions, not selection-actions.

So you'd need to restore call-selection-action since otherwise the user has no way to manually call an action.

Which actions are you referring to? We have return-actions, selection-actions and marks-actions.

Can we give an example?

[aadcg]

Also, on master, running run-selection-action (bound to C-j in Emacs-mode) has no effect.

[Ambrevar]

For the sake of clarity, follow-mode is a deprecated concept and we now call it auto-return-p.

I don't follow. Notice that auto-return-p only concerns return-actions, not selection-actions.

That's your confusion: follow-mode has been renamed to selection-actions-enabled-p.

Try the jump-to-heading command and navigate the selections to see its effect.

[aadcg]

That's your confusion: follow-mode has been renamed to selection-actions-enabled-p.

You're correct! Sorry.

Now let's go back to the main topic that you have raised:

You might have misunderstood the point of C-j: it's to run a selection action manually, in case follow-mode is not enabled.

So you'd need to restore call-selection-action since otherwise the user has no way to manually call an action.

selection-actions-enabled-p is nil by default. Some modes override it of course.

We have the command toggle-selection-actions-enabled that toggles selection-actions-enabled-p. Now that selection-actions can be manifold, we need an extra command to select the default action (set-selection-action).

Does it really make sense to have a command that enables a selection-action to run once?

[Ambrevar]

Now that selection-actions can be manifold, we need an extra command to select the default action (set-selection-action).

Correct!

Does it really make sense to have a command that enables a selection-action to run once?

Can you give an example?

[aadcg]

Can you give an example?

Ok, let's take this slowly. It seems that both @Ambrevar and @aartaka raised concern about the fact that I've deprecated the command run-selection-action. I'll explain my reasoning below.

Let's take the command jump-to-heading, for which selection-actions-enabled-p is set to t.

Let me describe the behavior on master. When we issue the command interactively, it's possible to disable selection-actions (scrolling in this case) by issuing the command toggle-selection-actions-enabled while the prompt is active. Now that selection-actions-enabled-p is set to nil, the command run-selection-action runs the action once. Personally, I think that toggle-selection-actions-enabled, while different, is general enough for me not to use run-selection-action.

With this PR, I add the possibility to define several selection-actions. Obviously a new command is needed to set the action, without leaving the prompt. Given my comment above on the generality of toggle-selection-actions-enabled, and the fact that a new command is needed, I thought it would be sensible to bind this new command to the key that was once bound to run-selection-action.

If you consider that we really need 3 commands for cover all of this, I don't oppose. I just think it may be overkill. Perhaps there are use cases that I'm actually disregarding!

[Ambrevar]

OK, makes sense now, thanks for the detailed review.

So yes, I believe we need 3 commands, because selection action may need to be only manual, in which case we need a trigger key. jump-to-heading only scrolls, so this is rather innocent, but imagine a selection action that, say, downloads the hint! In this case you want to be careful and not accidentally trigger the selection action.

EDIT: Clarify action -> command

[aartaka]

So yes, I believe we need 3 commands, because selection action may need to be only manual, in which case we need a trigger key.

Yes!

jump-to-heading only scrolls, so this is rather innocent, but imagine a selection action that, say, downloads the hint! In this case you want to be careful and not accidentally trigger the selection action.

jump-to-heading is not innocent—it throws you to the start of the document once you start selecting prompt suggestions! I even think that we should make its default-selection-action purely manual, because we don't yet have a good way to make it smooth ヽ(ｏ`皿′ｏ)ﾉ

EDIT: it's -> its

[aadcg]

Commit f96d8ba54 introduced a major bug in the hinting interface. At least the one who did it will clean up the mess! Sorry everyone.

[aartaka]

What about testing and documentation?

[aadcg]

With regards to testing, actions are not tested on the prompter library. It could be added but I think it's outside the scope of this PR.

I'll update the documentation on the manual and on the prompter's README.

[aadcg]

I've added a note on things that are left to be done.

@Ambrevar, could you have a final look? Should be almost ready to be merged.

[Ambrevar]

The fix for atlas-engineer/nyxt#2198 looks good and I'm happy with that, thanks!

However this pull request does too many things and I'm confused about the number of things that have changed.

• It's good practice to not include style / code refactoring together with important redesign changes.
• Same with refactoring / renaming of functions / symbols. If it's far reaching, it's better to keep it for a separate pull request. At the moment it's hard to have a good overview of what this pull request initially intended, namely to fix atlas-engineer/nyxt#2198.
• Don't forget to update the changelog, in particular about the addition / remove of commands and the change of keybindings.
• Why did you decide to change the M-return keybinding?

I'd suggest to close this pull request unmerged, and open 2 or 3 other request:

• first one that fixes atlas-engineer/nyxt#2198 (can be quickly merged),
• then one that fixes indentation, typos, etc. (also a quick merge),
• and finally one that refactors the code.

Thoughts?

[aadcg]

You have a point, even though it seems to be too much work for little to no benefit. Let's stick to what you suggest :)

[Ambrevar]

Sorry for being overly picky, but the prompter library is the most sensitive part of Nyxt, so we should try to be as clean as possible. It will help us a lot in the future if things go wrong.

[aadcg]

No need to close this PR, as I've been breaking the former one into smaller ones.

About keybindings:

We need a new keybinding for set-selection-action. Does C-c C-j make sense? @aartaka @Ambrevar @jmercouris

[Ambrevar]

C-c C-j is fine from a Helm user perspective, but maybe a bit arcane for everyone else.

What about C-c return? Or C-u return?

[aartaka]

• Why did you decide to change the M-return keybinding?

Because M-return should be reserved for some "strong" action closing the prompt-buffer, analogous to return and C-return (return stands either for "activate this now" or "I'm done, commit this action" in CUA). If we dilute the responsibility one takes when pressing M-return, then the character of other actions bound to *-return will not be perceived as final and decisive for a given prompt.

[aadcg]

C-c C-j is fine from a Helm user perspective, but maybe a bit arcane for everyone else.

What about C-c return? Or C-u return?

I guess I still prefer C-c C-j, but I don't have any strong opinion.

[aadcg]

• Why did you decide to change the M-return keybinding?

Because M-return should be reserved for some "strong" action closing the prompt-buffer, analogous to return and C-return (return stands either for "activate this now" or "I'm done, commit this action" in CUA). If we dilute the responsibility one takes when pressing M-return, then the character of other actions bound to *-return will not be perceived as final and decisive for a given prompt.

Seems absolutely sound to me, but let's perhaps leave it out of this PR. We should revise all keybindings before 3.0.

[aadcg]

Updated changelog, should be ready to merge. @Ambrevar.

[Ambrevar]

Looks good to me!

[aadcg]

Thanks everyone!