astoff / devdocs.el

Emacs viewer for DevDocs
279 stars 16 forks source link

feature request: ivy support #28

Closed bymoz089 closed 1 year ago

bymoz089 commented 1 year ago

As an ivy user, the completing-multiple function in devdocs--read-document don't give any completion candidates.

So I replaced (in above mentioned function) the following sexp (completing-read-multiple prompt cands), with this sexp:

(let (choices)
  (ivy-read prompt
            cands
            :require-match t
            :action (lambda (x)
                      (push (car x) choices)))
  choices)

to get completion with ivy.

Is there interest in a pull request for that, or could this solution be mentioned in the docs? (In order to help other ivy users with that issue.)

Edit: I found, that completing-read-multiple is difficult to emulate for quite a few other completion frameworks.

astoff commented 1 year ago

Hi, and thanks for the offer. I will not add workaround code for an incompatibility with the Emacs API, but mentioning it in the README might be a good idea. Now, what exactly happens? Do you just get to the good old completion UI, where you have to press TAB to see the *Completions* buffer?

I'm a bit confused by @minad's comment in the linked discussion. Vertico works just fine with CRM, and I don't see why it would be a dead end. If you want to read several values from a list, what else would you do?

minad commented 1 year ago

CRM is okay if you want to read a list if short candidates. Of course it works well with compliant UIs like Vertico or Icomplete. CRM is not great if you have long candidate strings. Overall it is not greatly designed and has limited use cases, also in the Emacs code base. As an alternative you could just as well call completing-read in a loop.

minad commented 1 year ago

In the linked discussion we talked about actions on multiple candidates in the context of Ivy and Embark. It turned out that we don't need CRM if we want to act on multiple candidates (embark-act-all). This leaves only a narrow niche where CRM is useful (selecting a list of short candidates), while we could almost achieve everything with CR+Embark alone, with better composability. The situation is comparable to functional languages with unary functions only.

astoff commented 1 year ago

As an alternative you could just as well call completing-read in a loop.

That I really dislike. I had to ask you how to get out of these loops when Vertico is active :-).

CRM lets you select multiple with "no overhead" in the common case of 1 selection. So I think it's very appropriate when choosing the desired documents.

minad commented 1 year ago

On 3/7/23 21:23, Augusto Stoffel wrote:

As an alternative you could just as well call completing-read in a loop.

That I really dislike. I had to ask /you/ how to get out of these loops when Vertico is active :-).

You could as well bind more convenient keys locally such that the UI would still optimize the common case of a single selection.

CRM lets you select multiple with "no overhead" in the common case of 1 selection. So I think it's very appropriate when choosing the desired documents.

Yes, CRM is appropriate when choosing from a list of short candidates. I use it myself for this purpose in my packages.

The point I made above is that CRM should not be used to replace every use of CR, just because we could in principle generalize operations to run on multiple candidates. Instead actions and acting on all candidates is the better tool. Does this clear up the confusion?

astoff commented 1 year ago

@minad It's hard to disagree with that :-).

@bymoz089: I will close this issue, since there will be no code changes, but if you want to explain here what's the best advice to be given to Ivy users, I'll update the readme accordingly.

bymoz089 commented 1 year ago

Now, what exactly happens?

There is just a prompt "Documents for this buffer: " nothing else. If you press TAB you get a list of candidates in the minibuffer, but the prompt only states "(): ". As a long time ivy user, I just was puzzled, what information was required at that prompt. It did not came into my mind to press TAB for completion. ;) With my "fix" all available candidates are shown.


Sorry for the confusion, I created with my link.


How about: creating a wrapper function for this completing-read-multiple call? Then users of other completion frameworks could easily redefine that wrapper function, without redefining devdoc--doc-title. Like so:

Creating that wrapper function in devdocs.el:

(defun devdocs-completing-read-multiple-wrapper (prompt candidates)
  "wrapper function returns a list of car of selected candidates."
  (completing-read-multiple prompt candidates))

And the sexp (completing-read-multiple prompt cands) in function devdoc--doc-title is replaced with:

(devdocs-completing-read-multiple-wrapper prompt cands)

Then users of ivy would redefine that wrapper, in their .init.el file, like so:

(defun devdocs-completing-read-multiple-wrapper (prompt candidates)
  "my own implementation of that wrapper to make it useful with ivy."
  (let (choices)
(ivy-read prompt
      candidates
      :require-match t
      :action (lambda (x)
            (push (car x) choices)))
choices))
astoff commented 1 year ago

There are workarounds that don't require me to change anything, e.g. something like this:

(advice-add #'devdocs--read-document :around
 (lambda (&rest args)
  (cl-letf (((symbol-function 'completing-read-multiple) <<<your replacement>>>))
    (appy args))))

You could also create a dedicated command to set devdocs-current-docs, then you'll never see the CRM prompt.

bymoz089 commented 1 year ago

Good idea, TIL. So the tested fix for the Readme would be:

(advice-add #'devdocs--read-document :around
            (lambda (&rest args)
              (cl-letf (((symbol-function 'completing-read-multiple)
                         (lambda (prompt cands)
                           (let (choices)
                             (ivy-read prompt
                                       cands
                                       :require-match t
                                       :action (lambda (x)
                                                 (push (car x) choices)))
                             choices))))
                (apply args))))
minad commented 1 year ago

@bymoz089 I wonder, if this is a generic fix for completing-read-multiple in the context of Ivy, shouldn't this be part of Ivy or mentioned there instead? devdocs.el is not the only package which uses completing-read-multiple.

bymoz089 commented 1 year ago

I thought about this too, but since completing-read-multiple has a few more parameters, this fix is rather special to this use case. A general fix will need some more work, but I will have a closer look at completing-read-multiple.

minad commented 1 year ago

@bymoz089 You are right. It won't work if you use other arguments and it may not even work for more complex completion tables.