Subject: Re: bug#75794: [PATCH] feat(icomplete): markers and vertical alignment

From: Rahul Martim Juliato <[email-masked]>
Date: Sun, 09 Feb 2025 20:08:56 -0300

Pong! Sorry for the delayed follow up.

Please find attached a new version of the patch, Below my
considerations: 

> Ping!  Rahul, would you please submit an improved version of the patch
> with my comments addressed, so that we could proceed to installing
> this?
>
>> Cc: [email-masked]
>> Date: Fri, 24 Jan 2025 10:55:27 +0200
>> From: Eli Zaretskii <[email-masked]>
>> 
>> > From: Rahul Martim Juliato <[email-masked]>
>> > Date: Thu, 23 Jan 2025 23:51:06 -0300
>> > 
>> > I am submitting a patch for icomplete-mode that introduces two
>> > enhancements aimed at improving the user experience with completion
>> > candidates:
>> > 
>> > Vertical Alignment: When using icomplete-vertical-mode for in-buffer
>> > completion, the candidates will now align vertically with the column
>> > where the cursor was when completion started. This provides a cleaner,
>> > more intuitive visual experience when scrolling through completion
>> > suggestions.
>> > 
>> > Customizable Markers: I’ve introduced two customizable markers for
>> > icomplete-vertical-mode. The first marker will be applied to the
>> > selected candidate, and the second will apply to the rest of the
>> > list. Both markers are customizable via Emacs faces, allowing users to
>> > tailor the appearance to their preferences.
>> > 
>> > These enhancements are intended to improve the usability and flexibility
>> > of the icomplete interface. The ability to align the completion
>> > candidates and customize the markers will make the completion process
>> > smoother, especially in environments where visual clarity is essential.
>> > 
>> > In case you would like to explore the context further, I’ve written two
>> > blog posts discussing in-buffer icomplete and these enhancements:
>> 
>> Thanks, please see some comments below.
>> 
>> > Add two enhancements to icomplete-mode:
>> > 
>> > - Vertical alignment when using icomplete-vertical-mode with
>> >   in-buffer completion, so candidates are aligned with
>> >   the column where the cursor was when completion started.
>> > 
>> > - Customizable markers to icomplete-vertical-mode, providing
>> >   each candidate a prefix if currently selected and another
>> >   prefix for the rest of the list. Faces can also be customized.
>> 
>> This commit log should include a ChangeLog-style descriptions of
>> added/modified variables and functions.  See CONTRIBUTE for the
>> details.
>> 

Done, please let me know if this OK or if I need to change something.

