search

minad / marginalia

:scroll: marginalia.el - Marginalia in the minibuffer
GNU General Public License v3.0
782 stars 27 forks source link

Docstrings for fish-completion-mode using marginalia? #87

Closed karthink closed 3 years ago

karthink commented 3 years ago

I'm trying to write a function marginala-annotate-fish-completion that works with the fish completion package to provide fish "docstrings" as annotations in eshell/shell. To do this I turn on fish-completion-mode and set completion-in-region-function to consult-completion-in-region in eshell/shell buffers.

Here is an example of the docstrings showing up along with the completions in Helm: 2021-07-12-144041_800x834_scrot

And here is how it shows up with consult-completion-in-region: 2021-07-12-150210_787x467_scrot

I'm facing two issues:

  1. The fish completion package provides a fish-completion--list-completions-with-desc function that provides an alist of completions with their docstrings when given a string to complete on:
(fish-completion--list-completions-with-desc "ls --")
(("--version" "Display version and exit")
 ("--help" "Display help and exit")
 ("--width" "Assume screen width")
 ("--tabsize" "Assume tab stops at each COLS")
 ("--time-style" "Select time style")
 ("--time" "Show time type")
 ("--sort" "Sort criteria")
 ...))

(Technically it returns a string, but I converted it to an alist here for convenience.) However it looks like the hypothetical marginalia-annotate-fish-completion will be called on each completion candidate separately, /i.e/ once on "--version", once on "--help" etc. Where do I store this list in relation to marginalia so I can just look up each docstring from it?

  1. fish-based completion function works by setting pcomplete-default-completion-function:
(setq pcomplete-default-completion-function 'fish-completion-shell-complete)

However it's not setting the completion category when it completes command line arguments, it evaluates to nil. So there's nothing for marginala to match against. When the completion candidates are files, fish-completion-shell-complete calls pcomplete-dirs-or-entries instead, and this does set the category as 'file. Where/How do I set the category for CLI arguments?

minad commented 3 years ago
  1. Yes, annotators are called once per candidate, this is due to the Emacs API. Cache the result in a minibuffer-local variable, depending on the (minibuffer-contents). If the contents change, call fish again.
  2. Check out marginalia-classifiers. You may have to write your own classifier detecting that this is a fish completion, since the prompt "Completion:" is too generic. Otherwise the marginalia prompt classifier would have worked.
karthink commented 3 years ago

EDIT 2: Please ignore this post. The next one has my tested code that almost works.

Thank you.

2 is easy, although still untested:

(defun marginalia-classify-fish-completion ()
  "Classify completion-category as `fish-completion'."
  (and (eq marginalia--this-command completion-in-region-function)
       (or (eq major-mode 'eshell-mode)
           (eq major-mode 'shell-mode))
       fish-completion-mode
       'fish-completion))

(add-hook 'marginalia-classifiers 'marginalia-classify-fish-completion)

This will fail when fish-completion calls pcomplete-dirs-or-entries to complete files, but I'm hoping one of the earlier clauses in marginalia--completion-metadata-get will set the category to 'file.

However I'm not sure about how to do 1: 

(add-to-list 'marginalia-annotator-registry
             '(fish-completion marginalia-annotate-fish-completion builtin none))

(defun marginalia-annotate-fish-completion (cand)
  "Annotate fish completion CAND with description."
  (when-let ((desc (alist-get cand all-fish-completions-with-docstrings)))
    (marginalia--field (format "%s" desc))))

How do I set and update all-fish-completions-with-docstrings depending on (minibuffer-contents)?

EDIT: Actually, I don't need to call out to fish at all when filtering candidates by typing in the minibuffer. I just need all-fish-completions-with-docstrings to be set once per invocation of the completion-in-region-function. So calling fish based on (minibuffer-contents) seems excessive.

karthink commented 3 years ago

OK, I almost have it figured out, but it's still failing to annotate.

Step 1:

Make Marginalia recognize shell command argument completions as the category fish-completion:

(defun marginalia-classify-fish-completion ()
  "Classify completion-category as `fish-completion'."
  (and (eq this-command 'completion-at-point)
       (or (eq major-mode 'eshell-mode)
           (eq major-mode 'shell-mode))
       fish-completion-mode
       'fish-completion))

(add-hook 'marginalia-classifiers 'marginalia-classify-fish-completion)

I have tested that this works. It's actually too general, it considers all completion function calls in eshell as in this category, including when completing elisp. I'll have to figure out how to be more specific later.

Step 2:

Store the results of calling out to fish for completions every time I run completion-at-point. I define a new completion-in-region function that does this. This is also too general and has false positives, but I've tested that it works for now:

(defun my/consult-completion-in-region (start end collection &optional predicate)
  "See `consult-completion-in-region'."
  (let* ((initial (buffer-substring-no-properties start end))
         (metadata (completion-metadata initial collection predicate))
         (category (completion-metadata-get metadata 'category)))
    (when (and (bound-and-true-p marginalia-mode)
               (bound-and-true-p fish-completion-mode)
               (eq category 'fish-completion))
      (setq my/all-fish-completions-with-desc
            (mapcar (lambda (e) (split-string e "\t"))
                    (split-string
                     (fish-completion--list-completions-with-desc
                      (buffer-substring-no-properties
                       (save-excursion (if (eq major-mode 'shell-mode)
                                           (comint-bol)
                                         (eshell-bol))
                                       (point))
                       (point)))
                     "\n" t)))))
  (consult-completion-in-region start end collection predicate))

(setq-local completion-in-region-function #'my/consult-completion-in-region)

The results from fish are stored in my/all-fish-completions-with-desc.

Step 3:

Make Marginalia apply annotations for this category. This is the step that's failing:

(defun marginalia-annotate-fish-completion (cand)
  "Annotate fish completion CAND with description."
  (when-let ((desc (alist-get cand my/all-fish-completions-with-desc nil nil 'equal)))
    (marginalia--field (car desc) :truncate 40)))

(add-to-list 'marginalia-annotator-registry
             '(fish-completion marginalia-annotate-fish-completion builtin none))

I edebugged marginalia-annotate-fish-completion, but it's never called, even when the category is recognized as fish-completion.

Any idea what I'm doing wrong?

minad commented 3 years ago

2 seems too complicated. Why not strategically place advices somewhere sniffing the fish completion results? Maybe another annotator is preferred regarding 3? Look into marginalia--completion-metadata-get.

karthink commented 3 years ago

marginalia--completion-metadata-get returns fish-completion as expected. It calls marginalia-classify-fish-completion a total of four times per invocation of complete-at-point though, I don't know why. Still don't understand why marginalia-annotate-fish-completion is never called.

Re: 2. I can probably advise one of the fish-completion functions to do this, yeah. That's a good idea.

minad commented 3 years ago

Does marginalia--completion-metadata-get called with the affixation-function or the annotation-function argument? You can check what the function returns in this case. Furthermore you can check the vertico--affixate function, where the annotation function lookup happens, what ends up there?

You are probably hitting these problems since you are using the minibuffer completion for completion in region (consult-completion-in-region) in combination with Marginalia. This is a use case I have not tried before.

karthink commented 3 years ago

Does marginalia--completion-metadata-get called with the affixation-function or the annotation-function argument?

This function is called several times for each invocation of consult-completion-in-region. The first three calls are with prop set to category, and the function returns fish-completion each time. Then with prop set to group-function, then affixation-function and annotation-function in sequence.

The affixation-function clause has this code:

     (when-let* ((cat (completion-metadata-get metadata 'category))
                 (annotate (marginalia--annotator cat)))
       (lambda (cands)
         (marginalia--context metadata
           (mapcar (lambda (x) (list x "" (or (funcall annotate x) ""))) cands))))

This call to completion-metadata-get returns nil, and thus annotate is not set to the right annotation function. Ditto for the call to completion-metadata-get in the annotation-function clause.

These last two calls seem to be from vertico-affixate. Since the annotation function is not being looked up correctly, I'm not seeing any in the result.

minad commented 3 years ago

This call to completion-metadata-get returns nil, and thus annotate is not set to the right annotation function. Ditto for the call to completion-metadata-get in the annotation-function clause.

So you say, in the calls before the category is returned correctly as fish-completion. But in this call completion-metadata-get returns nil? That's odd. marginalia--completion-metadata-get advices completion-metadata-get, so you should also see this call ending up in marginalia--completion-metadata-get. Are the functions called with different values for metadata?

karthink commented 3 years ago

Here are the calls I could trace, in order from the top:

Sorry about the indentation. I tried to manually show which function called what, but this is incomplete. I don't know how to generate an actual stacktrace with debug-on-entry in Emacs yet.

Every call to marginalia--completion-metadata-get seems to be with the same value of metadata. So I'm not sure what to make of it.

minad commented 3 years ago

Your classifier is wrong. It checks the major-mode, but the classifier is called in the mini buffer. There it will pick up the wrong mode.

karthink commented 3 years ago

Ah, that makes sense. Checking if the var fish-completion-mode is t doesn't work either, so it looks like I need a different signature for fish-completions that will work from the minibuffer.

karthink commented 3 years ago

OK, some success! 2021-07-14-180715_1071x434_scrot

Here is the marginalia-classify-fish-completion that I'm using now:

(defun marginalia-classify-fish-completion ()
  "Classify completion-category as `fish-completion'."

  (and (eq this-command 'completion-at-point)
       (with-current-buffer (funcall (if (minibufferp) #'cadr #'car) (buffer-list))
         (and fish-completion-mode
              (or (eq major-mode 'eshell-mode)
                  (eq major-mode 'shell-mode))))
       'fish-completion))

However the annotations disappear as soon as I type something: 2021-07-14-181040_1074x122_scrot

They also disappear as soon as I scroll in vertico (without typing anything): 2021-07-14-182147_1068x436_scrot

I'm guessing this is because marginalia-annotate-fish-completion is called (on each candidate) every time the minibuffer contents are updated? How would I keep the annotations persistent when filtering candidates? I don't need to call fish completions again since the filtered list can only be a subset of the original results, right?

minad commented 3 years ago

Btw why does this work in helm? Is there special support? I think that this should work out of the box with consult+vertico. I will install fish later and try this.

Edit: it seems there is a helm-fish-completion package. Do you use that?

Edit2: I think it would be significantly easier and more robust if you add an annotation-function to the fish-completion package directly.

minad commented 3 years ago

This snippet works for me:

(setq completion-in-region-function #'consult-completion-in-region)

(defvar last-fish-completions nil)

(defun add-fish-annotations (result)
  `(,@result :annotation-function fish-annotate))

(defun fish-annotate (cand)
  (when-let (ann (cdr (assoc cand last-fish-completions)))
    (concat (propertize " " 'display '(space :align-to center)) ann)))

(defun my-fish-completion--list-completions (raw-prompt)
  (setq last-fish-completions
        (mapcar (lambda (e)
                  (string-match "\\`\\([^\t]*\\)\t?\\(.*\\)\\'" e)
                  (cons (match-string 1 e) (match-string 2 e)))
                (split-string
                 (fish-completion--list-completions-with-desc raw-prompt)
                 "\n" t)))
  (mapcar #'car last-fish-completions))

(advice-add #'pcomplete-completions-at-point :filter-return #'add-fish-annotations)
(advice-add #'fish-completion--list-completions :override #'my-fish-completion--list-completions)

Pcomplete does not seem to offer a possibility to supply annotations unfortunately, therefore I am adding the :annotation-function to the return value of pcomplete-completions-at-point, which is the pcomplete capf function. The resulting solution here is certainly not very clean, but at least we don't have to add Marginalia to the equation. However it is questionable if this solution can be integrated with upstream fish-completion, since it breaks the pcomplete abstraction layer by adding the annotations.

Some background: We are going to a multitude of completion layers here.

  1. fish (the shell)
  2. fish-completion, registers itself with pcomplete
  3. pcomplete, registers itself as capf
  4. completion-at-point accesses capf
  5. completion-at-point presents the completions using completion-in-region
  6. completion-in-region calls the completion-in-region-function, in this case consult
  7. consult transfers the completion to the minibuffer, there it is managed by vertico

If you use Corfu instead, this would replace layer 6+7. The pcomplete layer 3 seems to be mostly legacy since it just sits between the capf layer and the actual completion backend, but I am not entirely sure. Layer 3 is the one which does not support annotations out of the box.

minad commented 3 years ago

Here is a better variant without global state. My suggestion is to propose this as a patch to the fish-completion package. The fish-completion--provide-annotation-function advice should only be installed when the fish-completion-mode is active.

(defun fish-completion--list-completions (raw-prompt)
  (mapcar (lambda (e)
            (string-match "\\`\\([^\t]*\\)\t?\\(.*\\)\\'" e)
            (propertize (match-string 1 e) 'fish-completion--annotation (match-string 2 e)))
          (split-string
           (fish-completion--list-completions-with-desc raw-prompt)
           "\n" t)))

(defun fish-completion--annotate (cand)
  (when-let* ((pos (next-single-property-change 0 'fish-completion--annotation cand))
              (ann (get-text-property pos 'fish-completion--annotation cand)))
    (concat (propertize " " 'display '(space :align-to center)) ann)))

(defun fish-completion--provide-annotation-function (table)
  (nconc table (list :annotation-function #'fish-completion--annotate)))

(advice-add #'pcomplete-completions-at-point :filter-return #'fish-completion--provide-annotation-function)
karthink commented 3 years ago

I tried this but I don't see the annotations. Is the property fish-completion--annotation supposed to be a face? It's not defined in your code above or in fish-completions.el.

EDIT: I debugged fish-completion--annotate and while it's being called on all the completion candidates, (next-single-property-change 0 'fish-completion--annotation cand) is returning nil.

Some background: We are going to a multitude of completion layers here.

Yeah, this is part of the reason I got very confused trying to set this up.

Edit: it seems there is a helm-fish-completion package. Do you use that?

Yeah, I used helm-fish-completions to get the docstrings in helm. Sorry, I didn't go into the details since it wasn't relevant for marginalia. This package defines helm-esh-pcomplete that works as a drop-in replacement for completion-at-point. I guess this is in keeping with the helm way of replacing chunks of Emacs instead of adding to them.

Edit2: I think it would be significantly easier and more robust if you add an annotation-function to the fish-completion package directly.

I see that from your code. My hack was really fragile. Thanks, I think I understand how marginalia/annotations work a little better now.

minad commented 3 years ago

Both these snippets work for me on 27.1 and there is not much that can go wrong. Try with emacs -Q step by step. fish-completion--annotation is a text property I introduced here. I suggest you ask on the fish-completion package to add something like this:

https://github.com/minad/marginalia/issues/87#issuecomment-880724829

Closing here since it is better to solve this on the level of fish-completion directly instead of going via Marginalia.

karthink commented 3 years ago

I suggest you ask on the fish-completion package to add something like this:

I'll do that, thank you.

By the way, your code above fails when completion-ignore-case set to t. Setting it to nil makes it work fine again. I tested this with emacs -Q.

It took a while to track down, but this is why it wasn't working for me. Here are the propertized candidates being passed to the annotation function when

#("--group-directories-first" 0 2 (face (orderless-match-face-0 completions-common-part)) 2 25 (fish-completion--annotation "Group directories before files"))
#("--group-directories-first" 0 2 (face (orderless-match-face-0 completions-common-part) fish-completion--annotation #1="Group directories before files") 2 25 (fish-completion--annotation #1#))
minad commented 3 years ago

That's odd. Why does this happen?

karthink commented 3 years ago

I can't figure out why this happens, but what's happening is clear. In one case the text property fish-completion--annotation you defined is applied to the entire candidate, in the other it's applied to the non-common part of each candidate. I got around it by modifying fish-completion--annotate:

(defun fish-completion--annotate (cand)
  (when-let* ((pos (or (next-single-property-change 0 'fish-completion--annotation cand)
                       0)
              (ann (get-text-property pos 'fish-completion--annotation cand)))
    (concat (propertize " " 'display '(space :align-to center)) ann)))

Also, pcomplete-completions-at-point is going to be advised globally whenever fish-completion-mode is active. Does this mean if a different comint mode (say) is using it, it will get a completion table with the :annotation-function #'fish-completion--annotate plist element appended? Is there some way of setting this up to only modify pcomplete-completions-at-point in the eshell/shell buffer?

aikrahguzar commented 2 years ago

Hi @minad and @karthink I have spent some time doing this for the doom eshell module and building on what you have above I think I have a pretty workable solution for this in the form of a capf.

The whole code is this,

(defun +eshell-fish-completion-list (raw-prompt)
  "Return list of completion candidates for RAW-PROMPT."
  (mapcar (lambda (e) (let ((res (split-string e "\t")))
                   (propertize (car res) 'fish-annotation (cadr res))))
          (split-string
           (fish-completion--list-completions-with-desc raw-prompt)
           "\n" t)))

(defun +eshell-fish-capf ()
  "A a capf for fish-completion."
  (when-let ((args (ignore-errors (eshell-complete-parse-arguments)))
             (table (+eshell-fish-completion-list (buffer-substring (cadr args) (point))))
             ((not (file-exists-p (car table)))))
    (list (car (last args)) (point) table
          :exclusive 'no
          :annotation-function #'+eshell-fish-completion-annotate)))

(defun +eshell-fish-completion-annotate (cand)
  (when-let* ((pos (or (next-single-property-change 0 'fish-annotation cand)
                       0))
              (ann (get-text-property pos 'fish-annotation cand)))
    (concat (propertize " " 'display '(space :align-to center)) ann)))

(defun +eshell-use-annotated-completions-h ()
  "Use annotaed fish completions."
  (if fish-completion-mode
      (add-hook 'completion-at-point-functions #'+eshell-fish-capf nil t)
    (remove-hook 'completion-at-point-functions #'+eshell-fish-capf t)))

(add-hook 'fish-completion-mode-hook #'+eshell-use-annotated-completions-h)

The only dependency on fish-completions is actually a call to fish -c complete -C some-string and a lot of what it has deal with is replaced by the call eshell-complete-parse-arguments which also takes care of things like subcommand completions.

The one weird thing about eshell-complete-parse-arguments is that if there is a sexp behind point, it tries to evaluate it and that can be potentially very time consuming. The pcomplete mechanism already calls this function so has this problem but now it can happen twice. I am not a heavy shell user so I don't know if that evaluation is useful for something or not. And it is entirely possible that I am missing problems with this approach for the same reason.

minad commented 2 years ago

On 8/13/22 22:46, aikrahguzar wrote:

Hi @minad https://github.com/minad and @karthink https://github.com/karthink I have spent some time doing this for the doom eshell module and building on what you have above I think I have a pretty workable solution for this in the form of a capf.

I've also considered using a Capf. Unfortunately the Capf won't integrate well with Pcomplete. Instead it will take over completely.

However I am not using fish completion anyway. I switched to (my personal clone of) pcmpl-args, which parses --help output. I am not fond of the idea of shelling out to fish, when we can do the same in Elisp.

aikrahguzar commented 2 years ago

I've also considered using a Capf. Unfortunately the Capf won't integrate well with Pcomplete. Instead it will take over completely.

There is the inversion of precedence between completions provided by fish and those by Pcomplete but it mostly that seems like an improvement for most use cases. Getting annotations for command names was difficult but doable but even more difficult was getting completions for subcommands. Pcomplete tries to complete a filename most of the time in that situation while fish does the right thing most of the time. This is the most likely due to people having written more completions for fish. For file and elisp completions the capf above doesn't activate so the pcomplete capf takes over.

However I am not using fish completion anyway. I switched to (my personal clone of) pcmpl-args, which parses --help output. I am not fond of the idea of shelling out to fish, when we can do the same in Elisp.

Can the annotation coming from parsing --help be included in the normal Pcomplete completions using advice? By the way fish does something similar. There is fish command fish_update_completions which parses the manual pages to create completions. The results for CMD are in ~/.local/share/fish/generated_completions/CMD for me. For example the first line of ~/.local/share/fish/generated_completions/emacsclient is,

complete -c emacsclient -s a -l alternate-editor -d 'If the Emacs server is not running, run the specified shell command instead'

Each line contains a similar completion. Completions that come from system packages are in /usr/share/fish/completions and /usr/share/fish/vendor_completions.d for me. So another options which doesn't involve shelling would be to parse these files once to generate completions for a capf (which doesn't seem too hard but not trivial either) or pcomplete (is there a way other than defining a pcomplete/CMD for each of these?).