Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance 'icomplete-vertical-mode' customization options.)

From: Rahul Martim Juliato <[email-masked]>
Date: Sun, 06 Apr 2025 21:29:32 -0300

João Távora <[email-masked]> writes:

> I've pushed the patch with the fix and followed up with a simpler one
> that simply renames internal functions to be shorter and in-line with
> our convention for internal function names (it's icomplete--, not
> icomplete-vertical-- since icomplete-vertical.el doesn't exist).
>

Thanks! Much easiear to read.

> I've also noticed that you didn't update the hand-holding comments of
> icomplete--render-vertical, and that there's a suspicious double call to
> a new icomplete--ensure-visible-lines-inside-buffer function.  I'm
> fairly sure this is only relevant for `icomplete--in-region-buffer' but
> I think the code should look like this.  Can you try this patch after my
> sig?
>

Yeah it looks like it is duplicated, but there's a silly reason for
it. The first call could be removed with not much trouble. I'll explain
below.

--

I'm attaching another diff where I'm putting togheter some more stuff.

- Removed unecessary ':group' from the newer defcustoms and deffaces

- Made use of 'string-width' instead of lenght, so charts that might
  vary on 'length' are proper counted, like "⇒é".

These, together with a proper comenting of
`icomplete--ensure-vertical-completion-list-visible` was also suggested
by Stefan, so I changed your changes a bit trying to explain what I
meant by 'real lines' on
'icomplete--ensure-vertical-completion-list-visible' doc. Stefan told me
there are references to 'screen lines' and 'logical lines' elsewhere, I
tried to adapt this doc to follow this:

```
(defun icomplete--ensure-vertical-completion-list-visible ()
  "Ensure the vertical completion list is visible.
Scroll the window so that there are at least `icomplete-prospects-height'
screen lines (i.e., visual lines, including wrapped lines) available
below point.  Wrapped lines are counted individually."
````

Now for the good part. I already apologies for the headache.

> I'm
> fairly sure this is only relevant for `icomplete--in-region-buffer' but

Yes, you're right, the use of
`icomplete--ensure-vertical-completion-list-visible` is exclusively for
`icomplete--in-region-buffer`, thanks for removing the `minibufferp`
clause and adding `icomplete--in-region-buffer` where necessary.

In theory we could simply make use of a single entry like:

```
  (when (and icomplete-scroll
             icomplete--scrolled-completions
             (null icomplete--scrolled-past))

     ;; Here, it should work with only this
     (when icomplete--in-region-buffer
       (icomplete--ensure-vertical-completion-list-visible))
```

Problem is: there is a small inconsistency here, IF the cursor is
somewhere on the bottom of the buffer, where the list won't fit, and you
try to complete, this will only make it fit if we've scrolled icomplete
before. So, the inconsitence: this will not ajust if needed on the first
time you complete, but from the second forward. Could I live with a
simple C-g, C-M-i once every time I fire Emacs and my first complete
happened to be on the last line (thus needing scrolling)? Sure, but I
also would love not have to do it, so, keep following :)

Ok, so, what about putting it as you suggested, as the first step of
Loopapalooza?

There's some racing condition if we wanna do it here, it will work, but
it will always fire-up twice, moving the buffer way more than needed. I
couldn't figure it out a mean to make this work properly.

That's the reason we're calling it "twice", the first condition will
only fire up if it is the first (non scrolled, non icomplete used)
state.

The second entry is there 'from the second completion' forward.

I tried a plethora of combinations with
`icomplete--scrolled-completions` and friends, the one that covers more
cases without refactoring too much code was this one. This is now
explained on commentaries you can find on the diff.

Also, I fixed some minor bug on your last diff where you called
`icomplete--ensure-vertical-completion-list-visible` without checking
for `icomplete--in-region-buffer` first and icomplete-vertical was
crashing on minibuffer.

Please feel free to edit my diff, we could:

1. keep both calls, like I just did in the patch, that are complementary

2. keep only the second call, and 'live with a very small inconsistency'

3. try to patch something else and figure out what is causing the race
condition and how to tackle this.

Phew, that's it. Thanks again for taking the time to review these
changes :)

 lisp/icomplete.el | 60 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 35842b53e6b..dfc0a9dd0d2 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -118,13 +118,11 @@ icomplete-section
 (defface icomplete-vertical-selected-prefix-indicator-face
   '((t :inherit font-lock-keyword-face :weight bold :foreground "slate blue"))
   "Face used by `icomplete-vertical-selected-prefix-indicator'."
-  :group 'icomplete
   :version "31.1")
 
 (defface icomplete-vertical-unselected-prefix-indicator-face
   '((t :inherit font-lock-keyword-face :weight normal :foreground "gray"))
   "Face used by `icomplete-vertical-unselected-prefix-indicator'."
-  :group 'icomplete
   :version "31.1")
 
 ;;;_* User Customization variables
@@ -185,7 +183,6 @@ icomplete-vertical-in-buffer-adjust-list
 cursor position when the completion started, not on the first column, as
 the default behavior."
   :type 'boolean
