Use Lparallel-powered prompter library

[Ambrevar]

Description

This uses the updated prompter library from the atlas-engineer/prompter#16 patch set.

Fixes #2134 (at least the hang).

Discussion

• Shall we merge this on 3-series? I've tried to retain as much back-compatibility as possible, but prompter itself is not fully backward compatible. That said, it's concerns only very "under the hood" elements (like prompter:result-channel which is now an Lparallel promise).

  Answer: Yes, let's merge it, prompter:result is only used as a lower-level implementation detail.

• Nyxt freezes when typing very fast in prompt buffer #2134 hang is fixed indeed, however keeping a key pressed is very CPU intensive and pretty slow in the UI. Not sure why, as if the UI was waiting the prompt-buffer. Issue on the Nyxt side? @aartaka @aadcg any idea?

• There are still dangling threads.

For the last 2 issues, I think I know what's going on: update-prompt is run on each key press, and evidently does not terminate properly.

Proposed solution: Use a strategy of "buffering" like the new prompter does, so we only update the buffer after some time delay.
Also while we are at it, switch from Calispel to Lparallel to avoid the accidental dangling threads.

Better idea? Listening to prompters update-notifier would save us having to reimplement this logic.

To do:

• More testing! @aadcg @jmercouris @aartaka Please give this a spin on a few prompt buffers.
• Ensure Nyxt freezes when typing very fast in prompt buffer #2134 is fixed.
• Update Git submodule
  DO NOT MERGE LAST COMMIT: Instead, wait for prompter master to be updated, then update the Git submodule.
• Update Guix package.

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:
  • 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.
  • (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.

[aartaka]

Will have to review prompter refactor, but this one looks OK to me.

[aartaka]

Attribute value API prevents dynamic attribute width computation with

(define-configuration :prompt-buffer
  ((dynamic-attribute-width-p t)))

Something with futures as values of attributes being passed to the function implying that values are strings.

[aartaka]

And I've allowed myself a liberty of fixing a destructor of buffer-source :)

[Ambrevar]

Something with futures as values of attributes being passed to the function implying that values are strings.

Oh right, this part of the API has changed.
Indeed now attributes can be futures, which is much cleaner in terms of implementation detail.

We need to call attribute-value or attributes-values with the right wait-p value.
I'll have a look later.

[aadcg]

Shall we merge this on 3-series?

Yes, as you rightly mention prompter is quite low-level as far as Nyxt is concerned.

More testing!

I'll get to it as soon as atlas-engineer/prompter#16 is merged. It's not very smart to start testing until that moment :)

---

Thanks for working on this. It's really important, and I think it will have a great impact on Nyxt!

[aadcg]

We should probably create an issue saying "grep for 4.0.0 to ensure we act prior to its release", otherwise we'll forget it :p

[Ambrevar]

Something with futures as values of attributes being passed to the function implying that values are strings.

@aartaka I can't reproduce. Can you give an exact recipe?

[Ambrevar]

While I still need an example to test the attribute problem, I think I know what's going on.

1. prompter:active-attributes may now return an Lparallel future instead of a string.
2. Old bug, still in prompter:active-attributes, in case of a function, we only return the key and the value of the attribute instead of the full list:

  (if (functionp (second attribute))
           (list (attribute-key attribute)
                 (lpara:future
                   (handler-case (funcall (second attribute) (value suggestion))
                     (error (c)
                       (format nil "keyword error: ~a" c)))))
           attribute)

Oops!

Generally, speaking, this list-based attribute-API is poorly designed and is in great need to be overhauled. But this belong to another prompter future overhaul.

Actionable changes for now:

• Return the whole list.
• The prompter:attribute-value method should return "" instead of a future when not ready (and not waiting).
• prompter:attributes-default should behave as attribute-value does.
• nyxt:render-attributes and nyxt:attribute-widths should use prompter:attribute-value and friends instead of accessing the list directly.

[Ambrevar]

See 41a2aa0 where I'm now using the attributes API instead of parsing the lists (thus never being exposed to the internals like lpara:future anymore). @aartaka Does it fix it for you?

[aartaka]

Something's off again:

The value
  ("URL"
   "https://github.com/atlas-engineer/nyxt/pull/2998#issuecomment-1580388883"
   NIL 3)

is not of type
  (OR STRING SYMBOL CHARACTER)
when binding SB-IMPL::STRING2
   [Condition of type TYPE-ERROR]

Test it with a C-l and this piece of config:

(define-configuration :prompt-buffer
  ((dynamic-attribute-width-p t)))

[Ambrevar]

Fixed.

[Ambrevar]

And I've allowed myself a liberty of fixing a destructor of buffer-source :)

@aartaka This introduces a new bug: Try pressing escape in C-l.
I don't understand what you are trying to fix here, the original function seemed to be fine.

[aartaka]

I don't understand what you are trying to fix here, the original function seemed to be fine.

Waaaaait, it was me getting some error in the destructor, checking the prompter:destructor documentation:

Function called with the source as parameter to clean it up.
It's called when `destroy' is called over `prompter'.

And deciding that prompter:destructor should be called with just one argument 😨

[aartaka]

Feel free to revert it!

[Ambrevar]

Something was off indeed, the prompter argument should not be there.

1. We must check why we didn't spot this error before. prompter should have reported it.
2. Fix it by keeping @aartaka 's fix but first checking if (prompter source) exists.

[jmercouris]

Please reopen when ready.