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#17831: 24.4.50; bad default value for `Man-width'

[Leo Liu]

Man-width defaults to the window width at the time of running `man'. But
if the frame is split horizontally it usually leads to a view with the
right-hand-side half unviewable.

Leo

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

forcemerge 17831 2588
thanks

> Man-width defaults to the window width at the time of running `man'.  But
> if the frame is split horizontally it usually leads to a view with the
> right-hand-side half unviewable.

Indeed.
As discussed earlier, I think the right fix is to display the buffer
right away and then fill it in the background.

Problem with it, is that currently we do "fill the buffer with
half-processed text; finish formatting; display", so if we only move the
"display" part, the user will see (temporarily) the half-processed text.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[Juri Linkov]

>> Man-width defaults to the window width at the time of running `man'.  But
>> if the frame is split horizontally it usually leads to a view with the
>> right-hand-side half unviewable.
>
> Indeed.
> As discussed earlier, I think the right fix is to display the buffer
> right away and then fill it in the background.
>
> Problem with it, is that currently we do "fill the buffer with
> half-processed text; finish formatting; display", so if we only move the
> "display" part, the user will see (temporarily) the half-processed text.

While testing the changes for bug#17809 I noticed a similar problem for
the *Completions* buffer.  It's filled with completions using the
default width of 80 columns while the buffer is not yet displayed.
Later when the buffer is displayed in a window with the frame's full-width
of 160 columns, it leaves too much empty space in the buffer.

As a general solution for such cases it seems the proper order is
to display an empty buffer first, and then fill it with the contents.

bug#17831: 24.4.50; bad default value for `Man-width'

[Leo Liu]

On 2014-06-23 08:53 -0400, Stefan Monnier wrote:
> Problem with it, is that currently we do "fill the buffer with
> half-processed text; finish formatting; display", so if we only move the
> "display" part, the user will see (temporarily) the half-processed text.

Indeed. On the other hand simply change the default value to 80 always
gives me a more readable output. But this doesn't fix the wrong
behaviour when using window width.

Leo

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#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> As a general solution for such cases it seems the proper order is
> to display an empty buffer first, and then fill it with the contents.

Agreed.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[martin rudalics]

 > As a general solution for such cases it seems the proper order is
 > to display an empty buffer first, and then fill it with the contents.

Obviously there are two way to tackle this:

Fill the buffer in some way and try to display it in a window with
suitable size.  That's what we've done so far and it fails typically
because it looks at the width of the selected window in order to do the
filling but displays the buffer in a window with different width.  The
first to report this problem was Lennart when filing bug#6000 more than
four years ago.

To fix this we could either use some sort of maximimum width (for me
more than 80 columns are not very readable anyway) and propose an
adequate display buffer action or implement simple heuristics to detect
how large the window used by `display-buffer' would be and fill the
buffer in some adequate manner.

Alternatively, we could display the buffer first, look at what size we
get, fill the buffer, and possibly resize the window afterwards.  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.

martin

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> To fix this we could either use some sort of maximimum width (for me
> more than 80 columns are not very readable anyway) and propose an

I'm with you here, but some users disagree (e.g. they want to use their
160-column *Cmpletions* window fully).

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

That's what we should aim for, I think.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[Eli Zaretskii]

> Date: Tue, 24 Jun 2014 09:13:54 +0200
> From: martin rudalics <rudalics@gmx.at>
> Cc: 17831@debbugs.gnu.org, Leo Liu <sdl.web@gmail.com>
> 
> To fix this we could either use some sort of maximimum width (for me
> more than 80 columns are not very readable anyway) and propose an
> adequate display buffer action or implement simple heuristics to detect
> how large the window used by `display-buffer' would be and fill the
> buffer in some adequate manner.

None of this will ever work 100% reliably in the "M-x man" case,
because while the command runs in the background, the user could
change the window and frame configuration at will.

> Alternatively, we could display the buffer first, look at what size we
> get, fill the buffer, and possibly resize the window afterwards.  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.

But this will momentarily flash incorrect (e.g., empty) display,
right?  Not nice, IMO.