-  :group 'icomplete
   :version "31.1")
 
 (defcustom icomplete-vertical-render-prefix-indicator nil
@@ -196,7 +193,6 @@ icomplete-vertical-render-prefix-indicator
 remaining of the candidates will receive the indicator controlled
 by `icomplete-vertical-unselected-prefix-indicator'."
   :type 'boolean
-  :group 'icomplete
   :version "31.1")
 
 (defcustom icomplete-vertical-selected-prefix-indicator
@@ -210,7 +206,6 @@ icomplete-vertical-selected-prefix-indicator
 By default, this is set to \"» \" if the character is displayable,
 otherwise, it falls back to \"> \"."
   :type 'string
-  :group 'icomplete
   :version "31.1")
 
 (defcustom icomplete-vertical-unselected-prefix-indicator "  "
@@ -220,7 +215,6 @@ icomplete-vertical-unselected-prefix-indicator
 list.  It can be further customized by the face
 `icomplete-vertical-unselected-prefix-indicator-face'."
   :type 'string
-  :group 'icomplete
   :version "31.1")
 
 ;;;_* Initialization
@@ -890,8 +884,8 @@ icomplete--adjust-lines-for-column
       (let* ((column (current-column))
              (prefix-indicator-width
               (if icomplete-vertical-render-prefix-indicator
-                  (max (length icomplete-vertical-selected-prefix-indicator)
-                       (length icomplete-vertical-unselected-prefix-indicator))
+                  (max (string-width icomplete-vertical-selected-prefix-indicator)
+                       (string-width icomplete-vertical-unselected-prefix-indicator))
                 0))
              (wrapped-line (with-current-buffer buffer
                              (save-excursion
@@ -899,7 +893,7 @@ icomplete--adjust-lines-for-column
                                (beginning-of-line)
                                (count-screen-lines (point) (car data)))))
              (window-width (+ (window-hscroll) (window-body-width)))
-             (longest-line-width (apply #'max (mapcar #'length lines)))
+             (longest-line-width (apply #'max (mapcar #'string-width lines)))
              (spaces-to-add
               (if (> wrapped-line 1)
                   (- column (* (- wrapped-line 1) (- window-width 5)))
@@ -914,16 +908,16 @@ icomplete--adjust-lines-for-column
                 lines))
     lines))
 
-(defun icomplete--ensure-visible-lines-inside-buffer ()
-  "Ensure the completion list is visible in regular buffers only.
-Scrolls the screen to be at least `icomplete-prospects-height' real lines
-away from the bottom.  Counts wrapped lines as real lines."
-  (unless (minibufferp)
-    (let* ((window-height (window-body-height))
-           (current-line (count-screen-lines (window-start) (point)))
-           (lines-to-bottom (- window-height current-line)))
-      (when (< lines-to-bottom icomplete-prospects-height)
-        (scroll-up (- icomplete-prospects-height lines-to-bottom))))))
+(defun icomplete--ensure-vertical-completion-list-visible ()
+  "Ensure the vertical completion list is visible.
+Scroll the window so that there are at least `icomplete-prospects-height'
+screen lines (i.e., visual lines, including wrapped lines) available
+below point.  Wrapped lines are counted individually."
+  (let* ((window-height (window-body-height))
+         (current-line (count-screen-lines (window-start) (point)))
+         (lines-to-bottom (- window-height current-line)))
+    (when (< lines-to-bottom icomplete-prospects-height)
+      (scroll-up (- icomplete-prospects-height lines-to-bottom)))))
 
 (defun icomplete--add-indicator-to-selected (comp)
   "Add indicator to completion COMP according to its selection state."
@@ -943,7 +937,20 @@ icomplete--render-vertical
                  (truncate (max-mini-window-lines) 1)))))
   ;; Welcome to loopapalooza!
   ;;
-  ;; First, be mindful of `icomplete-scroll' and manual scrolls.  If
+  ;; First, if the list hasn't been scrolled yet, we check whether the
+  ;; buffer should create some space for it. Such as when opening a
+  ;; buffer, jumping to the last line, and attempting to complete
+  ;; something. The following code will run only ONCE. After
+  ;; opening/closing the complete list for the first time, we call
+  ;; `icomplete--ensure-vertical-completion-list-visible' on the proper
+  ;; place.
+  (when (and icomplete-scroll
+             icomplete--in-region-buffer
+             (not icomplete--scrolled-completions)
+             (not icomplete--scrolled-past))
+    (icomplete--ensure-vertical-completion-list-visible))
+
+  ;; Then, be mindful of `icomplete-scroll' and manual scrolls.  If
   ;; `icomplete--scrolled-completions' and `icomplete--scrolled-past'
   ;; are:
   ;;
@@ -953,14 +960,17 @@ icomplete--render-vertical
   ;;   example);
   ;; - non-nil and nil, respectively, a refiltering took place and we
   ;;   may need to readjust them to the new filtered `comps'.
-  (when (and icomplete-scroll
-             (not icomplete--scrolled-completions)
-             (not icomplete--scrolled-past))
-    (icomplete--ensure-visible-lines-inside-buffer))
   (when (and icomplete-scroll
              icomplete--scrolled-completions
              (null icomplete--scrolled-past))
-    (icomplete--ensure-visible-lines-inside-buffer)
+
+    ;; This runs on every completion list render except the first (i.e., before
+    ;; the first scroll occurs). Therefore, the earlier call to
+    ;; `icomplete--ensure-vertical-completion-list-visible' isn't redundant,
+    ;; even though it may appear to be.
+    (when icomplete--in-region-buffer
+      (icomplete--ensure-vertical-completion-list-visible))
+
     (cl-loop with preds
              for (comp . rest) on comps
              when (equal comp (car icomplete--scrolled-completions))