Re: feature/icomplete-vertical

[Ergus <spacibba@aol.com>]

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

On Sat, Sep 19, 2020 at 04:03:36AM +0000, Gregory Heytings wrote:
>
>Hi Ergus,
>
>Thanks for your comments.
>
>
>Okay, these lines in my patch can just be dropped indeed.  This makes 
>the whole thing even simpler.
>
I thought the same on the beginning, but then the sizes issues came here
and there.

>
>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.
>
There is another corner case that is when

(frame-parameter nil 'minibuffer) is 'only. This was also a problem
reported some days ago needed when using frames and not the minibuffer.

>
>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.
>
The point is that they process the output and substitute the [] () {}
and | with an advise.

Not only in the external package but also some custom configurations
they sent by mail made many magics with this. And it was explicitly
requested by a user some days ago.
>
>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.
>
With the advise it is possible and more or less simple.

That's why I have been playing with the formats to see if I can provide
that with a simple code. But ofcourse, it is expected to be an
additional feature.

>
>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.
>
The other problem with the patch is that due to rounding and floor when
using different fonts there is too much extra space missing and
sometimes missed a candidate at the end. This was also reported some
days ago.


>Gregory

Look at the attached patch as it is partially simplified in my local
branch.

It can be still very improved (the separator part, for example needs
some extra checks to assert it contains at least a '\n', I am actually
thinking in removing the icomplete-format variable and just look for a
\n in the separator in the setup; it will be simpler indeed) but also
some actions are repeated with no reason.

But at least it includes the minimal changes, the corner cases with the
windows/frames/fonts size, fixes the prompt hidden issue, respects the
user separator variable and reduces the conditions we check to only 1 in
the setup.


Best,
Ergus.


[-- Attachment #2: icomplete-vertical.patch --]
[-- Type: text/plain, Size: 11542 bytes --]

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 4e546807b7..c1028e84d0 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -50,6 +50,7 @@
 ;;; Code:
 
 (require 'rfn-eshadow) ; rfn-eshadow-overlay
+(require 'cl-lib)
 
 (defgroup icomplete nil
   "Show completions dynamically in minibuffer."
@@ -57,7 +58,7 @@ icomplete
   :link '(info-link "(emacs)Icomplete")
   :group 'minibuffer)
 
-(defcustom icomplete-separator " | "
+(defcustom icomplete-separator nil
   "String used by Icomplete to separate alternatives in the minibuffer."
   :type 'string
   :version "24.4")
@@ -68,6 +69,12 @@ icomplete-hide-common-prefix
   :type 'boolean
   :version "24.4")
 
+(defcustom icomplete-format 'horizontal
+  "Enable `icomplete' vertical mode."
+  :type '(choice (const horizontal)
+                 (const vertical))
+  :version "28.1")
+
 (defvar icomplete-tidy-shadowed-file-names nil
   "If non-nil, automatically delete superfluous parts of file names.
 For example, if the user types ~/ after a long path name,
@@ -139,6 +146,22 @@ icomplete-minibuffer-setup-hook
   :group 'icomplete)
 
 
+(defvar-local icomplete--ellipsis nil
+  "Ellipsis symbol to indicate continuation.")
+
+(defvar-local icomplete--separator nil
+  "If there are multiple possibilities this separates them.")
+
+(defvar-local icomplete--list-indicators-format nil
+  "Indicator for when multiple prospects are available.
+means that further input is required to distinguish a single one")
+
+(defvar-local icomplete--last-format nil)
+(defvar-local icomplete--prospects nil)
+(defvar-local icomplete--match-braket nil)
+(defvar-local icomplete--prospects-max-pixel-height nil)
+
+
 ;;;_* Initialization
 
 ;;;_ + Internal Variables
@@ -437,6 +460,61 @@ icomplete-simple-completing-p
                (member table icomplete-with-completion-tables))))))
 
 ;;;_ > icomplete-minibuffer-setup ()
+
+(defun icomplete--vertical-prospects (prefix-len _determ comps)
+  "List of vertical completions limited."
+  ;; Max total rows to use, including the minibuffer content.
+  (let* ((single-line-height (line-pixel-height))
+         (line-height (* (cl-count ?\n icomplete--separator) single-line-height))
+         ;; Needed for prospects-max-height-pixel
+         (prospects-rows-pixel (* (+ 1
+                                     (cl-count ?\n icomplete--match-braket)
+                                     (cl-count ?\n icomplete--list-indicators-format))
+                                  single-line-height))
+         (prospects-max-pixel-height (min (+ prospects-rows-pixel
+                                             (* icomplete-prospects-height line-height))
+                                          icomplete--prospects-max-pixel-height))
+         limit prospects comp)
+
+
+    (while (and comps (not limit))
+      (setq comp (substring (pop comps) prefix-len)
+            prospects-rows-pixel (+ prospects-rows-pixel line-height))
+
+      (if (< prospects-rows-pixel prospects-max-pixel-height)
+          (push comp prospects)
+        (push icomplete--ellipsis prospects)
+        (setq limit t)))
+    (nreverse prospects)))
+
+
+(defun icomplete--horizontal-prospects (prefix-len determ comps)
+  "List of horizontal completions limited."
+
+  (let* (;; Max total length to use, including the minibuffer content.
+         (separator-width (string-width icomplete--separator))
+         (prospects-len (+ (string-width (or determ (format icomplete--match-braket "")))
+                           (string-width icomplete--separator)
+                           (+ 2 (string-width icomplete--ellipsis)) ;; take {…} into account
+                           (string-width (buffer-string))))
+         (prospects-max-len (* (+ 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)))
+         limit prospects comp)
+
+    (while (and comps (not limit))
+      (setq comp (substring (pop comps) prefix-len)
+            prospects-len (+ prospects-len (string-width comp) separator-width))
+
+      (if (< prospects-len prospects-max-len)
+          (push comp prospects)
+        (push icomplete--ellipsis prospects)
+        (setq limit t)))
+    (nreverse prospects)))
+
+
 (defun icomplete-minibuffer-setup ()
   "Run in minibuffer on activation to establish incremental completion.
 Usually run by inclusion in `minibuffer-setup-hook'."
@@ -446,7 +524,31 @@ icomplete-minibuffer-setup
     					 (current-local-map)))
     (add-hook 'pre-command-hook  #'icomplete-pre-command-hook  nil t)
     (add-hook 'post-command-hook #'icomplete-post-command-hook nil t)
-    (run-hooks 'icomplete-minibuffer-setup-hook)))
+    (run-hooks 'icomplete-minibuffer-setup-hook)
+
+    (setq-local icomplete--ellipsis (if (char-displayable-p ?…) "…" "..."))
+
+    (cond
+     ((eq icomplete-format 'vertical)
+      (setq-local icomplete--list-indicators-format " \n%s"
+                  icomplete--separator (or icomplete-separator "\n")
+                  icomplete--prospects 'icomplete--vertical-prospects
+                  icomplete--prospects-max-pixel-height
+                  (let ((minibuffer-parameter (frame-parameter nil 'minibuffer)))
+                    (cond
+                     ((eq minibuffer-parameter t)
+                      (cond ((floatp max-mini-window-height)
+                             (floor (* max-mini-window-height (frame-pixel-height))))
+                            ((integerp max-mini-window-height)
+                             (floor (* max-mini-window-height (frame-char-height))))
+                            (t
+                             (* icomplete-prospects-height (frame-char-height)))))
+                     ((eq minibuffer-parameter 'only)
+                      (frame-pixel-height))))))
+     (t (setq-local icomplete--list-indicators-format "{%s}"
+                    icomplete--separator (or icomplete-separator " | ")
+                    icomplete--prospects 'icomplete--horizontal-prospects)))))
+
 
 (defvar icomplete--in-region-buffer nil)
 
@@ -639,8 +741,6 @@ icomplete-completions
 
   (...) - a single prospect is identified and matching is enforced,
   [...] - a single prospect is identified but matching is optional, or
-  {...} - multiple prospects, separated by commas, are indicated, and
-          further input is required to distinguish a single one.
 
 If there are multiple possibilities, `icomplete-separator' separates them.
 
@@ -665,13 +765,13 @@ icomplete-completions
 	 (md (completion--field-metadata (icomplete--field-beg)))
 	 (comps (icomplete--sorted-completions))
          (last (if (consp comps) (last comps)))
-         (base-size (cdr last))
-         (open-bracket (if require-match "(" "["))
-         (close-bracket (if require-match ")" "]")))
+         (base-size (cdr last)))
+
+    (setq-local icomplete--match-braket (if require-match "(%s)" "[%s]"))
     ;; `concat'/`mapconcat' is the slow part.
     (if (not (consp comps))
 	(progn ;;(debug (format "Candidates=%S field=%S" candidates name))
-	  (format " %sNo matches%s" open-bracket close-bracket))
+	  (format icomplete--match-braket "No matches"))
       (if last (setcdr last nil))
       (let* ((most-try
               (if (and base-size (> base-size 0))
@@ -687,33 +787,18 @@ icomplete-completions
              ;; a prefix of most, or something else.
 	     (compare (compare-strings name nil nil
 				       most nil nil completion-ignore-case))
-	     (ellipsis (if (char-displayable-p ?…) "…" "..."))
 	     (determ (unless (or (eq t compare) (eq t most-try)
 				 (= (setq compare (1- (abs compare)))
 				    (length most)))
-		       (concat open-bracket
+		       (format icomplete--match-braket
 			       (cond
 				((= compare (length name))
                                  ;; Typical case: name is a prefix.
 				 (substring most compare))
                                 ;; Don't bother truncating if it doesn't gain
                                 ;; us at least 2 columns.
-				((< compare (+ 2 (string-width ellipsis))) most)
-				(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-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)))
+				((< compare (+ 2 (string-width icomplete--ellipsis))) most)
+				(t (concat icomplete--ellipsis (substring most compare)))))))
              ;; 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):
@@ -725,12 +810,13 @@ icomplete-completions
 	     (prefix (when icomplete-hide-common-prefix
 		       (try-completion "" comps)))
              (prefix-len
-	      (and (stringp prefix)
+              (and icomplete-hide-common-prefix
+                   (stringp prefix)
                    ;; Only hide the prefix if the corresponding info
                    ;; is already displayed via `most'.
                    (string-prefix-p prefix most t)
-                   (length prefix))) ;;)
-	     prospects comp limit)
+                   (length prefix)))
+	     prospects)
 	(if (or (eq most-try t) (not (consp (cdr comps))))
 	    (setq prospects nil)
 	  (when (member name comps)
@@ -751,20 +837,11 @@ icomplete-completions
 	    ;; To circumvent all the above problems, provide a visual
 	    ;; cue to the user via an "empty string" in the try
 	    ;; completion field.
-	    (setq determ (concat open-bracket "" close-bracket)))
+	    (setq determ (format icomplete--match-braket "")))
 	  ;; 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))))
-	(setq prospects (nreverse prospects))
+          (setq prospects
+                (funcall icomplete--prospects prefix-len determ comps)))
+
 	;; Decorate first of the prospects.
 	(when prospects
 	  (let ((first (copy-sequence (pop prospects))))
@@ -776,10 +853,8 @@ icomplete-completions
         (if last (setcdr last base-size))
 	(if prospects
 	    (concat determ
-		    "{"
-		    (mapconcat 'identity prospects icomplete-separator)
-		    (and limit (concat icomplete-separator ellipsis))
-		    "}")
+                    (format icomplete--list-indicators-format
+                            (mapconcat 'identity prospects icomplete--separator)))
 	  (concat determ " [Matched]"))))))
 
 ;;; Iswitchb compatibility