Re: feature/icomplete-vertical

[Gregory Heytings via "Emacs development discussions." <emacs-devel@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]


Hi Ergus,

Thanks for your comments.

>
> 1) Icomplete shouldn't call shrink-window because the minibuffer resize 
> must respect the max-mini-window-height and resize-mini-windows policy.
>

Okay, these lines in my patch can just be dropped indeed.  This makes the 
whole thing even simpler.

>
> 2) When using different fonts (as a user jixiuf did) the prompt 
> disappears because there is a mismatch between the visible lines and the 
> ones the minibuffer shows. So we need to fix that in a general way and 
> calculate sizes in pixels to avoid this problem.
>

I see.  I did not think about that potential problem before sending the 
patch.  I attach an updated patch (still very short) in which this problem 
is (I think) fixed.

>
> 3) In vertical the [] and () are annoying; more than in horizontal so we 
> should give some control for them. Now there is a mechanism a bit 
> complex, but just because it is under development. I am actually trying 
> to simplify that.
>

That could be an additional feature indeed.  I do not understand why they 
are annoying, it's common in completion mechanisms to have such 
indications, but why not.  But I think it is better to separate concerns.

>
> 4) Even in vertical the icomplete-separator must be respected somehow 
> because some users reported to use it (with the external package) to 
> separate the candidates from the left of the window, add prompts and so 
> on. (for example adding a > before the first candidate (changing the {) 
> and a space before the others (changing the separator)) So our changes 
> need to support all that in a simpler way. Also some users requested to 
> have two \n as a separator (don't ask me why).
>

That could also be an additional feature, again I don't really see why it 
would be useful, but why not.  OTOH, it is not possible to do this with 
the current version of icomplete in horizontal mode, and again I think it 
is better to separate concerns.

>
> 5) I opted for a more modular approach because filling the code with 
> several 'if' it harder to read and very error prone to maintain latter. 
> So I just separated the setups when vertical and horizontal so if we 
> want to add more policies and features we have to deal with less race 
> conditions.
>

The patch I propose has only five "if icomplete-vertical", that's not much 
I think.  And in fact it's easy to remove three of them, and to have only 
two "if icomplete-vertical" (see the other attached patch).  If horizontal 
icomplete remains the default behavior (as I think it will), these if's 
make it clear where the code differs between the two cases.

Gregory

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=icomplete.patch; charset=us-ascii, Size: 4592 bytes --]

--- icomplete.el.orig	2020-09-01 10:14:22.000000000 +0000
+++ icomplete.el	2020-09-19 03:20:09.000000000 +0000
@@ -62,6 +62,8 @@
   :type 'string
   :version "24.4")
 
+(defcustom icomplete-vertical nil "...")
+
 (defcustom icomplete-hide-common-prefix t
   "When non-nil, hide common prefix from completion candidates.
 When nil, show candidates in full."
@@ -107,6 +109,8 @@
   :type 'integer
   :version "26.1")
 
+(defvar icomplete--prospects-actual-height 0 "...")
+
 (defcustom icomplete-compute-delay .3
   "Completions-computation stall, used only with large-number completions.
 See `icomplete-delay-completions-threshold'."
@@ -431,6 +435,13 @@
   "Run in minibuffer on activation to establish incremental completion.
 Usually run by inclusion in `minibuffer-setup-hook'."
   (when (and icomplete-mode (icomplete-simple-completing-p))
+    (setq icomplete--prospects-actual-height
+	  (min icomplete-prospects-height
+	       (1- (floor (/ (* (if (floatp max-mini-window-height)
+				    (frame-native-height)
+				  (frame-char-height))
+				max-mini-window-height)
+			     (line-pixel-height))))))
     (set (make-local-variable 'completion-show-inline-help) nil)
     (use-local-map (make-composed-keymap icomplete-minibuffer-map
     					 (current-local-map)))
@@ -650,18 +661,22 @@
 				(t (concat ellipsis (substring most compare))))
 			       close-bracket)))
 	     ;;"-prospects" - more than one candidate