bug#17831: 24.4.50; bad default value for `Man-width'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 24 Jun 2014 08:53:14 -0400
> Cc: 17831@debbugs.gnu.org, Leo Liu <sdl.web@gmail.com>
> 
> > To fix this we could either use some sort of maximimum width (for me
> > more than 80 columns are not very readable anyway) and propose an
> 
> I'm with you here, but some users disagree (e.g. they want to use their
> 160-column *Cmpletions* window fully).
> 
> > Alternatively, we could display the buffer first, look at what size we
> > get, fill the buffer, and possibly resize the window afterwards.
> 
> That's what we should aim for, I think.

You are talking about 2 different situations as if they are identical.
But IMO they are very different.  "M-x man" runs the text formatter in
the background, while the user is free to change the window
configuration at will.  I see no way that we will be able to solve
this reliably.

By contrast, in the *Completions* use case the code that formats the
text runs synchronously, so having in place some protocol that would
allow to query about the dimensions of the window display-buffer
etc. _will_ get, and then immediately use these dimensions to format
the candidate list, is probably all we need.  The alternative you
favor is IMO worse: it will momentarily flash incorrect display, which
I think will look un-professional.

Returning to the "M-x man" use case, I think the possibilities
supported via the Man-width option is the best we can do.  So any
users that are unhappy should be pointed to that option.

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> But this will momentarily flash incorrect (e.g., empty) display,
> right?  Not nice, IMO.

That doesn't seem to bother users of C-x v =


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> By contrast, in the *Completions* use case the code that formats the
> text runs synchronously, so having in place some protocol that would
> allow to query about the dimensions of the window display-buffer
> etc. _will_ get, and then immediately use these dimensions to format
> the candidate list, is probably all we need.  The alternative you
> favor is IMO worse: it will momentarily flash incorrect display, which
> I think will look un-professional.

In the *Completions* case the empty buffer won't be temporarily
displayed (because there's no redisplay going on before it's filled).

The unprofessionalism doesn't bother me too much for M-x man, especially
since the current behavior is broken IMNSHO: it pops up a window
asynchronously, i.e. at a time you might be doing something else and not
expecting this disruption.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: martin rudalics <rudalics@gmx.at>,  juri@jurta.org,  17831@debbugs.gnu.org,  sdl.web@gmail.com
> Date: Tue, 24 Jun 2014 13:31:01 -0400
> 
> > But this will momentarily flash incorrect (e.g., empty) display,
> > right?  Not nice, IMO.
> 
> That doesn't seem to bother users of C-x v =

Which again uses an asynchronous text producer.  That's not the same
as *Completions*, so I don't think we should discuss these two issues
as one.

bug#17831: 24.4.50; bad default value for `Man-width'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rudalics@gmx.at,  17831@debbugs.gnu.org,  sdl.web@gmail.com
> Date: Tue, 24 Jun 2014 13:33:50 -0400
> 
> > By contrast, in the *Completions* use case the code that formats the
> > text runs synchronously, so having in place some protocol that would
> > allow to query about the dimensions of the window display-buffer
> > etc. _will_ get, and then immediately use these dimensions to format
> > the candidate list, is probably all we need.  The alternative you
> > favor is IMO worse: it will momentarily flash incorrect display, which
> > I think will look un-professional.
> 
> In the *Completions* case the empty buffer won't be temporarily
> displayed (because there's no redisplay going on before it's filled).

Are you sure? what about echo-area messages, which might trigger
redisplay?

But if you are right, then this is just another case of what I
suggested, i.e. being able to query Emacs about the dimensions before
actually displaying.

> The unprofessionalism doesn't bother me too much for M-x man, especially
> since the current behavior is broken IMNSHO: it pops up a window
> asynchronously, i.e. at a time you might be doing something else and not
> expecting this disruption.

So perhaps TRT is simply to get rid of the asynchronous operation in
the first place.

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

>> > But this will momentarily flash incorrect (e.g., empty) display,
>> > right?  Not nice, IMO.
>> That doesn't seem to bother users of C-x v =

Doesn't bother web-browser users either, BTW.

> Which again uses an asynchronous text producer.  That's not the same
> as *Completions*, so I don't think we should discuss these two issues
> as one.

