bug#81537: 32.0.50; Fixing icomplete-in-buffer requiring users to advice-add

Eshel Yaron [04/Aug  7:20am +02] wrote:
> I took a shot at modifying Icomplete to install a proper
> completion-in-region-function, and I think the results aren't that bad.
> It completely and cleanly detaches icomplete-in-buffer from the default
> *Completions* buffer, which I think is a good thing.  See patch below;
> it's supposed to affect only the in-buffer case, not the primary
> minibuffer case.

Thanks.  The insight you had, that I didn't, is to not try to rewrite
icomplete--in-region-setup, but just move it to a different place.

>> and also invalidate code in people's
>> initialisation files.  For example, I found while trying things out that
>> some Icomplete code in my initialization file was relying on the value
>> of completion-all-sorted-completions in a way that subtly broke once I
>> tried moving away from the default completion infrastructure.  The code
>> would surely be fixable, but I wouldn't want to make users do that.
>
> Could you share this piece of configuration?
> It sounds like it might rely on undocumented implementation details, in
> which case it's OK if it needs some tweaking for new Emacs versions.
> But it also possible it'll work just as well with the patch below.

I have a command which finishes by adding :after advice to
icomplete-post-command-hook.  In that advice it does something on the
condition that (length= completion-all-sorted-completions 1).

> Could you please try the following patch?
> The most important advantage this has over the change you installed is
> that it doesn't require minibuffer.el to "know" about Icomplete.
> If this patch doesn't work as expected and we're convinced that using a
> proper completion-in-region-function is a dead-end, we can still free
> minibuffer.el from knowledge of Icomplete by adding a frontend-agnostic
> hook in minibuffer.el that Icomplete would leverage.
> But let's see if we can get by with the existing API first...
>
> diff --git a/lisp/icomplete.el b/lisp/icomplete.el
> index c8da0e9bf9b..7b7d10f96fc 100644
> --- a/lisp/icomplete.el
> +++ b/lisp/icomplete.el
> @@ -537,6 +537,39 @@ fido-mode
>      (add-hook 'minibuffer-setup-hook #'icomplete-minibuffer-setup)
>      (add-hook 'minibuffer-setup-hook #'icomplete--fido-mode-setup)))
>
> +(defun icomplete-in-buffer-force-complete-and-exit ()
> +  (interactive "" icomplete-in-buffer-mode)
> +  (minibuffer-force-complete
> +   (icomplete--field-beg) (icomplete--field-end) 'dont-cycle)
> +  (icomplete-in-buffer-mode -1))
> +
> +(defun icomplete-in-buffer-abort ()
> +  (interactive "" icomplete-in-buffer-mode)
> +  (icomplete-in-buffer-mode -1)
> +  (keyboard-quit))

This part of your patch is what worries me the most.  We have had very
long discussions about getting the several different exiting functions
*just* right, over the years.  I'm loathe to add more subtly different
ones.

Also, there are some subtle behaviours encoded in what I added to
minibuffer.el a few days ago, too, about typing tab twice.  In your
patch I think those would get lost.

I'd like to see your proposal for adding a hook but I may have similar
trepidation about installing it ...

Sometimes you just have to grandfather things in.

-- 
Sean Whitton