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

No way to provide initial input #16

Closed
s-kostyaev opened this issue Nov 30, 2020 · 14 comments
Closed

No way to provide initial input #16

s-kostyaev opened this issue Nov 30, 2020 · 14 comments
Labels
feature New feature or request help wanted Please contribute

Comments

@s-kostyaev
Copy link

s-kostyaev commented Nov 30, 2020

Hi! Thanks for this cool project.

There is no way to provide initial input into consult--read. So there is no way to implement consult-line-from-isearch or consult-line-symbol-at-point. It can be very useful. Also, after this change we can add initial input as consult-line optional argument (like swiper does). This makes implementation that kind of functions trivial.

@minad
Copy link
Owner

minad commented Nov 30, 2020

Thank you! There is the default input. However the initial-argument seems deprecated by the completing-read api, so I am intentionally not supporting the argument here. Could you please describe a bit more in detail what these functions are supposed to do, since I am not very familiar with all the ivy/swiper details?

@s-kostyaev
Copy link
Author

If you call isearch insert some search input and call consult-line-from-isearch it behaves like you call consult-line and insert exact same input. consult-line-from-isearch begin search for symbol at point.

@s-kostyaev
Copy link
Author

I have tried to use default to implement it and I can't.

@minad minad added the feature New feature or request label Nov 30, 2020
@minad
Copy link
Owner

minad commented Nov 30, 2020

Thank you. I think this is indeed not possible without using the initial input argument. I wonder about the motivation for the deprecation, e.g. it needs a bit of digging through the mailing list and git blame. Then we can think of an alternative solution and ask for example the selectrum developers what they are proposing to use instead. I would like to make this project work well with both icomplete and selectrum.

@minad minad added the help wanted Please contribute label Nov 30, 2020
@minad
Copy link
Owner

minad commented Nov 30, 2020

@s-kostyaev I decided to implement support for initial input since I think it can be useful in the special cases you mentioned f5ef8ba. Please let me know if it works for you. Note however that consult--read is private API, therefore and also since it benefits others, I would encourage you to make PRs if you have useful commands lying around :) If you don't have time for that, please add the command to the wishlist #6 such that we can at least keep it in mind!

@minad minad closed this as completed Nov 30, 2020
@s-kostyaev
Copy link
Author

Thanks. I will provide PR as soon as I have free time. Also, why do you think selectrum has no support for initial input? It has:
https://github.com/raxod502/selectrum/blob/945f8fe18933a53b2012a1d179587d90a70bb536/selectrum.el#L1514

@minad
Copy link
Owner

minad commented Dec 1, 2020

@s-kostyaev Yes, it has. However the initial-input argument of completing-read is for some reason not passed through to selectrum-read (https://github.com/raxod502/selectrum/blob/945f8fe18933a53b2012a1d179587d90a70bb536/selectrum.el#L1616). I guess the reason is that commands using completing-read should generally not use initial-input and selectrum wants to make sure that only commands which are explicitly designed to work well with selectrum are allowed to use it. Maybe @clemera can tell us more. My implementation of consult--read is using the selectrum initial-argument option if selectrum is used, for icomplete the initial argument is simply passed to completing-read.

consult/consult.el

Lines 327 to 335 in c880adb

;; HACK: We are explicitly injecting the default input, since default inputs are deprecated
;; in the completing-read API. Selectrum's completing-read consequently does not support
;; them. Maybe Selectrum should add support for initial inputs, even if this is deprecated
;; since the argument does not seem to go away any time soon. There are a few special cases
;; where one wants to use an initial input, even though it should not be overused and the use
;; of initial inputs is discouraged by the Emacs documentation.
(setq consult--selectrum-options
`(,@(unless default-top '(:no-move-default-candidate t))
,@(when initial `(:initial-input ,initial))))

Maybe I should add that if consult uses initial input it is probably ok, since we make sure that it works well with selectrum. So I think we are good, but I hope maybe the selectrum developers change their mind since I don't like jumping through such hoops to achieve what I am looking for. Also note that consult--selectrum-options is a hack of mine which allows passing options to selectrum despite using completing-read. But I hope this can be added to the public selectrum api at some point, see radian-software/selectrum#244. The story is unnecessarily complicated as of now imho.

@clemera
Copy link
Contributor

clemera commented Dec 1, 2020

I don't know more about this, I believe it was there from the start so @raxod502 may have more information. I also wondered about this a few times (why it is officially deprecated). Note you could also do something like:

(minibuffer-with-setup-hook
    (lambda () (insert "initial input"))
  (completing-read "Test: " #'help--symbol-completion-table))

But I just noticed this doesn't currently work correctly with selectrum.

@minad
Copy link
Owner

minad commented Dec 1, 2020

@clemera Thank you for chiming in. Yes I had this before using a minibuffer-with-setup-hook, erase-minibuffer-contents, insert etc. But the solution I use now is more robust.

Edit: How I read the upstream deprecation - It is deprecated for "most use-cases". But I guess it will continue to work forever and I don't see how they will get rid of this argument. Well they could also do the selectrum move and simply ignore the argument, but I don't think this is a great solution. In particular since selectrum itself supports initial input. The concept itself is therefore not deprecated. Maybe upstream should undeprecate this argument again. I think it is enough if one says that initial arguments are generally discouraged because they are intrusive etc. But as far as I see it from this issue, there are some special valid uses cases.

@s-kostyaev
Copy link
Author

I don't know more about this, I believe it was there from the start so @raxod502 may have more information. I also wondered about this a few times (why it is officially deprecated). Note you could also do something like:

(minibuffer-with-setup-hook
    (lambda () (insert "initial input"))
  (completing-read "Test: " #'help--symbol-completion-table))

But I just noticed this doesn't currently work correctly with selectrum.

Selectrum currently use exactly the same solution:
https://github.com/raxod502/selectrum/blob/945f8fe18933a53b2012a1d179587d90a70bb536/selectrum.el#L1229

@minad
Copy link
Owner

minad commented Dec 1, 2020

@s-kostyaev Sure. But there are many things happening before and after. Therefore it is not a good idea to write my own hook which crashes this party.

@clemera
Copy link
Contributor

clemera commented Dec 1, 2020

The concept itself is therefore not deprecated. Maybe upstream should undeprecate this argument again. I think it is enough if one says that initial arguments are generally discouraged because they are intrusive etc. But as far as I see it from this issue, there are some special valid uses cases.

+1

@raxod502
Copy link

Yeah, I think this may be an artifact of external packages having a lot more creativity with how to use the completing-read API than was anticipated upstream. Perhaps none of the built-in commands need something like this, and all the uses of initial-input upstream have been hacks that would have been better served by default. And so the argument was deprecated, despite the fact that there are also other use cases that are not hacks.

I was not thinking about these use cases when I developed Selectrum, so I read the docstring of completing-read (which says initial-input is deprecated) and thought of it as one more wart I could do away with. But in light of the discussion in this thread I think it would make a lot of sense to pass through initial-input from completing-read into selectrum-read.

@minad
Copy link
Owner

minad commented Dec 15, 2020

@raxod502 Thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request help wanted Please contribute
Projects
None yet
Development

No branches or pull requests

4 participants