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

reopen 81537
quit

Hi,

Sean Whitton <[email protected]> writes:

> We've discussed this before and Eshel and Juri suggested that the thing
> to do is make icomplete-in-buffer a proper completion-in-region-function
> value, instead of hanging off the default completion infrastructure as
> it does now.

Right.

> I've been looking into this over the weekend and I think that it would
> not be a good idea to try to do that.  Icomplete is heavily dependent on
> the default completion process in a couple of senses:
> - it relies on various completion- and completion-- functions and
>   variables, and some of the state changes
> - in its primary mode, in the minibuffer, it is architected as a pure
>   supplement to the default completion, instead of a whole completions
>   interface of its own.

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.

> So, if we were to try to rearchitect Icomplete to work as a bona fide
> completion-in-region-function, we would likely break various subtle
> behavioural tweaks that have gone into Icomplete over the years that
> make it work as well as it does,

I'm not an Icomplete user, so it's definitely possible that I've missed
some subtlety, but my few tests didn't reveal such breakage.

> 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 haven't got much experience in this area so perhaps the above contains
> some misconceptions.  Let me know how it seems to you all.
> Otherwise, I propose the following patch for master.

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))
+
+(defvar-keymap icomplete-in-buffer-mode-map
+  :doc "Keymap used by Icomplete during in-buffer completion."
+  :parent icomplete-minibuffer-map
+  "C-j" #'icomplete-in-buffer-force-complete-and-exit
+  "C-g" #'icomplete-in-buffer-abort)
+
+(define-minor-mode icomplete-in-buffer-mode
+  "Minor mode that is enabled during Icomplete in-buffer completion."
+  :interactive nil
+  (if icomplete-in-buffer-mode
+      (add-hook 'post-command-hook #'icomplete-post-command-hook nil t)
+    (remove-hook 'post-command-hook #'icomplete-post-command-hook t)
+    (delete-overlay icomplete-overlay)))
+
+(defvar completion-in-region--data)
+
+(defun icomplete--in-region (beg end col prd)
+  (setq completion-in-region--data
+	`(,(if (markerp beg) beg (copy-marker beg))
+          ,(copy-marker end t) ,col ,prd))
+  (icomplete-in-buffer-mode 1))
+
 ;;;_ > icomplete-mode (&optional prefix)
 ;;;###autoload
 (define-minor-mode icomplete-mode
@@ -556,11 +589,11 @@ icomplete-mode
 \\{icomplete-minibuffer-map}"
   :global t
   (remove-hook 'minibuffer-setup-hook #'icomplete-minibuffer-setup)
-  (remove-hook 'completion-in-region-mode-hook #'icomplete--in-region-setup)
+  (remove-function completion-in-region-function #'icomplete--in-region)
   (when icomplete-mode
     (fido-mode -1)
     (when icomplete-in-buffer
-      (add-hook 'completion-in-region-mode-hook #'icomplete--in-region-setup))
+      (add-function :override completion-in-region-function #'icomplete--in-region))
     (add-hook 'minibuffer-setup-hook #'icomplete-minibuffer-setup)))
 
 (defun icomplete--completion-table ()



Best,

Eshel