For the non-async case, the empty buffer will not appear for lack
of redisplay.

> Are you sure? what about echo-area messages, which might trigger redisplay?

That can happen, but I wouldn't worry about it.

> > The unprofessionalism doesn't bother me too much for M-x man, especially
> > since the current behavior is broken IMNSHO: it pops up a window
> > asynchronously, i.e. at a time you might be doing something else and not
> > expecting this disruption.
> So perhaps TRT is simply to get rid of the asynchronous operation in
> the first place.

The annoying disruption is when "M-x man" takes a long time to get&generate
the full output.  I.e. it's the case where it's especially important to
use asynchronous operation.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rudalics@gmx.at,  juri@jurta.org,  17831@debbugs.gnu.org,  sdl.web@gmail.com
> Date: Tue, 24 Jun 2014 15:35:04 -0400
> 
> > So perhaps TRT is simply to get rid of the asynchronous operation in
> > the first place.
> 
> The annoying disruption is when "M-x man" takes a long time to get&generate
> the full output.

Most of that can easily be thrown away.  Just look at what it does.

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

>> > So perhaps TRT is simply to get rid of the asynchronous operation in
>> > the first place.
>> The annoying disruption is when "M-x man" takes a long time to get&generate
>> the full output.
> Most of that can easily be thrown away.  Just look at what it does.

Maybe it could be sped up, indeed.
But I still prefer an async process that gives me:
- empty buffer after 0s
- full first page displayed after 0.1s
- buffer fully filled and ready after 5s
over a sync process that blocks for 3s.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[Juri Linkov]

> Which again uses an asynchronous text producer.  That's not the same
> as *Completions*, so I don't think we should discuss these two issues
> as one.

It's not the same, indeed.  This is why in the previous message
I referred to bug#17809 as a similar problem, but not the same.

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#17831: 24.4.50; bad default value for `Man-width'

[Juri Linkov]

> But I still prefer an async process that gives me:
> - empty buffer after 0s
> - full first page displayed after 0.1s
> - buffer fully filled and ready after 5s
> over a sync process that blocks for 3s.

This simple patch displays the buffer immediately,
but then slowly fills it with unformatted output
that doesn't look nice.  So maybe better would be to create
a temporary hidden buffer, do formatting in background,
and copy the formatted text to the displayed buffer.

