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

X-debbugs-cc: [email protected], [email protected], [email protected]

I wrote these entries for NEWS.30:

    ** 'completion-auto-help' now affects 'icomplete-in-buffer'.
    Previously, 'completion-auto-help' mostly affected only minibuffer
    completion.  Now, if 'completion-auto-help' has the value 'lazy', then
    Icomplete's in-buffer display of possible completions will only appear
    after the 'completion-at-point' command has been invoked twice, and if
    'completion-auto-help' is nil, then Icomplete's in-buffer display is
    completely suppressed.  Thus, if you use 'icomplete-in-buffer', ensure
    'completion-auto-help' is not customized to 'lazy' or nil.
    
    ** The "*Completions*" buffer now always accompanies 'icomplete-in-buffer'.
    Previously, it was not consistent whether the "*Completions*" buffer would
    appear when using 'icomplete-in-buffer'.  Now the "*Completions*" buffer
    and Icomplete's in-buffer display of possible completions always
    appear together.  If you would prefer to see only Icomplete's
    in-buffer display, and not the "*Completions*" buffer, you can add this
    to your init file:
    
        (advice-add 'completion-at-point :after #'minibuffer-hide-completions)

We say "If you would prefer ..." but really all users would not want to
see both displays.  Someone who really wants to see both should be the
one who has to add some advice to *show* the completions.
Also, populating the *Completions* buffer only to then hide it is a
performance hit.

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.

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.

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

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.

--
 etc/NEWS           | 10 ++++++++++
 lisp/icomplete.el  | 10 +---------
 lisp/minibuffer.el | 20 ++++++++++++++++----
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 23b97971105..ead3f7fb72c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -97,6 +97,16 @@ You can restore the previous behavior by removing
 ---
 *** The variable 'completion-ignore-case' is now customizable.
 
+---
+*** 'icomplete-in-buffer' users need no longer advise 'completion-at-point'.
+The "*Completions*" buffer no longer pops up in addition to Icomplete's
+in-buffer display of completions, by default.
+Therefore, the instructions in the NEWS file for Emacs 30.1 to add
+
+    (advice-add 'completion-at-point :after #'minibuffer-hide-completions)
+
+to your initialization file no longer apply, and you may remove it.
+
 
 * Editing Changes in Emacs 32.1
 
diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 9a1202c30af..c8da0e9bf9b 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -154,15 +154,7 @@ icomplete-max-delay-chars
 (defcustom icomplete-in-buffer nil
   "If non-nil, use Icomplete when completing in buffers other than minibuffer.
 This affects commands like `completion-in-region', but not commands
-that use their own completions setup.
-
-If you would prefer to see only Icomplete's in-buffer display, but do
-not want the \"*Completions*\" buffer to pop up in those cases, add
-this advice to your init file:
-
-  (advice-add \\='completion-at-point
-              :after #\\='minibuffer-hide-completions)
-"
+that use their own completions setup."
   :type 'boolean)
 
 (defcustom icomplete-minibuffer-setup-hook nil
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 6c13fcabe54..c96de7e4ec1 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1581,6 +1581,9 @@ completion--in-boundaries-p
          (end (+ (length string) (cdr boundaries))))
     (>= start pos end)))
 
+(defun completion--icomplete-in-buffer-p ()
+  (eq (bound-and-true-p icomplete--in-region-buffer) (current-buffer)))
+
 (defun completion--do-completion (beg end &optional
                                       try-completion-function expect-exact)
   "Do the completion and return a summary of what happened.
@@ -1689,7 +1692,8 @@ completion--do-completion
                ((pcase completion-auto-help
                   ('visible (minibuffer--completions-visible))
                   ('always t))
-                (minibuffer-completion-help beg end))
+                (unless (completion--icomplete-in-buffer-p)
+                 (minibuffer-completion-help beg end)))
                (t (minibuffer-hide-completions)
                   (when exact
                     ;; If completion did not put point at end of field,
@@ -1702,13 +1706,15 @@ completion--do-completion
 	      (if (pcase completion-auto-help
                     ('lazy (eq this-command last-command))
                     (_ completion-auto-help))
-                  (minibuffer-completion-help beg end)
+                  (unless (completion--icomplete-in-buffer-p)
+                    (minibuffer-completion-help beg end))
                 (completion--message "Next char not unique")))
              ;; If the last exact completion and this one were the same, it
              ;; means we've already given a "Complete, but not unique" message
              ;; and the user's hit TAB again, so now we give him help.
              (t
-              (when (and (eq this-command last-command) completion-auto-help)
+              (when (and (eq this-command last-command) completion-auto-help
+                         (not (completion--icomplete-in-buffer-p)))
                 (minibuffer-completion-help beg end))
               (completion--done completion 'exact
                                 (unless (or expect-exact
@@ -1774,7 +1780,13 @@ completion--in-region-1
    (t (prog1 (pcase (completion--do-completion beg end)
                (#b000 nil)
                (_     t))
-        (if (window-live-p minibuffer-scroll-window)
+        (if (or (window-live-p minibuffer-scroll-window)
+                ;; FIXME: This reintroduces bug#67001 for
+                ;; `icomplete-in-region' users.  It's not as bad for
+                ;; them, because Icomplete users probably expect to have
+                ;; to C-g out of completion before using other bindings,
+                ;; but it would still be better to fix it.  --spwhitton
+                (completion--icomplete-in-buffer-p))
             (and (eq completion-auto-select t)
                  (eq t (frame-visible-p (window-frame minibuffer-scroll-window)))
                  ;; When the completion list window was displayed, select it.
-- 
Sean Whitton