feature/icomplete-vertical

feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>
>> If there was a built-in vertical mode it would be better / more 
>> intuitive.
>
> Could you try the branch feature/icomplete-vertical? I need some testers 
> before adding it to master.
>

Alas no, I have been using the following to have icomplete-vertical for 
quite some time, it works perfectly well, so I don't see why a more 
complex implementation would be necessary.

(setq icomplete-prospects-height 6)
(setq icomplete-separator "\n")
(defun icomplete-vertical-minibuffer-setup ()
   (setq truncate-lines t)
   (setq-local completion-ignore-case t)
   (setq-local read-file-name-completion-ignore-case t)
   (setq-local read-buffer-completion-ignore-case t)
   (setq icomplete-hide-common-prefix nil))
(add-hook 'icomplete-minibuffer-setup-hook #'icomplete-vertical-minibuffer-setup)
(defun icomplete-vertical-reformat-completions (completions)
   (save-match-data
     (let ((cnp (substring-no-properties completions)))
       (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" cnp)
           (format "%s \n%s"
                   (or (match-string 1 cnp) "")
                   (replace-regexp-in-string "^" (make-string (current-column) ? ) (match-string 2 cnp)))
         cnp))))
(defun icomplete-vertical-adjust-minibuffer-height (completions)
   (let* ((comp (icomplete-vertical-reformat-completions completions))
          (complen (length (split-string comp "\n"))))
     (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1- (window-height)))))
     comp))
(advice-add 'icomplete-completions :filter-return #'icomplete-vertical-adjust-minibuffer-height)

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 12, 2020 at 01:10:57PM +0000, Gregory Heytings wrote:
>
>>
>>>If there was a built-in vertical mode it would be better / more 
>>>intuitive.
>>
>>Could you try the branch feature/icomplete-vertical? I need some 
>>testers before adding it to master.
>>
>
>Alas no, I have been using the following to have icomplete-vertical 
>for quite some time, it works perfectly well, so I don't see why a 
>more complex implementation would be necessary.
>
>(setq icomplete-prospects-height 6)
>(setq icomplete-separator "\n")
>(defun icomplete-vertical-minibuffer-setup ()
>  (setq truncate-lines t)
>  (setq-local completion-ignore-case t)
>  (setq-local read-file-name-completion-ignore-case t)
>  (setq-local read-buffer-completion-ignore-case t)
>  (setq icomplete-hide-common-prefix nil))
>(add-hook 'icomplete-minibuffer-setup-hook #'icomplete-vertical-minibuffer-setup)
>(defun icomplete-vertical-reformat-completions (completions)
>  (save-match-data
>    (let ((cnp (substring-no-properties completions)))
>      (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" cnp)
>          (format "%s \n%s"
>                  (or (match-string 1 cnp) "")
>                  (replace-regexp-in-string "^" (make-string (current-column) ? ) (match-string 2 cnp)))
>        cnp))))
>(defun icomplete-vertical-adjust-minibuffer-height (completions)
>  (let* ((comp (icomplete-vertical-reformat-completions completions))
>         (complen (length (split-string comp "\n"))))
>    (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1- (window-height)))))
>    comp))
>(advice-add 'icomplete-completions :filter-return #'icomplete-vertical-adjust-minibuffer-height)

1) Internal functionalities try not to use advises.
2) The branch is not actually more complex, it just generates the
formatted vertical output form the beginning.
3) It does more or less the same you are doing but with a simpler
config:

(icomplete-mode t)
(icomplete-format 'vertical)

4) We add arrow bindings to move
5) Add completion matching faces is also coming.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>
> 1) Internal functionalities try not to use advises.
> 2) The branch is not actually more complex, it just generates the formatted vertical output form the beginning.
> 3) It does more or less the same you are doing but with a simpler config:
>
> (icomplete-mode t)
> (icomplete-format 'vertical)
>
> 4) We add arrow bindings to move
> 5) Add completion matching faces is also coming.
>

Okay, I did not mean to disregard what you did, but I did this before it 
was an internal functionality (based on a code snippet I found somewhere). 
I doubt the config you indicate would suffice to do what I have (and 
want), unless there is an option to do "(replace-regexp-in-string "^" 
(make-string (current-column) ? ) (match-string 2 cnp))".  That is, to 
have the completion candidates presented under the cursor (instead of on 
the left of the minibuffer).

Re: feature/icomplete-vertical

[jixiuf]

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



> 2020年9月12日 下午9:33，Ergus <spacibba@aol.com> 写道：
> 
> On Sat, Sep 12, 2020 at 01:10:57PM +0000, Gregory Heytings wrote:
>> 
>>> 
>>>> If there was a built-in vertical mode it would be better / more intuitive.
>>> 
>>> Could you try the branch feature/icomplete-vertical? I need some testers before adding it to master.
>>> 
>> 
>> Alas no, I have been using the following to have icomplete-vertical for quite some time, it works perfectly well, so I don't see why a more complex implementation would be necessary.
>> 
>> (setq icomplete-prospects-height 6)
>> (setq icomplete-separator "\n")
>> (defun icomplete-vertical-minibuffer-setup ()
>> (setq truncate-lines t)
>> (setq-local completion-ignore-case t)
>> (setq-local read-file-name-completion-ignore-case t)
>> (setq-local read-buffer-completion-ignore-case t)
>> (setq icomplete-hide-common-prefix nil))
>> (add-hook 'icomplete-minibuffer-setup-hook #'icomplete-vertical-minibuffer-setup)
>> (defun icomplete-vertical-reformat-completions (completions)
>> (save-match-data
>>   (let ((cnp (substring-no-properties completions)))
>>     (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" cnp)
>>         (format "%s \n%s"
>>                 (or (match-string 1 cnp) "")
>>                 (replace-regexp-in-string "^" (make-string (current-column) ? ) (match-string 2 cnp)))
>>       cnp))))
>> (defun icomplete-vertical-adjust-minibuffer-height (completions)
>> (let* ((comp (icomplete-vertical-reformat-completions completions))
>>        (complen (length (split-string comp "\n"))))
>>   (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1- (window-height)))))
>>   comp))
>> (advice-add 'icomplete-completions :filter-return #'icomplete-vertical-adjust-minibuffer-height)
> 
> 1) Internal functionalities try not to use advises.
> 2) The branch is not actually more complex, it just generates the
> formatted vertical output form the beginning.
> 3) It does more or less the same you are doing but with a simpler
> config:
> 
> (icomplete-mode t)
> (icomplete-format 'vertical)
> 
> 4) We add arrow bindings to move
> 5) Add completion matching faces is also coming.
> 

Some feedback about  branch : feature/icomplete-vertical  

