bug#17809: 24.4.50; Completions display

bug#17809: 24.4.50; Completions display

[Juri Linkov]

Severity: wishlist
Tags: patch

This is a follow-up to bug#17554.  There are two existing
display actions that could help to better display *Completions*:

- display-buffer-at-bottom for minibuffer-completion-help
  to display *Completions* near the active minibuffer
  where it would be easier to find them;

- display-buffer-below-selected for completion-at-point
  that could work like displaying *Marking Files* in Dired
  to display *Completions* near the point of completion
  in the current buffer.

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-06-02 00:18:22 +0000
+++ lisp/minibuffer.el	2014-06-19 06:55:32 +0000
@@ -1796,7 +1796,25 @@ (defun minibuffer-completion-help (&opti
              ;; window, mark it as softly-dedicated, so bury-buffer in
              ;; minibuffer-hide-completions will know whether to
              ;; delete the window or not.
-             (display-buffer-mark-dedicated 'soft))
+             (display-buffer-mark-dedicated 'soft)
+             ;; Disable `pop-up-windows' temporarily to allow
+             ;; `display-buffer--maybe-pop-up-frame-or-window'
+             ;; in the overridden actions below to pop up a frame
+             ;; if `pop-up-frames' is non-nil, but not to pop up a window.
+             (pop-up-windows nil)
+             (display-buffer-base-action
+              ;; First check for defined `display-buffer-base-action'
+              ;; that might be already bound by `completion-at-point'.
+              (or (and (car-safe display-buffer-base-action)
+                       display-buffer-base-action)
+                  ;; This is a copy of `display-buffer-fallback-action'
+                  ;; where `display-buffer-use-some-window' is replaced
+                  ;; with `display-buffer-at-bottom'.
+                  '((display-buffer--maybe-same-window
+                     display-buffer-reuse-window
+                     display-buffer--maybe-pop-up-frame-or-window
+                     display-buffer-at-bottom)
+                    (window-height . fit-window-to-buffer)))))
         (with-output-to-temp-buffer "*Completions*"
           ;; Remove the base-size tail because `sort' requires a properly
           ;; nil-terminated list.
@@ -2073,7 +2091,17 @@ (defun completion-at-point ()
 The completion method is determined by `completion-at-point-functions'."
   (interactive)
   (let ((res (run-hook-wrapped 'completion-at-point-functions
-                               #'completion--capf-wrapper 'all)))
+                               #'completion--capf-wrapper 'all))
+        ;; Display buffer like in `minibuffer-completion-help',
+        ;; but instead of `display-buffer-at-bottom'
+        ;; use `display-buffer-below-selected'.
+        (pop-up-windows nil)
+        (display-buffer-base-action
+         '((display-buffer--maybe-same-window
+            display-buffer-reuse-window
+            display-buffer--maybe-pop-up-frame-or-window
+            display-buffer-below-selected)
+           (window-height . shrink-window-if-larger-than-buffer))))
     (pcase res
       (`(,_ . ,(and (pred functionp) f)) (funcall f))
       (`(,hookfun . (,start ,end ,collection . ,plist))

bug#17809: 24.4.50; Completions display

[Juri Linkov]

When the width of the window with *Completions* is more
than the default 80 columns, it leaves too much empty space.

So it's better first to display an empty buffer,
and then fill it with the contents using its final width,
but currently this is not supported.

In `with-output-to-temp-buffer' I tried to reverse
the order of lines:

  (internal-temp-output-buffer-show ,buf)
  (progn ,@body)

and it seems to work, but the problem is that
an alist of display-buffer with

  (window-height . fit-window-to-buffer)

is performed on the empty buffer, and reduces it to 3 lines.
Then later (progn ,@body) fills it with completions,
but the window height remains 3 lines.

I see no way to perform these steps using the current
display-buffer actions:

1. display the buffer
2. fill it
3. call action alists with fit-window-to-buffer

bug#17809: 24.4.50; Completions display

[Juri Linkov]

> Alternatively, we could display the buffer first, look at what size we
> get, fill the buffer, and possibly resize the window afterwards.

I tried to do this in bug#17809, but stumbled on the problem
that resizing the window after filling is not supported
by display-buffer actions alist.

> For `with-temp-buffer-window' this means that we would have to fill the
> buffer either via `temp-buffer-window-show-hook' or in QUIT-FUNCTION.

Maybe filling the buffer is possible in QUIT-FUNCTION, but instead of
`with-temp-buffer-window', `minibuffer-completion-help' uses
`with-output-to-temp-buffer' that has no QUIT-FUNCTION arg.

bug#17809: 24.4.50; Completions display

[martin rudalics]

 > I tried to do this in bug#17809, but stumbled on the problem
 > that resizing the window after filling is not supported
 > by display-buffer actions alist.

`display-buffer' obviously expects the buffer to be "ready" at the time
of fitting the window to it.

 >> For `with-temp-buffer-window' this means that we would have to fill the
 >> buffer either via `temp-buffer-window-show-hook' or in QUIT-FUNCTION.
 >
 > Maybe filling the buffer is possible in QUIT-FUNCTION, but instead of
 > `with-temp-buffer-window', `minibuffer-completion-help' uses
 > `with-output-to-temp-buffer' that has no QUIT-FUNCTION arg.

Then `temp-buffer-show-hook' should cut it.

martin

bug#17809: 24.4.50; Completions display

[Juri Linkov]

>>> For `with-temp-buffer-window' this means that we would have to fill the
>>> buffer either via `temp-buffer-window-show-hook' or in QUIT-FUNCTION.
>>
>> Maybe filling the buffer is possible in QUIT-FUNCTION, but instead of
>> `with-temp-buffer-window', `minibuffer-completion-help' uses
>> `with-output-to-temp-buffer' that has no QUIT-FUNCTION arg.
>
> Then `temp-buffer-show-hook' should cut it.

You mean calling `fit-window-to-buffer' in `temp-buffer-show-hook' explicitly
instead of using an action alist `(window-height . fit-window-to-buffer)'?

But the problem will still remain, and to solve it we need to fill the buffer
after displaying that would be possible only after changing the order of calls
in `with-output-to-temp-buffer' from

       (prog1 (progn ,@body)
         (internal-temp-output-buffer-show ,buf))

to

       (progn
         (internal-temp-output-buffer-show ,buf)
         (progn ,@body)

bug#17809: 24.4.50; Completions display

[Stefan Monnier]

> You mean calling `fit-window-to-buffer' in `temp-buffer-show-hook'
> explicitly instead of using an action alist `(window-height
> . fit-window-to-buffer)'?

IIUC that would mean that it couldn't be configured via
display-buffer-alist, right?


        Stefan

bug#17809: 24.4.50; Completions display

[martin rudalics]

 > You mean calling `fit-window-to-buffer' in `temp-buffer-show-hook' explicitly
 > instead of using an action alist `(window-height . fit-window-to-buffer)'?

Yes.

 > But the problem will still remain, and to solve it we need to fill the buffer
 > after displaying that would be possible only after changing the order of calls
 > in `with-output-to-temp-buffer' from
 >
 >         (prog1 (progn ,@body)
 >           (internal-temp-output-buffer-show ,buf))
 >
 > to
 >
 >         (progn
 >           (internal-temp-output-buffer-show ,buf)
 >           (progn ,@body)
 >

I miss you here.  My order would be:

(1) Get the buffer ready in BODY.

(2) Display it in `internal-temp-output-buffer-show'.

(3) Fill it in `temp-buffer-show-hook'.

martin

bug#17809: 24.4.50; Completions display

[martin rudalics]

 >> explicitly instead of using an action alist `(window-height
 >> . fit-window-to-buffer)'?
 >
 > IIUC that would mean that it couldn't be configured via
 > display-buffer-alist, right?

Obviously.  Do we want to control filling via `display-buffer'?

Anyway, the output happens via `with-output-to-temp-buffer' which has no
direct means to pass an appropriate action to `display-buffer'.  If we
used `with-temp-buffer-window' instead, we could pass an ACTION argument
to fill the buffer according to the size of the window used instead of
adjusting the window to the buffer size.  In that case the configuration
could be controlled via `display-buffer-alist'.

martin

bug#17809: 24.4.50; Completions display

[Juri Linkov]

>> But the problem will still remain, and to solve it we need to fill the buffer
>> after displaying that would be possible only after changing the order of calls
>> in `with-output-to-temp-buffer' from
>>
>>         (prog1 (progn ,@body)
>>           (internal-temp-output-buffer-show ,buf))
>>
>> to
>>
>>         (progn
>>           (internal-temp-output-buffer-show ,buf)
>>           (progn ,@body)
>>
>
> I miss you here.  My order would be:
>
> (1) Get the buffer ready in BODY.
>
> (2) Display it in `internal-temp-output-buffer-show'.
>
> (3) Fill it in `temp-buffer-show-hook'.

This is possible, but then (window-height . fit-window-to-buffer)
is called on an empty buffer.

I found a way to achieve a good result by calling
`with-output-to-temp-buffer' twice: first with
empty body that displays the buffer, and the second call
finds the displayed window, fills it, and calls
(window-height . fit-window-to-buffer) at the end:

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-06-02 00:18:22 +0000
+++ lisp/minibuffer.el	2014-06-27 23:47:38 +0000
@@ -1796,7 +1796,22 @@ (defun minibuffer-completion-help (&opti
              ;; window, mark it as softly-dedicated, so bury-buffer in
              ;; minibuffer-hide-completions will know whether to
              ;; delete the window or not.
-             (display-buffer-mark-dedicated 'soft))
+             (display-buffer-mark-dedicated 'soft)
+             (display-buffer-base-action
+              ;; This is a copy of `display-buffer-fallback-action'
+              ;; where `display-buffer-use-some-window' is replaced
+              ;; with `display-buffer-at-bottom'.
+              '((display-buffer--maybe-same-window
+                 display-buffer-reuse-window
+                 display-buffer--maybe-pop-up-frame-or-window
+                 display-buffer-at-bottom)
+                (window-height . fit-window-to-buffer))))
+        ;; Display the *Completions* buffer before inserting
+        ;; completion candidates to be able to fill the contents
+        ;; evenly using the final window width in the second call
+        ;; to `with-output-to-temp-buffer'.
+        (with-output-to-temp-buffer "*Completions*"
+          ;; Empty body
+          )
         (with-output-to-temp-buffer "*Completions*"
           ;; Remove the base-size tail because `sort' requires a properly
           ;; nil-terminated list.

bug#17809: 24.4.50; Completions display

[Juri Linkov]

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

>>> explicitly instead of using an action alist `(window-height
>>> . fit-window-to-buffer)'?
>>
>> IIUC that would mean that it couldn't be configured via
>> display-buffer-alist, right?
>
> Obviously.  Do we want to control filling via `display-buffer'?

Maybe it would be a good thing to add a new action parameter to
`display-buffer' that would be like the existing `quit-function'
in `with-current-buffer-window', and will call its body between
displaying the buffer and applying final actions alists like
(window-height . fit-window-to-buffer)

> Anyway, the output happens via `with-output-to-temp-buffer' which has no
> direct means to pass an appropriate action to `display-buffer'.  If we
> used `with-temp-buffer-window' instead, we could pass an ACTION argument
> to fill the buffer according to the size of the window used instead of
> adjusting the window to the buffer size.  In that case the configuration
> could be controlled via `display-buffer-alist'.

`dired-mark-pop-up' already uses `with-current-buffer-window' that is
like `with-temp-buffer-window', but still has the same problem
as the screenshot below demonstrates: it inserts the contents
to the buffer before displaying it, so it has no way to get the
final window dimensions while inserting the contents, and leaves
empty columns.

It's possible in `dired-mark-pop-up' to move formatting
(i.e. the call to `dired-format-columns-of-files')
from the BODY arg of `with-current-buffer-window'
to its QUIT-FUNCTION arg, and it will fill the empty columns,
because the formatting will happen after displaying the buffer.

But then (window-height . fit-window-to-buffer) will be called
on the empty buffer, that will always produce a small window
with 3-lines height.


[-- Attachment #2: completions.png --]
[-- Type: image/png, Size: 73885 bytes --]

bug#17809: 24.4.50; Completions display

[martin rudalics]

 >> (1) Get the buffer ready in BODY.
 >>
 >> (2) Display it in `internal-temp-output-buffer-show'.
 >>
 >> (3) Fill it in `temp-buffer-show-hook'.
 >
 > This is possible, but then (window-height . fit-window-to-buffer)
 > is called on an empty buffer.

Then the function called in (3) should do `fit-window-to-buffer' when
it's done with filling.

 > I found a way to achieve a good result by calling
 > `with-output-to-temp-buffer' twice: first with
 > empty body that displays the buffer, and the second call
 > finds the displayed window, fills it, and calls
 > (window-height . fit-window-to-buffer) at the end:

This sounds abusive.  And note that `display-buffer-base-action' is
a user variable.

martin

bug#17809: 24.4.50; Completions display

[martin rudalics]

 > Maybe it would be a good thing to add a new action parameter to
 > `display-buffer' that would be like the existing `quit-function'
 > in `with-current-buffer-window', and will call its body between
 > displaying the buffer and applying final actions alists like
 > (window-height . fit-window-to-buffer)

`display-buffer' has no body and hardly ever will get one.

 > `dired-mark-pop-up' already uses `with-current-buffer-window' that is
 > like `with-temp-buffer-window', but still has the same problem
 > as the screenshot below demonstrates: it inserts the contents
 > to the buffer before displaying it, so it has no way to get the
 > final window dimensions while inserting the contents, and leaves
 > empty columns.
 >
 > It's possible in `dired-mark-pop-up' to move formatting
 > (i.e. the call to `dired-format-columns-of-files')
 > from the BODY arg of `with-current-buffer-window'
 > to its QUIT-FUNCTION arg, and it will fill the empty columns,
 > because the formatting will happen after displaying the buffer.
 >
 > But then (window-height . fit-window-to-buffer) will be called
 > on the empty buffer, that will always produce a small window
 > with 3-lines height.

Then don't do that.  If you want to do both - formatting and fitting -
have QUIT-FUNCTION do that.  So in BODY put the text into the buffer and
in QUIT-FUNCTION first format the text according to what the window size
gives and then try to fit the window to the formatted text.

martin

bug#17809: 24.4.50; Completions display

[Juri Linkov]

>> But then (window-height . fit-window-to-buffer) will be called
>> on the empty buffer, that will always produce a small window
>> with 3-lines height.
>
> Then don't do that.  If you want to do both - formatting and fitting -
> have QUIT-FUNCTION do that.  So in BODY put the text into the buffer and
> in QUIT-FUNCTION first format the text according to what the window size
> gives and then try to fit the window to the formatted text.

Then the users will lose the ability to override window fitting
by redefining `window-height' in `display-buffer-alist'.
Not sure how important this is.

> And note that `display-buffer-base-action' is a user variable.

Isn't `display-buffer-alist' a customizable variable intended for users?

I see no other way for a function to override `display-buffer-fallback-action',
but still allow the users to customize it in `display-buffer-alist'.
Only `display-buffer-base-action' can be used for this.

bug#17809: 24.4.50; Completions display

[Juri Linkov]

forcemerge 17809 12618
thanks

[ I discovered the same bug report from Dani, so merging both. ]

>> Maybe it would be a good thing to add a new action parameter to
>> `display-buffer' that would be like the existing `quit-function'
>> in `with-current-buffer-window', and will call its body between
>> displaying the buffer and applying final actions alists like
>> (window-height . fit-window-to-buffer)
>
> `display-buffer' has no body and hardly ever will get one.

I really see no difference between the already existing
QUIT-FUNCTION and a new possible BODY-FUNCTION.

Currently QUIT-FUNCTION is called this way in
`with-current-buffer-window':

       (if (functionp ,quit-function)
           (funcall ,quit-function ,window ,value)

and BODY-FUNCTION could be called in `window--display-buffer'
with something like:

       (if (functionp (cdr (assq 'body-function alist)))
           (funcall (cdr (assq 'body-function alist)))

Then, for instance, `dired-mark-pop-up' instead of:

        (with-current-buffer-window
         buffer
         (cons 'display-buffer-below-selected
               '((window-height . fit-window-to-buffer)))
         #'(lambda (window _value)
             (with-selected-window window
               (with-current-buffer buffer
                 (let ((inhibit-read-only t))
                   (dired-format-columns-of-files
                    (if (eq (car files) t) (cdr files) files))

will use:

        (with-current-buffer-window
         buffer
         (cons 'display-buffer-below-selected
               '((window-height . fit-window-to-buffer)
                 (body-function .
                  (lambda ()
                    (with-current-buffer buffer
                      (let ((inhibit-read-only t))
                        (dired-format-columns-of-files
                         (if (eq (car files) t) (cdr files) files))

Is there a problem with this approach?

bug#17809: 24.4.50; Completions display

[Juri Linkov]

>> Maybe it would be a good thing to add a new action parameter to
>> `display-buffer' that would be like the existing `quit-function'
>> in `with-current-buffer-window', and will call its body between
>> displaying the buffer and applying final actions alists like
>> (window-height . fit-window-to-buffer)
>
> `display-buffer' has no body and hardly ever will get one.

I agree that there is no way to add body to `display-buffer'.

So the cleanest solution would be to add a new macro
`with-displayed-buffer-window' that is like
`with-current-buffer-window' and `with-temp-buffer-window',
but will run its body in the displayed window.

I've tested it with different configurations, and it produces
nice results when used in the minibuffer, at point, in
horizontally split windows, in Dired *Marked Files*,
with pop-up-frames=t, etc.

=== modified file 'lisp/window.el'
--- lisp/window.el	2014-06-18 07:57:27 +0000
+++ lisp/window.el	2014-07-04 23:30:54 +0000
@@ -216,6 +216,35 @@ (defmacro with-current-buffer-window (bu
 	   (funcall ,quit-function ,window ,value)
 	 ,value))))
 
+(defmacro with-displayed-buffer-window (buffer-or-name action quit-function &rest body)
+  "Show a buffer BUFFER-OR-NAME and evaluate BODY in that buffer.
+This construct is like `with-current-buffer-window' but unlike that
+displays the buffer specified by BUFFER-OR-NAME before running BODY."
+  (declare (debug t))
+  (let ((buffer (make-symbol "buffer"))
+	(window (make-symbol "window"))
+	(value (make-symbol "value")))
+    `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
+	    (standard-output ,buffer)
+	    ,window ,value)
+       (with-current-buffer ,buffer
+	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
+
+       (let ((inhibit-read-only t)
+	     (inhibit-modification-hooks t))
+	 (setq ,value (progn ,@body)))
+
+       (with-selected-window ,window
+	 (goto-char (point-min)))
+
+       (when (functionp (cdr (assq 'window-height (cdr ,action))))
+	 (ignore-errors
+	   (funcall (cdr (assq 'window-height (cdr ,action))) ,window)))
+
+       (if (functionp ,quit-function)
+	   (funcall ,quit-function ,window ,value)
+	 ,value))))
+
 ;; The following two functions are like `window-next-sibling' and
 ;; `window-prev-sibling' but the WINDOW argument is _not_ optional (so
 ;; they don't substitute the selected window for nil), and they return

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-06-25 10:36:51 +0000
+++ lisp/minibuffer.el	2014-07-04 23:28:38 +0000
@@ -1794,8 +1794,29 @@ (defun minibuffer-completion-help (&opti
              ;; window, mark it as softly-dedicated, so bury-buffer in
              ;; minibuffer-hide-completions will know whether to
              ;; delete the window or not.
-             (display-buffer-mark-dedicated 'soft))
-        (with-output-to-temp-buffer "*Completions*"
+             (display-buffer-mark-dedicated 'soft)
+             ;; Disable `pop-up-windows' temporarily to allow
+             ;; `display-buffer--maybe-pop-up-frame-or-window'
+             ;; in the display actions below to pop up a frame
+             ;; if `pop-up-frames' is non-nil, but not to pop up a window.
+             (pop-up-windows nil))
+        (with-displayed-buffer-window
+	 "*Completions*"
+	 ;; This is a copy of `display-buffer-fallback-action'
+	 ;; where `display-buffer-use-some-window' is replaced
+	 ;; with `display-buffer-at-bottom'.
+	 `((display-buffer--maybe-same-window
+	    display-buffer-reuse-window
+	    display-buffer--maybe-pop-up-frame-or-window
+	    ;; Use `display-buffer-below-selected' for inline completions,
+	    ;; but not in the minibuffer (e.g. in `eval-expression')
+	    ;; for which `display-buffer-at-bottom' is used.
+	    ,(if (and completion-in-region-mode-predicate
+		      (not (minibuffer-selected-window)))
+		 'display-buffer-below-selected
+	       'display-buffer-at-bottom))
+	   (window-height . fit-window-to-buffer))
+	 nil
           ;; Remove the base-size tail because `sort' requires a properly
           ;; nil-terminated list.
           (when last (setcdr last nil))

=== modified file 'lisp/dired.el'
--- lisp/dired.el	2014-06-21 19:45:59 +0000
+++ lisp/dired.el	2014-07-04 23:30:52 +0000
@@ -3103,8 +3103,7 @@ (defun dired-mark-pop-up (buffer-or-name
 	  ;; Mark *Marked Files* window as softly-dedicated, to prevent
 	  ;; other buffers e.g. *Completions* from reusing it (bug#17554).
 	  (display-buffer-mark-dedicated 'soft))
-      (with-current-buffer buffer
-	(with-current-buffer-window
+      (with-displayed-buffer-window
 	 buffer
 	 (cons 'display-buffer-below-selected
 	       '((window-height . fit-window-to-buffer)))
@@ -3117,6 +3116,7 @@ (defun dired-mark-pop-up (buffer-or-name
 	 ;; Handle (t FILE) just like (FILE), here.  That value is
 	 ;; used (only in some cases), to mean just one file that was
 	 ;; marked, rather than the current line file.
+       (with-current-buffer buffer
 	 (dired-format-columns-of-files
 	  (if (eq (car files) t) (cdr files) files))
 	 (remove-text-properties (point-min) (point-max)

bug#17809: 24.4.50; Completions display

[Stefan Monnier]

> +(defmacro with-displayed-buffer-window (buffer-or-name action quit-function &rest body)

That looks OK.  Two details, tho:

> +	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
[...]
> +       (when (functionp (cdr (assq 'window-height (cdr ,action))))

Let's not evaluate `action' twice.  I.e. we need a (macroexp-let2 ..)
wrapper for it.


> +       (if (functionp ,quit-function)
> +	   (funcall ,quit-function ,window ,value)

Same for quit-function, and additionally, we want to make sure we
evaluate `quit-function' before `body'.


        Stefan

bug#17809: 24.4.50; Completions display

[Juri Linkov]

>> +(defmacro with-displayed-buffer-window (buffer-or-name action quit-function &rest body)
>
> That looks OK.  Two details, tho:
>
>> +	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
> [...]
>> +       (when (functionp (cdr (assq 'window-height (cdr ,action))))
>
> Let's not evaluate `action' twice.  I.e. we need a (macroexp-let2 ..)
> wrapper for it.

I added this now in the next patch.

>> +       (if (functionp ,quit-function)
>> +	   (funcall ,quit-function ,window ,value)
>
> Same for quit-function, and additionally, we want to make sure we
> evaluate `quit-function' before `body'.

`quit-function' is used in `dired-mark-pop-up' to activate the minibuffer,
so it should be the final thing after displaying the *Marked Files* buffer.
Maybe `quit-function' is not an appropriate name for such action?

=== modified file 'lisp/window.el'
--- lisp/window.el	2014-06-18 07:57:27 +0000
+++ lisp/window.el	2014-07-06 23:32:27 +0000
@@ -185,6 +185,7 @@ (defmacro with-temp-buffer-window (buffe
   (let ((buffer (make-symbol "buffer"))
 	(window (make-symbol "window"))
 	(value (make-symbol "value")))
+    (macroexp-let2 macroexp-copyable-p vquit-function quit-function
     `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
 	    (standard-output ,buffer)
 	    ,window ,value)
@@ -192,9 +193,9 @@ (defmacro with-temp-buffer-window (buffe
        (with-current-buffer ,buffer
 	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
 
-       (if (functionp ,quit-function)
-	   (funcall ,quit-function ,window ,value)
-	 ,value))))
+	 (if (functionp ,vquit-function)
+	     (funcall ,vquit-function ,window ,value)
+	   ,value)))))
 
 (defmacro with-current-buffer-window (buffer-or-name action quit-function &rest body)
   "Evaluate BODY with a buffer BUFFER-OR-NAME current and show that buffer.
@@ -205,6 +206,7 @@ (defmacro with-current-buffer-window (bu
   (let ((buffer (make-symbol "buffer"))
 	(window (make-symbol "window"))
 	(value (make-symbol "value")))
+    (macroexp-let2 macroexp-copyable-p vquit-function quit-function
     `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
 	    (standard-output ,buffer)
 	    ,window ,value)
@@ -212,9 +214,40 @@ (defmacro with-current-buffer-window (bu
 	 (setq ,value (progn ,@body))
 	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
 
-       (if (functionp ,quit-function)
-	   (funcall ,quit-function ,window ,value)
-	 ,value))))
+	 (if (functionp ,vquit-function)
+	     (funcall ,vquit-function ,window ,value)
+	   ,value)))))
+
+(defmacro with-displayed-buffer-window (buffer-or-name action quit-function &rest body)
+  "Show a buffer BUFFER-OR-NAME and evaluate BODY in that buffer.
+This construct is like `with-current-buffer-window' but unlike that
+displays the buffer specified by BUFFER-OR-NAME before running BODY."
+  (declare (debug t))
+  (let ((buffer (make-symbol "buffer"))
+	(window (make-symbol "window"))
+	(value (make-symbol "value")))
+    (macroexp-let2 macroexp-copyable-p vaction action
+      (macroexp-let2 macroexp-copyable-p vquit-function quit-function
+	`(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
+		(standard-output ,buffer)
+		,window ,value)
+	   (with-current-buffer ,buffer
+	     (setq ,window (temp-buffer-window-show ,buffer ,vaction)))
+
+	   (let ((inhibit-read-only t)
+		 (inhibit-modification-hooks t))
+	     (setq ,value (progn ,@body)))
+
+	   (with-selected-window ,window
+	     (goto-char (point-min)))
+
+	   (when (functionp (cdr (assq 'window-height (cdr ,vaction))))
+	     (ignore-errors
+	       (funcall (cdr (assq 'window-height (cdr ,vaction))) ,window)))
+
+	   (if (functionp ,vquit-function)
+	       (funcall ,vquit-function ,window ,value)
+	     ,value))))))
 
 ;; The following two functions are like `window-next-sibling' and
 ;; `window-prev-sibling' but the WINDOW argument is _not_ optional (so

bug#17809: 24.4.50; Completions display

[Stefan Monnier]

>>> +       (if (functionp ,quit-function)
>>> +	   (funcall ,quit-function ,window ,value)
>> Same for quit-function, and additionally, we want to make sure we
>> evaluate `quit-function' before `body'.
> `quit-function' is used in `dired-mark-pop-up' to activate the minibuffer,
> so it should be the final thing after displaying the *Marked Files* buffer.
> Maybe `quit-function' is not an appropriate name for such action?

I'm talking about the evaluation of quit-function, not the place
it's called.  `quit-function' is an expression that returns a function,
so it's important to know when that expression is run since it may end
up returning a different function if run at a different time.


        Stefan

bug#17809: 24.4.50; Completions display

[Stefan Monnier]

> === modified file 'lisp/window.el'
> --- lisp/window.el	2014-06-18 07:57:27 +0000
> +++ lisp/window.el	2014-07-06 23:32:27 +0000
> @@ -185,6 +185,7 @@ (defmacro with-temp-buffer-window (buffe
>    (let ((buffer (make-symbol "buffer"))
>  	(window (make-symbol "window"))
>  	(value (make-symbol "value")))
> +    (macroexp-let2 macroexp-copyable-p vquit-function quit-function
>      `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
>  	    (standard-output ,buffer)
>  	    ,window ,value)
> @@ -192,9 +193,9 @@ (defmacro with-temp-buffer-window (buffe
>         (with-current-buffer ,buffer
>  	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
>
> -       (if (functionp ,quit-function)
> -	   (funcall ,quit-function ,window ,value)
> -	 ,value))))
> +	 (if (functionp ,vquit-function)
> +	     (funcall ,vquit-function ,window ,value)
> +	   ,value)))))

That looks right, except it means that `quit-function' is
evaluated before `buffer-or-name', which is contrary to the expectation
that arguments are usually evaluated left-to-right.
 

        Stefan

bug#17809: 24.4.50; Completions display

[Juri Linkov]

> That looks right, except it means that `quit-function' is
> evaluated before `buffer-or-name', which is contrary to the expectation
> that arguments are usually evaluated left-to-right.

I believe all your concerns are addressed in this patch
unless I have more misunderstandings.

=== modified file 'lisp/window.el'
--- lisp/window.el	2014-06-18 07:57:27 +0000
+++ lisp/window.el	2014-07-07 06:49:07 +0000
@@ -185,16 +185,19 @@ (defmacro with-temp-buffer-window (buffe
   (let ((buffer (make-symbol "buffer"))
 	(window (make-symbol "window"))
 	(value (make-symbol "value")))
-    `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
+    (macroexp-let2 nil vbuffer-or-name buffer-or-name
+      (macroexp-let2 nil vaction action
+	(macroexp-let2 nil vquit-function quit-function
+	  `(let* ((,buffer (temp-buffer-window-setup ,vbuffer-or-name))
 	    (standard-output ,buffer)
 	    ,window ,value)
        (setq ,value (progn ,@body))
        (with-current-buffer ,buffer
-	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
+	       (setq ,window (temp-buffer-window-show ,buffer ,vaction)))
 
-       (if (functionp ,quit-function)
-	   (funcall ,quit-function ,window ,value)
-	 ,value))))
+	     (if (functionp ,vquit-function)
+		 (funcall ,vquit-function ,window ,value)
+	       ,value)))))))
 
 (defmacro with-current-buffer-window (buffer-or-name action quit-function &rest body)
   "Evaluate BODY with a buffer BUFFER-OR-NAME current and show that buffer.
@@ -205,16 +208,51 @@ (defmacro with-current-buffer-window (bu
   (let ((buffer (make-symbol "buffer"))
 	(window (make-symbol "window"))
 	(value (make-symbol "value")))
-    `(let* ((,buffer (temp-buffer-window-setup ,buffer-or-name))
+    (macroexp-let2 nil vbuffer-or-name buffer-or-name
+      (macroexp-let2 nil vaction action
+	(macroexp-let2 nil vquit-function quit-function
+	  `(let* ((,buffer (temp-buffer-window-setup ,vbuffer-or-name))
 	    (standard-output ,buffer)
 	    ,window ,value)
        (with-current-buffer ,buffer
 	 (setq ,value (progn ,@body))
-	 (setq ,window (temp-buffer-window-show ,buffer ,action)))
+	       (setq ,window (temp-buffer-window-show ,buffer ,vaction)))
 
-       (if (functionp ,quit-function)
-	   (funcall ,quit-function ,window ,value)
-	 ,value))))
+	     (if (functionp ,vquit-function)
+		 (funcall ,vquit-function ,window ,value)
+	       ,value)))))))
+
+(defmacro with-displayed-buffer-window (buffer-or-name action quit-function &rest body)
+  "Show a buffer BUFFER-OR-NAME and evaluate BODY in that buffer.
+This construct is like `with-current-buffer-window' but unlike that
+displays the buffer specified by BUFFER-OR-NAME before running BODY."
+  (declare (debug t))
+  (let ((buffer (make-symbol "buffer"))
+	(window (make-symbol "window"))
+	(value (make-symbol "value")))
+    (macroexp-let2 nil vbuffer-or-name buffer-or-name
+      (macroexp-let2 nil vaction action
+	(macroexp-let2 nil vquit-function quit-function
+	  `(let* ((,buffer (temp-buffer-window-setup ,vbuffer-or-name))
+		  (standard-output ,buffer)
+		  ,window ,value)
+	     (with-current-buffer ,buffer
+	       (setq ,window (temp-buffer-window-show ,buffer ,vaction)))
+
+	     (let ((inhibit-read-only t)
+		   (inhibit-modification-hooks t))
+	       (setq ,value (progn ,@body)))
+
+	     (with-selected-window ,window
+	       (goto-char (point-min)))
+
+	     (when (functionp (cdr (assq 'window-height (cdr ,vaction))))
+	       (ignore-errors
+		 (funcall (cdr (assq 'window-height (cdr ,vaction))) ,window)))
+
+	     (if (functionp ,vquit-function)
+		 (funcall ,vquit-function ,window ,value)
+	       ,value)))))))
 
 ;; The following two functions are like `window-next-sibling' and
 ;; `window-prev-sibling' but the WINDOW argument is _not_ optional (so

bug#17809: 24.4.50; Completions display

[Stefan Monnier]

> I believe all your concerns are addressed in this patch
> unless I have more misunderstandings.

Looks fine, thanks.

> +	     (with-selected-window ,window
> +	       (goto-char (point-min)))

I believe set-window-point is simpler and more efficient.


        Stefan

bug#17809: 24.4.50; Completions display

[Juri Linkov]

Version: 24.4.50

> Looks fine, thanks.
>
>> +	     (with-selected-window ,window
>> +	       (goto-char (point-min)))
>
> I believe set-window-point is simpler and more efficient.

Now installed to the trunk with set-window-point.

bug#17809: 24.4.50; Completions display

[Juri Linkov]

> - display-buffer-below-selected for completion-at-point
>   that could work like displaying *Marking Files* in Dired
>   to display *Completions* near the point of completion
>   in the current buffer.

I noticed that at-bottom is used instead of better below-selected
for some completion-at-point commands.  For example,
completing a color in the Customization UI displays the
*Completions* at the bottom even when there is no active minibuffer.

Since completion-at-point commands set the value of `this-command'
I wonder wouldn't it be right to check its value?

=== modified file 'lisp/minibuffer.el'
--- lisp/minibuffer.el	2014-07-08 19:15:28 +0000
+++ lisp/minibuffer.el	2014-10-05 21:59:09 +0000
@@ -1811,7 +1811,7 @@ (defun minibuffer-completion-help (&opti
              ;; Use `display-buffer-below-selected' for inline completions,
              ;; but not in the minibuffer (e.g. in `eval-expression')
              ;; for which `display-buffer-at-bottom' is used.
-             ,(if (and completion-in-region-mode-predicate
+             ,(if (and (eq this-command 'completion-at-point)
                        (not (minibuffer-selected-window)))
                   'display-buffer-below-selected
                 'display-buffer-at-bottom))