=== modified file 'lisp/man.el'
--- lisp/man.el	2014-05-09 07:02:00 +0000
+++ lisp/man.el	2014-06-24 23:47:12 +0000
@@ -1056,6 +1056,7 @@ (defun Man-getpage-in-background (topic)
       (require 'env)
       (message "Invoking %s %s in the background" manual-program man-args)
       (setq buffer (generate-new-buffer bufname))
+      (Man-notify-when-ready buffer)
       (with-current-buffer buffer
 	(setq buffer-undo-list t)
 	(setq Man-original-frame (selected-frame))

bug#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> This simple patch displays the buffer immediately,
> but then slowly fills it with unformatted output
> that doesn't look nice.

Indeed, that's why I haven't made this change in Emacs years ago (when
I made it in my local copy after getting too annoyed at the window-popup
disruption).  The solution is obvious, tho: "Just" move the reformatting
to the process-filter.


        Stefan

bug#17831: 24.4.50; bad default value for `Man-width'

[martin rudalics]

 > None of this will ever work 100% reliably in the "M-x man" case,
 > because while the command runs in the background, the user could
 > change the window and frame configuration at will.

Indeed.

 >> Alternatively, we could display the buffer first, look at what size we
 >> get, fill the buffer, and possibly resize the window afterwards.  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.
 >
 > But this will momentarily flash incorrect (e.g., empty) display,
 > right?

While the buffer gets filled asynchronously?  Yes.  But it should work
in the synchronous case where the order would be (1) display the empty
buffer (2) fill it (3) resize the window accordingly.

 > Not nice, IMO.

We could show some sort of placeholder here.

martin

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#17831: 24.4.50; bad default value for `Man-width'

[martin rudalics]

 > Returning to the "M-x man" use case, I think the possibilities
 > supported via the Man-width option is the best we can do.  So any
 > users that are unhappy should be pointed to that option.

Agreed.  But then `display-buffer' should be passed the value specified
there together with a suitable function to make sure that the new window
doesn't get too small.

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#17831: 24.4.50; bad default value for `Man-width'

[Juri Linkov]

>> This simple patch displays the buffer immediately,
>> but then slowly fills it with unformatted output
>> that doesn't look nice.
>
> Indeed, that's why I haven't made this change in Emacs years ago (when
> I made it in my local copy after getting too annoyed at the window-popup
> disruption).  The solution is obvious, tho: "Just" move the reformatting
> to the process-filter.

Using the same approach like processing escape sequences in grep/compilation
produces a nice result for M-x man: it displays the first formatted page
immediately and continues formatting the rest of the man page in background.
There are still small details to iron out but this is basically achieved
with the following patch.

There is one problem that I noticed on large man pages: formatting
small chunks by process-filter is little slower than formatting the
whole output like it currently does.  Could it be possible that
a slowdown is caused by `narrow-to-region'?  Then it's possible
to add two new arguments `min' and `max' to `Man-fontify-manpage'
to limit the processed region manually instead of using narrowing.

=== modified file 'lisp/man.el'
--- lisp/man.el	2014-05-09 07:02:00 +0000
+++ lisp/man.el	2014-06-27 00:49:22 +0000
@@ -1056,20 +1056,22 @@ (defun Man-getpage-in-background (topic)
       (require 'env)
       (message "Invoking %s %s in the background" manual-program man-args)
       (setq buffer (generate-new-buffer bufname))
+      (Man-notify-when-ready buffer)
       (with-current-buffer buffer
 	(setq buffer-undo-list t)
 	(setq Man-original-frame (selected-frame))
 	(setq Man-arguments man-args))
       (Man-start-calling
        (if (fboundp 'start-process)
-	    (set-process-sentinel
-	     (start-process manual-program buffer
-			    (if (memq system-type '(cygwin windows-nt))
-				shell-file-name
-			      "sh")
-			    shell-command-switch
-			    (format (Man-build-man-command) man-args))
-	     'Man-bgproc-sentinel)
+	   (let ((proc (start-process
+			manual-program buffer
+			(if (memq system-type '(cygwin windows-nt))
+			    shell-file-name
+			  "sh")
+			shell-command-switch
+			(format (Man-build-man-command) man-args))))
+	     (set-process-sentinel proc 'Man-bgproc-sentinel)
+	     (set-process-filter proc 'Man-bgproc-filter))
 	  (let ((exit-status
 		 (call-process shell-file-name nil (list buffer nil) nil
 			       shell-command-switch
@@ -1312,6 +1317,33 @@ (defun Man-cleanup-manpage (&optional in
   (Man-softhyphen-to-minus)
   (message "%s man page cleaned up" Man-arguments))
 
+(defun Man-bgproc-filter (process string)
+  "Manpage background process filter.
+When manpage command is run asynchronously, PROCESS is the process
+object for the manpage command; when manpage command is run
+synchronously, PROCESS is the name of the buffer where the manpage
+command is run.  Second argument STRING is the entire string of output."
+  (save-excursion
+    (let ((Man-buffer (if (stringp process) (get-buffer process)
+			(process-buffer process))))
+      (if (null (buffer-name Man-buffer)) ;; deleted buffer
+	  (or (stringp process)
+	      (set-process-buffer process nil))
+
+	(with-current-buffer Man-buffer
+	  (let ((inhibit-read-only t)
+	        (beg (marker-position (process-mark process))))
+	    (goto-char beg)
+	    (insert string)
+	    (save-restriction
+	      (narrow-to-region
+	       (save-excursion (goto-char beg) (line-beginning-position))
+	       (point))
+	      (if Man-fontify-manpage-flag
+		  (Man-fontify-manpage)
+		(Man-cleanup-manpage)))
+	    (set-marker (process-mark process) (point-max))))))))
+
 (defun Man-bgproc-sentinel (process msg)
   "Manpage background process sentinel.
 When manpage command is run asynchronously, PROCESS is the process
@@ -1365,9 +1398,6 @@ (defun Man-bgproc-sentinel (process msg)
 		 ))
         (if delete-buff
             (kill-buffer Man-buffer)
-          (if Man-fontify-manpage-flag
-              (Man-fontify-manpage)
-            (Man-cleanup-manpage))
 
           (run-hooks 'Man-cooked-hook)
 	  (Man-mode)
@@ -1378,11 +1408,6 @@ (defun Man-bgproc-sentinel (process msg)
 		(user-error "Can't find the %s manpage"
                             (Man-page-from-arguments args)))
 	    (set-buffer-modified-p nil))))
-	;; Restore case-fold-search before calling
-	;; Man-notify-when-ready because it may switch buffers.
-
-	(if (not delete-buff)
-	    (Man-notify-when-ready Man-buffer))
 
 	(if err-mess
 	    (error "%s" err-mess))

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#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> Using the same approach like processing escape sequences in grep/compilation
> produces a nice result for M-x man: it displays the first formatted page
> immediately and continues formatting the rest of the man page in background.
> There are still small details to iron out but this is basically achieved
> with the following patch.

Looks good.

> There is one problem that I noticed on large man pages: formatting
> small chunks by process-filter is little slower than formatting the
> whole output like it currently does.

That's expected.

> Could it be possible that a slowdown is caused by `narrow-to-region'?

`narrow-to-region' is cheap.  But doing it by chunks is necessarily
slower, because there is a non-negligible overhead "per chunk".
`narrow-to-region' is part of that overhead, but there are others much
more costly, such as going through redisplay (which may even sometimes
have to update some part of the display, such as the "nn%" thingy since
that depends on the total buffer size).


        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#17831: 24.4.50; bad default value for `Man-width'

[Juri Linkov]

>> Could it be possible that a slowdown is caused by `narrow-to-region'?
>
> `narrow-to-region' is cheap.  But doing it by chunks is necessarily
> slower, because there is a non-negligible overhead "per chunk".
> `narrow-to-region' is part of that overhead, but there are others much
> more costly, such as going through redisplay (which may even sometimes
> have to update some part of the display, such as the "nn%" thingy since
> that depends on the total buffer size).

If it's not possible to increase the default chunk size to call
process-filter less often (e.g. now for `M-x man RET bash' it's called
90 times), then some users might want to disable this using a new option
like `Man-sync'.  OTOH, since most of man pages are much smaller,
maybe this is not a problem.

But still the users need an indication that the formatting
is not finished.  grep/compilation and vc display a string
like "waiting..." or "compiling..." in the mode-line, so
man.el could display in the mode-line "formatting..."

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#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

> If it's not possible to increase the default chunk size to call
> process-filter less often (e.g. now for `M-x man RET bash' it's called
> 90 times), then some users might want to disable this using a new option
> like `Man-sync'.  OTOH, since most of man pages are much smaller,
> maybe this is not a problem.

I wouldn't worry 'bout it for now.  And this is not a problem specific
to M-x man.

> But still the users need an indication that the formatting
> is not finished.  grep/compilation and vc display a string
> like "waiting..." or "compiling..." in the mode-line, so
> man.el could display in the mode-line "formatting..."

Sound fine,


        Stefan

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#17831: 24.4.50; bad default value for `Man-width'

[Juri Linkov]

>> But still the users need an indication that the formatting
>> is not finished.  grep/compilation and vc display a string
>> like "waiting..." or "compiling..." in the mode-line, so
>> man.el could display in the mode-line "formatting..."
>
> Sound fine,

After testing I see no problems with this patch:

=== modified file 'lisp/man.el'
--- lisp/man.el	2014-05-09 07:02:00 +0000
+++ lisp/man.el	2014-06-29 23:37:38 +0000
@@ -1056,21 +1056,28 @@ (defun Man-getpage-in-background (topic)
       (require 'env)
       (message "Invoking %s %s in the background" manual-program man-args)
       (setq buffer (generate-new-buffer bufname))
+      (Man-notify-when-ready buffer)
       (with-current-buffer buffer
 	(setq buffer-undo-list t)
 	(setq Man-original-frame (selected-frame))
-	(setq Man-arguments man-args))
+	(setq Man-arguments man-args)
+	(Man-mode)
+	(setq mode-line-process
+	      (concat " " (propertize "[formatting...]"
+				      'face 'mode-line-emphasis))))
       (Man-start-calling
        (if (fboundp 'start-process)
-	    (set-process-sentinel
-	     (start-process manual-program buffer
+	   (let ((proc (start-process
+			manual-program buffer
 			    (if (memq system-type '(cygwin windows-nt))
 				shell-file-name
 			      "sh")
 			    shell-command-switch
-			    (format (Man-build-man-command) man-args))
-	     'Man-bgproc-sentinel)
-	  (let ((exit-status
+			(format (Man-build-man-command) man-args))))
+	     (set-process-sentinel proc 'Man-bgproc-sentinel)
+	     (set-process-filter proc 'Man-bgproc-filter))
+	 (let* ((inhibit-read-only t)
+		(exit-status
 		 (call-process shell-file-name nil (list buffer nil) nil
 			       shell-command-switch
 			       (format (Man-build-man-command) man-args)))
@@ -1082,6 +1089,10 @@ (defun Man-getpage-in-background (topic)
 			   (format "exited abnormally with code %d"
 				   exit-status)))
 		(setq msg exit-status))
+	   (with-current-buffer buffer
+	     (if Man-fontify-manpage-flag
+		 (Man-fontify-manpage)
+	       (Man-cleanup-manpage)))
 	    (Man-bgproc-sentinel bufname msg)))))
       buffer))
 
@@ -1168,7 +1179,6 @@ (defun Man-fontify-manpage ()
   "Convert overstriking and underlining to the correct fonts.
 Same for the ANSI bold and normal escape sequences."
   (interactive)
-  (message "Please wait: formatting the %s man page..." Man-arguments)
   (goto-char (point-min))
   ;; Fontify ANSI escapes.
   (let ((ansi-color-apply-face-function
@@ -1183,7 +1193,7 @@ (defun Man-fontify-manpage ()
 	;; Multibyte characters exist.
 	(progn
 	  (goto-char (point-min))
-	  (while (search-forward "__\b\b" nil t)
+	  (while (and (search-forward "__\b\b" nil t) (not (eobp)))
 	    (backward-delete-char 4)
 	    (put-text-property (point) (1+ (point)) 'face 'Man-underline))
 	  (goto-char (point-min))
@@ -1191,7 +1201,7 @@ (defun Man-fontify-manpage ()
 	    (backward-delete-char 4)
 	    (put-text-property (1- (point)) (point) 'face 'Man-underline))))
     (goto-char (point-min))
-    (while (search-forward "_\b" nil t)
+    (while (and (search-forward "_\b" nil t) (not (eobp)))
       (backward-delete-char 2)
       (put-text-property (point) (1+ (point)) 'face 'Man-underline))
     (goto-char (point-min))
@@ -1223,8 +1233,7 @@ (defun Man-fontify-manpage ()
     (while (re-search-forward Man-heading-regexp nil t)
       (put-text-property (match-beginning 0)
 			 (match-end 0)
-			 'face 'Man-overstrike)))
-  (message "%s man page formatted" (Man-page-from-arguments Man-arguments)))
+			 'face 'Man-overstrike))))
 
 (defun Man-highlight-references (&optional xref-man-type)
   "Highlight the references on mouse-over.
@@ -1286,8 +1295,6 @@ (defun Man-cleanup-manpage (&optional in
 but when called interactively, do those jobs even if the sed
 script would have done them."
   (interactive "p")
-  (message "Please wait: cleaning up the %s man page..."
-	   Man-arguments)
   (if (or interactive (not Man-sed-script))
       (progn
 	(goto-char (point-min))
@@ -1309,8 +1316,36 @@ (defun Man-cleanup-manpage (&optional in
   ;; their preceding chars (but don't put Man-overstrike).  (Bug#5566)
   (goto-char (point-min))
   (while (re-search-forward ".\b" nil t) (backward-delete-char 2))
-  (Man-softhyphen-to-minus)
-  (message "%s man page cleaned up" Man-arguments))
+  (Man-softhyphen-to-minus))
+
+(defun Man-bgproc-filter (process string)
+  "Manpage background process filter.
+When manpage command is run asynchronously, PROCESS is the process
+object for the manpage command; when manpage command is run
+synchronously, PROCESS is the name of the buffer where the manpage
+command is run.  Second argument STRING is the entire string of output."
+  (save-excursion
+    (let ((Man-buffer (process-buffer process)))
+      (if (null (buffer-name Man-buffer)) ;; deleted buffer
+	  (set-process-buffer process nil)
+
+	(with-current-buffer Man-buffer
+	  (let ((inhibit-read-only t)
+	        (beg (marker-position (process-mark process))))
+	    (save-excursion
+	      (goto-char beg)
+	      (insert string)
+	      (save-restriction
+		(narrow-to-region
+		 (save-excursion
+		   (goto-char beg)
+		   (line-beginning-position))
+		 (point))
+		(if Man-fontify-manpage-flag
+		    (Man-fontify-manpage)
+		  (Man-cleanup-manpage)))
+	      (set-marker (process-mark process) (point-max)))))))))
 
 (defun Man-bgproc-sentinel (process msg)
   "Manpage background process sentinel.
@@ -1329,6 +1364,7 @@ (defun Man-bgproc-sentinel (process msg)
 	    (set-process-buffer process nil))
 
       (with-current-buffer Man-buffer
+	(save-excursion
 	(let ((case-fold-search nil))
 	  (goto-char (point-min))
 	  (cond ((or (looking-at "No \\(manual \\)*entry for")
@@ -1364,28 +1400,34 @@ (defun Man-bgproc-sentinel (process msg)
 		       (insert (format "\nprocess %s" msg))))
 		 ))
         (if delete-buff
-            (kill-buffer Man-buffer)
-          (if Man-fontify-manpage-flag
-              (Man-fontify-manpage)
-            (Man-cleanup-manpage))
+		(if (get-buffer-window Man-buffer)
+		    (quit-window t (get-buffer-window Man-buffer))
+		  (kill-buffer Man-buffer))
 
           (run-hooks 'Man-cooked-hook)
-	  (Man-mode)
+
+	      (Man-build-page-list)
+	      (Man-strip-page-headers)
+	      (Man-unindent)
+	      (Man-goto-page 1 t)
 
 	  (if (not Man-page-list)
  	      (let ((args Man-arguments))
-		(kill-buffer (current-buffer))
-		(user-error "Can't find the %s manpage"
+		    (if (get-buffer-window (current-buffer))
+			(quit-window t (get-buffer-window (current-buffer)))
+		      (kill-buffer (current-buffer)))
+		    (message "Can't find the %s manpage"
                             (Man-page-from-arguments args)))
-	    (set-buffer-modified-p nil))))
-	;; Restore case-fold-search before calling
-	;; Man-notify-when-ready because it may switch buffers.
 
-	(if (not delete-buff)
-	    (Man-notify-when-ready Man-buffer))
+		(if Man-fontify-manpage-flag
+		    (message "%s man page formatted" (Man-page-from-arguments Man-arguments))
+		  (message "%s man page cleaned up" Man-arguments))
+		(unless (and (processp process) (not (eq (process-status process) 'exit)))
+		  (setq mode-line-process nil))
+		(set-buffer-modified-p nil)))))
 
 	(if err-mess
-	    (error "%s" err-mess))
+	    (message "%s" err-mess))
 	))))
 
 (defun Man-page-from-arguments (args)
@@ -1458,11 +1500,7 @@ (define-derived-mode Man-mode fundamenta
   (set (make-local-variable 'outline-regexp) Man-heading-regexp)
   (set (make-local-variable 'outline-level) (lambda () 1))
   (set (make-local-variable 'bookmark-make-record-function)
-       'Man-bookmark-make-record)
-  (Man-build-page-list)
-  (Man-strip-page-headers)
-  (Man-unindent)
-  (Man-goto-page 1 t))
+       'Man-bookmark-make-record))
 
 (defsubst Man-build-section-alist ()
   "Build the list of manpage sections."

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#17831: 24.4.50; bad default value for `Man-width'

[Stefan Monnier]

>> Sound fine,
> After testing I see no problems with this patch:

Great, thanks,


        Stefan

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