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: 75794@debbugs.gnu.org >> Date: Fri, 24 Jan 2025 10:55:27 +0200 >> From: Eli Zaretskii >> >> > From: Rahul Martim Juliato >> > 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