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

Sean Whitton <[email protected]> writes:

> Eshel Yaron [04/Aug  7:20am +02] wrote:
[...]
>>> 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).

Got it.  I wouldn't worry about retaining compatibility with something
like that anyway, since completion-all-sorted-completions is very much
an implementation detail.  But as it happens, I think it should work
also with my patch.

>> 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.

Is there some documentation of that "just right" behavior?
Or specific things you notice that aren't just right?

> 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.

If it has to do with the *Completions* buffer/window, then it would
surely be lost with my patch, because the idea is exactly to divorce
icomplete-in-buffer from the default completion frontend.
But if that behavior is desirable, it can probably be added back quite
easily by binding TAB in icomplete-in-buffer-mode-map.

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

It's just a mechanical change: rename completion--icomplete-in-buffer-p
to completion--FOO (where FOO doesn't include "icomplete", or any other
frontend name), and replace its Icomplete-specific body with:

  (run-hook-with-args-until-success completion-FOO-functions)

Then, in icomplete-mode, add the current
completion--icomplete-in-buffer-p to completion-FOO-functions.
That should preserve the exact behavior of the existing code, and so
hopefully avoid that trepidation. :)


Eshel