>> > +(defface icomplete-vertical-selected-prefix-face
>> > +  '((t :inherit font-lock-keyword-face :weight bold :foreground "cyan"))
>> > +  "Face used for the prefix set by `icomplete-vertical-selected-prefix-marker'."
>> > +  :group 'icomplete
>> > +  :version "31")
>> > +
>> > +(defface icomplete-vertical-unselected-prefix-face
>> > +  '((t :inherit font-lock-keyword-face :weight normal :foreground "gray"))
>> > +  "Face used for the prefix set by `icomplete-vertical-unselected-prefix-marker'."
>> > +  :group 'icomplete
>> > +  :version "31")
>>               ^^^^
>> This should be "31.1" (here and elsewhere in the patch.
>> 

Done.

>> > +(defcustom icomplete-vertical-in-buffer-adjust-list t
>> > +  "Control whether in-buffer completion should align the cursor position.
>> 
>> Is it necessary to turn the new behavior ON by default?  Would people
>> be surprised or annoyed by this behavior change?
>> 

I agree with you, it was ON for testing purposes only. Changed the
default to nil.

>> > +(defcustom icomplete-vertical-render-prefix-marker t
>> > +  "Control whether a marker is added as a prefix to each candidate.
>> 
>> I think you are talking about some indication, not a marker.  "Marker"
>> in Emacs has special meaning, not what you mean by that here.  So I
>> suggest to reword this (and other) doc string(s) accordingly.
>> 

Nice, in all functions and doc-strings this is now an 'indicator'.

>> > +(defcustom icomplete-vertical-selected-prefix-marker "» "
>> > +  "Prefix string used to mark the selected completion candidate.
>> > +If `icomplete-vertical-render-prefix-marker' is t, the string
>> > +setted here is used as a prefix of the currently selected entry in the
>>    ^^^^^^
>> "defined", not "setted".
>> 

Done.

>> > +(defcustom icomplete-vertical-unselected-prefix-marker "  "
>> > +  "Prefix string used on the unselected completion candidates.
>> > +If `icomplete-vertical-render-prefix-marker' is t, the string
>> > +setted here is used as a prefix for all unselected entries in the list.
>>    ^^^^^^
>> Same.
>> 

Done.

>> > +(defun icomplete-vertical--adjust-lines-for-column (lines buffer data)
>> > +  "Adjust the LINES to align with the column in BUFFER based on DATA."
>> > +  (if icomplete-vertical-in-buffer-adjust-list
>> > +      (let ((column
>> > +             (with-current-buffer buffer
>> > +               (save-excursion
>> > +                 (goto-char (car data))
>> > +                 (current-column)))))
>> > +        (dolist (l lines)
>> > +          (add-text-properties
>> > +           0 1 `(display ,(concat (make-string column ?\s) (substring l 0 1)))
>> > +           l))
>> > +        lines)
>> > +    lines))
>> 
>> Did you test this with a window horizontally scrolled (when
>> truncate-lines is non-nil)?  What about continuation lines?  In both
>> these cases, current-column is different from the visual column on the
>> screen, counted from the left edge of the window.
>> 

I think you meant when `truncate-lines' is `nil'. When it is 't, no
problems. When it is nil and we're dealing with wrapped lines, this
"works" with the annoying addition of a 'blank wrapped line' between
candidates.

I could not figure out how to fix it properly. Any tips?

For now, I annotated the doc-string informing this is a feature
limitation.

>> 
>> 
>> 
>> And one other comment: these are user-visible changes, so I think they
>> warrant a NEWS entry.

Done. Please tell me if we need to add/remove something to it.

-- 

Rahul Martim Juliato

>From 08d3557af357d9160734fca2fb5e99b0e21f2eb6 Mon Sep 17 00:00:00 2001
From: Rahul Martim Juliato <[email-masked]>
Date: Sun, 9 Feb 2025 18:47:30 -0300
Subject: [PATCH] Enhance 'icomplete-vertical-mode' with new user options and
 faces

New user options have been added to enhance 'icomplete-vertical-mode':
- 'icomplete-vertical-in-buffer-adjust-list': Aligns in-buffer
completion to the original cursor column.
- 'icomplete-vertical-render-prefix-indicator': When enabled, adds a
prefix indicator to completion candidates.
- 'icomplete-vertical-selected-prefix-indicator': Specifies the prefix
string for the selected candidate.
- 'icomplete-vertical-unselected-prefix-indicator': Specifies the prefix
string for unselected candidates.

New faces introduced:
- 'icomplete-vertical-selected-prefix-indicator-face': Controls the
appearance of the selected candidate prefix.
- 'icomplete-vertical-unselected-prefix-indicator-face': Controls the
appearance of unselected candidate prefixes.

* etc/NEWS: Document the new user options and faces.
* lisp/icomplete.el(icomplete-vertical-mode): Implement the new options
and faces.
---
 etc/NEWS          | 20 +++++++++++
 lisp/icomplete.el | 92 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index acaa88d5574..e146ead596f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -176,6 +176,26 @@ will still be on that candidate after "*Completions*" is updated with a
 new list of completions.  The candidate is automatically deselected when
 the "*Completions*" buffer is hidden.
 
++++
+*** New user options for 'icomplete-vertical-mode'.
+New user options have been added to enhance 'icomplete-vertical-mode':
+
+- 'icomplete-vertical-in-buffer-adjust-list': Aligns in-buffer
+  completion to the original cursor column.
+- 'icomplete-vertical-render-prefix-indicator': When enabled, adds a
+  prefix indicator to completion candidates.
+- 'icomplete-vertical-selected-prefix-indicator': Specifies the prefix
+  string for the selected candidate.
+- 'icomplete-vertical-unselected-prefix-indicator': Specifies the prefix
+  string for unselected candidates.
+
+New faces:
+
+- 'icomplete-vertical-selected-prefix-indicator-face': Controls the
+  appearance of the selected candidate prefix.
+- 'icomplete-vertical-unselected-prefix-indicator-face': Controls the
+  appearance of unselected candidate prefixes.
+
 ** Windows
 
 +++
diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index d1d1d4a9d81..69264e53c7b 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -115,6 +115,18 @@ icomplete-section
   "Face used by `icomplete-vertical-mode' for the section title."
   :version "28.1")
 
+(defface icomplete-vertical-selected-prefix-indicator-face
+  '((t :inherit font-lock-keyword-face :weight bold :foreground "cyan"))
+  "Face used for the prefix set 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 for the prefix set by `icomplete-vertical-unselected-prefix-indicator'."
+  :group 'icomplete
+  :version "31.1")
+
 ;;;_* User Customization variables
 (defcustom icomplete-prospects-height 2
   ;; We used to compute how many lines 100 characters would take in
@@ -166,6 +178,50 @@ icomplete-minibuffer-setup-hook
 icompletion is occurring."
   :type 'hook)
 
+(defcustom icomplete-vertical-in-buffer-adjust-list nil
+  "Control whether in-buffer completion should align the cursor position.
+If this is t and `icomplete-in-buffer' is t, and `icomplete-vertical-mode'
+is activated, the in-buffer vertical completions are shown aligned to the
+cursor position when the completion started, not on the first column, as
+the default behaviour.
+
+NOTE: this currently works better when lines are not wrapped, such as
+when `truncate-lines' is t."
+  :type 'boolean
+  :group 'icomplete
+  :version "31.1")
+
+(defcustom icomplete-vertical-render-prefix-indicator nil
+  "Control whether a indicator is added as a prefix to each candidate.
+If this is t and `icomplete-vertical-mode' is activated, a indicator,
+controlled by `icomplete-vertical-selected-prefix-indicator' is shown
+as a prefix to the current under selection candidate, while the
+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 "» "
+  "Prefix string used to mark the selected completion candidate.
+If `icomplete-vertical-render-prefix-indicator' is t, the string
+defined here is used as a prefix of the currently selected entry in the
+list.  It can be further customized by the face
+`icomplete-vertical-selected-prefix-indicator-face'."
+  :type 'string
+  :group 'icomplete
+  :version "31.1")
+
+(defcustom icomplete-vertical-unselected-prefix-indicator "  "
+  "Prefix string used on the unselected completion candidates.
+If `icomplete-vertical-render-prefix-indicator' is t, the string
+defined here is used as a prefix for all unselected entries in the list.
+list.  It can be further customized by the face
+`icomplete-vertical-unselected-prefix-indicator-face'."
+  :type 'string
+  :group 'icomplete
+  :version "31.1")
+
 
 ;;;_* Initialization
 
@@ -828,6 +884,32 @@ icomplete--augment
                  else collect (list tr prefix suffix ))
       annotated)))
 
+(defun icomplete-vertical--adjust-lines-for-column (lines buffer data)
+  "Adjust the LINES to align with the column in BUFFER based on DATA."
+  (if icomplete-vertical-in-buffer-adjust-list
+      (let ((column
+             (with-current-buffer buffer
+               (save-excursion
+                 (goto-char (car data))
+                 (current-column)))))
+        (dolist (l lines)
+          (add-text-properties
+           0 1 `(display ,(concat (make-string column ?\s) (substring l 0 1)))
+           l))
+        lines)
+    lines))
+
+(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))
+      (concat (propertize icomplete-vertical-selected-prefix-indicator
+                          'face 'icomplete-vertical-selected-prefix-indicator-face)
+              comp)
+    (concat (propertize icomplete-vertical-unselected-prefix-indicator
+                        'face 'icomplete-vertical-unselected-prefix-indicator-face)
+            comp)))
+
 (cl-defun icomplete--render-vertical
     (comps md &aux scroll-above scroll-below
            (total-space ; number of mini-window lines available
@@ -843,7 +925,7 @@ icomplete--render-vertical
   ;; - both nil, there is no manual scroll;
   ;; - both non-nil, there is a healthy manual scroll that doesn't need
   ;;   to be readjusted (user just moved around the minibuffer, for
-  ;;   example)l
+  ;;   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
@@ -903,13 +985,14 @@ icomplete--render-vertical
      when section
      collect (propertize section 'face 'icomplete-section) into lines-aux
      and count 1 into nsections-aux
+     for comp = (icomplete-vertical--add-indicator-to-selected comp)
      when (get-text-property 0 'icomplete-selected comp)
      do (add-face-text-property 0 (length comp)
                                 'icomplete-selected-match 'append comp)
      collect (concat prefix
-                     (make-string (- max-prefix-len (length prefix)) ? )
+                     (make-string (max 0 (- max-prefix-len (length prefix))) ? )
                      (completion-lazy-hilit comp)
-                     (make-string (- max-comp-len (length comp)) ? )
+                     (make-string (max 0 (- max-comp-len (length comp))) ? )
                      suffix)
      into lines-aux
      finally (setq lines lines-aux
@@ -924,6 +1007,9 @@ icomplete--render-vertical
                  ((> (length scroll-above) (length scroll-below)) nsections)
                  (t (min (ceiling nsections 2) (length scroll-above))))
            lines))
+    (when icomplete--in-region-buffer
+      (setq lines (icomplete-vertical--adjust-lines-for-column
+                   lines icomplete--in-region-buffer completion-in-region--data)))
     ;; At long last, render final string return value.  This may still
     ;; kick out lines at the end.
     (concat " \n"
-- 
2.39.5