astoff / devdocs.el

Emacs viewer for DevDocs
289 stars 17 forks source link

Use thing-at-point for initial input of completing-read #6

Closed 0x28 closed 3 years ago

0x28 commented 3 years ago

Thank you for this package, it's really useful.

This PR changes the completing-read call in devdocs--read-entry. The symbol at point is now used for the INITIAL-INPUT parameter instead of the DEF parameter. This causes the completion to be already narrowed when the completion buffer is visible. The following screenshots show the difference:

Before: image

After: image

astoff commented 3 years ago

I can see why someone might prefer this behavior, but the initial input can be a nuisance depending on your configuration and workflow (I for one wouldn't want this).

What would be an acceptable change is to include an optional initial-input argument to devdocs-lookup, so anyone interested in this can easily write a wrapper command. What do you think?

Also, note that this package is on GNU ELPA, so FSF copyright assignment is required to accept a contribution, unless you haven't contributed to Emacs or other GNU ELPA packages and the PR remains below 15 lines. Is this the case for you?

0x28 commented 3 years ago

Adding an initial-input argument to devdocs-lookup sounds like a good idea. What I don't understand is the usage of the symbol at point for the DEF parameter. If I select this default value (Vec in the first screenshot) I get an Args out of range: 0, 0 error. Am I missing something here?

I haven't signed a CLA at the moment, but like you said, the PR is below 15 lines.

astoff commented 3 years ago

Yes, I noticed the weird first candidate in your screenshot. Which completion UI is this? I've tested with Ivy and Vertico, and this doesn't happen.

You are not supposed to be able to choose Vec, since require-match is true. The way this was supposed to work is that pressing M-n inserts the default candidate, Vec; after that, you should be in the exact same state as the second screenshot.

0x28 commented 3 years ago

I'm using Ivy. I tried it again with emacs -Q but I get the same result as before:

(progn
  (add-to-list 'load-path "~/.emacs.d/elpa/ivy-20210602.1349/")
  (add-to-list 'load-path "~/.emacs.d/elpa/devdocs-20210618.1621/")
  (require 'ivy)
  (require 'devdocs)
  (ivy-mode))

;; call devdocs-lookup ...

M-n works an intended, but the first candidate is always the symbol at point. Maybe it's a recent bug in Ivy?

When I eval the following snippet while Ivy is active, I get the same behavior:

(completing-read "test: " '("A" "B" "C" "D")
         nil
         t
         nil
         nil
         "thing-at-point")
astoff commented 3 years ago

I can reproduce the problem with Ivy version 0.13 (the latest release), so it's not a recent issue. AFAICS it's perfectly reasonable to supply history/default items that don't match a candidate even when require-match is true, so this must be considered a bug in Ivy.

0x28 commented 3 years ago

After looking at the Ivy implementation in more detail, I understand what the problem is. Using the DEF parameter with ivy-completing-read results in this default value to be prepended to the completion candidates. Pressing RET on an empty prompt returns this default value, which is correct according to the documentation of built-in completing-read function. Of course, this behavior is incompatible with the "future history" one would expect from M-n.

I guess this use case isn't really supported by Ivy at the moment.

0x28 commented 3 years ago

I added the initial-input parameter to devdocs-lookup. I think it's useful even if Ivy doesn't behave the way one would expect.

astoff commented 3 years ago

All right, looks good. Can you append


Copyright-paperwork-exempt: yes

to the commit message, to keep the record?

0x28 commented 3 years ago

Yes, done.

astoff commented 3 years ago

Thanks!