Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 25, 2021
Merged

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

merged 1 commit into from
Jun 25, 2021

Conversation

0x28
Copy link
Contributor

@0x28 0x28 commented Jun 21, 2021

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
Copy link
Owner

astoff commented Jun 22, 2021

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
Copy link
Contributor Author

0x28 commented Jun 22, 2021

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
Copy link
Owner

astoff commented Jun 22, 2021

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
Copy link
Contributor Author

0x28 commented Jun 22, 2021

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
Copy link
Owner

astoff commented Jun 22, 2021

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
Copy link
Contributor Author

0x28 commented Jun 22, 2021

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
Copy link
Contributor Author

0x28 commented Jun 24, 2021

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
Copy link
Owner

astoff commented Jun 25, 2021

All right, looks good. Can you append


Copyright-paperwork-exempt: yes

to the commit message, to keep the record?

Copyright-paperwork-exempt: yes
@0x28
Copy link
Contributor Author

0x28 commented Jun 25, 2021

Yes, done.

@astoff astoff merged commit 2eba589 into astoff:main Jun 25, 2021
@astoff
Copy link
Owner

astoff commented Jun 25, 2021

Thanks!

@astoff astoff mentioned this pull request Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants