GNU bug report logs - #77546
Regression in defaults: Re: Enhance 'icomplete-vertical-mode' customization options.

Previous Next

To reply to this bug, email your comments to 77546 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: rahul.juliato <at> gmail.com
Subject: Regression in defaults: Re: Enhance 'icomplete-vertical-mode'
 customization options.
Date: Sat, 05 Apr 2025 10:34:17 +0100

Helo Rahul,

Your change introduces a undocumented regression in the default use of
fido-vertical-mode

b98fe25c2ee2ac2d82b337c49d1aa1dfed2417eb is the first bad commit
commit b98fe25c2ee2ac2d82b337c49d1aa1dfed2417eb
Author: Rahul Martim Juliato <rahul.juliato <at> gmail.com>
Date:   Sat Mar 29 12:55:59 2025 -0300

    Enhance 'icomplete-vertical-mode' customization options.


 etc/NEWS          |  20 +++++++++
 lisp/icomplete.el | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 140 insertions(+), 3 deletions(-)

Using simply this recipe

   emacs -Q -nw -f fido-vertical-mode -f find-file

You will see that after your change two characters are wasted on the
left side of every candidate displayed in the vertical arrangement.

I have no opinion on the change, except that usually the current
behaviour that's been in Emacs for at least 2 major versions should be
kept, and those wishing to take advantage of the new functionality
should customize accordingly.

So perhaps the default values of the customization options you have
introduced need to be changed, or some changes specific to
fido-vertical-mode have to be introduced.

Thanks,
João

Message #8 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: 77546 <at> debbugs.gnu.org, rahul.juliato <at> gmail.com
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sat, 5 Apr 2025 13:18:25 +0100

This seems to be just a bug in the implementation.  This patch
fixes it (and fixes some odd cl-loop usage as well):

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index d0cc5674ba7..1ccfcc3edf4 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -927,8 +927,7 @@ icomplete-vertical--ensure-visible-lines-inside-buffer

 (defun icomplete-vertical--add-indicator-to-selected (comp)
   "Add indicators to the selected/unselected COMP completions."
-  (if (and icomplete-vertical-render-prefix-indicator
-           (get-text-property 0 'icomplete-selected comp))
+  (if (get-text-property 0 'icomplete-selected comp)
       (concat (propertize icomplete-vertical-selected-prefix-indicator
                           'face
'icomplete-vertical-selected-prefix-indicator-face)
               comp)
@@ -1012,8 +1011,11 @@ icomplete--render-vertical
     ;; Serialize completions and section titles into a list
     ;; of lines to render
     (cl-loop
-     for (comp prefix suffix section) in tuples
-     do (setq comp (icomplete-vertical--add-indicator-to-selected comp))
+     for (comp-no-indicator prefix suffix section) in tuples
+     for comp =
+     (if icomplete-vertical-render-prefix-indicator
+         (icomplete-vertical--add-indicator-to-selected comp-no-indicator)
+       comp-no-indicator)
      when section
      collect (propertize section 'face 'icomplete-section) into lines-aux
      and count 1 into nsections-aux

Message #11 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sat, 05 Apr 2025 12:39:25 -0300

João Távora <joaotavora <at> gmail.com> writes:

> This seems to be just a bug in the implementation.  This patch
> fixes it (and fixes some odd cl-loop usage as well):
>
>

Nice catch!

Thanks for this patch João, I agree with you the icomplete enhancements
should not touch defaults.

>        (concat (propertize icomplete-vertical-selected-prefix-indicator
>                            'face
> 'icomplete-vertical-selected-prefix-indicator-face)
>                comp)

It looks like this got wrapped by the mail client, just so anyone else
can copy/paste your diff:


diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index d0cc5674ba7..1ccfcc3edf4 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -927,8 +927,7 @@ icomplete-vertical--ensure-visible-lines-inside-buffer

 (defun icomplete-vertical--add-indicator-to-selected (comp)
   "Add indicators to the selected/unselected COMP completions."
-  (if (and icomplete-vertical-render-prefix-indicator
-           (get-text-property 0 'icomplete-selected comp))
+  (if (get-text-property 0 'icomplete-selected comp)
       (concat (propertize icomplete-vertical-selected-prefix-indicator
                           'face 'icomplete-vertical-selected-prefix-indicator-face)
               comp)
@@ -1012,8 +1011,11 @@ icomplete--render-vertical
     ;; Serialize completions and section titles into a list
     ;; of lines to render
     (cl-loop
-     for (comp prefix suffix section) in tuples
-     do (setq comp (icomplete-vertical--add-indicator-to-selected comp))
+     for (comp-no-indicator prefix suffix section) in tuples
+     for comp =
+     (if icomplete-vertical-render-prefix-indicator
+         (icomplete-vertical--add-indicator-to-selected comp-no-indicator)
+       comp-no-indicator)
      when section
      collect (propertize section 'face 'icomplete-section) into lines-aux
      and count 1 into nsections-aux


I just tested it and can confirm that the issue you described is fixed,
and the new features still seem to be working as expected.

Could you clarify something else for me?

What’s the usual protocol here? Do we wait for a maintainer to pick up
the bug and merge the fix?

Thanks again!

Message #14 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: Rahul Martim Juliato <rahuljuliato <at> gmail.com>, 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sat, 05 Apr 2025 13:33:12 -0300

[Message part 1 (text/plain, inline)]

Rahul Martim Juliato <rahuljuliato <at> gmail.com> writes:


> It looks like this got wrapped by the mail client, just so anyone else
> can copy/paste your diff:

Small addendum, I feel silly, my copy of your diff was also wrapped.

Attaching it here :)

[icomplete-joao-tavora-patch.diff (text/x-patch, attachment)]

Message #17 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
Cc: 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sat, 5 Apr 2025 18:05:50 +0100

[Message part 1 (text/plain, inline)]

That's ok. Thanks for confirming and testing. I'll push the patch later and
close this.

João Távora

On Sat, Apr 5, 2025, 17:33 Rahul Martim Juliato <rahuljuliato <at> gmail.com>
wrote:

> Rahul Martim Juliato <rahuljuliato <at> gmail.com> writes:
>
>
> > It looks like this got wrapped by the mail client, just so anyone else
> > can copy/paste your diff:
>
> Small addendum, I feel silly, my copy of your diff was also wrapped.
>
> Attaching it here :)
>
>

[Message part 2 (text/html, inline)]

Message #20 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
Cc: 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sun, 06 Apr 2025 14:20:56 +0100

João Távora <joaotavora <at> gmail.com> writes:

> That's ok. Thanks for confirming and testing. I'll push the patch later and close this.

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

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?

João

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 35842b53e6b..481e7164889 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -914,16 +914,15 @@ 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
+(defun icomplete--ensure-vertical-completion-list-visible ()
+  "Ensure vertical completion list is visible.
+Scroll 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))))))
+  (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 +942,11 @@ icomplete--render-vertical
                  (truncate (max-mini-window-lines) 1)))))
   ;; Welcome to loopapalooza!
   ;;
-  ;; First, be mindful of `icomplete-scroll' and manual scrolls.  If
+  ;; First, take care of a special case for icomplete--in-region-buffer
+  (when icomplete--in-region-buffer
+    (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 +956,9 @@ 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)
     (cl-loop with preds
              for (comp . rest) on comps
              when (equal comp (car icomplete--scrolled-completions))

Message #23 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: Rahul Martim Juliato <rahuljuliato <at> gmail.com>, 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sun, 06 Apr 2025 21:29:32 -0300

[Message part 1 (text/plain, inline)]

João Távora <joaotavora <at> gmail.com> 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 "⇒é".


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 :)

[to-joao-icomplete.diff (text/x-patch, attachment)]

Message #26 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
Cc: 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Mon, 07 Apr 2025 17:18:28 +0100

Rahul Martim Juliato <rahuljuliato <at> gmail.com> writes:

> 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 :)

Ican take a look at this, but please give me a minimal Emacs -Q
recipe to see this icomplete-in-buffer vertical thing working in
practice.  I can't really get it to work, or know how to activate it.
(Please no long configs or package installations if you can).

This is what I'm trying, for reference:

     emacs -Q                                              \
         --eval '(setq icomplete-in-buffer t)'             \
         -f fido-vertical-mode                             \
         --eval '(setq completion-styles (quote (flex)))'  \
         --eval '(advice-add (quote completion-at-point)   \
                   :after                                  \
                   (function minibuffer-hide-completions))'

It seems to do something but I always see just one candidate, no
vertical list to scroll.  I've also tried with plain
icomplete-vertical-mode.

João

Message #29 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
To: João Távora <joaotavora <at> gmail.com>
Cc: Rahul Martim Juliato <rahuljuliato <at> gmail.com>, 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Mon, 07 Apr 2025 19:44:06 -0300

João Távora <joaotavora <at> gmail.com> writes:
>
> Ican take a look at this, but please give me a minimal Emacs -Q
> recipe to see this icomplete-in-buffer vertical thing working in

Sure!

I tried to shave it down to basics. Still a bit long though:

emacs -Q \
  --eval "(setq tab-always-indent 'complete)" \
  --eval "(setq icomplete-in-buffer t)" \
  --eval "(setq icomplete-prospects-height 10)" \
  --eval "(setq icomplete-scroll t)" \
  --eval "(setq icomplete-vertical-in-buffer-adjust-list t)" \
  --eval "(setq icomplete-vertical-render-prefix-indicator t)" \
  --eval "(setq icomplete-vertical-selected-prefix-indicator \"> \")" \
  --eval "(setq icomplete-vertical-unselected-prefix-indicator \"  \")" \
  --eval "(icomplete-mode 1)" \
  --eval "(icomplete-vertical-mode 1)" \
  --eval "(define-key icomplete-minibuffer-map (kbd \"C-n\") #'icomplete-forward-completions)" \
  --eval "(define-key icomplete-minibuffer-map (kbd \"C-p\") #'icomplete-backward-completions)" \
  --eval "(advice-add #'completion-at-point :after #'minibuffer-hide-completions)"


I use this, with tab tab to complete and C-p / C-n
to scroll the list.

You could omit this if you use C-M-i:
  --eval "(setq tab-always-indent 'copmplete)"

And this, as they are new options provided by the patch and these are defaults:
  --eval "(setq icomplete-vertical-selected-prefix-indicator \"> \")" \
  --eval "(setq icomplete-vertical-unselected-prefix-indicator \"  \")" \  

This defaults to 2, and will show only one candidate as you described,
change at will:
  --eval "(setq icomplete-prospects-height 10)" \


-- 

Rahul

Message #32 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
Cc: rahuljuliato <at> gmail.com, joaotavora <at> gmail.com, 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sat, 26 Apr 2025 14:07:51 +0300

Ping!  How can we make further progress with this bug report?

> Cc: Rahul Martim Juliato <rahuljuliato <at> gmail.com>, 77546 <at> debbugs.gnu.org
> From: Rahul Martim Juliato <rahuljuliato <at> gmail.com>
> Date: Mon, 07 Apr 2025 19:44:06 -0300
> 
> João Távora <joaotavora <at> gmail.com> writes:
> >
> > Ican take a look at this, but please give me a minimal Emacs -Q
> > recipe to see this icomplete-in-buffer vertical thing working in
> 
> Sure!
> 
> I tried to shave it down to basics. Still a bit long though:
> 
> emacs -Q \
>   --eval "(setq tab-always-indent 'complete)" \
>   --eval "(setq icomplete-in-buffer t)" \
>   --eval "(setq icomplete-prospects-height 10)" \
>   --eval "(setq icomplete-scroll t)" \
>   --eval "(setq icomplete-vertical-in-buffer-adjust-list t)" \
>   --eval "(setq icomplete-vertical-render-prefix-indicator t)" \
>   --eval "(setq icomplete-vertical-selected-prefix-indicator \"> \")" \
>   --eval "(setq icomplete-vertical-unselected-prefix-indicator \"  \")" \
>   --eval "(icomplete-mode 1)" \
>   --eval "(icomplete-vertical-mode 1)" \
>   --eval "(define-key icomplete-minibuffer-map (kbd \"C-n\") #'icomplete-forward-completions)" \
>   --eval "(define-key icomplete-minibuffer-map (kbd \"C-p\") #'icomplete-backward-completions)" \
>   --eval "(advice-add #'completion-at-point :after #'minibuffer-hide-completions)"
> 
> 
> I use this, with tab tab to complete and C-p / C-n
> to scroll the list.
> 
> You could omit this if you use C-M-i:
>   --eval "(setq tab-always-indent 'copmplete)"
> 
> And this, as they are new options provided by the patch and these are defaults:
>   --eval "(setq icomplete-vertical-selected-prefix-indicator \"> \")" \
>   --eval "(setq icomplete-vertical-unselected-prefix-indicator \"  \")" \  
> 
> This defaults to 2, and will show only one candidate as you described,
> change at will:
>   --eval "(setq icomplete-prospects-height 10)" \
> 
> 
> -- 
> 
> Rahul
> 
> 
> 
> 

Message #35 received at 77546 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Rahul Martim Juliato <rahuljuliato <at> gmail.com>, 77546 <at> debbugs.gnu.org
Subject: Re: bug#77546: Acknowledgement (Regression in defaults: Re: Enhance
 'icomplete-vertical-mode' customization options.)
Date: Sat, 26 Apr 2025 12:57:23 +0100

On Sat, Apr 26, 2025 at 12:07 PM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> Ping!  How can we make further progress with this bug report?

The regression noted in the subject line is fixed, but there are still
some problems regarding the code around that could see improvements.
According to previous messages, that code seems to have been
developed on trial-and-error basis rather than a full understanding of
the code.

Rahul has provided a patch that among other things adds comments
clarifying the situation. It doesn't look bad, but I think someone should
look into it  better and experiment with alternatives.

I've gotten the reproduction recipe to work, but I haven't had time to
spend on it.  Personally I think the bug should stay open, or maybe
until I or someone else has time to really dive into this and provide
a good patch that simplifies the code and still answers all of Rahul's
UI concerns.

João

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.