-	     (prospects-len (+ (string-width
-				(or determ (concat open-bracket close-bracket)))
-			       (string-width icomplete-separator)
-			       (+ 2 (string-width ellipsis)) ;; take {…} into account
-			       (string-width (buffer-string))))
+	     (prospects-len (if icomplete-vertical
+				0
+			      (+ (string-width
+				  (or determ (concat open-bracket close-bracket)))
+				 (string-width icomplete-separator)
+				 (+ 2 (string-width ellipsis)) ;; take {…} into account
+				 (string-width (buffer-string)))))
              (prospects-max
-              ;; Max total length to use, including the minibuffer content.
-              (* (+ icomplete-prospects-height
-                    ;; If the minibuffer content already uses up more than
-                    ;; one line, increase the allowable space accordingly.
-                    (/ prospects-len (window-width)))
-                 (window-width)))
+	      (if icomplete-vertical
+		  (- icomplete--prospects-actual-height (/ (point) (window-width)))
+		;; Max total length to use, including the minibuffer content.
+		(* (+ icomplete-prospects-height
+                      ;; If the minibuffer content already uses up more than
+                      ;; one line, increase the allowable space accordingly.
+                      (/ prospects-len (window-width)))
+                   (window-width))))
              ;; Find the common prefix among `comps'.
              ;; We can't use the optimization below because its assumptions
              ;; aren't always true, e.g. when completion-cycling (bug#10850):
@@ -705,12 +720,18 @@
 	    (setq comp
 		  (if prefix-len (substring (car comps) prefix-len) (car comps))
 		  comps (cdr comps))
-	    (setq prospects-len
-                  (+ (string-width comp)
-		     (string-width icomplete-separator)
-		     prospects-len))
+	    (if icomplete-vertical
+		(if (> (length comp) 0)
+		    (setq prospects-len (1+ prospects-len)))
+	      (setq prospects-len
+                    (+ (string-width comp)
+		       (string-width icomplete-separator)
+		       prospects-len)))
 	    (if (< prospects-len prospects-max)
-		(push comp prospects)
+		(if icomplete-vertical
+		    (if (> (length comp) 0)
+			(push comp prospects))
+		  (push comp prospects))
 	      (setq limit t))))
 	(setq prospects (nreverse prospects))
         ;; Return the first match if the user hits enter.
@@ -726,11 +747,16 @@
         ;; is cached.
         (if last (setcdr last base-size))
 	(if prospects
-	    (concat determ
-		    "{"
-		    (mapconcat 'identity prospects icomplete-separator)
-		    (and limit (concat icomplete-separator ellipsis))
-		    "}")
+	    (if icomplete-vertical
+		(concat determ
+			" \n"
+			(mapconcat 'identity prospects "\n")
+			(and limit (concat "\n" ellipsis)))
+	      (concat determ
+		      "{"
+		      (mapconcat 'identity prospects icomplete-separator)
+		      (and limit (concat icomplete-separator ellipsis))
+		      "}"))
 	  (concat determ " [Matched]"))))))
 
 ;;; Iswitchb compatibility

[-- Attachment #3: Type: text/x-diff, Size: 3567 bytes --]

--- a/icomplete.el	2020-09-01 10:14:22.000000000 +0000
+++ b/icomplete.el	2020-09-19 03:54:43.000000000 +0000
@@ -62,6 +62,8 @@
   :type 'string
   :version "24.4")
 
+(defcustom icomplete-vertical nil "...")
+
 (defcustom icomplete-hide-common-prefix t
   "When non-nil, hide common prefix from completion candidates.
 When nil, show candidates in full."
@@ -107,6 +109,8 @@
   :type 'integer
   :version "26.1")
 
+(defvar icomplete--prospects-actual-height 0 "...")
+
 (defcustom icomplete-compute-delay .3
   "Completions-computation stall, used only with large-number completions.
 See `icomplete-delay-completions-threshold'."
@@ -431,6 +435,13 @@
   "Run in minibuffer on activation to establish incremental completion.
 Usually run by inclusion in `minibuffer-setup-hook'."
   (when (and icomplete-mode (icomplete-simple-completing-p))
+    (setq icomplete--prospects-actual-height
+	  (min icomplete-prospects-height
+	       (1- (floor (/ (* (if (floatp max-mini-window-height)
+				    (frame-native-height)
+				  (frame-char-height))
+				max-mini-window-height)
+			     (line-pixel-height))))))
     (set (make-local-variable 'completion-show-inline-help) nil)
     (use-local-map (make-composed-keymap icomplete-minibuffer-map
     					 (current-local-map)))
@@ -701,17 +712,30 @@
 	    ;; completion field.
 	    (setq determ (concat open-bracket "" close-bracket)))
 	  ;; Compute prospects for display.
-	  (while (and comps (not limit))
-	    (setq comp
-		  (if prefix-len (substring (car comps) prefix-len) (car comps))
-		  comps (cdr comps))
-	    (setq prospects-len
-                  (+ (string-width comp)
-		     (string-width icomplete-separator)
-		     prospects-len))
-	    (if (< prospects-len prospects-max)
-		(push comp prospects)
-	      (setq limit t))))
+	  (if icomplete-vertical
+	      (let ((prospects-len 0)
+		    (prospects-max (- icomplete--prospects-actual-height (/ (point) (window-width)))))
+		(while (and comps (not limit))
+		  (setq comp
+			(if prefix-len (substring (car comps) prefix-len) (car comps))
+			comps (cdr comps))
+		  (if (> (length comp) 0)
+		      (setq prospects-len (1+ prospects-len)))
+		  (if (< prospects-len prospects-max)
+		      (if (> (length comp) 0)
+			  (push comp prospects))
+		    (setq limit t))))
+	    (while (and comps (not limit))
+	      (setq comp
+		    (if prefix-len (substring (car comps) prefix-len) (car comps))
+		    comps (cdr comps))
+	      (setq prospects-len
+                    (+ (string-width comp)
+		       (string-width icomplete-separator)
+		       prospects-len))
+	      (if (< prospects-len prospects-max)
+		  (push comp prospects)
+		(setq limit t)))))
 	(setq prospects (nreverse prospects))
         ;; Return the first match if the user hits enter.
         (when icomplete-show-matches-on-no-input
@@ -726,11 +750,16 @@
         ;; is cached.
         (if last (setcdr last base-size))
 	(if prospects
-	    (concat determ
-		    "{"
-		    (mapconcat 'identity prospects icomplete-separator)
-		    (and limit (concat icomplete-separator ellipsis))
-		    "}")
+	    (if icomplete-vertical
+		(concat determ
+			" \n"
+			(mapconcat 'identity prospects "\n")
+			(and limit (concat "\n" ellipsis)))
+	      (concat determ
+		      "{"
+		      (mapconcat 'identity prospects icomplete-separator)
+		      (and limit (concat icomplete-separator ellipsis))
+		      "}"))
 	  (concat determ " [Matched]"))))))
 
 ;;; Iswitchb compatibility