New function to use completing-read as the UI for completion-at-point

[oantolin]

This function was requested by @tomfitzhenry in the wishlist issue #6.

[minad]

Thank you! I will take a closer look at this soon. But probably after we got the marginalia-mode ready since then I have more capacity to add new functions.

[minad]

@oantolin I looked a bit at this and I generally agree that something like this can have its place here. Selectrum has this function, see https://github.com/raxod502/selectrum/blob/9d7ae14e4f8f43e36e68aa6194d754d114dbe707/selectrum.el#L1666. It differs a bit from your version. While it is not so nice to have this kind of duplication I think it makes sense for selectrum to provide this separately since a selectrum user might want to have that feature without buying into consult for example. Then the question is if it would make sense to have this in icomplete-vertical rather than here? Maybe you prefer to separate the concerns somehow and would rather like to see this in the consult utility packag, as a utility. Since consult is a bit of a kitchen sink package, I guess we should not draw too many lines regarding to what should be included here and what not. But I guess it makes sense to think about it a bit. @clemera what is your opinion regarding this, from the selectrum maintainer perspective?

[clemera]

There are various choices how to handle special cases like in Selectrum we delegate to read-file-name when the completion table has category file and in this proposed version there is some special handling when you are already in the minibuffer which then drops you into default completion using the Completions buffer which can make sense for icomplete but not for selectrum for example. Those choices could all be controlled by some variables but I don't know if it makes sense to try to provide a general enough version for all frameworks as most frameworks come with their own version anyway.

[clemera]

To comment on the code: Note that the proposed version doesn't handle :exit-function.

[oantolin]

I think it makes more sense to have a function like this is consult than in icomplete-vertical: not only does it not having anything to do with verticality (one would also want this with horizontal icomplete), it doesn't even really have anything to do with a completion UI. I thought that was exactly the point of consult: to collect completion functionality that is UI-independent.

@clemera has a good point about :exit-function. I didn't handle it because a I didn't know about it. I can fix that.

Also, @clemera mentioned that this function handles the case when called from the minibuffer specially, and points out that completion--in-region is not appropriate for all completion frameworks. That's a good point, and I will remove the special handling of the recursive case (well, other than temporarily enabling recursive minibuffers).

I'll make those changes and then you can decide.

[clemera]

it doesn't even really have anything to do with a completion UI. I thought that was exactly the point of consult: to collect completion functionality that is UI-independent.

I agree with the changes you mention it should be independent so might be nice to have it here.

[minad]

Ok, good! Will these work with selectrum too? Just out of interest - Is it sufficiently generic, since @clemera mentioned something that these functions are usually completion-system-specific? But as far as I see it if the completion system only provides the basic completing-read class of functions it makes sense to implement something like completion-at-point generically and externally and then it should be here. The other question is the one of if it is useful. If it is only ever useful to icomplete-vertical users it could go as well there, even if it is not strictly icomplete or even less vertical-related. I am still a bit undecided what should go where and finding the right boundaries. But I am not against having this as a kitchen sink package as long as the content is sufficiently high quality :)

[oantolin]

I think it is generic. There is still one thing my function does differently than the one in selectrum, which is that it always uses completing-read. A more closely analogous function would use read-file-name if the category metadatum indicates we are completing files. I guess I can implement that. With that change, this function would behave for selectrum users almost identically to the function in selectrum.

And it isn't just useful to icomplete-vertical users: it is also useful for icomplete users that don't use icomplete-vertical, for users of fido-mode, for users of the *Completions* buffer, and for users (plural!, since I now know of one other) of embark-completing-read.

[oantolin]

Hey, @clemera, is the return value of selectrum-completion-in-region correct? The docstring for completion-in-region says a completion-in-region-function should return nil if there was no valid completion and t otherwise.

[oantolin]

Oh boy the recursive minibuffer thing is driving me crazy! I now remember why I delegated to complete--in-region in the first place. Let me think a bit about it.

[minad]

I like the recursive minibuffer. I even made this https://github.com/minad/recursion-indicator 🤣

[oantolin]

I like recursive minibuffers too, and have them enabled, but I also tend to press TAB during minibuffer completion, and this function pops me into a recursive minibuffer each time.

[clemera]

Hey, @clemera, is the return value of selectrum-completion-in-region correct? The docstring for completion-in-region says a completion-in-region-function should return nil if there was no valid completion and t otherwise.

@oantolin
Oops, thanks that needs to be fixed!

[clemera]

@minad

since @clemera mentioned something that these functions are usually completion-system-specific?

They typically bring their own version and that is unlikely to change but might still be worth to a have a generic version that can be used by all of them.

[minad]

@clemera

They typically bring their own version and that is unlikely to change but might still be worth to a have a generic version that can be used by all of them.

Thank you for looking at it. I guess having this here makes sense since we want to collect generic functions here!

@oantolin You said two things:

Oh boy the recursive minibuffer thing is driving me crazy! I now remember why I delegated to complete--in-region in the first place. Let me think a bit about it.

There is still one thing my function does differently than the one in selectrum, which is that it always uses completing-read. A more closely analogous function would use read-file-name if the category metadatum indicates we are completing files. I guess I can implement that. With that change, this function would behave for selectrum users almost identically to the function in selectrum.

Do you want to fix something regarding to complete--in-region (use that?) and also use read-file-name before this is merged? Then we should add a bit of documentation to the readme - I can do that after merging.

[oantolin]

I will switch to read-file-name of the completion category is file.

As for whether to use this from the minibuffer or not: would it make sense to add a customize variable controlling that behavior?

If you don't want a variable, I think I'd leave the behavior as it is now (recurse always). I think I've made my peace with it. It just drove me crazy for a few hours.

[minad]

You can leave it as is without a variable. read-file-name would be good.

[oantolin]

OK, I'm done.

[clemera]

@oantolin
Shouldn't the action function only run when there was a completion?

[oantolin]

Shouldn't the action function only run when there was a completion?

You mean exit function, right? Oh, wow, that was a silly mistake! Thanks for noticing it. I'll fix it right away.

[minad]

@oantolin Is this ready?

[oantolin]

@oantolin Is this ready?

I believe so. I'm already using it and it seems fine.