I tried this branch with config:

  (icomplete-mode 1)
  (setq icomplete-format 'vertical)



And found this bug still exists:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379

Some related links about how to work around this bug:

https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm86m0q?utm_source=share&utm_medium=web2x&context=3
https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm8z6h0?utm_source=share&utm_medium=web2x&context=3

Hope the problem of   disappearing prompt and entered text problem can be fixed in your branch. 



[-- Attachment #2: Type: text/html, Size: 5823 bytes --]

Re: feature/icomplete-vertical

[Ergus]

On Mon, Sep 14, 2020 at 02:44:59PM +0800, jixiuf wrote:
>
>
>> 2020年9月12日 下午9:33，Ergus <spacibba@aol.com> 写道：
>>
>> On Sat, Sep 12, 2020 at 01:10:57PM +0000, Gregory Heytings wrote:
>>>
>>>>
>>>>> If there was a built-in vertical mode it would be better / more intuitive.
>>>>
>>>> Could you try the branch feature/icomplete-vertical? I need some testers before adding it to master.
>>>>
>>>
>>> Alas no, I have been using the following to have icomplete-vertical for quite some time, it works perfectly well, so I don't see why a more complex implementation would be necessary.
>>>
>>> (setq icomplete-prospects-height 6)
>>> (setq icomplete-separator "\n")
>>> (defun icomplete-vertical-minibuffer-setup ()
>>> (setq truncate-lines t)
>>> (setq-local completion-ignore-case t)
>>> (setq-local read-file-name-completion-ignore-case t)
>>> (setq-local read-buffer-completion-ignore-case t)
>>> (setq icomplete-hide-common-prefix nil))
>>> (add-hook 'icomplete-minibuffer-setup-hook #'icomplete-vertical-minibuffer-setup)
>>> (defun icomplete-vertical-reformat-completions (completions)
>>> (save-match-data
>>>   (let ((cnp (substring-no-properties completions)))
>>>     (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" cnp)
>>>         (format "%s \n%s"
>>>                 (or (match-string 1 cnp) "")
>>>                 (replace-regexp-in-string "^" (make-string (current-column) ? ) (match-string 2 cnp)))
>>>       cnp))))
>>> (defun icomplete-vertical-adjust-minibuffer-height (completions)
>>> (let* ((comp (icomplete-vertical-reformat-completions completions))
>>>        (complen (length (split-string comp "\n"))))
>>>   (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1- (window-height)))))
>>>   comp))
>>> (advice-add 'icomplete-completions :filter-return #'icomplete-vertical-adjust-minibuffer-height)
>>
>> 1) Internal functionalities try not to use advises.
>> 2) The branch is not actually more complex, it just generates the
>> formatted vertical output form the beginning.
>> 3) It does more or less the same you are doing but with a simpler
>> config:
>>
>> (icomplete-mode t)
>> (icomplete-format 'vertical)
>>
>> 4) We add arrow bindings to move
>> 5) Add completion matching faces is also coming.
>>
>
>Some feedback about  branch : feature/icomplete-vertical
>
>I tried this branch with config:
>
>  (icomplete-mode 1)
>  (setq icomplete-format 'vertical)
>
>
>
>And found this bug still exists:
>
>https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
>https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379
>
>Some related links about how to work around this bug:
>
>https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm86m0q?utm_source=share&utm_medium=web2x&context=3
>https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm8z6h0?utm_source=share&utm_medium=web2x&context=3
>
>Hope the problem of   disappearing prompt and entered text problem can be fixed in your branch.
>
>

Hi:

I can't reproduce https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
with this config.

emacs -Q
(icomplete-mode 1)
(setq icomplete-format 'vertical)

The prompt is not disappearing for me. Is there some other
indication/config missing?

Re: feature/icomplete-vertical

[Ergus]

On Mon, Sep 14, 2020 at 02:44:59PM +0800, jixiuf wrote:
>
>
>Some feedback about  branch : feature/icomplete-vertical
>
>I tried this branch with config:
>
>  (icomplete-mode 1)
>  (setq icomplete-format 'vertical)
>
>
>
>And found this bug still exists:
>
>https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
>https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379
>
>Some related links about how to work around this bug:
>
>https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm86m0q?utm_source=share&utm_medium=web2x&context=3
>https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm8z6h0?utm_source=share&utm_medium=web2x&context=3
>
>Hope the problem of   disappearing prompt and entered text problem can be fixed in your branch.
>
>
This is already fixed.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

Hi Ergus,

I gave your icomplete-vertical a try.  It seems to work well so far, but 
there is one thing that does not work as expected (or, at least, as I 
expect it to work): for some reason when you complete a filename 
(find-file), the first candidate './' has been taken out of the candidate 
list (with both icomplete-format 'vertical and 'horizontal), which means 
that I see ("[]" is the point):

Find file: ~/[]/ | { <other candidates> }

or:

Find file: ~/[]./
<other candidates>

I do not understand why this is the case.

Likewise, when there are multiple candidates, icomplete-vertical behaves 
in a counterintuitive way with find-file, for example I see (with two 
candidate directories "foofoo and "foobar"):

Find file: ~/foo[]foofoo
foobar

Re: feature/icomplete-vertical

[Ergus]

On Mon, Sep 14, 2020 at 05:13:40PM +0000, Gregory Heytings via Emacs development discussions. wrote:
>
>Hi Ergus,
>
>I gave your icomplete-vertical a try.  It seems to work well so far, 
>but there is one thing that does not work as expected (or, at least, 
>as I expect it to work): for some reason when you complete a filename 
>(find-file), the first candidate './' has been taken out of the 
>candidate list (with both icomplete-format 'vertical and 'horizontal), 
>which means that I see ("[]" is the point):
>
>Find file: ~/[]/ | { <other candidates> }
>
>or:
>
>Find file: ~/[]./
><other candidates>
>
>I do not understand why this is the case.
>
>Likewise, when there are multiple candidates, icomplete-vertical 
>behaves in a counterintuitive way with find-file, for example I see 
>(with two candidate directories "foofoo and "foobar"):
>
>Find file: ~/foo[]foofoo
>foobar
>

You are right. This is a change I introduced recently to simplify
something else. I will fix it asap; the idea is that you may see:

Find file: ~/foofoo
foobar

and with a different option this:

Find file: ~/foo
foofoo
foobar

with the first foofoo fontified. As someone requested also to have this:

Find file: ~/foo
                 foofoo
                 foobar

or

Find file: ~/foo
              foofoo
              foobar

And personally I want this:

Find file: ~/foo
> foofoo
   foobar


So I am working in all of them at the time avoiding adding too much
complexity. But the code get complex because I have to keep old
variables like the separator which I don't need (because I am using
formats)

I'll keep you posted when I fix this. It is not very complex indeed but
I am very busy these days.

Re: feature/icomplete-vertical

[jixiuf]

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



> 2020年9月14日 下午11:02，Ergus <spacibba@aol.com> 写道：
> 
> On Mon, Sep 14, 2020 at 02:44:59PM +0800, jixiuf wrote:
>> 
>> 
>> Some feedback about  branch : feature/icomplete-vertical
>> 
>> I tried this branch with config:
>> 
>> (icomplete-mode 1)
>> (setq icomplete-format 'vertical)
>> 
>> 
>> 
>> And found this bug still exists:
>> 
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379
>> 
>> Some related links about how to work around this bug:
>> 
>> https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm86m0q?utm_source=share&utm_medium=web2x&context=3
>> https://www.reddit.com/r/emacs/comments/fswt7c/using_icomplete_vertically/fm8z6h0?utm_source=share&utm_medium=web2x&context=3
>> 
>> Hope the problem of   disappearing prompt and entered text problem can be fixed in your branch.
>> 
>> 
> This is already fixed.
> 


With commit f80a97fedf81cb0af955a4f799d5ac3105570a25 

```
(setq icomplete-format 'vertical)
(setq icomplete-hide-common-prefix nil)
(setq icomplete-show-matches-on-no-input t)
(icomplete-mode 1)

```

1. I got  this error when I press C-h f (describe-function)
Error in post-command-hook (icomplete-post-command-hook): (error "Invalid format operation %
“)


2. M-x:man 
 got :

Even with (setq icomplete-hide-common-prefix nil) , the prefix is still hidden.




[-- Attachment #2.1: Type: text/html, Size: 3089 bytes --]

[-- Attachment #2.2: 粘贴的图形-1.png --]
[-- Type: image/png, Size: 15305 bytes --]

Re: feature/icomplete-vertical

[jixiuf]

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



> 2020年9月18日 上午8:53，Ergus <spacibba@aol.com> 写道：
> 
> On Wed, Sep 16, 2020 at 11:22:19PM +0800, jixiuf wrote:
>> 
>> 
>> 
>> 
>> With commit f80a97fedf81cb0af955a4f799d5ac3105570a25
>> 
>> ```
>> (setq icomplete-format 'vertical)
>> (setq icomplete-hide-common-prefix nil)
>> (setq icomplete-show-matches-on-no-input t)
>> (icomplete-mode 1)
>> 
>> ```
>> 
>> 1. I got  this error when I press C-h f (describe-function)
>> Error in post-command-hook (icomplete-post-command-hook): (error "Invalid format operation %
>> “)
>> 
>> 
>> 2. M-x:man
>> got :
>> 
>> Even with (setq icomplete-hide-common-prefix nil) , the prefix is still hidden.
>> 
> Both are fixed now. Please try.
Thanks, it is fixed .

But  the bug of  "disappearing prompt and entered text problem" came back again, when I  (setq max-mini-window-height 10)  

```

(setq icomplete-show-matches-on-no-input t)
(icomplete-mode 1)
(setq icomplete-format 'vertical)

(defface vmacs-minibuffer-font
 `((t :inherit default :height 1.3))
 "The default font for minibuffer buffer.
Monospaced font whihc is fixed idth and height is recommended."
 :group 'minibuffer)

(defun vmacs-minibuffer-hook()
 (set (make-local-variable 'buffer-face-mode-face) 'vmacs-minibuffer-font)
 (buffer-face-mode t))

(add-hook 'minibuffer-setup-hook #'vmacs-minibuffer-hook)

(setq max-mini-window-height 10)   ;; add  this line
```

[-- Attachment #2.1: Type: text/html, Size: 3054 bytes --]

[-- Attachment #2.2: 粘贴的图形-1.png --]
[-- Type: image/png, Size: 25676 bytes --]

Re: feature/icomplete-vertical

[Ergus]

On Fri, Sep 18, 2020 at 11:08:48AM +0800, jixiuf wrote:
>
>
>Thanks, it is fixed .
>
>But  the bug of  "disappearing prompt and entered text problem" came back again, when I  (setq max-mini-window-height 10)
>
Indeed this is nasty because the issue is not exactly in icomplete. I
will explain a bit:

max-mini-window-height sets the height of mini-windows BUT when it is
set as an int it sets the minibuffer height using the number of lines
but using the main frame's font height.

In your case the problem is that the face in minibuffer is different
than in the rest of the frame and your font is bigger. So for the moment
there is only a solutions fromicomp lete I can do without forcing a
resize (something I am trying to avoid):

Use your font to increment the height and stop when fixed (which will
not respect the 10 lines as you want because your font is bigger)

Alternatively I can try to fix the resize mechanism properly by fixing
where the minibuffer resize happens and how it gets the font... but this
will require more time (I think it is the right thing to do.)



>```
>
>(setq icomplete-show-matches-on-no-input t)
>(icomplete-mode 1)
>(setq icomplete-format 'vertical)
>
>(defface vmacs-minibuffer-font
> `((t :inherit default :height 1.3))
> "The default font for minibuffer buffer.
>Monospaced font whihc is fixed idth and height is recommended."
> :group 'minibuffer)
>
>(defun vmacs-minibuffer-hook()
> (set (make-local-variable 'buffer-face-mode-face) 'vmacs-minibuffer-font)
> (buffer-face-mode t))
>
>(add-hook 'minibuffer-setup-hook #'vmacs-minibuffer-hook)
>
>(setq max-mini-window-height 10)   ;; add  this line
>```

Re: feature/icomplete-vertical

[Ergus]

On Fri, Sep 18, 2020 at 11:08:48AM +0800, jixiuf wrote:
>
>
>Thanks, it is fixed .
>
>But  the bug of  "disappearing prompt and entered text problem" came back again, when I  (setq max-mini-window-height 10)
>
>```
>
>(setq icomplete-show-matches-on-no-input t)
>(icomplete-mode 1)
>(setq icomplete-format 'vertical)
>
>(defface vmacs-minibuffer-font
> `((t :inherit default :height 1.3))
> "The default font for minibuffer buffer.
>Monospaced font whihc is fixed idth and height is recommended."
> :group 'minibuffer)
>
>(defun vmacs-minibuffer-hook()
> (set (make-local-variable 'buffer-face-mode-face) 'vmacs-minibuffer-font)
> (buffer-face-mode t))
>
>(add-hook 'minibuffer-setup-hook #'vmacs-minibuffer-hook)
>
>(setq max-mini-window-height 10)   ;; add  this line
>```

After asking Stefan he agrees that the problem is in the documentation
of max-mini-window-height that should specify that the height is
expressed in terms of the default's frame font.

So according to that it is fixed now in icomplete-vertical and waiting
for a patch to improve the documentation of max-mini-window-height.

Best,
Ergus.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Fri, 18 Sep 2020 19:05:38 +0200
> From: Ergus <spacibba@aol.com>
> Cc: emacs-devel@gnu.org
> 
> So according to that it is fixed now in icomplete-vertical and waiting
> for a patch to improve the documentation of max-mini-window-height.

I fixed the documentation of max-mini-window-height to be more clear
about the meaning of an integer value.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

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


Hi Ergus,

I don't understand why you change so many things in icomplete.el for 
icomplete-vertical.  I attach a patch which implements icomplete-vertical 
by adding only 20 lines to that file.  It seems to work correctly.  Use it 
with:

(icomplete-mode 1)
(setq icomplete-vertical t)

and set `icomplete-prospects-height' to the maximal number of completion 
candidates you want to display, for example:

(setq icomplete-prospects-height 10)

Gregory

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

--- a/icomplete.el	2020-09-01 10:14:22.000000000 +0000
+++ b/icomplete.el	2020-09-18 21:14:15.000000000 +0000
@@ -62,6 +62,10 @@
   :type 'string
   :version "24.4")
 
+(defcustom icomplete-vertical nil "...")
+
+(defvar icomplete--prospects-count 0 "...")
+
 (defcustom icomplete-hide-common-prefix t
   "When non-nil, hide common prefix from completion candidates.
 When nil, show candidates in full."
@@ -573,7 +577,10 @@
               ;; marker's stickiness to figure out whether to place the cursor
               ;; before or after the string, so let's spoon-feed it the pos.
               (put-text-property 0 1 'cursor t text)
-              (overlay-put icomplete-overlay 'after-string text))))))))
+              (overlay-put icomplete-overlay 'after-string text)
+	      (if (and (> (window-height) icomplete--prospects-count)
+		       (< icomplete--prospects-count icomplete-prospects-height))
+		  (shrink-window (- icomplete-prospects-height icomplete--prospects-count))))))))))
 
 ;;;_ > icomplete-completions (name candidates predicate require-match)
 (defun icomplete-completions (name candidates predicate require-match)
@@ -650,18 +657,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-height
+		;; 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,13 +716,20 @@
 	    (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 icomplete--prospects-count prospects-len)
 	(setq prospects (nreverse prospects))
         ;; Return the first match if the user hits enter.
         (when icomplete-show-matches-on-no-input
@@ -726,11 +744,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

Re: feature/icomplete-vertical

[Stefan Monnier]

> I don't understand why you change so many things in icomplete.el for
> icomplete-vertical.

I think it's because the display suggests a change to the way you
interact with it.


        Stefan

Re: feature/icomplete-vertical

[Ergus]

On Fri, Sep 18, 2020 at 09:39:58PM +0000, Gregory Heytings via Emacs development discussions. wrote:
>
>Hi Ergus,
>
>I don't understand why you change so many things in icomplete.el for 
>icomplete-vertical.  I attach a patch which implements 
>icomplete-vertical by adding only 20 lines to that file.  It seems to 
>work correctly.  Use it with:
>
>(icomplete-mode 1)
>(setq icomplete-vertical t)
>
>and set `icomplete-prospects-height' to the maximal number of 
>completion candidates you want to display, for example:
>
>(setq icomplete-prospects-height 10)
>
>Gregory


Hi Gregory, thanks for commenting.

I know the branch has too many changes. Usually I do a full refactor and
a squash before merging to master to simplify all the experimental
things I try during development. My error was probably to call it
feature and not scratch branch.

The changes I introduced are actually not too many, the others are just
details and customs to avoid to interfere or changing the defaults of
icomplete while developing. Most of them will disappear in the final
patch or come latter as different features. (I have the bad habit to
overwrite too much my histories ;p)

OTOH: The problem with the approach in your patch (that you can see is
essentially similar to the first commit in the branch) are basically:

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

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.

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.

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

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.

For now I am only worried about functionality; when that works as
expected and cover all the popular use cases I will cleanup I promise :)

Best,
Ergus

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

[-- 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

Re: feature/icomplete-vertical

[Ergus]

[-- 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

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

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

Hmmm... in that case (a corner case, indeed, which does not seem to be 
covered by the current icomplete implementation), it means that the 
minibuffer is not resized att all?  TBH, it seems to me that using 
icomplete with a minibuffer-only frame does not have much sense.

That being said, it is always possible to imagine more corner cases, for 
example a completion candidate that would fill more than one line, or a 
completion candidate list with different fonts (I'm not sure that this is 
actually feasible), or completion candidates with embedded newlines, or...

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

I don't think so, but I could be wrong.  I tested my code with many 
different parameters, and did not see the "extra space" you mention, in 
all cases the completions fit perfectly in the minibuffer, and the prompt 
is never hidden.

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

Thanks, I'll have a look.

Gregory

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> Look at the attached patch as it is partially simplified in my local 
>> branch.
>
> Thanks, I'll have a look.
>

It seems to me that it does not work as well as my code, alas.  A few 
points:

- with C-x C-f the default completion, between angle brackets in the 
original icomplete code, is not shown anymore, unless there is a single 
candidate (for example in a directory with "foo", "bar1", "bar2", "f" will 
give "f[oo]" but "b" will give "b[]"); in other words, the "[]" after the 
point usually serves no purpose

- likewise, with M-x there is a "()" after the point that usually serves 
no purpose

- if I type "M-x icomplete-m", I see the following:

M-x icomplete-m(ode)
|

where "|" represents the cursor (on the next line!)

- when the current line after the prompt is too long the prompt disappears

- when completing a unique completion with TAB the point is moved to the 
next line

Gregory

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 19, 2020 at 08:35:24AM +0000, Gregory Heytings via Emacs development discussions. wrote:
>
>>
>>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.
>>
>
>Hmmm... in that case (a corner case, indeed, which does not seem to be 
>covered by the current icomplete implementation), it means that the 
>minibuffer is not resized att all?  TBH, it seems to me that using 
>icomplete with a minibuffer-only frame does not have much sense.
>
Some users of maple-minibuffer-mode uses them with icomplete + the
external package.

>That being said, it is always possible to imagine more corner cases, 
>for example a completion candidate that would fill more than one line,
I know, for this the users set visual-line in minibuffer. Which I
probably will do to.

>or a completion candidate list with different fonts (I'm not sure that
>this is actually feasible)

This is the reason of the "format" approach I was trying; and why
working directly in pixels as it is simple to get the current line
height in pixels too and increment it more precisely. But I consider
this as a different feature too, so it is not in that patch.

>, or completion candidates with embedded newlines, or...

Covered with the format approach too, but also a new feature.
>
>>
>>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.
>>
>
>I don't think so, but I could be wrong.  I tested my code with many 
>different parameters, and did not see the "extra space" you mention, 
>in all cases the completions fit perfectly in the minibuffer, and the 
>prompt is never hidden.
>
Or I could, but I had issues before with this before.

>>
>>Look at the attached patch as it is partially simplified in my local 
>>branch.
>>
>
>Thanks, I'll have a look.
>
>Gregory
>

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 19, 2020 at 10:30:13AM +0000, Gregory Heytings via Emacs development discussions. wrote:
>
>>>Look at the attached patch as it is partially simplified in my 
>>>local branch.
>>
>>Thanks, I'll have a look.
>>
>
>It seems to me that it does not work as well as my code, alas.  A few 
>points:
>
>- with C-x C-f the default completion, between angle brackets in the 
>original icomplete code, is not shown anymore, unless there is a 
>single candidate (for example in a directory with "foo", "bar1", 
>"bar2", "f" will give "f[oo]" but "b" will give "b[]"); in other 
>words, the "[]" after the point usually serves no purpose
>
>- likewise, with M-x there is a "()" after the point that usually 
>serves no purpose
>
As you can see in my patch I didn't change that part.

>- if I type "M-x icomplete-m", I see the following:
>
>M-x icomplete-m(ode)
>|
>
>where "|" represents the cursor (on the next line!)
>
I can't reproduce this, normally this happened when there was a missing
space before the newline in icomplete--list-indicators-format, but it is
in the patch....


>- when the current line after the prompt is too long the prompt disappears
>
Yes, set visual-line-mode -1 for this. Is the only solution so far and what I
will do in the setup probably.

>- when completing a unique completion with TAB the point is moved to 
>the next line
>
Same problem of icomplete--list-indicators-format. Did you applied the
patch cleanly?

>Gregory
>

Re: feature/icomplete-vertical

[Ergus]

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

Hi Gregory:

Please look at the attached patch where I use a modified version of your
line approach instead of pixels. Because I can't reproduce most of the
issues you mention.

This change is not permanent, but it is the only substantial difference
with your and my code; so lets try that and if this fixes your issue I
will check the pixels approach.

Thanks in advance,
Ergus

On Sat, Sep 19, 2020 at 10:30:13AM +0000, Gregory Heytings wrote:
>
>
>It seems to me that it does not work as well as my code, alas.  A few 
>points:
>
>- with C-x C-f the default completion, between angle brackets in the 
>original icomplete code, is not shown anymore, unless there is a 
>single candidate (for example in a directory with "foo", "bar1", 
>"bar2", "f" will give "f[oo]" but "b" will give "b[]"); in other 
>words, the "[]" after the point usually serves no purpose
>
>- likewise, with M-x there is a "()" after the point that usually 
>serves no purpose
>
>- if I type "M-x icomplete-m", I see the following:
>
>M-x icomplete-m(ode)
>|
>
>where "|" represents the cursor (on the next line!)
>
>- when the current line after the prompt is too long the prompt disappears
>
>- when completing a unique completion with TAB the point is moved to 
>the next line
>
>Gregory

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

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 4e546807b7..6905883cad 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-height nil)
+
+
 ;;;_* Initialization
 
 ;;;_ + Internal Variables
@@ -437,6 +460,52 @@ 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* (;; Needed for prospects-max-height-pixel
+         (prospects-rows (+ 1 (cl-count ?\n icomplete--list-indicators-format)))
+         limit prospects comp)
+
+    (while (and comps (not limit))
+      (setq comp (substring (pop comps) prefix-len)
+            prospects-rows (1+ prospects-rows))
+
+      (if (< prospects-rows icomplete--prospects-max-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 +515,32 @@ 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-height
+                  (let ((minibuffer-parameter (frame-parameter nil 'minibuffer)))
+                    (min (floor (cond
+                                 ((eq minibuffer-parameter t)
+                                  (if (floatp max-mini-window-height)
+                                      (* max-mini-window-height (frame-pixel-height))
+                                    (* max-mini-window-height (frame-char-height))))
+                                 ;; TODO : minibuffer-parameter can have other values.
+                                 ((eq minibuffer-parameter 'only)
+                                  (frame-pixel-height)))
+                                (* (cl-count ?\n icomplete--separator) (line-pixel-height)))
+                         (+ 2 icomplete-prospects-height)))))
+     ;; Default is horizontal
+     (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 +733,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 +757,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 +779,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 +802,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 +829,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 +845,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

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

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


>> - when the current line after the prompt is too long the prompt 
>> disappears
>
> Yes, set visual-line-mode -1 for this. Is the only solution so far and 
> what I will do in the setup probably.
>

No, it is not the only solution, my code handles that case without using 
visual-line-mode.  By the way, I just checked it (with your code) and 
apparently enabling visual-line-mode does not solve that problem.

>
> Did you applied the patch cleanly?
>

As far as I can see yes.  I already tried your code twice, did you try 
mine?  I attach the improved version which handles the minibuffer-only 
case.

Anyway, IMO you not are using the best approach on this project, you are 
trying to do too many things at once.  Completion mechanisms are a very 
complex thing, and I believe the best way is to do small incremental 
improvements with minimal changes, and to test them thoroughly before 
implementing the next improvement.

[-- Attachment #2: Type: text/x-diff, Size: 3722 bytes --]

--- a/icomplete.el	2020-09-01 10:14:22.000000000 +0000
+++ b/icomplete.el	2020-09-19 11:49:08.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-available-height 0 "...")
+
 (defcustom icomplete-compute-delay .3
   "Completions-computation stall, used only with large-number completions.
 See `icomplete-delay-completions-threshold'."
@@ -431,6 +435,15 @@
   "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-available-height
+	  (min icomplete-prospects-height
+	       (1- (floor (/ (if (eq (frame-parameter nil 'minibuffer) 'only)
+                                 (frame-native-height)
+                               (* (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 +714,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-available-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 +752,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

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 19, 2020 at 11:56:19AM +0000, Gregory Heytings via Emacs development discussions. wrote:
>
>>>- when the current line after the prompt is too long the prompt 
>>>disappears
>>
>>Yes, set visual-line-mode -1 for this. Is the only solution so far 
>>and what I will do in the setup probably.
>>
>
>No, it is not the only solution, my code handles that case without 
>using visual-line-mode.
>
If there is visual-lines -1 + truncate-lines that takes care of this in
a consistent way using the internal infrastructure. Why should I use
something else?

>By the way, I just checked it (with your 
>code) and apparently enabling visual-line-mode does not solve that 
>problem.

Did you added truncate-lines locally?

As I said before, the patch was preliminar and needs some fixes I do
slowly as I am not a lisper. I just diff my local changes to send them
to you

>>
>>Did you applied the patch cleanly?
>>
>
>As far as I can see yes.  I already tried your code twice, did you try 
>mine?  I attach the improved version which handles the minibuffer-only 
>case.
>

The changes indeed do only one thing at the time and it separated the
vertical from the horizontal completely. Which is IMO simpler that just
add if and else conditions here and there or hard code the mapcar
ignoring the user separator.

>Anyway, IMO you not are using the best approach on this project, you 
>are trying to do too many things at once.

You can do it if you want. No problem with that. Actually I tried to
convince the creator of the external package to do it and asked in the
mailing list some months ago if anyone wanted to do it and nobody
replied.

>Completion mechanisms are a 
>very complex thing, and I believe the best way is to do small 
>incremental improvements with minimal changes, and to test them 
>thoroughly before implementing the next improvement.

This is not a big change at all. This is just some cosmetics to an
output. I don't interact with he completion system and the code is not
complex at all.

Actually it is just 2 functions. The rest is the same just in a
different place. But actually your code and mine do exactly the same
just using different conditions.

If you see my first commits it was almost as simple as yours but then
reports started coming and I needed to fix them unless some of them
(like the disappearing prompt) were old bug already reported.

IMO this is an incremental change, but I don't try to compete for a
smaller patch but simpler and free of known issues. Like the ones you
are reporting.

Best
Ergus

Re: feature/icomplete-vertical

[Stefan Monnier]

> +                  (let ((minibuffer-parameter (frame-parameter nil 'minibuffer)))
> +                    (min (floor (cond
> +                                 ((eq minibuffer-parameter t)
> +                                  (if (floatp max-mini-window-height)
> +                                      (* max-mini-window-height (frame-pixel-height))
> +                                    (* max-mini-window-height (frame-char-height))))
> +                                 ;; TODO : minibuffer-parameter can have other values.
> +                                 ((eq minibuffer-parameter 'only)
> +                                  (frame-pixel-height)))
> +                                (* (cl-count ?\n icomplete--separator) (line-pixel-height)))
> +                         (+ 2 icomplete-prospects-height)))))

Why do we care about (cl-count ?\n icomplete--separator)?

I mean, the purpose of this code AFAIK is to *estimate* an upper bound
on the number of candidates that will be visible.  Depending on various
factors (special fonts, text-properties, you name it), this estimate
will at time be wrong, so in any case we will have to make sure that the
resulting behavior is still acceptable when we end up displaying more
(or less) candidates than fits.

Using 1 instead of (cl-count ?\n icomplete--separator) will err on the
side of putting too many rather than too few candidates, which should
have as sole negative impact a negligible performance hit.


        Stefan

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 19, 2020 at 09:00:05AM -0400, Stefan Monnier wrote:
>> +                  (let ((minibuffer-parameter (frame-parameter nil 'minibuffer)))
>> +                    (min (floor (cond
>> +                                 ((eq minibuffer-parameter t)
>> +                                  (if (floatp max-mini-window-height)
>> +                                      (* max-mini-window-height (frame-pixel-height))
>> +                                    (* max-mini-window-height (frame-char-height))))
>> +                                 ;; TODO : minibuffer-parameter can have other values.
>> +                                 ((eq minibuffer-parameter 'only)
>> +                                  (frame-pixel-height)))
>> +                                (* (cl-count ?\n icomplete--separator) (line-pixel-height)))
>> +                         (+ 2 icomplete-prospects-height)))))

Hi Stefan:
>
>Why do we care about (cl-count ?\n icomplete--separator)?
>
>I mean, the purpose of this code AFAIK is to *estimate* an upper bound
>on the number of candidates that will be visible.  Depending on various
>factors (special fonts, text-properties, you name it), this estimate
>will at time be wrong, so in any case we will have to make sure that the
>resulting behavior is still acceptable when we end up displaying more
>(or less) candidates than fits.
>
That's why I work with pixels. Because we can get the height of every
added line and do a better estimation... but we need to add the text
properties before the increment... which is a different topic because
ATM this is not happening.

>Using 1 instead of (cl-count ?\n icomplete--separator) will err on the
>side of putting too many rather than too few candidates, which should
>have as sole negative impact a negligible performance hit.
>
>
>        Stefan
>

I actually added the cl-count for the opposite reason; in case the user
adds a separator with more than one \n.

Because the idea was to change the condition to enable vertical mode for
when the separator contains at least a \n. Because otherwise the
horizontal formatting could be more accurate. So having both (format and
separator) may conflict unless we prioritize one of them; which IMO
should be the separator as it is the already existent variable.

My intention is to remove the icomplete-format variable completely and
use the right condition based only on the user's separator because
that's something that 2 users mentioned when I asked in this mailing
list to try the branch. This is needed to keep compatibility with the
external package.

OTOH the problem of adding too many candidates (IIUC) is the hiding
prompt issue which comes from there but also the ... will be hiden. So
we need to add the exact amount of lines as accurate as possible.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>
> I mean, the purpose of this code AFAIK is to *estimate* an upper bound 
> on the number of candidates that will be visible.  Depending on various 
> factors (special fonts, text-properties, you name it), this estimate 
> will at time be wrong, so in any case we will have to make sure that the 
> resulting behavior is still acceptable when we end up displaying more 
> (or less) candidates than fits.
>
> Using 1 instead of (cl-count ?\n icomplete--separator) will err on the 
> side of putting too many rather than too few candidates, which should 
> have as sole negative impact a negligible performance hit.
>

Alas an estimate is not enough in this case.  As Ergus just wrote, if 
there are too many candidates the prompt disappears, leaving the cursor at 
the beginning of the minibuffer, which is counterintuitive.  A simple 
example: after (setq max-mini-window-height 1), with "M-x a" the "M-x" 
prompt and the "a" disappear.  Likwise after (setq icomplete-separator 
"\n").  IOW, to avoid this it is necessary to compute the number of 
completion candidates that will be shown in the minibuffer, depending on 
its height, its font size, and so forth.

Re: feature/icomplete-vertical

[Ergus]

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

On Sat, Sep 19, 2020 at 01:59:18PM +0000, Gregory Heytings wrote:

Hi Stefan and Gregory:

Here it is so far my last patch about vertical only feature. Please feel
free to correct it.

I removed the format variable and now to get vertical we only need to
add at least a \n to the separator. With the simple cases I tested it
works.

Stefan:

As you requested for a single feature I will remove the remote branch
again and push one only with this commit.

Gregory:

I took the freedom to add one condition from your code and added it to
mine. (The one for empty candidates)

I didn't do this before because I was keeping the first candidate in the
same line so it made sense to keep the empty one. So after all I will
condition this condition in a future too; when I add options to
remove/change the [] and ().

The other one

(prospects-max (- icomplete--prospects-available-height (/ (point) (window-width))))

I think is good for continuation lines, but I am not sure if that's
better than truncate line and horizontally scroll because in that case
the proposed candidates will be very long as well and continuing them
will reduce their number to the half too. (I took ivy as a pattern for
this)

So I will wait for other users' suggestions about what they prefer OR
take this condition conditionally in case the user explicitly disables
the lines truncation.

Best,
Ergus


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

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 4e546807b7..c071691b07 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."
@@ -139,6 +140,19 @@ icomplete-minibuffer-setup-hook
   :group 'icomplete)
 
 
+(defvar-local icomplete--ellipsis nil
+  "Ellipsis symbol to indicate continuation.")
+
+(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--prospects nil)
+(defvar-local icomplete--match-braket nil)
+(defvar-local icomplete--prospects-max-height nil)
+(defvar-local icomplete--lines-per-candidate 0)
+
+
 ;;;_* Initialization
 
 ;;;_ + Internal Variables
@@ -437,6 +451,62 @@ 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* (;; Needed for prospects-max-height-pixel
+         (single-line-height (line-pixel-height))
+         ;; in general this should be done for every line
+         (line-height (* icomplete--lines-per-candidate single-line-height))
+         (prospects-rows (* (1+ (cl-count ?\n icomplete--list-indicators-format))
+                            single-line-height))
+         limit prospects comp comp-len)
+
+    (while (and comps (not limit))
+      (setq comp (substring (pop comps) prefix-len)
+            comp-len (> (length comp) 0))
+
+      (if comp-len
+           (setq prospects-rows (+ prospects-rows line-height)))
+
+      (if (< prospects-rows icomplete--prospects-max-height)
+          (if comp-len
+              (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))
+
+      (when (> (length comp) 0)
+          (setq 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 +516,29 @@ 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 icomplete--ellipsis (if (char-displayable-p ?…) "…" "...")
+          icomplete--lines-per-candidate (cl-count ?\n icomplete-separator))
+
+    (if (> icomplete--lines-per-candidate 0)
+        (setq-local icomplete--list-indicators-format " \n%s"
+                    icomplete--prospects 'icomplete--vertical-prospects
+                    icomplete--prospects-max-height
+                    (let ((minibuffer-parameter (frame-parameter nil 'minibuffer)))
+                      (min (cond
+                            ((eq minibuffer-parameter t)
+                             (if (floatp max-mini-window-height)
+                                 (* max-mini-window-height (frame-pixel-height))
+                               (* max-mini-window-height (frame-char-height))))
+                            ;; TODO : minibuffer-parameter can have other values.
+                            ((eq minibuffer-parameter 'only)
+                             (frame-pixel-height)))
+                           (* (+ 2 icomplete-prospects-height) (line-pixel-height)))))
+
+      (setq-local icomplete--list-indicators-format "{%s}"
+                  icomplete--prospects 'icomplete--horizontal-prospects))))
+
 
 (defvar icomplete--in-region-buffer nil)
 
@@ -639,8 +731,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 +755,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 +777,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 +800,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 +827,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 +843,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

Re: feature/icomplete-vertical

[Stefan Monnier]

> That's why I work with pixels.

Yes, that's fine.

> Because we can get the height of every added line and do a better
> estimation... but we need to add the text properties before the
> increment... which is a different topic because ATM this is
> not happening.

You don't want to go there.  You end up having to reimplement the job
done by the redisplay.  Instead, just output "a bit more" than
necessary, and then ask the redisplay code whether it fits (so you
reuse the redisplay code instead of reimplementing it).

>>Using 1 instead of (cl-count ?\n icomplete--separator) will err on the
>>side of putting too many rather than too few candidates, which should
>>have as sole negative impact a negligible performance hit.
> I actually added the cl-count for the opposite reason; in case the user
> adds a separator with more than one \n.

That's not the opposite reason, that's the case I had in mind.  By using
1 instead of (cl-count ?\n icomplete--separator) you'll be
over-estimating (by the factor that would have been returned by
`cl-count`) the number of candidates that can fit when (cl-count ?\n
icomplete--separator) is greater than 1.

> So we need to add the exact amount of lines as accurate as possible.

I *strongly* recommend you design the behavior under the assumption that
it's OK if there are a few more lines in the (mini)buffer than are
actually visible.


        Stefan

Re: feature/icomplete-vertical

[Stefan Monnier]

> Alas an estimate is not enough in this case.  As Ergus just wrote, if there
>  are too many candidates the prompt disappears, leaving the cursor at the
>  beginning of the minibuffer, which is counterintuitive.  A simple example:
>  after (setq max-mini-window-height 1), with "M-x a" the "M-x" prompt and
>  the "a" disappear.

That can (and should) be fixed without having to reduce the number of
candidates inserted in the (mini)buffer.


        Stefan

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 19, 2020 at 11:37:07AM -0400, Stefan Monnier wrote:
>> Alas an estimate is not enough in this case.  As Ergus just wrote, if there
>>  are too many candidates the prompt disappears, leaving the cursor at the
>>  beginning of the minibuffer, which is counterintuitive.  A simple example:
>>  after (setq max-mini-window-height 1), with "M-x a" the "M-x" prompt and
>>  the "a" disappear.
>
>That can (and should) be fixed without having to reduce the number of
>candidates inserted in the (mini)buffer.
>
>
>        Stefan
>
It will be great if you give me an idea about how to do that. Because
all the code seems to be designed with this assumption in mind. And also
ido and the others had similar issues with this.

I mean, if we can go around the prompt issue all this will be much
easier.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> Alas an estimate is not enough in this case.  As Ergus just wrote, if 
>> there are too many candidates the prompt disappears, leaving the cursor 
>> at the beginning of the minibuffer, which is counterintuitive.  A 
>> simple example: after (setq max-mini-window-height 1), with "M-x a" the 
>> "M-x" prompt and the "a" disappear.
>
> That can (and should) be fixed without having to reduce the number of 
> candidates inserted in the (mini)buffer.
>

But how?  What should the algorithm be?  Put a too long list of candidates 
in the minibuffer, redisplay, check if the resulting height is larger than 
then available height (with window-text-pixel-size?), if so remove a 
candidate, redisplay, ...?  Hmmm, no, you just wrote "without having to 
reduce the number of candidates inserted in the (mini)buffer", so it must 
be something else?

Re: feature/icomplete-vertical

[Stefan Monnier]

>>That can (and should) be fixed without having to reduce the number of
>>candidates inserted in the (mini)buffer.
> It will be great if you give me an idea about how to do that.

You need to figure out why the redisplay decides to hide the prompt
rather than some other part of the (mini)buffer.

Usually, the deciding factor is the position of `point` (the redisplay
tries hard to always keep `point` visible).


        Stefan

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>>> That can (and should) be fixed without having to reduce the number of 
>>> candidates inserted in the (mini)buffer.
>>
>> It will be great if you give me an idea about how to do that.
>
> You need to figure out why the redisplay decides to hide the prompt 
> rather than some other part of the (mini)buffer.
>
> Usually, the deciding factor is the position of `point` (the redisplay 
> tries hard to always keep `point` visible).
>

In this case it is not, apparently at least.  As you can see with the 
(setq max-mini-window-height 1) M-x a example, the typical effect is to 
put the point at the first position of the window (top left), thereby 
hiding what precedes (the prompt and the characters typed so far).

Re: feature/icomplete-vertical

[Stefan Monnier]

>>>> That can (and should) be fixed without having to reduce the number of
>>>> candidates inserted in the (mini)buffer.
>>> It will be great if you give me an idea about how to do that.
>> You need to figure out why the redisplay decides to hide the prompt rather
>> than some other part of the (mini)buffer.
>> Usually, the deciding factor is the position of `point` (the redisplay
>> tries hard to always keep `point` visible).
> In this case it is not, apparently at least.  As you can see with the (setq
> max-mini-window-height 1) M-x a example, the typical effect is to put the
> point at the first position of the window (top left), thereby hiding what
> precedes (the prompt and the characters typed so far).

Yes, hence the question is "why".  I suspect it has to do with the way
we insert the extra text and by teaking it we may work around
this problem.

We could/should also consider this as a problem in the redisplay, so
maybe you should file a bug report for it.

In any case, your example shows that it's not a new problem introduced
by icomplete-vertical, so fixing it should likely be orthogonal to the
addition of icomplete-vertical.


        Stefan

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>
> Yes, hence the question is "why".  I suspect it has to do with the way 
> we insert the extra text and by teaking it we may work around this 
> problem.
>

Do you have pointers where one might try to find the "why"?  xdisp.c?

>
> We could/should also consider this as a problem in the redisplay, so 
> maybe you should file a bug report for it.
>

It's a well-known problem, with already several bugs filed.

>
> In any case, your example shows that it's not a new problem introduced 
> by icomplete-vertical, so fixing it should likely be orthogonal to the 
> addition of icomplete-vertical.
>

Indeed.  In fact AFAICT so far everyone tries to work-around that 
bug/feature/limitation/... by doing what we were trying to do, which 
amounts (as you wrote) to redoing or approximating what redisplay does.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sat, 19 Sep 2020 16:19:10 +0000
> cc: Ergus <spacibba@aol.com>, Eli Zaretskii <eliz@gnu.org>, casouri@gmail.com, 
>  emacs-devel@gnu.org
> From: Gregory Heytings via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> > We could/should also consider this as a problem in the redisplay, so 
> > maybe you should file a bug report for it.
> >
> 
> It's a well-known problem, with already several bugs filed.

Which problems, and what bugs, please?

Re: feature/icomplete-vertical

[Ergus]

On Sat, Sep 19, 2020 at 07:58:35PM +0300, Eli Zaretskii wrote:
>> Date: Sat, 19 Sep 2020 16:19:10 +0000
>> cc: Ergus <spacibba@aol.com>, Eli Zaretskii <eliz@gnu.org>, casouri@gmail.com,
>>  emacs-devel@gnu.org
>> From: Gregory Heytings via "Emacs development discussions." <emacs-devel@gnu.org>
>>
>> > We could/should also consider this as a problem in the redisplay, so
>> > maybe you should file a bug report for it.
>> >
>>
>> It's a well-known problem, with already several bugs filed.
>
>Which problems, and what bugs, please?
>

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379

RE: feature/icomplete-vertical

[Drew Adams]

[Adding back emacs-devel to the cc list...]

> TBH, it seems to me that using icomplete with a
> minibuffer-only frame does not have much sense.

Why would you think so?

I've been using Icomplete since it existed.  And
I use a standalone minibuffer frame.

I've even - since 1995 - provided a library of
Icomplete enhancements, `icomplete+.el'.  They're
independent of whether someone uses a standalone
minibuffer frame.

https://www.emacswiki.org/emacs/IcompleteMode#IcompleteModePlus

https://www.emacswiki.org/emacs/download/icomplete%2b.el

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sat, 19 Sep 2020 19:06:45 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Gregory Heytings <ghe@sdf.org>, casouri@gmail.com,
> 	monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> >> It's a well-known problem, with already several bugs filed.
> >
> >Which problems, and what bugs, please?
> >
> 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379

Those are fixed, AFAICT.

Re: feature/icomplete-vertical

[Ergus]

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

But why are se having the same issue then?

On September 19, 2020 7:21:12 PM GMT+02:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sat, 19 Sep 2020 19:06:45 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: Gregory Heytings <ghe@sdf.org>, casouri@gmail.com,
>> 	monnier@iro.umontreal.ca, emacs-devel@gnu.org
>> 
>> >> It's a well-known problem, with already several bugs filed.
>> >
>> >Which problems, and what bugs, please?
>> >
>> 
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379
>
>Those are fixed, AFAICT.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[-- Attachment #2: Type: text/html, Size: 1441 bytes --]

Re: feature/icomplete-vertical

[Dmitry Gutov]

On 19.09.2020 20:21, Eli Zaretskii wrote:
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
>> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379
> Those are fixed, AFAICT.

You might want to read the comments to remember the discussions.

The bugs are fixed in an ad-hoc way, with the general problem left intact.

The present issue is exactly the one I described in comment 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379#61. You called it a 
"future bug".

Re: feature/icomplete-vertical

[Stefan Monnier]

> But why are se having the same issue then?

Because the underlying problem is not actually fixed.
See bug#43519 where I'm trying to focus on the underlying problem.


        Stefan

RE: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> TBH, it seems to me that using icomplete with a minibuffer-only frame 
>> does not have much sense.
>
> Why would you think so?
>
> I've been using Icomplete since it existed.  And I use a standalone 
> minibuffer frame.
>

My sentence was indeed not clear enough.  What I meant is: it seems to me 
that using *vertical* icomplete with a minibuffer-only frame does not make 
much sense.

RE: feature/icomplete-vertical

[Drew Adams]

> >> TBH, it seems to me that using icomplete with a minibuffer-only frame
> >> does not have much sense.
> >
> > Why would you think so?
> >
> > I've been using Icomplete since it existed.  And I use a standalone
> > minibuffer frame.
> 
> My sentence was indeed not clear enough.  What I meant is: it seems to me
> that using *vertical* icomplete with a minibuffer-only frame does not make
> much sense.

That could be.

A frame can be resized of course - like a window.
I don't see why what applies to a window, in this
respect wouldn't also apply to a minibuffer-only
frame.

But I won't be using my standalone minibuffer frame
with verticle icomplete, so I have nothing special
to say about that.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sat, 19 Sep 2020 22:47:48 +0200
> CC: ghe@sdf.org,casouri@gmail.com,monnier@iro.umontreal.ca
> From: Ergus <spacibba@aol.com>
> 
> But why are se having the same issue then?

AFAIU, you are asking the display engine to do the impossible.

But let's continue this discussion in the bug that Stefan filed.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Cc: ghe@sdf.org, casouri@gmail.com, monnier@iro.umontreal.ca,
>  emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 20 Sep 2020 00:40:55 +0300
> 
> On 19.09.2020 20:21, Eli Zaretskii wrote:
> >> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24293
> >> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379
> > Those are fixed, AFAICT.
> 
> You might want to read the comments to remember the discussions.
> 
> The bugs are fixed in an ad-hoc way, with the general problem left intact.

You may wish re-reading the discussions, which I hope will clarify
that the fix was where it should be, and that the general problem is
an illusion, probably due to a desire to have application-level
problems go away by some magic.

> The present issue is exactly the one I described in comment 
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=39379#61. You called it a 
> "future bug".

There's a new bug#43519 which discusses this situation (if you think
it is exactly the one you described there -- I think you are wrong),
so if you are interested in what I think about it, please read that
discussion and comment there.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

Hi Ergus,

I don't know if you're following the discussion on bug#43519, but there is 
in fact an almost trivial way to implement icomplete-vertical:

(setq icomplete-separator "\n")
(setq icomplete-prospects-height 10)
(add-hook 'icomplete-minibuffer-setup-hook (function (lambda () (setq resize-mini-windows nil) (enlarge-window icomplete-prospects-height))))

It needs some tweaking, but it works.

Re: feature/icomplete-vertical

[João Távora]

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

I'm late to the party here, but I'd just like to add that
Gregory's solution based on advice can easily be be integrated into
icomplete-mode directly.  I do like that it's such a simple solution,
doesn't include new defcustoms, modes and such.  I didn't have
a in-depth look, but it looks like your solution Ergus, is quite a bit
more complex.  Why?  What extras does it bring to the table?
And are we sure we need those extras in the first version of this?

Or am I missing something and is Gregory's solution fundamentally
unusable?

My 2c,
João

On Sat, Sep 12, 2020 at 2:33 PM Ergus <spacibba@aol.com> wrote:

> On Sat, Sep 12, 2020 at 01:10:57PM +0000, Gregory Heytings wrote:
> >
> >>
> >>>If there was a built-in vertical mode it would be better / more
> >>>intuitive.
> >>
> >>Could you try the branch feature/icomplete-vertical? I need some
> >>testers before adding it to master.
> >>
> >
> >Alas no, I have been using the following to have icomplete-vertical
> >for quite some time, it works perfectly well, so I don't see why a
> >more complex implementation would be necessary.
> >
> >(setq icomplete-prospects-height 6)
> >(setq icomplete-separator "\n")
> >(defun icomplete-vertical-minibuffer-setup ()
> >  (setq truncate-lines t)
> >  (setq-local completion-ignore-case t)
> >  (setq-local read-file-name-completion-ignore-case t)
> >  (setq-local read-buffer-completion-ignore-case t)
> >  (setq icomplete-hide-common-prefix nil))
> >(add-hook 'icomplete-minibuffer-setup-hook
> #'icomplete-vertical-minibuffer-setup)
> >(defun icomplete-vertical-reformat-completions (completions)
> >  (save-match-data
> >    (let ((cnp (substring-no-properties completions)))
> >      (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}"
> cnp)
> >          (format "%s \n%s"
> >                  (or (match-string 1 cnp) "")
> >                  (replace-regexp-in-string "^" (make-string
> (current-column) ? ) (match-string 2 cnp)))
> >        cnp))))
> >(defun icomplete-vertical-adjust-minibuffer-height (completions)
> >  (let* ((comp (icomplete-vertical-reformat-completions completions))
> >         (complen (length (split-string comp "\n"))))
> >    (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1-
> (window-height)))))
> >    comp))
> >(advice-add 'icomplete-completions :filter-return
> #'icomplete-vertical-adjust-minibuffer-height)
>
> 1) Internal functionalities try not to use advises.
> 2) The branch is not actually more complex, it just generates the
> formatted vertical output form the beginning.
> 3) It does more or less the same you are doing but with a simpler
> config:
>
> (icomplete-mode t)
> (icomplete-format 'vertical)
>
> 4) We add arrow bindings to move
> 5) Add completion matching faces is also coming.
>
>

-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 3764 bytes --]

Re: feature/icomplete-vertical

[Ergus]

On Sun, Sep 20, 2020 at 10:47:44AM +0000, Gregory Heytings via Emacs development discussions. wrote:
>
>Hi Ergus,
>
>I don't know if you're following the discussion on bug#43519, but 
>there is in fact an almost trivial way to implement 
>icomplete-vertical:
>
>(setq icomplete-separator "\n")
>(setq icomplete-prospects-height 10)

>(add-hook 'icomplete-minibuffer-setup-hook (function (lambda () (setq resize-mini-windows nil) (enlarge-window icomplete-prospects-height))))
>
>It needs some tweaking, but it works.
>
That's indeed similar to what the external package does with some extras
(remove the {} and add the extra line after the {, truncate the long
ones, resize automatically when less candidates based on an option).

https://github.com/oantolin/icomplete-vertical

Then the only thin we need is (setq resize-mini-windows nil) and doesn't
need to be in the icomplete-minibuffer-setup-hook.

The problem was that I tried to avoid calling enlarge-window and I
thought there was a good reason to do that... my bad.

Re: feature/icomplete-vertical

[Ergus]

Hi Joao:

The problem was that I was trying to avoid calling enlarge-window from
icomplete and rely on the internal resize mechanism (maybe that was the
main error) to avoid calculating the height in general. And of course
the prompt issue that came out when using different fonts as a
surprise. I tried to solve that without the enlarge to respect also the
resize-mini-windows either when t or grow-only.

Initially I separated icomplete--prospects only, but then all the height
calculation complexity came and that's why I ended with the complex
approach. I didn't know that the prompt issue disappeared with

(setq resize-mini-windows nil)

What I wonder about this is the potential conflicts when another package
or function relies on resize-mini-windows. And that the ... is not shown
to indicate that there are more candidates.

OTOH: My main goal (more than the vertical) was to implement a sort of
zsh like completion. (similar to what I did for *Completions* buffer) so
at least I will request to avoid using only and advise.


On Sun, Sep 20, 2020 at 11:55:52AM +0100, Jo�o T�vora wrote:
>I'm late to the party here, but I'd just like to add that
>Gregory's solution based on advice can easily be be integrated into
>icomplete-mode directly.  I do like that it's such a simple solution,
>doesn't include new defcustoms, modes and such.  I didn't have
>a in-depth look, but it looks like your solution Ergus, is quite a bit
>more complex.  Why?  What extras does it bring to the table?
>And are we sure we need those extras in the first version of this?
>
>Or am I missing something and is Gregory's solution fundamentally
>unusable?
>
>My 2c,
>Jo�o
>
>On Sat, Sep 12, 2020 at 2:33 PM Ergus <spacibba@aol.com> wrote:
>
>> On Sat, Sep 12, 2020 at 01:10:57PM +0000, Gregory Heytings wrote:
>> >
>> >>
>> >>>If there was a built-in vertical mode it would be better / more
>> >>>intuitive.
>> >>
>> >>Could you try the branch feature/icomplete-vertical? I need some
>> >>testers before adding it to master.
>> >>
>> >
>> >Alas no, I have been using the following to have icomplete-vertical
>> >for quite some time, it works perfectly well, so I don't see why a
>> >more complex implementation would be necessary.
>> >
>> >(setq icomplete-prospects-height 6)
>> >(setq icomplete-separator "\n")
>> >(defun icomplete-vertical-minibuffer-setup ()
>> >  (setq truncate-lines t)
>> >  (setq-local completion-ignore-case t)
>> >  (setq-local read-file-name-completion-ignore-case t)
>> >  (setq-local read-buffer-completion-ignore-case t)
>> >  (setq icomplete-hide-common-prefix nil))
>> >(add-hook 'icomplete-minibuffer-setup-hook
>> #'icomplete-vertical-minibuffer-setup)
>> >(defun icomplete-vertical-reformat-completions (completions)
>> >  (save-match-data
>> >    (let ((cnp (substring-no-properties completions)))
>> >      (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}"
>> cnp)
>> >          (format "%s \n%s"
>> >                  (or (match-string 1 cnp) "")
>> >                  (replace-regexp-in-string "^" (make-string
>> (current-column) ? ) (match-string 2 cnp)))
>> >        cnp))))
>> >(defun icomplete-vertical-adjust-minibuffer-height (completions)
>> >  (let* ((comp (icomplete-vertical-reformat-completions completions))
>> >         (complen (length (split-string comp "\n"))))
>> >    (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1-
>> (window-height)))))
>> >    comp))
>> >(advice-add 'icomplete-completions :filter-return
>> #'icomplete-vertical-adjust-minibuffer-height)
>>
>> 1) Internal functionalities try not to use advises.
>> 2) The branch is not actually more complex, it just generates the
>> formatted vertical output form the beginning.
>> 3) It does more or less the same you are doing but with a simpler
>> config:
>>
>> (icomplete-mode t)
>> (icomplete-format 'vertical)
>>
>> 4) We add arrow bindings to move
>> 5) Add completion matching faces is also coming.
>>
>>
>
>-- 
>Jo�o T�vora

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sun, 20 Sep 2020 15:04:35 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Gregory Heytings <ghe@sdf.org>, Eli Zaretskii <eliz@gnu.org>,
> 	Yuan Fu <casouri@gmail.com>, emacs-devel <emacs-devel@gnu.org>
> 
> Initially I separated icomplete--prospects only, but then all the height
> calculation complexity came and that's why I ended with the complex
> approach. I didn't know that the prompt issue disappeared with
> 
> (setq resize-mini-windows nil)
> 
> What I wonder about this is the potential conflicts when another package
> or function relies on resize-mini-windows. And that the ... is not shown
> to indicate that there are more candidates.

resize-mini-windows is a user option, so resetting it globally is
almost certainly a non-starter.

Re: feature/icomplete-vertical

[Ergus]

On Sun, Sep 20, 2020 at 04:15:41PM +0300, Eli Zaretskii wrote:
>> Date: Sun, 20 Sep 2020 15:04:35 +0200
>> From: Ergus <spacibba@aol.com>
>> Cc: Gregory Heytings <ghe@sdf.org>, Eli Zaretskii <eliz@gnu.org>,
>> 	Yuan Fu <casouri@gmail.com>, emacs-devel <emacs-devel@gnu.org>
>>
>> Initially I separated icomplete--prospects only, but then all the height
>> calculation complexity came and that's why I ended with the complex
>> approach. I didn't know that the prompt issue disappeared with
>>
>> (setq resize-mini-windows nil)
>>
>> What I wonder about this is the potential conflicts when another package
>> or function relies on resize-mini-windows. And that the ... is not shown
>> to indicate that there are more candidates.
>
>resize-mini-windows is a user option, so resetting it globally is
>almost certainly a non-starter.

The other thing I don;t like about this solution is that it does the
resize in advance.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> Initially I separated icomplete--prospects only, but then all the 
>> height calculation complexity came and that's why I ended with the 
>> complex approach. I didn't know that the prompt issue disappeared with
>>
>> (setq resize-mini-windows nil)
>>
>> What I wonder about this is the potential conflicts when another 
>> package or function relies on resize-mini-windows. And that the ... is 
>> not shown to indicate that there are more candidates.
>
> resize-mini-windows is a user option, so resetting it globally is almost 
> certainly a non-starter.
>

The three lines I sent:

(setq icomplete-separator "\n")
(setq icomplete-prospects-height 10)
(add-hook 'icomplete-minibuffer-setup-hook (function (lambda () (setq resize-mini-windows nil) (enlarge-window icomplete-prospects-height))))

had no other purpose except demonstrating that (setq resize-mini-windows 
nil) solves the problem of disappearing prompts.  Of course I do not 
consider that these lines should be blindly used, in fact I wrote "It 
needs some tweaking, but it works."  Indeed one should do something with 
user preferences, and use a setq-local.  The point is that doing this is 
much simpler than trying to calculate the height of the completion 
candidate list, which amount (as Stefan wrote) to redoing what redisplay 
does.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sun, 20 Sep 2020 14:07:45 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: Ergus <spacibba@aol.com>, casouri@gmail.com, joaotavora@gmail.com,
>         emacs-devel@gnu.org
> 
> 
> > resize-mini-windows is a user option, so resetting it globally is almost 
> > certainly a non-starter.
> >
> 
> The three lines I sent:
> 
> (setq icomplete-separator "\n")
> (setq icomplete-prospects-height 10)
> (add-hook 'icomplete-minibuffer-setup-hook (function (lambda () (setq resize-mini-windows nil) (enlarge-window icomplete-prospects-height))))
> 
> had no other purpose except demonstrating that (setq resize-mini-windows 
> nil) solves the problem of disappearing prompts.  Of course I do not 
> consider that these lines should be blindly used

Please note that I wasn't replying to your proposal, I was replying to
Jimmy's.

> trying to calculate the height of the completion candidate list,
> which amount (as Stefan wrote) to redoing what redisplay does.

I don't agree with Stefan, if this interpretation of what he said is
correct.  We have window-text-pixel-size to measure the size of text
on display without redoing what redisplay does.

Re: feature/icomplete-vertical

[Ergus]

On Sun, Sep 20, 2020 at 02:07:45PM +0000, Gregory Heytings wrote:
>
>>>Initially I separated icomplete--prospects only, but then all the 
>>>height calculation complexity came and that's why I ended with the 
>>>complex approach. I didn't know that the prompt issue disappeared 
>>>with
>>>
>>>(setq resize-mini-windows nil)
>>>
>>>What I wonder about this is the potential conflicts when another 
>>>package or function relies on resize-mini-windows. And that the 
>>>... is not shown to indicate that there are more candidates.
>>
>>resize-mini-windows is a user option, so resetting it globally is 
>>almost certainly a non-starter.
>>
>
>The three lines I sent:
>
>(setq icomplete-separator "\n")
>(setq icomplete-prospects-height 10)
>(add-hook 'icomplete-minibuffer-setup-hook (function (lambda () (setq resize-mini-windows nil) (enlarge-window icomplete-prospects-height))))
>
>had no other purpose except demonstrating that (setq 
>resize-mini-windows nil) solves the problem of disappearing prompts.  
>Of course I do not consider that these lines should be blindly used, 
>in fact I wrote "It needs some tweaking, but it works."  Indeed one 
>should do something with user preferences, and use a setq-local.  The 
>point is that doing this is much simpler than trying to calculate the 
>height of the completion candidate list, which amount (as Stefan 
>wrote) to redoing what redisplay does.

Hi Gregory:

When the display engine have resize-mini-windows not nil it will try to
resize the windows after an action in minibuffer. This happens
automatically and setting it as local or so will still overwrite the
user choice.

What we need is to inhibit that behavior while icomplete is active We
could add a hook to minibuffer-exit-hook to reset resize-mini-windows to
it's original value (and remove itself)

Something like (untested)

(setq icomplete-separator "\n")
(setq icomplete-prospects-height 10)
(defvar icomplete-saved-resize-mini-windows nil)

(defun icomplete-minibuffer-exit-hook ()
     (setq resize-mini-windows icomplete-saved-resize-mini-windows)
     (remove-hook 'minibuffer-exit-hook #'icomplete-minibuffer-exit-hook))

(add-hook 'icomplete-minibuffer-setup-hook
           '(lambda ()
               (setq icomplete-saved-resize-mini-windows resize-mini-windows)
               (setq resize-mini-windows nil)
               (enlarge-window icomplete-prospects-height)
               (add-hook 'minibuffer-exit-hook #icomplete-minibuffer-exit-hook)))

Should probably work. (But this still looks like a workaround more than
a solution to me.)

Re: feature/icomplete-vertical

[Ergus]

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

On Sun, Sep 20, 2020 at 05:28:50PM +0300, Eli Zaretskii wrote:
>
>Please note that I wasn't replying to your proposal, I was replying to
>Jimmy's.
>
>> trying to calculate the height of the completion candidate list,
>> which amount (as Stefan wrote) to redoing what redisplay does.
>
>I don't agree with Stefan, if this interpretation of what he said is
>correct.  We have window-text-pixel-size to measure the size of text
>on display without redoing what redisplay does.

Hi Eli that's what I have been doing so far, but the approach looks a
bit too complex. It is actually not, but I understand that a better
simpler one is desirable.

You can look at the attached patch.

Any way it seems to me a bit buggy that (only) the prompt disappears in
this situation and how it behaves when we use the arrows. In general
without this issue we should be allowed to set \n separator and it
basically should work out of the box without requiring my code at all.

Best,
Ergus

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

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 4e546807b7..c071691b07 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."
@@ -139,6 +140,19 @@ icomplete-minibuffer-setup-hook
   :group 'icomplete)
 
 
+(defvar-local icomplete--ellipsis nil
+  "Ellipsis symbol to indicate continuation.")
+
+(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--prospects nil)
+(defvar-local icomplete--match-braket nil)
+(defvar-local icomplete--prospects-max-height nil)
+(defvar-local icomplete--lines-per-candidate 0)
+
+
 ;;;_* Initialization
 
 ;;;_ + Internal Variables
@@ -437,6 +451,62 @@ 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* (;; Needed for prospects-max-height-pixel
+         (single-line-height (line-pixel-height))
+         ;; in general this should be done for every line
+         (line-height (* icomplete--lines-per-candidate single-line-height))
+         (prospects-rows (* (1+ (cl-count ?\n icomplete--list-indicators-format))
+                            single-line-height))
+         limit prospects comp comp-len)
+
+    (while (and comps (not limit))
+      (setq comp (substring (pop comps) prefix-len)
+            comp-len (> (length comp) 0))
+
+      (if comp-len
+           (setq prospects-rows (+ prospects-rows line-height)))
+
+      (if (< prospects-rows icomplete--prospects-max-height)
+          (if comp-len
+              (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))
+
+      (when (> (length comp) 0)
+          (setq 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 +516,29 @@ 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 icomplete--ellipsis (if (char-displayable-p ?…) "…" "...")
+          icomplete--lines-per-candidate (cl-count ?\n icomplete-separator))
+
+    (if (> icomplete--lines-per-candidate 0)
+        (setq-local icomplete--list-indicators-format " \n%s"
+                    icomplete--prospects 'icomplete--vertical-prospects
+                    icomplete--prospects-max-height
+                    (let ((minibuffer-parameter (frame-parameter nil 'minibuffer)))
+                      (min (cond
+                            ((eq minibuffer-parameter t)
+                             (if (floatp max-mini-window-height)
+                                 (* max-mini-window-height (frame-pixel-height))
+                               (* max-mini-window-height (frame-char-height))))
+                            ;; TODO : minibuffer-parameter can have other values.
+                            ((eq minibuffer-parameter 'only)
+                             (frame-pixel-height)))
+                           (* (+ 2 icomplete-prospects-height) (line-pixel-height)))))
+
+      (setq-local icomplete--list-indicators-format "{%s}"
+                  icomplete--prospects 'icomplete--horizontal-prospects))))
+
 
 (defvar icomplete--in-region-buffer nil)
 
@@ -639,8 +731,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 +755,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 +777,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 +800,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 +827,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 +843,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

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sun, 20 Sep 2020 16:49:03 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, casouri@gmail.com, joaotavora@gmail.com,
> 	emacs-devel@gnu.org
> 
> (setq icomplete-separator "\n")
> (setq icomplete-prospects-height 10)
> (defvar icomplete-saved-resize-mini-windows nil)
> 
> (defun icomplete-minibuffer-exit-hook ()
>      (setq resize-mini-windows icomplete-saved-resize-mini-windows)
>      (remove-hook 'minibuffer-exit-hook #'icomplete-minibuffer-exit-hook))
> 
> (add-hook 'icomplete-minibuffer-setup-hook
>            '(lambda ()
>                (setq icomplete-saved-resize-mini-windows resize-mini-windows)
>                (setq resize-mini-windows nil)
>                (enlarge-window icomplete-prospects-height)
>                (add-hook 'minibuffer-exit-hook #icomplete-minibuffer-exit-hook)))

Like I said previously: beware, the temporary resize-mini-windows
setting should be in effect when redisplay of the mini-window runs,
and I'm not quite sure that happens when your lambda-function is
called.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sun, 20 Sep 2020 17:04:54 +0200
> From: Ergus <spacibba@aol.com>
> Cc: Gregory Heytings <ghe@sdf.org>, casouri@gmail.com, joaotavora@gmail.com,
> 	emacs-devel@gnu.org
> 
> >I don't agree with Stefan, if this interpretation of what he said is
> >correct.  We have window-text-pixel-size to measure the size of text
> >on display without redoing what redisplay does.
> 
> Hi Eli that's what I have been doing so far, but the approach looks a
> bit too complex.

I don't see a call to window-text-pixel-size.  I see calls to
string-width, which is less accurate.

> It is actually not, but I understand that a better
> simpler one is desirable.

"An implementation should be as simple as possible, but no simpler."

> Any way it seems to me a bit buggy that (only) the prompt disappears in
> this situation

Not the prompt disappears, the entire text of the buffer disappears
from view.  What is left is the text of the after-string from an
overlay.  That's because the window-start of the mini-window is set at
EOB of the minibuffer.

Re: feature/icomplete-vertical

[Ergus]

>
>Not the prompt disappears, the entire text of the buffer disappears
>from view.  What is left is the text of the after-string from an
>overlay.  That's because the window-start of the mini-window is set at
>EOB of the minibuffer.
>
Sorry the after-string concept is not clear for me yet :( and the
documentation was not clear either, so I can't understand the issue;
that's why I didn't participate in the bug thread.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> trying to calculate the height of the completion candidate list, which 
>> amount (as Stefan wrote) to redoing what redisplay does.
>
> I don't agree with Stefan, if this interpretation of what he said is 
> correct.  We have window-text-pixel-size to measure the size of text on 
> display without redoing what redisplay does.
>

Do you mean that every function which wants to display something in the 
minibuffer should use window-text-pixel-size to create a text that fits in 
the minibuffer without hiding the prompt?

And that this is better than only expecting such functions to approximate 
the available space, and to rely on the fact that if the text is too large 
the display engine will hide the end of the text instead of the prompt?

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Sun, 20 Sep 2020 17:09:46 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: spacibba@aol.com, emacs-devel@gnu.org, casouri@gmail.com,
>         joaotavora@gmail.com
> 
> >> trying to calculate the height of the completion candidate list, which 
> >> amount (as Stefan wrote) to redoing what redisplay does.
> >
> > I don't agree with Stefan, if this interpretation of what he said is 
> > correct.  We have window-text-pixel-size to measure the size of text on 
> > display without redoing what redisplay does.
> >
> 
> Do you mean that every function which wants to display something in the 
> minibuffer should use window-text-pixel-size to create a text that fits in 
> the minibuffer without hiding the prompt?

I didn't say that every function needs to do it, or anything to that
effect.  I was responding to the claim that measuring the size of text
on display needs to redo what redisplay does.

> And that this is better than only expecting such functions to approximate 
> the available space, and to rely on the fact that if the text is too large 
> the display engine will hide the end of the text instead of the prompt?

I didn't say that, either, not in such a general form, anyway.

I explained the state of affairs for the issue at hand in the bug
report; if something there is unclear, I will happily answer specific
questions regarding the issue we are facing.

Re: feature/icomplete-vertical

[Ergus]

Hi:

I made some corrections to simplify the icomplete-vertical feature
branch and pushed (forced) some days ago. (sorry for that, I should have
used a scratch branch instead)

Now the icomplete-format variable is removed and the user only needs to
add at least one '\n' to the separator. I also use
window-text-pixel-size to correct the issue with long lines but still
perform the height calculations in pixels to properly show the ellipsis
as Eli recommended.

I keep the funcall approach because IMO it is cleaner, and reduces
unneeded and redundant things. (And because it is compatible with
something else I am working in)

If some of the previous testers could give it a second try. When it is
fine I will add some documentation and merge in master.

BTW: If someone could give a look to the completions-highlight feature
branch too and make recommendations, report issues?

Best,
Ergus

Re: feature/icomplete-vertical

[jixiuf]

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



> 2020年10月2日 上午12:48，Ergus <spacibba@aol.com> 写道：
> 
> Hi:
> 
> I made some corrections to simplify the icomplete-vertical feature
> branch and pushed (forced) some days ago. (sorry for that, I should have
> used a scratch branch instead)
> 
> Now the icomplete-format variable is removed and the user only needs to
> add at least one '\n' to the separator. I also use
> window-text-pixel-size to correct the issue with long lines but still
> perform the height calculations in pixels to properly show the ellipsis
> as Eli recommended.
> 
> I keep the funcall approach because IMO it is cleaner, and reduces
> unneeded and redundant things. (And because it is compatible with
> something else I am working in)
> 
> If some of the previous testers could give it a second try. When it is
> fine I will add some documentation and merge in master.
I have tried your branch and it worked perfectly. 

1. And Is there any plan to implements  a completion style like helm or  orderless(https://github.com/oantolin/orderless <https://github.com/oantolin/orderless>).
 Divides the pattern into space-separated components, and matches candidates that match all of the components in any order.

2. And Is there any plan to implements async-completing-read for icomplete ,
 one of my use cases involve an external command to generate the list of candidates, 
and that command may take some time to run( like conusel-rg).

Hope those feature can be implemented in emacs core.	


> 
> BTW: If someone could give a look to the completions-highlight feature
> branch too and make recommendations, report issues?
> 
> Best,
> Ergus
> 


[-- Attachment #2: Type: text/html, Size: 2716 bytes --]

Re: feature/icomplete-vertical

[Manuel Uberti]

Hi Ergus,

just tried the branch both from vanilla Emacs and from Emacs with my
configuration and it works as expected.

Thank you (and everyone else involved) for working on this.


All the best.

-- 
Manuel Uberti
www.manueluberti.eu

Re: feature/icomplete-vertical

[Ergus]

On Fri, Oct 02, 2020 at 12:45:41PM +0800, jixiuf wrote:
>
>
>I have tried your branch and it worked perfectly.
>
>1. And Is there any plan to implements  a completion style like helm or  orderless(https://github.com/oantolin/orderless <https://github.com/oantolin/orderless>).
> Divides the pattern into space-separated components, and matches candidates that match all of the components in any order.
>
Not that I am aware of. In principle we already have some completions
styles you could try...

C-h v: completion-styles-alist

AFAIR flex was added recently and it is similar to what you mention
(just a bit different requiring the order).

It is pretty straightforward to create and customize new completions and
differently to icomplete-vertical; orderless does not need hacks (like
advices, minibuffer resize or extra hooks) to do it's work fine. So I
don't see a need to add more completions to vanilla (I am not opposed to
that either; specially if the code is simple).

If you want more completion styles you could make a feature request
and/or contact oantolin to add part of his code to vanilla OR at least
add ordeless to elpa to make the package more "official".

>2. And Is there any plan to implements async-completing-read for icomplete ,
> one of my use cases involve an external command to generate the list of candidates,
>and that command may take some time to run( like conusel-rg).
>

Not from me ATM. I just implemented this in the core to avoid the need
of some hacks in icomplete-vertical. But I don't want to add excessive
complexity to icomplete. To do that I support adding ivy+counsel to
vanilla as mentioned in another thread some weeks ago.

>
>Hope those feature can be implemented in emacs core.	
>
>
>>
>> BTW: If someone could give a look to the completions-highlight feature
>> branch too and make recommendations, report issues?
>>
>> Best,
>> Ergus
>>
>

Re: feature/icomplete-vertical

[João Távora]

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

Hi Ergus,

Thanks, I had a look at your patch.  I'm personally not too fond of that
kind of
complexity coming into lisp/icomplete.el especially when then are simpler
formulae for achieving a funcional vertical icomplete system.  My solution
is even
simpler than Gregory's for example (and based on his):

(setq icomplete-prospects-height 6)
(setq icomplete-separator "\n")

(defun icomplete-vertical-adjust-minibuffer-height (completions)
  (let* ((comp completions)
          (complen (length (split-string comp "\n"))))
     (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1-
(window-height)))))
     comp))
(advice-add 'icomplete-completions :filter-return
#'icomplete-vertical-adjust-minibuffer-height)

So, in my view, all that's needed is to fix the window height problem
(addressed separately
in a discussion which I haven't been following), and then add sufficient
hookage so that
a _separate_ icomplete-vertical.el package with all the advanced features
you are working
on can be developed.

By the way, do you mind listing here exactly which ones those are in
relation to the
system attained  by the code above?

João


On Thu, Oct 1, 2020 at 5:48 PM Ergus <spacibba@aol.com> wrote:

> Hi:
>
> I made some corrections to simplify the icomplete-vertical feature
> branch and pushed (forced) some days ago. (sorry for that, I should have
> used a scratch branch instead)
>
> Now the icomplete-format variable is removed and the user only needs to
> add at least one '\n' to the separator. I also use
> window-text-pixel-size to correct the issue with long lines but still
> perform the height calculations in pixels to properly show the ellipsis
> as Eli recommended.
>
> I keep the funcall approach because IMO it is cleaner, and reduces
> unneeded and redundant things. (And because it is compatible with
> something else I am working in)
>
> If some of the previous testers could give it a second try. When it is
> fine I will add some documentation and merge in master.
>
> BTW: If someone could give a look to the completions-highlight feature
> branch too and make recommendations, report issues?
>
> Best,
> Ergus
>


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 2923 bytes --]

Re: feature/icomplete-vertical

[Ergus]

On Mon, Oct 05, 2020 at 12:47:39AM +0100, Jo�o T�vora wrote:

Hi Joao:

Please look at the previous messages in this same thread to see the
problems with your approach.

So far there is:

1) The problem when using different font in minibuffer reported by Jixiuf 
2) the long line issue from Gregory (but also mentioned before)
3) the ellipsis issue mentioned by Eli
4) The prompt issue (which seems is not going to be solved as it is more
a design choice or a feature than a real issue)
5) Respect the user customs max-mini-window-height and
resize-mini-windows so we shouldn't call enlarge-window and as an
core package we must not conflict with such customs.
6) The removal of {} in vertical mode because in that case they are
redundant.
7) The compatibility with packages like maple-minibuffer reported by
Dimitry.
8) The case when using multiline separator.
9) Move the first candidate to a newline instead of keeping it inline.

The patch I propose is not complex at all considering all the above
conditions. The only complexity comes from the height calculation in
pixels which is something that will need to be done in any case either
for 1, 2, 3 or 4.

>Hi Ergus,
>
>Thanks, I had a look at your patch.  I'm personally not too fond of that
>kind of
>complexity coming into lisp/icomplete.el especially when then are simpler
>formulae for achieving a funcional vertical icomplete system.  My solution
>is even
>simpler than Gregory's for example (and based on his):
>
>(setq icomplete-prospects-height 6)
>(setq icomplete-separator "\n")
>
>(defun icomplete-vertical-adjust-minibuffer-height (completions)
>  (let* ((comp completions)
>          (complen (length (split-string comp "\n"))))
>     (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1-
>(window-height)))))
>     comp))
>(advice-add 'icomplete-completions :filter-return
>#'icomplete-vertical-adjust-minibuffer-height)
>
>So, in my view, all that's needed is to fix the window height problem
>(addressed separately
>in a discussion which I haven't been following), and then add sufficient
>hookage so that
>a _separate_ icomplete-vertical.el package with all the advanced features
>you are working
>on can be developed.
>
>By the way, do you mind listing here exactly which ones those are in
>relation to the
>system attained  by the code above?
>
Look at the other branch I mentioned
(completions-highlight-modifications).

I want to bring such zsh-like completion style to icomplete (when have
some time). It must be simpler than in the *Completions* because it
won't need to switch window or add overlays.

Ergus

>Jo�o
>
>
>On Thu, Oct 1, 2020 at 5:48 PM Ergus <spacibba@aol.com> wrote:
>
>> Hi:
>>
>> I made some corrections to simplify the icomplete-vertical feature
>> branch and pushed (forced) some days ago. (sorry for that, I should have
>> used a scratch branch instead)
>>
>> Now the icomplete-format variable is removed and the user only needs to
>> add at least one '\n' to the separator. I also use
>> window-text-pixel-size to correct the issue with long lines but still
>> perform the height calculations in pixels to properly show the ellipsis
>> as Eli recommended.
>>
>> I keep the funcall approach because IMO it is cleaner, and reduces
>> unneeded and redundant things. (And because it is compatible with
>> something else I am working in)
>>
>> If some of the previous testers could give it a second try. When it is
>> fine I will add some documentation and merge in master.
>>
>> BTW: If someone could give a look to the completions-highlight feature
>> branch too and make recommendations, report issues?
>>
>> Best,
>> Ergus
>>
>
>
>-- 
>Jo�o T�vora

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 5 Oct 2020 00:47:39 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, Gregory Heytings <ghe@sdf.org>, Yuan Fu <casouri@gmail.com>, 
> 	emacs-devel <emacs-devel@gnu.org>, Juri Linkov <juri@linkov.net>
> 
> (defun icomplete-vertical-adjust-minibuffer-height (completions)
>   (let* ((comp completions)
>           (complen (length (split-string comp "\n"))))
>      (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1- (window-height)))))
>      comp))
> (advice-add 'icomplete-completions :filter-return #'icomplete-vertical-adjust-minibuffer-height)

What happens with your solution if you need to enlarge the mini-window
to a height that is greater than the frame's height?

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

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


Hi Ergus and João,

As I explained in a separate thread, at the moment the best way to 
implement icomplete-vertical (and in general to display completion 
candidates in the minibuffer) is to convince Emacs to start displaying the 
minibuffer at its beginning.  This avoids all known problems.

With this solution, displaying completion candidates after point in a 
minibuffer is a trivial task: it suffices to insert the completion 
candidates in the minibuffer, and Emacs will display as many of these 
candidates as possible, given the user preferences 
(max-mini-window-height, resize-mini-windows, ...), the size of the Emacs 
frame, the phase of the moon, ...  This works on Emacs 24, 25, 26, 27 and 
28, with fixed and variable width fonts.

Part 1 of the solution (which solves the "root" problem, and is not 
specific to icomplete-vertical):

(defvar-local start-display-at-beginning-of-minibuffer nil)
(defun start-display-at-beginning-of-minibuffer (&rest args)
   (when (and start-display-at-beginning-of-minibuffer (minibufferp))
     (set-window-start-at-begin (point-min) (point))))
(defun set-window-start-at-begin (beg end)
   (when (< (+ beg 2) end)
     (set-window-start nil beg)
     (unless (pos-visible-in-window-p end nil t)
       (set-window-start-at-begin (+ beg (/ (- end beg) 2)) end))))
(add-hook 'window-scroll-functions #'start-display-at-beginning-of-minibuffer)
(add-hook 'post-command-hook #'start-display-at-beginning-of-minibuffer)

Part 2 of the solution (which implements icomplete-vertical):

(setq icomplete-separator "\n")
(add-hook 'icomplete-minibuffer-setup-hook (lambda () (setq start-display-at-beginning-of-minibuffer t)))
(defun icomplete-vertical-reformat-completions (completions)
   (save-match-data
     (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" completions)
         (format "%s \n%s" (or (match-string 1 completions) "") (match-string 2 completions))
       completions)))
(advice-add 'icomplete-completions :filter-return #'icomplete-vertical-reformat-completions)

The only limit of this solution is that is is not possible to display an 
ellipsis ("...") at the end of the completion candidates list, to indicate 
that some completion candidates are not displayed.  This is a minor 
limitation, and IMO using one screen line just to display "..." is a waste 
of resources.

Gregory

Re: feature/icomplete-vertical

[Manuel Uberti]

On 05/10/20 09:52, Gregory Heytings via Emacs development discussions. wrote:
> 
> Hi Ergus and João,
> 
> As I explained in a separate thread, at the moment the best way to implement
> icomplete-vertical (and in general to display completion candidates in the
> minibuffer) is to convince Emacs to start displaying the minibuffer at its
> beginning.  This avoids all known problems.
> 
> With this solution, displaying completion candidates after point in a minibuffer
> is a trivial task: it suffices to insert the completion candidates in the
> minibuffer, and Emacs will display as many of these candidates as possible,
> given the user preferences (max-mini-window-height, resize-mini-windows, ...),
> the size of the Emacs frame, the phase of the moon, ...  This works on Emacs 24,
> 25, 26, 27 and 28, with fixed and variable width fonts.
> 
> Part 1 of the solution (which solves the "root" problem, and is not specific to
> icomplete-vertical):
> 
> (defvar-local start-display-at-beginning-of-minibuffer nil)
> (defun start-display-at-beginning-of-minibuffer (&rest args)
>   (when (and start-display-at-beginning-of-minibuffer (minibufferp))
>     (set-window-start-at-begin (point-min) (point))))
> (defun set-window-start-at-begin (beg end)
>   (when (< (+ beg 2) end)
>     (set-window-start nil beg)
>     (unless (pos-visible-in-window-p end nil t)
>       (set-window-start-at-begin (+ beg (/ (- end beg) 2)) end))))
> (add-hook 'window-scroll-functions #'start-display-at-beginning-of-minibuffer)
> (add-hook 'post-command-hook #'start-display-at-beginning-of-minibuffer)
> 
> Part 2 of the solution (which implements icomplete-vertical):
> 
> (setq icomplete-separator "\n")
> (add-hook 'icomplete-minibuffer-setup-hook (lambda () (setq
> start-display-at-beginning-of-minibuffer t)))
> (defun icomplete-vertical-reformat-completions (completions)
>   (save-match-data
>     (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" completions)
>         (format "%s \n%s" (or (match-string 1 completions) "") (match-string 2
> completions))
>       completions)))
> (advice-add 'icomplete-completions :filter-return
> #'icomplete-vertical-reformat-completions)
> 
> The only limit of this solution is that is is not possible to display an
> ellipsis ("...") at the end of the completion candidates list, to indicate that
> some completion candidates are not displayed.  This is a minor limitation, and
> IMO using one screen line just to display "..." is a waste of resources.
> 
> Gregory

Hi Gregory,

FWIW, your code works absolutely fine for me and I was able to replace the
external icomplete-vertical package with your solution.

Thanks for working on this.

-- 
Manuel Uberti
www.manueluberti.eu

Re: feature/icomplete-vertical

[João Távora]

Ergus <spacibba@aol.com> writes:

> On Mon, Oct 05, 2020 at 12:47:39AM +0100, Jo�o T�vora wrote:
>
> Hi Joao:
>
> Please look at the previous messages in this same thread to see the
> problems with your approach.
>
> So far there is:
>
> 1) The problem when using different font in minibuffer reported by
> Jixiuf 2) the long line issue from Gregory (but also mentioned before)
> 3) the ellipsis issue mentioned by Eli
> 4) The prompt issue (which seems is not going to be solved as it is more
> a design choice or a feature than a real issue)
> 5) Respect the user customs max-mini-window-height and
> resize-mini-windows so we shouldn't call enlarge-window and as an
> core package we must not conflict with such customs.
> 6) The removal of {} in vertical mode because in that case they are
> redundant.
> 7) The compatibility with packages like maple-minibuffer reported by
> Dimitry.
> 8) The case when using multiline separator.
> 9) Move the first candidate to a newline instead of keeping it inline.
>
> The patch I propose is not complex at all considering all the above
> conditions. The only complexity comes from the height calculation in
> pixels which is something that will need to be done in any case either
> for 1, 2, 3 or 4.

Thanks for listing these again, for my benefit.  I've not been following
this very closely, and needed a summary to give me context after trying
your patch again, as you requested.

To be clear: I'm not "against" this functionality or even against it
becoming a part of Emacs or Emacs core.  I think solving the problems
you list above is fine (though I haven't been by them, at least not yet)

I'm not too much concerned about the complexity of implementation (such
as the height calculation).  I am concerned the changes to the user
interface, of which your patch seems to bring many, or at least more
than I would have expected.  It seems to be that your solution mixes
bugfixes and new features, which is what I object to.  I think your
solution could be trimmed down so that it solves _only_ the technical
problems you list above without adding new user interfaces.

In other words, don't see why the points above, which sound like
basically bugs, need any new "defcustom".  This is why I asked you to
(re)list them.  Then I can start with my naive implementation and fix
them one by one, without adding new interfaces.  Of course, then I will
be repeating some work you've already done, so this is why I'm arguing
for this separation in _your_ work.

Could you structure your work so that one part fixes the above problems,
and another part adds the new features, like customizing bracket types
and such, and is should be discussed as a separate matter.

After my sig, some comments that illustrate my points.

I took this diff with git diff master...feature/icomplete-vertical

João

> -(defcustom icomplete-separator " | "
> -  "String used by Icomplete to separate alternatives in the minibuffer."
> -  :type 'string
> -  :version "24.4")
> +(defcustom icomplete-vertical nil
> +  "Enable `icomplete' vertical mode."
> +  :type 'bool
> +  :version "28.1")

I would think it more elegant to keep `icomplete-separator` as the entry
point to verticality.  Certainly it should be possible to analyse the
string and decide whether it implies verticality and needs special
worries.

  (defcustom icomplete-hide-common-prefix t
   "When non-nil, hide common prefix from completion candidates.
@@ -97,6 +97,12 @@ icomplete-first-match
   "Face used by Icomplete for highlighting first match."
   :version "24.4")
 
> +(defface icomplete-common-match '((t :inherit 'highlight
> +                                     :underline t
> +                                     :weight bold))
> +  "Face used by Icomplete for highlighting common completion."
> +  :version "28.1")


What is a "common match"?  Is this exclusive to icomplete verticalness?
Doesn't seem so, is this not a side feature?

> -(defcustom icomplete-minibuffer-setup-hook nil
> +(defvar icomplete-ellipsis nil)
> +
> +(defcustom icomplete--minibuffer-setup-hook nil
>    "Icomplete-specific customization of minibuffer setup.

Why has this minibuffer hook been made an internal variable?

> -    (define-key map (kbd "<right>") 'icomplete-forward-completions)
> -    (define-key map (kbd "<left>") 'icomplete-backward-completions)

Why have these been removed from icomplete-fido-mode-map?  Also, I have
C-r and C-r in that map, will it work in the verticality?  I would hope
it would, as it does not in my naive verticality implementation.

> -;;;_ > icomplete-minibuffer-setup ()
> -(defun icomplete-minibuffer-setup ()
> +;; Vertical functions
> +
> +(defcustom icomplete-separator-vertical " \n"
> +  "String used by Icomplete to separate alternatives in the minibuffer."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-list-indicators-vertical (cons "" "")
> +  "Indicator bounds to list alternatives in the minibuffer."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-require-indicators-vertical (cons "" "")
> +  "Indicator bounds for match in the minibuffer when require-match."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-not-require-indicators-vertical (cons "" "")
> +  "Indicator bounds for match in the minibuffer when not require-match."
> +  :type 'string
> +  :version "28.1")

These are examples of "interface bloat" that do not seem to directly
address the 9 problems you listed in the beginning of the message
(again, I am not against these features per se, mind you)

> +(defvar icomplete--vertical-mode-map

If this keymap is "internal" (as indicated by the two "--") how am I
supposed to customize the keybindings?

> +(defcustom icomplete-list-indicators-horizontal (cons "{" "}")
> +  "Indicator bounds to list alternatives in the minibuffer."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-require-indicators-horizontal (cons "(" ")")
> +  "Indicator bounds for match in the minibuffer when require-match."
> +  :type 'string
> +  :version "28.1")
> +
> +(defcustom icomplete-not-require-indicators-horizontal (cons "[" "]")
> +  "Indicator bounds for match in the minibuffer when not require-match."
> +  :type 'string
> +  :version "28.1")

Again, these seem like improvements that are not directly related to
verticality.  (Maybe they are very good improvements, but still...

João

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Mon, 5 Oct 2020 00:47:39 +0100
>> Cc: Eli Zaretskii <eliz@gnu.org>, Gregory Heytings <ghe@sdf.org>, Yuan Fu <casouri@gmail.com>, 
>> 	emacs-devel <emacs-devel@gnu.org>, Juri Linkov <juri@linkov.net>
>> 
>> (defun icomplete-vertical-adjust-minibuffer-height (completions)
>>   (let* ((comp completions)
>>           (complen (length (split-string comp "\n"))))
>>      (if (> complen 1) (enlarge-window (- icomplete-prospects-height (1- (window-height)))))
>>      comp))
>> (advice-add 'icomplete-completions :filter-return #'icomplete-vertical-adjust-minibuffer-height)
>
> What happens with your solution if you need to enlarge the mini-window
> to a height that is greater than the frame's height?

I don't know.  Wouldn't I need an extremely stubby frame to achieve
that?  Anyway, that's not even my code.  As I explained to Ergus, just
wanted to underline that a solution without new user interfaces can be a
starting point with bugs that are fixed as bugs.  I doubt that
systematically fixing these bugs would necessarily leads us to a
solution with N new defcustoms and a new minor mode.

João

Re: feature/icomplete-vertical

[João Távora]

Gregory Heytings <ghe@sdf.org> writes:

> Part 1 of the solution (which solves the "root" problem, and is not
> specific to icomplete-vertical):

Right.  I appreciate this separation: solve a bug _then_ implement a
feature.

Now, as far as I understand Eli and remaining maintainers are not OK
with your bugfix.  This I have no opinion about, except for finding your
expectation of "auto-enlargement" reasonable at first sight.  Other than
that, I haven't studied the matter.  But hopefully we should all agree
on basic icomplete-independent behaviour and come up with a fix that is
acceptable for all.

> Part 2 of the solution (which implements icomplete-vertical):

> (setq icomplete-separator "\n")
> (add-hook 'icomplete-minibuffer-setup-hook (lambda () (setq start-display-at-beginning-of-minibuffer t)))
> (defun icomplete-vertical-reformat-completions (completions)
>   (save-match-data
>     (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" completions)
>         (format "%s \n%s" (or (match-string 1 completions) "") (match-string 2 completions))
>       completions)))n
> (advice-add 'icomplete-completions :filter-return
> #'icomplete-vertical-reformat-completions)
>
> The only limit of this solution is that is is not possible to display
> an ellipsis ("...") at the end of the completion candidates list, to
> indicate that some completion candidates are not displayed.  This is a
> minor limitation, and IMO using one screen line just to display "..."
> is a waste of resources.

Yeah, I agree.  For example, in my verticality thing I would even remove
one extra feature of your implementation that I don't find useful: the
indentation of the candidates.  These are features that can be of course
implemented _separately_.

João

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 05 Oct 2020 10:13:30 +0100
> Cc: ghe@sdf.org, spacibba@aol.com, juri@linkov.net, casouri@gmail.com,
>  emacs-devel@gnu.org
> 
> > What happens with your solution if you need to enlarge the mini-window
> > to a height that is greater than the frame's height?
> 
> I don't know.  Wouldn't I need an extremely stubby frame to achieve
> that?

Or a lot of completion candidates.

My point is that enlarging the mini-window only works up to a limit.
So it cannot solve every case; eventually, you will need to truncate
the list of candidates in some way, and show to the user that the list
was truncated.

Re: feature/icomplete-vertical

[João Távora]

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

On Mon, Oct 5, 2020 at 10:46 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Date: Mon, 05 Oct 2020 10:13:30 +0100
> > Cc: ghe@sdf.org, spacibba@aol.com, juri@linkov.net, casouri@gmail.com,
> >  emacs-devel@gnu.org

> Or a lot of completion candidates.

That's odd, I've been C-x C-f'ing to directories with "a lot" of files
and I don't notice any problems.  What size of "lot" did you have in
mind?

> My point is that enlarging the mini-window only works up to a limit.
> So it cannot solve every case; eventually, you will need to truncate
> the list of candidates in some way, and show to the user that the list
> was truncated.

Make sense. I do notice some truncation in the candidates in this new
vertical mode I've been trying for a day or so, but I expect truncation of
candidates anyway, so I haven't missed the feature of being shown
explicitly that truncation is happening.  Anyway, whoever is making
this truncation happen (display engine?) should be the responsible for
showing that warning hint.  I don't see why it should be related to
"candidates" or completion at all.

Though maybe the responsible for the truncation can provide a
way (a hook, a variable, a function?) for the user of the minibuffer to
select
the appropriate hint.  My point here is that this variable/function
shouldn't
be called icomplete-truncation-hint, but rather mini-window-truncation-hint.

Hope I've explained myself,
João

[-- Attachment #2: Type: text/html, Size: 2161 bytes --]

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 5 Oct 2020 10:57:35 +0100
> Cc: Gregory Heytings <ghe@sdf.org>, Ergus <spacibba@aol.com>, Juri Linkov <juri@linkov.net>, 
> 	Yuan Fu <casouri@gmail.com>, emacs-devel <emacs-devel@gnu.org>
> 
> > Or a lot of completion candidates.
> 
> That's odd, I've been C-x C-f'ing to directories with "a lot" of files
> and I don't notice any problems.  What size of "lot" did you have in
> mind?

More than can be displayed, one candidate on each line, by the frame's
dimensions, I guess.

> Make sense. I do notice some truncation in the candidates in this new
> vertical mode I've been trying for a day or so, but I expect truncation of 
> candidates anyway, so I haven't missed the feature of being shown 
> explicitly that truncation is happening.  Anyway, whoever is making 
> this truncation happen (display engine?) should be the responsible for 
> showing that warning hint.  I don't see why it should be related to 
> "candidates" or completion at all.

The truncation is done by icomplete, not by the display engine.  The
"warning" is supposed to be in the form of the ellipsis displayed
after the last candidate that is shown.

> Though maybe the responsible for the truncation can provide a 
> way (a hook, a variable, a function?) for the user of the minibuffer to select 
> the appropriate hint.  My point here is that this variable/function shouldn't 
> be called icomplete-truncation-hint, but rather mini-window-truncation-hint.

The code which displays the min-window is more-or-less the generic
Emacs window-display code, it doesn't care that not all of the stuff
fits in the window.  If an application wants to fit the buffer in the
window, or display some hint about truncation, it's the application's
business to do these things.

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Mon, 5 Oct 2020 10:57:35 +0100
>> Cc: Gregory Heytings <ghe@sdf.org>, Ergus <spacibba@aol.com>, Juri Linkov <juri@linkov.net>, 
>> 	Yuan Fu <casouri@gmail.com>, emacs-devel <emacs-devel@gnu.org>
>> 
>> > Or a lot of completion candidates.
>> 
>> That's odd, I've been C-x C-f'ing to directories with "a lot" of files
>> and I don't notice any problems.  What size of "lot" did you have in
>> mind?
>
> More than can be displayed, one candidate on each line, by the frame's
> dimensions, I guess.

I've certainly got more than that, and the problem doesn't happen.

>> Though maybe the responsible for the truncation can provide a 
>> way (a hook, a variable, a function?) for the user of the minibuffer to select 
>> the appropriate hint.  My point here is that this variable/function shouldn't 
>> be called icomplete-truncation-hint, but rather mini-window-truncation-hint.
>
> The code which displays the min-window is more-or-less the generic
> Emacs window-display code, it doesn't care that not all of the stuff
> fits in the window.

That's the code that honours max-mini-window-height, right?  Though that
doesn't seem to be what's kicking in here, I believe it should be.

Anyway, I think it's reasonable to suggest I think, that whoever is
truncating the display has someway of notifying the client (whoever
requested the display), that truncation happened, or is about to happen.

> If an application wants to fit the buffer in the window, or display
> some hint about truncation, it's the application's business to do
> these things.

I guess you can argue that, but this implies there are ways to predict
truncation (since being notified of it seems to be what you're opposed
to).

So how is the application to know if its n lines, of lengths L = {l1,
..., li, ..., ln}, it wants to display (not necessarily by buffer
insertion) in the mini-window need truncation and starting in which
line?  Does it need to perform calculations with max-mini-window-height?
If so, is there a "canonical" way to perform these calculations that
accounts for fontsizes, frame widths, etc?  To be clear, I find this
information useful for other domais, notably designing the way the Eldoc
should show information in the echo area.

João

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> Part 1 of the solution (which solves the "root" problem, and is not 
>> specific to icomplete-vertical):
>
> Right.  I appreciate this separation: solve a bug _then_ implement a 
> feature.
>

Thank you :-)

>
> Now, as far as I understand Eli and remaining maintainers are not OK 
> with your bugfix.
>

No, Eli is "not against using this as a workaround [...] until the better 
solution arrives".  It's not yet clear what that "better solution" will 
be, and when it will be implemented.  And for code that needs to work on 
older Emacsen, this will remain a good solution.

>> (setq icomplete-separator "\n")
>> (add-hook  'icomplete-minibuffer-setup-hook (lambda () (setq start-display-at-beginning-of-minibuffer t)))
>> (defun icomplete-vertical-reformat-completions (completions)
>>   (save-match-data
>>     (if (string-match "^\\((.*)\\|\\[.+\\]\\)?{\\(\\(?:.\\|\n\\)+\\)}" completions)
>>         (format "%s \n%s" (or (match-string 1 completions) "") (match-string 2 completions))
>>       completions)))
>> (advice-add 'icomplete-completions :filter-return #'icomplete-vertical-reformat-completions)
>
> For example, in my verticality thing I would even remove one extra 
> feature of your implementation that I don't find useful: the indentation 
> of the candidates.
>

Indeed, I forgot to remove this from my code when I first sent my proposed 
solution two weeks ago.  This extra feature is not anymore present in the 
above code.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: ghe@sdf.org,  spacibba@aol.com,  juri@linkov.net,  casouri@gmail.com,
>   emacs-devel@gnu.org
> Date: Mon, 05 Oct 2020 11:52:59 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> That's odd, I've been C-x C-f'ing to directories with "a lot" of files
> >> and I don't notice any problems.  What size of "lot" did you have in
> >> mind?
> >
> > More than can be displayed, one candidate on each line, by the frame's
> > dimensions, I guess.
> 
> I've certainly got more than that, and the problem doesn't happen.

Then either you truncate the candidate's (i.e., don't show all of it),
or your code isn't working properly.

> > The code which displays the min-window is more-or-less the generic
> > Emacs window-display code, it doesn't care that not all of the stuff
> > fits in the window.
> 
> That's the code that honours max-mini-window-height, right?

Yes.  Followed by the normal window redisplay.

> Though that doesn't seem to be what's kicking in here, I believe it
> should be.

Not sure I can parse this properly.  What exactly isn't "kicking in"?

> Anyway, I think it's reasonable to suggest I think, that whoever is
> truncating the display has someway of notifying the client (whoever
> requested the display), that truncation happened, or is about to happen.

I think whoever truncates the display is the same as the client.
Unless I misunderstand what you mean by "truncate".

> > If an application wants to fit the buffer in the window, or display
> > some hint about truncation, it's the application's business to do
> > these things.
> 
> I guess you can argue that, but this implies there are ways to predict
> truncation (since being notified of it seems to be what you're opposed
> to).

There are ways.  They aren't necessarily easy, but if your application
does care about not everything being visible in a window, the
application must do something about it, because the "normal" Emacs
display doesn't treat partial display of a buffer as something
special, since that is what happens all the time in Emacs.

> So how is the application to know if its n lines, of lengths L = {l1,
> ..., li, ..., ln}, it wants to display (not necessarily by buffer
> insertion) in the mini-window need truncation and starting in which
> line?

We have window-text-pixel-size for that.

> Does it need to perform calculations with max-mini-window-height?

Yes.

> If so, is there a "canonical" way to perform these calculations that
> accounts for fontsizes, frame widths, etc?  To be clear, I find this
> information useful for other domais, notably designing the way the Eldoc
> should show information in the echo area.

Not sure what should be canonical here.  AFAIU, just using
window-text-pixel-size and comparing with the window dimensions is all
that's needed.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> If an application wants to fit the buffer in the window, or display 
>> some hint about truncation, it's the application's business to do these 
>> things.
>
> I guess you can argue that, but this implies there are ways to predict 
> truncation (since being notified of it seems to be what you're opposed 
> to).
>
> So how is the application to know if its n lines, of lengths L = {l1, 
> ..., li, ..., ln}, it wants to display (not necessarily by buffer 
> insertion) in the mini-window need truncation and starting in which 
> line?  Does it need to perform calculations with max-mini-window-height? 
> If so, is there a "canonical" way to perform these calculations that 
> accounts for fontsizes, frame widths, etc?  To be clear, I find this 
> information useful for other domais, notably designing the way the Eldoc 
> should show information in the echo area.
>

This is feasible, but IMHO very (and needlessly) difficult.  Basically you 
need to work with pixel dimensions, and recalculate everything manually: 
first calculate the (maximal) size of the miniwindow (given the user 
preferences, in particular max-mini-window-height), then calculate the 
size of its contents with window-text-pixel-size.  You should add one 
character (or line) at at time, and recalculate the new size each time.

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> If so, is there a "canonical" way to perform these calculations that 
>> accounts for fontsizes, frame widths, etc?  To be clear, I find this 
>> information useful for other domais, notably designing the way the 
>> Eldoc should show information in the echo area.
>
> Not sure what should be canonical here.  AFAIU, just using 
> window-text-pixel-size and comparing with the window dimensions is all 
> that's needed.
>

Just calculating the window dimensions is already a nontrivial task. 
There are two cases: a miniwindow-only frame, and the "normal" case.  In 
the first case you need to use frame-height, and multiply it with the 
pixel height of the "default" face.  In the second case you can get the 
maximal dimensions with max-mini-window-height, and multiply it by the 
pixel height of the "default" face.  But you cannot just multiply 
max-mini-window-height by that height, that would be too easy.  There are 
again two cases: either max-mini-window-height is an integer, in which 
case you can just do that multiplication, or it is a floating point 
number, in which case you have to multiply that number by frame-height and 
truncate it, and multiply the resulting number by the pixel height of the 
"default" face...

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> That's odd, I've been C-x C-f'ing to directories with "a lot" of files
>> >> and I don't notice any problems.  What size of "lot" did you have in
>> >> mind?
>> >
>> > More than can be displayed, one candidate on each line, by the frame's
>> > dimensions, I guess.
>> 
>> I've certainly got more than that, and the problem doesn't happen.
>
> Then either you truncate the candidate's (i.e., don't show all of it),
> or your code isn't working properly.

Well, it's not my code, it's Gregory's.  I just evaluated it and it
seems to be working decently for the day or so.  But I'm fine to admit
there may be corner cases where it would fail, I just haven't detected
those failures.

>> > The code which displays the min-window is more-or-less the generic
>> > Emacs window-display code, it doesn't care that not all of the stuff
>> > fits in the window.
>> That's the code that honours max-mini-window-height, right?
> Yes.  Followed by the normal window redisplay.
>> Though that doesn't seem to be what's kicking in here, I believe it
>> should be.
> Not sure I can parse this properly.  What exactly isn't "kicking in"?

Don't worry. I meant I have that variable set to 3, but I'm seeing a
6-line-high minibuffer prompt, so something in Gregory's code seems to
be overriding it.  By "should be" I'm stating that that overriding
shouldn't happen, i.e. I'm searching for a better solution.

>> Anyway, I think it's reasonable to suggest I think, that whoever is
>> truncating the display has someway of notifying the client (whoever
>> requested the display), that truncation happened, or is about to happen.
>
> I think whoever truncates the display is the same as the client.
> Unless I misunderstand what you mean by "truncate".

Truncation is being given something to display, such as here:

  (let ((minibuffer-setup-hook (lambda ()
                                 (insert (format "one\ntwo\nthree\nfour")))))
    (read-minibuffer "hey"))

and _not_ actually displaying it becasue of some window size constraint.

> There are ways.  They aren't necessarily easy, but if your application
> does care about not everything being visible in a window, the
> application must do something about it, because the "normal" Emacs
> display doesn't treat partial display of a buffer as something
> special, since that is what happens all the time in Emacs.

Right.  But is there a very large cost in implementing a mechanism of
notification that applications can optionally subscribe to take action
when this common thing happens to them?

> Not sure what should be canonical here.  AFAIU, just using
> window-text-pixel-size and comparing with the window dimensions is all
> that's needed.

Thanks, but I don't understand how that could help here.  The docstring
says:

   This function exists to allow Lisp programs to adjust the dimensions
   of WINDOW to the buffer text it needs to display.

But we need a subtly different use which would be:

   This function exists to allow Lisp programs to adjust the buffer text
   so that it fits visually in WINDOW.

I don't know how to use the function to do this.  In other words, I have
some buffer text and I would like to know:

1) If it fits in WINDOW;
2) If it doesn't, where is the cutoff, as a text position.

I'm re-reading the docstring to see it this is possible, but I don't
think the function is tailored for this use case.  Anyway, if that
function existed it would be what I called the canonical solution for
this case.

João

Re: feature/icomplete-vertical

[João Távora]

Gregory Heytings <ghe@sdf.org> writes:

>>> If an application wants to fit the buffer in the window, or display
>>> some hint about truncation, it's the application's business to do
>>> these things.
>>
>> I guess you can argue that, but this implies there are ways to
>> predict truncation (since being notified of it seems to be what
>> you're opposed to).
>>
>> So how is the application to know if its n lines, of lengths L =
>> {l1, ..., li, ..., ln}, it wants to display (not necessarily by
>> buffer insertion) in the mini-window need truncation and starting in
>> which line?  Does it need to perform calculations with
>> max-mini-window-height? If so, is there a "canonical" way to perform
>> these calculations that accounts for fontsizes, frame widths, etc?
>> To be clear, I find this information useful for other domais,
>> notably designing the way the Eldoc should show information in the
>> echo area.
>>
>
> This is feasible, but IMHO very (and needlessly) difficult.  Basically
> you need to work with pixel dimensions, and recalculate everything
> manually: first calculate the (maximal) size of the miniwindow (given
> the user preferences, in particular max-mini-window-height), then
> calculate the size of its contents with window-text-pixel-size.  You
> should add one character (or line) at at time, and recalculate the new
> size each time.

Yes, as far as I understand, in the general case, it must be one
character at a time, since each character might have a different pixel
width.  At some point there is a cutoff and things are not displayed
anymore.  I was hoping that that knowledge, which is held somehere in
the system at some point, could be imparted to the application.

Anyway, whatever the mechanism (notification or painstaking
calculation), we should first write a function that does this.  That is
the bugfix in my opinion.  Then we can work on simplifying that
function's implementation, if it turns out to be slow or problematic.

João

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Mon, 05 Oct 2020 11:11:28 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: João Távora <joaotavora@gmail.com>, spacibba@aol.com,
>         emacs-devel@gnu.org, casouri@gmail.com, juri@linkov.net
> 
> > Not sure what should be canonical here.  AFAIU, just using 
> > window-text-pixel-size and comparing with the window dimensions is all 
> > that's needed.
> 
> Just calculating the window dimensions is already a nontrivial task. 
> There are two cases: a miniwindow-only frame, and the "normal" case.  In 
> the first case you need to use frame-height, and multiply it with the 
> pixel height of the "default" face.  In the second case you can get the 
> maximal dimensions with max-mini-window-height, and multiply it by the 
> pixel height of the "default" face.  But you cannot just multiply 
> max-mini-window-height by that height, that would be too easy.  There are 
> again two cases: either max-mini-window-height is an integer, in which 
> case you can just do that multiplication, or it is a floating point 
> number, in which case you have to multiply that number by frame-height and 
> truncate it, and multiply the resulting number by the pixel height of the 
> "default" face...

Well, if someone wants to write a function which does all that, I'm
sure it will be welcome (assuming that window.el doesn't yet have
something like that already).

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: ghe@sdf.org,  spacibba@aol.com,  juri@linkov.net,  casouri@gmail.com,
>   emacs-devel@gnu.org
> Date: Mon, 05 Oct 2020 12:24:55 +0100
> 
> Truncation is being given something to display, such as here:
> 
>   (let ((minibuffer-setup-hook (lambda ()
>                                  (insert (format "one\ntwo\nthree\nfour")))))
>     (read-minibuffer "hey"))
> 
> and _not_ actually displaying it becasue of some window size constraint.
> 
> > There are ways.  They aren't necessarily easy, but if your application
> > does care about not everything being visible in a window, the
> > application must do something about it, because the "normal" Emacs
> > display doesn't treat partial display of a buffer as something
> > special, since that is what happens all the time in Emacs.
> 
> Right.  But is there a very large cost in implementing a mechanism of
> notification that applications can optionally subscribe to take action
> when this common thing happens to them?

I don't think the cost is large, but someone needs to do the writing.
Emacs was never designed to display completions this way, so it's not
surprising these mechanisms weren't implemented until now.

>    This function exists to allow Lisp programs to adjust the buffer text
>    so that it fits visually in WINDOW.
> 
> I don't know how to use the function to do this.  In other words, I have
> some buffer text and I would like to know:
> 
> 1) If it fits in WINDOW;
> 2) If it doesn't, where is the cutoff, as a text position.

Basically, remove lines one by one until it fits.

We could write a similar function to do what you want, but please note
that such a function will not understand the semantics of the text, so
it could suggest truncating it at some place which makes no sense.  If
the caller would like a smarter 'service", we will have to come up
with some method of telling the function where it makes sense to
truncate the text.

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Not sure what should be canonical here.  AFAIU, just using 
>> > window-text-pixel-size and comparing with the window dimensions is all 
>> > that's needed.
>> 
>> Just calculating the window dimensions is already a nontrivial task. 

> Well, if someone wants to write a function which does all that, I'm
> sure it will be welcome (assuming that window.el doesn't yet have
> something like that already).

I agree with this.

João

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> This is feasible, but IMHO very (and needlessly) difficult.  Basically 
>> you need to work with pixel dimensions, and recalculate everything 
>> manually: first calculate the (maximal) size of the miniwindow (given 
>> the user preferences, in particular max-mini-window-height), then 
>> calculate the size of its contents with window-text-pixel-size.  You 
>> should add one character (or line) at at time, and recalculate the new 
>> size each time.
>
> Yes, as far as I understand, in the general case, it must be one 
> character at a time, since each character might have a different pixel 
> width.  At some point there is a cutoff and things are not displayed 
> anymore.  I was hoping that that knowledge, which is held somehere in 
> the system at some point, could be imparted to the application.
>
> Anyway, whatever the mechanism (notification or painstaking 
> calculation), we should first write a function that does this.  That is 
> the bugfix in my opinion.  Then we can work on simplifying that 
> function's implementation, if it turns out to be slow or problematic.
>

There is no need to calculate anything with the 
"start-display-at-beginning-of-minibuffer" solution I sent a few hours 
ago.  Emacs does everything for you, at no cost.

Or is there something that you need (e.g. for Eldoc) and that this 
solution does not do?

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> Right.  But is there a very large cost in implementing a mechanism of
>> notification that applications can optionally subscribe to take action
>> when this common thing happens to them?
>
> I don't think the cost is large, but someone needs to do the writing.
> Emacs was never designed to display completions this way, so it's not
> surprising these mechanisms weren't implemented until now.

Right.  Though it's not only for completion.

>>    This function exists to allow Lisp programs to adjust the buffer text
>>    so that it fits visually in WINDOW.
>> 
>> I don't know how to use the function to do this.  In other words, I have
>> some buffer text and I would like to know:
>> 
>> 1) If it fits in WINDOW;
>> 2) If it doesn't, where is the cutoff, as a text position.
>
> Basically, remove lines one by one until it fits.

This was the approach I took in eldoc.el, but I found it's complex.  And
it assumes we want to cut off at logical line positions.  In a recent
proposal of bug#43543 I proposed taking it out of eldoc.el (presumably
to be replaced later with some more generic means).

> We could write a similar function to do what you want, but please note
> that such a function will not understand the semantics of the text, so
> it could suggest truncating it at some place which makes no sense.
> If the caller would like a smarter 'service", we will have to come up
> with some method of telling the function where it makes sense to
> truncate the text.

Right. My idea is that a "dumb" service gives us a text position and the
application, which does know the semantics of the things being
displayed, decides based on that.

For example, icomplete would remove the candidate where the truncation
happened and replace it with an elipsis, whereas eldoc would remove the
piece of documentation entirely and maybe replace it with a hint to
visit the *eldoc* buffer.

João

Re: feature/icomplete-vertical

[João Távora]

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

On Mon, Oct 5, 2020 at 12:54 PM Gregory Heytings <ghe@sdf.org> wrote:
>
> >> This is feasible, but IMHO very (and needlessly) difficult.  Basically
> >> you need to work with pixel dimensions, and recalculate everything
> >> manually: first calculate the (maximal) size of the miniwindow (given
> >> the user preferences, in particular max-mini-window-height), then
> >> calculate the size of its contents with window-text-pixel-size.  You
> >> should add one character (or line) at at time, and recalculate the new
> >> size each time.
> >
> > Yes, as far as I understand, in the general case, it must be one
> > character at a time, since each character might have a different pixel
> > width.  At some point there is a cutoff and things are not displayed
> > anymore.  I was hoping that that knowledge, which is held somehere in
> > the system at some point, could be imparted to the application.
> >
> > Anyway, whatever the mechanism (notification or painstaking
> > calculation), we should first write a function that does this.  That is
> > the bugfix in my opinion.  Then we can work on simplifying that
> > function's implementation, if it turns out to be slow or problematic.
> >
>
> There is no need to calculate anything with the
> "start-display-at-beginning-of-minibuffer" solution I sent a few hours
> ago.  Emacs does everything for you, at no cost.
>
> Or is there something that you need (e.g. for Eldoc) and that this
> solution does not do?

I suppose not, but I take it that that solution is a non-starter
because of Eli's and Stefan's objections to it. Maybe you
somehow placate those objections.

João

[-- Attachment #2: Type: text/html, Size: 2053 bytes --]

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

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


>> There is no need to calculate anything with the 
>> "start-display-at-beginning-of-minibuffer" solution I sent a few hours 
>> ago.  Emacs does everything for you, at no cost.
>>
>> Or is there something that you need (e.g. for Eldoc) and that this 
>> solution does not do?
> 
> I suppose not, but I take it that that solution is a non-starter because 
> of Eli's and Stefan's objections to it.
>

I did not see objections from Stefan.  And the objection of Eli is only, 
AFAIU, that in a longer term a better solution should be implemented.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 05 Oct 2020 12:54:52 +0100
> Cc: ghe@sdf.org, spacibba@aol.com, emacs-devel@gnu.org, casouri@gmail.com,
>  juri@linkov.net
> 
> > We could write a similar function to do what you want, but please note
> > that such a function will not understand the semantics of the text, so
> > it could suggest truncating it at some place which makes no sense.
> > If the caller would like a smarter 'service", we will have to come up
> > with some method of telling the function where it makes sense to
> > truncate the text.
> 
> Right. My idea is that a "dumb" service gives us a text position and the
> application, which does know the semantics of the things being
> displayed, decides based on that.
> 
> For example, icomplete would remove the candidate where the truncation
> happened and replace it with an elipsis, whereas eldoc would remove the
> piece of documentation entirely and maybe replace it with a hint to
> visit the *eldoc* buffer.

In use cases that are not the most trivial ones, knowing what to
remove could not be easy.  For example, what if the part that exceeds
the window dimensions comes from display or overlay string?  Also,
does the application tolerate partially-visible lines?  So some
additional information will need to be made available to the function,
I think.

Patches welcome.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Mon, 05 Oct 2020 12:02:30 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: Eli Zaretskii <eliz@gnu.org>, Juri Linkov <juri@linkov.net>,
>         Ergus <spacibba@aol.com>, Yuan Fu <casouri@gmail.com>,
>         emacs-devel <emacs-devel@gnu.org>
> 
> > I suppose not, but I take it that that solution is a non-starter because 
> > of Eli's and Stefan's objections to it.
> >
> 
> I did not see objections from Stefan.  And the objection of Eli is only, 
> AFAIU, that in a longer term a better solution should be implemented.

No, my objections are that your proposal is not safe wrt recursive
editing, and will affect uses of mini-windows other than prompting the
user, where these effects aren't intended.

AFAIU, Stefan agreed that this is a problem in your proposal.

Re: feature/icomplete-vertical

[Ergus]

Hi Joao

The changes you show are the old version of the branch where I was
trying some experiments... I should have called it "scratch" to that
first version. I understand your concerns now. But not, this is not the
new version

I cleaned all of it in a recent forced push some weeks ago. Please give
a look at the new version. It is simpler and doesn't give any change in
the interface.

Ergus



On Mon, Oct 05, 2020 at 10:06:22AM +0100, Jo�o T�vora wrote:
>Ergus <spacibba@aol.com> writes:
>
>> On Mon, Oct 05, 2020 at 12:47:39AM +0100, Jo�o T�vora wrote:
>>
>> Hi Joao:
>>
>> Please look at the previous messages in this same thread to see the
>> problems with your approach.
>>
>> So far there is:
>>
>> 1) The problem when using different font in minibuffer reported by
>> Jixiuf 2) the long line issue from Gregory (but also mentioned before)
>> 3) the ellipsis issue mentioned by Eli
>> 4) The prompt issue (which seems is not going to be solved as it is more
>> a design choice or a feature than a real issue)
>> 5) Respect the user customs max-mini-window-height and
>> resize-mini-windows so we shouldn't call enlarge-window and as an
>> core package we must not conflict with such customs.
>> 6) The removal of {} in vertical mode because in that case they are
>> redundant.
>> 7) The compatibility with packages like maple-minibuffer reported by
>> Dimitry.
>> 8) The case when using multiline separator.
>> 9) Move the first candidate to a newline instead of keeping it inline.
>>
>> The patch I propose is not complex at all considering all the above
>> conditions. The only complexity comes from the height calculation in
>> pixels which is something that will need to be done in any case either
>> for 1, 2, 3 or 4.
>
>Thanks for listing these again, for my benefit.  I've not been following
>this very closely, and needed a summary to give me context after trying
>your patch again, as you requested.
>
>To be clear: I'm not "against" this functionality or even against it
>becoming a part of Emacs or Emacs core.  I think solving the problems
>you list above is fine (though I haven't been by them, at least not yet)
>
>I'm not too much concerned about the complexity of implementation (such
>as the height calculation).  I am concerned the changes to the user
>interface, of which your patch seems to bring many, or at least more
>than I would have expected.  It seems to be that your solution mixes
>bugfixes and new features, which is what I object to.  I think your
>solution could be trimmed down so that it solves _only_ the technical
>problems you list above without adding new user interfaces.
>
>In other words, don't see why the points above, which sound like
>basically bugs, need any new "defcustom".  This is why I asked you to
>(re)list them.  Then I can start with my naive implementation and fix
>them one by one, without adding new interfaces.  Of course, then I will
>be repeating some work you've already done, so this is why I'm arguing
>for this separation in _your_ work.
>
>Could you structure your work so that one part fixes the above problems,
>and another part adds the new features, like customizing bracket types
>and such, and is should be discussed as a separate matter.
>
>After my sig, some comments that illustrate my points.
>
>I took this diff with git diff master...feature/icomplete-vertical
>
>Jo�o
>
>> -(defcustom icomplete-separator " | "
>> -  "String used by Icomplete to separate alternatives in the minibuffer."
>> -  :type 'string
>> -  :version "24.4")
>> +(defcustom icomplete-vertical nil
>> +  "Enable `icomplete' vertical mode."
>> +  :type 'bool
>> +  :version "28.1")
>
>I would think it more elegant to keep `icomplete-separator` as the entry
>point to verticality.  Certainly it should be possible to analyse the
>string and decide whether it implies verticality and needs special
>worries.
>
>  (defcustom icomplete-hide-common-prefix t
>   "When non-nil, hide common prefix from completion candidates.
>@@ -97,6 +97,12 @@ icomplete-first-match
>   "Face used by Icomplete for highlighting first match."
>   :version "24.4")
>
>> +(defface icomplete-common-match '((t :inherit 'highlight
>> +                                     :underline t
>> +                                     :weight bold))
>> +  "Face used by Icomplete for highlighting common completion."
>> +  :version "28.1")
>
>
>What is a "common match"?  Is this exclusive to icomplete verticalness?
>Doesn't seem so, is this not a side feature?
>
>> -(defcustom icomplete-minibuffer-setup-hook nil
>> +(defvar icomplete-ellipsis nil)
>> +
>> +(defcustom icomplete--minibuffer-setup-hook nil
>>    "Icomplete-specific customization of minibuffer setup.
>
>Why has this minibuffer hook been made an internal variable?
>
>> -    (define-key map (kbd "<right>") 'icomplete-forward-completions)
>> -    (define-key map (kbd "<left>") 'icomplete-backward-completions)
>
>Why have these been removed from icomplete-fido-mode-map?  Also, I have
>C-r and C-r in that map, will it work in the verticality?  I would hope
>it would, as it does not in my naive verticality implementation.
>
>> -;;;_ > icomplete-minibuffer-setup ()
>> -(defun icomplete-minibuffer-setup ()
>> +;; Vertical functions
>> +
>> +(defcustom icomplete-separator-vertical " \n"
>> +  "String used by Icomplete to separate alternatives in the minibuffer."
>> +  :type 'string
>> +  :version "28.1")
>> +
>> +(defcustom icomplete-list-indicators-vertical (cons "" "")
>> +  "Indicator bounds to list alternatives in the minibuffer."
>> +  :type 'string
>> +  :version "28.1")
>> +
>> +(defcustom icomplete-require-indicators-vertical (cons "" "")
>> +  "Indicator bounds for match in the minibuffer when require-match."
>> +  :type 'string
>> +  :version "28.1")
>> +
>> +(defcustom icomplete-not-require-indicators-vertical (cons "" "")
>> +  "Indicator bounds for match in the minibuffer when not require-match."
>> +  :type 'string
>> +  :version "28.1")
>
>These are examples of "interface bloat" that do not seem to directly
>address the 9 problems you listed in the beginning of the message
>(again, I am not against these features per se, mind you)
>
>> +(defvar icomplete--vertical-mode-map
>
>If this keymap is "internal" (as indicated by the two "--") how am I
>supposed to customize the keybindings?
>
>> +(defcustom icomplete-list-indicators-horizontal (cons "{" "}")
>> +  "Indicator bounds to list alternatives in the minibuffer."
>> +  :type 'string
>> +  :version "28.1")
>> +
>> +(defcustom icomplete-require-indicators-horizontal (cons "(" ")")
>> +  "Indicator bounds for match in the minibuffer when require-match."
>> +  :type 'string
>> +  :version "28.1")
>> +
>> +(defcustom icomplete-not-require-indicators-horizontal (cons "[" "]")
>> +  "Indicator bounds for match in the minibuffer when not require-match."
>> +  :type 'string
>> +  :version "28.1")
>
>Again, these seem like improvements that are not directly related to
>verticality.  (Maybe they are very good improvements, but still...
>
>Jo�o
>

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> I did not see objections from Stefan.  And the objection of Eli is 
>> only, AFAIU, that in a longer term a better solution should be 
>> implemented.
>
> No, my objections are that your proposal is not safe wrt recursive 
> editing, and will affect uses of mini-windows other than prompting the 
> user, where these effects aren't intended.
>
> AFAIU, Stefan agreed that this is a problem in your proposal.
>

I don't know which proposal you refer to in that sentence, but I guess 
it's the (second) patch I sent on bug#43572 a week ago, against Emacs C 
core.

The solution in this thread works at the Lisp level, and will not affect 
other uses of the miniwindow, because it relies on a minibuffer-local 
variable.  AFAICS it is also safe wrt recursive editing.

Re: feature/icomplete-vertical

[João Távora]

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

On Mon, Oct 5, 2020 at 1:23 PM Ergus <spacibba@aol.com> wrote:
>
> Hi Joao
>
> The changes you show are the old version of the branch where I was
> trying some experiments... I should have called it "scratch" to that
> first version. I understand your concerns now. But not, this is not the
> new version
>
> I cleaned all of it in a recent forced push some weeks ago. Please give
> a look at the new version. It is simpler and doesn't give any change in
> the interface.

OK, I see it now. GreAat!  If that is the case, then I don't have
anything against it in principle. How does it solve the problem
that Gregory is also trying to solve?

João

[-- Attachment #2: Type: text/html, Size: 904 bytes --]

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Mon, 05 Oct 2020 12:25:13 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: spacibba@aol.com, emacs-devel@gnu.org, casouri@gmail.com,
>         joaotavora@gmail.com, juri@linkov.net
> 
> > No, my objections are that your proposal is not safe wrt recursive 
> > editing, and will affect uses of mini-windows other than prompting the 
> > user, where these effects aren't intended.
> >
> > AFAIU, Stefan agreed that this is a problem in your proposal.
> >
> 
> I don't know which proposal you refer to in that sentence, but I guess 
> it's the (second) patch I sent on bug#43572 a week ago, against Emacs C 
> core.

I referred to the same proposal to which João referred.

> The solution in this thread works at the Lisp level, and will not affect 
> other uses of the miniwindow, because it relies on a minibuffer-local 
> variable.  AFAICS it is also safe wrt recursive editing.

Using a buffer-local variable doesn't necessarily eliminate the
problems with recursive-edit.  And using window-scroll-functions for
this purpose is a no-no.  So I cannot endorse this others solution,
either.

Re: feature/icomplete-vertical

[Ergus]

On Mon, Oct 05, 2020 at 01:28:57PM +0100, Jo�o T�vora wrote:
>On Mon, Oct 5, 2020 at 1:23 PM Ergus <spacibba@aol.com> wrote:
>>
>> Hi Joao
>>
>> The changes you show are the old version of the branch where I was
>> trying some experiments... I should have called it "scratch" to that
>> first version. I understand your concerns now. But not, this is not the
>> new version
>>
>> I cleaned all of it in a recent forced push some weeks ago. Please give
>> a look at the new version. It is simpler and doesn't give any change in
>> the interface.
>
>OK, I see it now. GreAat!  If that is the case, then I don't have
>anything against it in principle. How does it solve the problem
>that Gregory is also trying to solve?
>
Everything is solved by truncating the candidates properly considering
the max-mini-window-height, font size, window-text-pixel-size and so on
in advance.

As I said, the prompt problem is actually a design choice with some
"bad" consequences in our case, but not in general.

>Jo�o

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Mon, 05 Oct 2020 12:54:52 +0100
>> Cc: ghe@sdf.org, spacibba@aol.com, emacs-devel@gnu.org, casouri@gmail.com,
>>  juri@linkov.net
>> 
>> > We could write a similar function to do what you want, but please note
>> > that such a function will not understand the semantics of the text, so
>> > it could suggest truncating it at some place which makes no sense.
>> > If the caller would like a smarter 'service", we will have to come up
>> > with some method of telling the function where it makes sense to
>> > truncate the text.
>> 
>> Right. My idea is that a "dumb" service gives us a text position and the
>> application, which does know the semantics of the things being
>> displayed, decides based on that.
>> 
>> For example, icomplete would remove the candidate where the truncation
>> happened and replace it with an elipsis, whereas eldoc would remove the
>> piece of documentation entirely and maybe replace it with a hint to
>> visit the *eldoc* buffer.
>
> In use cases that are not the most trivial ones, knowing what to
> remove could not be easy.  For example, what if the part that exceeds
> the window dimensions comes from display or overlay string?  Also,
> does the application tolerate partially-visible lines?  So some
> additional information will need to be made available to the function,
> I think.

I think the easiest way to make this function is to give it a buffer
where text exists as buffer text and truncate-lines is what it it.  Then
it can say exactly where it _would_ cut off.  The application can then
use that data to inform the display of text and/or overlay strings, and
potentially remove "semantic elements" accordingly.

João

Re: feature/icomplete-vertical

[Stefan Monnier]

> The truncation is done by icomplete, not by the display engine.  The
> "warning" is supposed to be in the form of the ellipsis displayed
> after the last candidate that is shown.

BTW, I think this is a UI mistake in Icomplete.

The simpler way to solve this problem is to replace the "ellipsis to
indicate truncation" with a "thingy to show the end of the list", so if
the output is truncated by the display you'll notice it because of the
lack of "thingy".

In the "horizontal" output, I think the most natural choice for the
"thingy" would be the closing "}".


        Stefan


PS: And no, this is still not a solution for the case where you want
the vertical icomplete list to appear *above* the prompt.  :-(

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> The solution in this thread works at the Lisp level, and will not 
>> affect other uses of the miniwindow, because it relies on a 
>> minibuffer-local variable.  AFAICS it is also safe wrt recursive 
>> editing.
>
> Using a buffer-local variable doesn't necessarily eliminate the problems 
> with recursive-edit. And using window-scroll-functions for this purpose 
> is a no-no.  So I cannot endorse this others solution, either.
>

Once again I have to stop a discussion with you, because I do not want to 
start a public dispute with you.  You reject everything I propose with 
vague statements about "potential problems", without any detail or recipe 
that would help me or others to see whether these "potential problems" are 
real or minor ones.

Note that what you object to (using set-window-start in 
window-scroll-functions) is, in fact, already used twice in Emacs core.

In em-smart.el, eshell-smart-initialize adds eshell-smart-scroll-window to 
window-scroll-functions, eshell-smart-scroll-window calls 
eshell-smart-redisplay, and eshell-smart-redisplay uses set-window-start. 
Likewise, follow.el adds follow-avoid-tail-recenter to 
window-scroll-functions, and follow-avoid-tail-recenter uses 
set-window-start.

In both cases, the code was already present in Emacs 21.  Which means that 
it has been used for twenty years without known problems.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
>   casouri@gmail.com,  juri@linkov.net
> Date: Mon, 05 Oct 2020 14:07:05 +0100
> 
> I think the easiest way to make this function is to give it a buffer
> where text exists as buffer text and truncate-lines is what it it.  Then
> it can say exactly where it _would_ cut off.

That doesn't work for displaying text with continuation lines, does
it?

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Mon, 05 Oct 2020 13:19:53 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: spacibba@aol.com, emacs-devel@gnu.org, casouri@gmail.com,
>         joaotavora@gmail.com, juri@linkov.net
> 
> > Using a buffer-local variable doesn't necessarily eliminate the problems 
> > with recursive-edit. And using window-scroll-functions for this purpose 
> > is a no-no.  So I cannot endorse this others solution, either.
> 
> Once again I have to stop a discussion with you, because I do not want to 
> start a public dispute with you.

You say that, and then you start a public dispute anyway.  Not sure
what to make of that.

> You reject everything I propose with vague statements about
> "potential problems", without any detail or recipe that would help
> me or others to see whether these "potential problems" are real or
> minor ones.

I'm not sure what detail I need to provide.  Isn't it clear that using
a buffer-local variable doesn't resolve problems with using that same
buffer in another level of recursive-edit?  Or that changing the
window-start position from under the feet of the display engine is
something that should be avoided?  Why would you need examples to
realize that a design based on this cannot be clean, in the sense that
use cases where it causes trouble will eventually surface?

> Note that what you object to (using set-window-start in 
> window-scroll-functions) is, in fact, already used twice in Emacs core.
> 
> In em-smart.el, eshell-smart-initialize adds eshell-smart-scroll-window to 
> window-scroll-functions, eshell-smart-scroll-window calls 
> eshell-smart-redisplay, and eshell-smart-redisplay uses set-window-start. 
> Likewise, follow.el adds follow-avoid-tail-recenter to 
> window-scroll-functions, and follow-avoid-tail-recenter uses 
> set-window-start.
> 
> In both cases, the code was already present in Emacs 21.  Which means that 
> it has been used for twenty years without known problems.

We routinely fix bugs in code that was written 20 and 30 years ago.
So the age of a problematic code doesn't mean it won't one day surface
as a bug.  As users and third-party packages write more and more
complex code for Emacs, we discover bugs that lay hidden for many
years.

Yes, the display engine includes some extra safety nets that might let
these bad practices work, at least in some use cases.  But that
doesn't mean the warnings in the documentation and in this discussion
should be disregarded.  You have just shown here how that can lead to
redisplay glitches.

Re: feature/icomplete-vertical

[martin rudalics]

 > Just calculating the window dimensions is already a nontrivial
 > task. There are two cases: a miniwindow-only frame, and the "normal"
 > case.  In the first case you need to use frame-height, and multiply it
 > with the pixel height of the "default" face.  In the second case you
 > can get the maximal dimensions with max-mini-window-height, and
 > multiply it by the pixel height of the "default" face.  But you cannot
 > just multiply max-mini-window-height by that height, that would be too
 > easy.  There are again two cases: either max-mini-window-height is an
 > integer, in which case you can just do that multiplication, or it is a
 > floating point number, in which case you have to multiply that number
 > by frame-height and truncate it, and multiply the resulting number by
 > the pixel height of the "default" face...

You underestimate the complexity of resizing the minibuffer window.
'max-mini-window-height' and, say 'frame-pixel-height', are by no means
sufficient to determine whether the minibuffer window can be really made
that large.  You have to check whether the remaining windows on the same
frame can be made sufficiently small in order to accommodate the
enlarged minibuffer window, including the case where you have
fixed-height or height-preserved windows.  Look at the code of
'window--resize-mini-window' and 'window--resize-root-window-vertically'
to see how this can be done.  And even if your code works, you may have
modified the start positions of many other windows on the same frame
when the minibuffer window shrinks back.

I'd never use the default minibuffer window for displaying larger lists
of vertically arranged objects.  On GUIs use a separate child frame, on
TTYs a side window instead.

martin

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>> Just calculating the window dimensions is already a nontrivial task. 
>> There are two cases: a miniwindow-only frame, and the "normal" case. 
>> In the first case you need to use frame-height, and multiply it with 
>> the pixel height of the "default" face.  In the second case you can get 
>> the maximal dimensions with max-mini-window-height, and multiply it by 
>> the pixel height of the "default" face.  But you cannot just multiply 
>> max-mini-window-height by that height, that would be too easy.  There 
>> are again two cases: either max-mini-window-height is an integer, in 
>> which case you can just do that multiplication, or it is a floating 
>> point number, in which case you have to multiply that number by 
>> frame-height and truncate it, and multiply the resulting number by the 
>> pixel height of the "default" face...
>
> You underestimate the complexity of resizing the minibuffer window.
>

I do not, quite the contrary.  Indeed the presentation above (whose 
purpose was to show that doing this is too complex) is simplified.

>
> And even if your code works,
>

It's not my code, quite the contrary.  I explained in every possible 
detail that nobody should have to do this.

>
> I'd never use the default minibuffer window for displaying larger lists 
> of vertically arranged objects.  On GUIs use a separate child frame, on 
> TTYs a side window instead.
>

There is no reason to not do something that can be done easily.  You can 
look at and try the code I sent a few hours ago, it works perfectly well 
both in GUIs and on TTYs, with Emacs 24 to 28.

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
>>   casouri@gmail.com,  juri@linkov.net
>> Date: Mon, 05 Oct 2020 14:07:05 +0100
>> 
>> I think the easiest way to make this function is to give it a buffer
>> where text exists as buffer text and truncate-lines is what it it.  Then
>> it can say exactly where it _would_ cut off.
>
> That doesn't work for displaying text with continuation lines, does
> it?

I'm not sure what you mean.  Maybe?  Can you provide an example?  The
idea is to use this buffer to "stage" what needs to be displayed, and
gather guiding information from that, not necessarily to _do_ that
displaying on that particular "stage".

João

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
>   casouri@gmail.com,  juri@linkov.net
> Date: Mon, 05 Oct 2020 20:03:32 +0100
> 
> >> I think the easiest way to make this function is to give it a buffer
> >> where text exists as buffer text and truncate-lines is what it it.  Then
> >> it can say exactly where it _would_ cut off.
> >
> > That doesn't work for displaying text with continuation lines, does
> > it?
> 
> I'm not sure what you mean.  Maybe?  Can you provide an example?  The
> idea is to use this buffer to "stage" what needs to be displayed, and
> gather guiding information from that, not necessarily to _do_ that
> displaying on that particular "stage".

If the real buffer will be displayed without truncating lines, then
each physical line might take more than one screen line.  You need to
account for that when you decide where to truncate.

But that is so obvious that I fear I'm missing something important
here.

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> You reject everything I propose with vague statements about
>> "potential problems", without any detail or recipe that would help
>> me or others to see whether these "potential problems" are real or
>> minor ones.
>
> I'm not sure what detail I need to provide.  Isn't it clear that using
> a buffer-local variable doesn't resolve problems with using that same
> buffer in another level of recursive-edit?  Or that changing the
> window-start position from under the feet of the display engine is
> something that should be avoided?  Why would you need examples to
> realize that a design based on this cannot be clean, in the sense that
> use cases where it causes trouble will eventually surface?

I think can understand some of Gregory's frustration.  If those problems
are real, it should be possible to showcase them.  Though that's not
always easy to do, those examples could better illustrate where the
fractures in Gregory's design are, and encourage him to undestand the
problem and find alternatives.  Or, as we often do, clearly document
shortcomings and establish the specific application domain of a
particular technique.  I for one, wouldn't mind a fix that worked only
in Emacs 28, so maybe some C-level handholding can be added.

João

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
>>   casouri@gmail.com,  juri@linkov.net
>> Date: Mon, 05 Oct 2020 20:03:32 +0100
>> 
>> >> I think the easiest way to make this function is to give it a buffer
>> >> where text exists as buffer text and truncate-lines is what it it.  Then
>> >> it can say exactly where it _would_ cut off.
>> >
>> > That doesn't work for displaying text with continuation lines, does
>> > it?
>> 
>> I'm not sure what you mean.  Maybe?  Can you provide an example?  The
>> idea is to use this buffer to "stage" what needs to be displayed, and
>> gather guiding information from that, not necessarily to _do_ that
>> displaying on that particular "stage".
>
> If the real buffer will be displayed without truncating lines, then
> each physical line might take more than one screen line.  You need to
> account for that when you decide where to truncate.

Yes, but presumably, the (vapourware) function we are talking about
would interpret the value of truncate-lines "in the stage", and you'd
set that to the same value that you plan to use in the "real buffer".

João

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: Gregory Heytings <ghe@sdf.org>,  spacibba@aol.com,  emacs-devel@gnu.org,
>   casouri@gmail.com,  juri@linkov.net
> Date: Mon, 05 Oct 2020 20:14:30 +0100
> 
> > I'm not sure what detail I need to provide.  Isn't it clear that using
> > a buffer-local variable doesn't resolve problems with using that same
> > buffer in another level of recursive-edit?  Or that changing the
> > window-start position from under the feet of the display engine is
> > something that should be avoided?  Why would you need examples to
> > realize that a design based on this cannot be clean, in the sense that
> > use cases where it causes trouble will eventually surface?
> 
> I think can understand some of Gregory's frustration.  If those problems
> are real, it should be possible to showcase them.

The second one was showcased by Gregory himself.

The first one should be clear: binding a local variable to a value
will have that value in effect for all the recursive uses of the same
minibuffer, something that isn't necessarily expected by the nested
users.

> Though that's not always easy to do, those examples could better
> illustrate where the fractures in Gregory's design are, and
> encourage him to undestand the problem and find alternatives.

Sorry, I'm unable to encourage Gregory to understand the shortcomings,
no matter how much I tried, I guess it's my fault.  Although it sounds
like the same situation is repeating with Martin's advice regarding
mini-window resizing, so maybe it isn't entirely my fault after all.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
>   casouri@gmail.com,  juri@linkov.net
> Date: Mon, 05 Oct 2020 20:19:22 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: João Távora <joaotavora@gmail.com>
> >> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
> >>   casouri@gmail.com,  juri@linkov.net
> >> Date: Mon, 05 Oct 2020 20:03:32 +0100
> >> 
> >> >> I think the easiest way to make this function is to give it a buffer
> >> >> where text exists as buffer text and truncate-lines is what it it.  Then
> >> >> it can say exactly where it _would_ cut off.
> >> >
> >> > That doesn't work for displaying text with continuation lines, does
> >> > it?
> >> 
> >> I'm not sure what you mean.  Maybe?  Can you provide an example?  The
> >> idea is to use this buffer to "stage" what needs to be displayed, and
> >> gather guiding information from that, not necessarily to _do_ that
> >> displaying on that particular "stage".
> >
> > If the real buffer will be displayed without truncating lines, then
> > each physical line might take more than one screen line.  You need to
> > account for that when you decide where to truncate.
> 
> Yes, but presumably, the (vapourware) function we are talking about
> would interpret the value of truncate-lines "in the stage", and you'd
> set that to the same value that you plan to use in the "real buffer".

I guess I misunderstand your original proposal: did you mean to leave
truncate-lines at its value in the window where the text will be
displayed?  If so, that's OK, but then why did you mentioned that
variable? there are many more that affect the display, and they all
should be left at their values in the window.

In a nutshell, the function we are discussing should get the window in
which the text will be displayed as the argument, so that its layout
calculations are accurate.  The code should be very similar to
window-text-pixel-size, just the stop condition should be different.

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think can understand some of Gregory's frustration.  If those problems
>> are real, it should be possible to showcase them.
> The second one was showcased by Gregory himself.

If I read that correctly that was a synthethic, not a real-life example.

> Sorry, I'm unable to encourage Gregory to understand the shortcomings,
> no matter how much I tried, I guess it's my fault.

It's not a question of "fault" or persuasion, it's a simple question
that code speaks louder than words: good examples can be taken through a
debugger and revealing where in actual code the shortcomings of a given
approach manifests themselves.

> Although it sounds like the same situation is repeating with Martin's
> advice regarding mini-window resizing, so maybe it isn't entirely my
> fault after all.

If I read correctly, Martin reinforced how non-trivial calculations
regarding mini-window sizing is, thus _strenghtening_ the case for
having a solution where the application isn't responsible for that, as
you suggest.

João

Re: feature/icomplete-vertical

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

> I guess I misunderstand your original proposal: did you mean to leave
> truncate-lines at its value in the window where the text will be
> displayed?  If so, that's OK, but then why did you mentioned that
> variable? there are many more that affect the display, and they all
> should be left at their values in the window.

I mentioned it because it's a particularly notable one (and I'm not
immediately familiar with many others).  Didn't imagine it would throw
you off, sorry.

> In a nutshell, the function we are discussing should get the window in
> which the text will be displayed as the argument, so that its layout
> calculations are accurate.  The code should be very similar to
> window-text-pixel-size, just the stop condition should be different.

Yes, exactly.

João

Re: feature/icomplete-vertical

[Gregory Heytings via Emacs development discussions.]

>>> I'm not sure what detail I need to provide.  Isn't it clear that using 
>>> a buffer-local variable doesn't resolve problems with using that same 
>>> buffer in another level of recursive-edit?  Or that changing the 
>>> window-start position from under the feet of the display engine is 
>>> something that should be avoided?  Why would you need examples to 
>>> realize that a design based on this cannot be clean, in the sense that 
>>> use cases where it causes trouble will eventually surface?
>>
>> I think can understand some of Gregory's frustration.  If those 
>> problems are real, it should be possible to showcase them.
>
> The second one was showcased by Gregory himself.
>

It's unbelievable you dare to write such a sentence, and from my point of 
view makes it clear how hard it is to have a discussion with you.  For 
those who are not aware of the context:

- I explained (to Stefan) what a part of my code did, and said "it does 
this because otherwise point could become invisible during a second or 
two"

- Eli jumps in the conversation and says "no, that's not possible, point 
can never become invisible, if it does it's a bug, please tell me how this 
can happen"

- I write a recipe to demonstrate how this can happen

- Eli replies that, even though point becomes invisible with the recipe, 
this is after all not a bug, and that the bug is in the recipe

- Eli now uses the recipe I sent him as an argument to explain that my 
code, in which I (obviously) do not do what I did in the recipe, can cause 
"potential problems"

How could anyone remain calm with this?  I can't.

Re: feature/icomplete-vertical

[João Távora]

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

On Mon, Oct 5, 2020 at 8:44 PM Gregory Heytings <ghe@sdf.org> wrote:

>
> How could anyone remain calm with this?  I can't.
>

But please do :-)

João

[-- Attachment #2: Type: text/html, Size: 505 bytes --]

Re: feature/icomplete-vertical

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: ghe@sdf.org,  spacibba@aol.com,  emacs-devel@gnu.org,
>   casouri@gmail.com,  juri@linkov.net
> Date: Mon, 05 Oct 2020 20:32:26 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I think can understand some of Gregory's frustration.  If those problems
> >> are real, it should be possible to showcase them.
> > The second one was showcased by Gregory himself.
> 
> If I read that correctly that was a synthethic, not a real-life example.

I fail to see how that is a disadvantage.  Most examples that
illustrate some point are synthetic, because real-life examples run
the risk of hiding the important stuff behind gobs of irrelevant code.
That's why, for example, we ask for simple reproduction recipes when
people report bugs.

> > Sorry, I'm unable to encourage Gregory to understand the shortcomings,
> > no matter how much I tried, I guess it's my fault.
> 
> It's not a question of "fault" or persuasion, it's a simple question
> that code speaks louder than words: good examples can be taken through a
> debugger and revealing where in actual code the shortcomings of a given
> approach manifests themselves.

I'm sorry, but I can only afford hand-holding up to a limit.  I
provided explanations and described how the relevant parts of the
display engine work.  I also reiterated considerations that should be
apparent even without any details, such as that global variables get
in the way of reentrancy, even if they are buffer-local variables.  I
invite you to read the 3 relevant threads and see for yourself.  If
that is not enough, and if the Emacs code doesn't speak loud enough to
be of help, then Gregory should simply take my word for it, as someone
who has many more hours hacking Emacs than he does.  I simply cannot
afford investing so much of the little time I have into educating
people here, and expecting that would be unreasonable and even unfair.

> > Although it sounds like the same situation is repeating with Martin's
> > advice regarding mini-window resizing, so maybe it isn't entirely my
> > fault after all.
> 
> If I read correctly, Martin reinforced how non-trivial calculations
> regarding mini-window sizing is, thus _strenghtening_ the case for
> having a solution where the application isn't responsible for that, as
> you suggest.

That's not my interpretation.  Gregory said that measuring the
pixel-size of text is hard, to which Martin replied that resizing the
mini-window accurately can be even harder.  Then Gregory dismissed
Martin's opinion out of hand.  Martin has more experience with the
window- and frame-related code in Emacs than all the rest of us, so
dismissing his opinions on that is not recommended.

Re: feature/icomplete-vertical

[Eli Zaretskii]

> Date: Mon, 05 Oct 2020 19:44:37 +0000
> From: Gregory Heytings <ghe@sdf.org>
> cc: João Távora <joaotavora@gmail.com>, spacibba@aol.com,
>         emacs-devel@gnu.org, casouri@gmail.com, juri@linkov.net
> 
> - Eli now uses the recipe I sent him as an argument to explain that my 
> code, in which I (obviously) do not do what I did in the recipe, can cause 
> "potential problems"

That's not what I said in response to João.  I said that the recipe
showcases how using window-scroll-functions to affect redisplay can
cause bugs.