bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Eliza Velasquez]

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

Tags: patch

If `async-shell-command-display-buffer' was nil, it did not respect
`display-buffer-alist' entries with `display-buffer-no-window'.  This
behavior has been fixed.

For example, I never want to see any shell command buffers with no
output, and I also never want to see any shell command buffers named
`*shell:mpv*'.

It seems possible to me, but very unlikely, that someone was depending
on the existing behavior in some way.  I can imagine an alternate
version of this patch that instead adds a new possible value to
`async-shell-command-display-buffer', but this seems like a clear bug to
me.  I will defer to the judgement of someone more senior on this.

In GNU Emacs 29.0.92 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.38, cairo version 1.16.0)
Windowing system distributor 'The X.Org Foundation', version 11.0.12101008
System Description: NixOS 23.05 (Stoat)

Configured using:
 'configure
 --prefix=/nix/store/vc9yalw7cbbk21406nx5vb94k5rb5h4k-emacs-gtk3-29.0.92
 --disable-build-details --with-modules --with-x-toolkit=gtk3 --with-xft
 --with-cairo --with-native-compilation --with-tree-sitter
 --with-xinput2 --with-xwidgets'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-async-shell-command-display-buffer-display.patch --]
[-- Type: text/patch, Size: 1159 bytes --]

From 0a6c0268f7f304d051201b024ad27f50f8682e45 Mon Sep 17 00:00:00 2001
From: Eliza Velasquez <eliza@eliza.sh>
Date: Fri, 30 Jun 2023 17:35:44 -0700
Subject: [PATCH] Fix `async-shell-command-display-buffer' display

If `async-shell-command-display-buffer' was nil, it did not respect
`display-buffer-alist' entries with `display-buffer-no-window'. This
behavior has been fixed.
---
 lisp/simple.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 646da8aafaa..7b382c512b6 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4734,7 +4734,7 @@ shell-command
                                       (when (buffer-live-p buf)
                                         (remove-function (process-filter proc)
                                                          nonce)
-                                        (display-buffer buf))))
+                                        (display-buffer buf '(nil (allow-no-window . t))))))
                                   `((name . ,nonce)))))))
 	  ;; Otherwise, command is executed synchronously.
 	  (shell-command-on-region (point) (point) command
-- 
2.40.1


[-- Attachment #3: Type: text/plain, Size: 11 bytes --]


-- 
Eliza

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Eli Zaretskii]

> From: Eliza Velasquez <eliza@eliza.sh>
> Date: Fri, 30 Jun 2023 18:00:33 -0700
> 
> If `async-shell-command-display-buffer' was nil, it did not respect
> `display-buffer-alist' entries with `display-buffer-no-window'.  This
> behavior has been fixed.

I'm probably missing something, but how can display-buffer fail to
support any action function, such as display-buffer-no-window?

Martin, what am I missing here?

> For example, I never want to see any shell command buffers with no
> output, and I also never want to see any shell command buffers named
> `*shell:mpv*'.

That's fine, but those are your preferences.  I'd feel uncomfortable
with forcing them on everyone, if we already have a way of tailoring
this behavior by user customizations.

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Eliza Velasquez]

On Sat, Jul 01 2023 at 10:24 +03, Eli Zaretskii <eliz@gnu.org> wrote:

> I'm probably missing something, but how can display-buffer fail to
> support any action function, such as display-buffer-no-window?
>
> Martin, what am I missing here?

I was also confused.  Based on the documentation for
`display-buffer-no-window', it seems that callers are supposed to
explicitly pass an `(allow-no-window . t)' cons pair when calling
`display-buffer' as a signal that they can correctly handle a return
value of nil.  If it's absent, `display-buffer-no-window' seems to err
on the side of caution, assume the caller can't handle nil, displays the
window anyway, and returns it like all the other display functions.

Technically it seems that you can add `(allow-no-window . t)' to
`display-buffer-alist' to always force the buffer never to appear, but
that doesn't seem at all like its intended use.

> That's fine, but those are your preferences.  I'd feel uncomfortable
> with forcing them on everyone, if we already have a way of tailoring
> this behavior by user customizations.

I might not have been clear with what I meant here, sorry; I mean that
in my own personal config, when I run `mpv', its output appears in a
buffer named `*shell:mpv*' instead of `*Async Shell Command*', and I
have an explicit entry for it in `display-buffer-alist' so that it
doesn't appear via `display-buffer-no-window'.  This was functioning
well, except the moment I set `async-shell-command-display-buffer' to
nil, the buffer displayed itself the moment mpv began to write to
stdout.

A minimally reproable example in `emacs -Q':

--8<---------------cut here---------------start------------->8---
(setq display-buffer-alist
      '(("\\*Async Shell Command\\*"
	 (display-buffer-no-window))))
(setq async-shell-command-display-buffer nil)
--8<---------------cut here---------------end--------------->8---

`M-& echo foo RET' will unexpectedly show the `*Async Shell Command*'
buffer.

-- 
Eliza

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Eli Zaretskii]

> From: Eliza Velasquez <eliza@eliza.sh>
> Cc: 64394@debbugs.gnu.org
> Date: Sat, 01 Jul 2023 00:52:32 -0700
> 
> I might not have been clear with what I meant here, sorry; I mean that
> in my own personal config, when I run `mpv', its output appears in a
> buffer named `*shell:mpv*' instead of `*Async Shell Command*', and I
> have an explicit entry for it in `display-buffer-alist' so that it
> doesn't appear via `display-buffer-no-window'.  This was functioning
> well, except the moment I set `async-shell-command-display-buffer' to
> nil, the buffer displayed itself the moment mpv began to write to
> stdout.

But that's exactly what this variable is about, AFAIU:

  Whether to display the command buffer immediately.
  If t, display the buffer immediately; if nil, wait until there
  is output.

Note the last part.

So why do you think this behavior is a problem?

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Eliza Velasquez]

On Sat, Jul 01 2023 at 11:12 +03, Eli Zaretskii <eliz@gnu.org> wrote:

> But that's exactly what this variable is about, AFAIU:
>
>   Whether to display the command buffer immediately.
>   If t, display the buffer immediately; if nil, wait until there
>   is output.
>
> Note the last part.
>
> So why do you think this behavior is a problem?

On a philosophical level: It's surprising to me in that previous example
that if `async-shell-command-display-buffer' is t, the buffer is /not/
displayed (according to `display-buffer-alist'), but if it's nil, it
/is/ displayed, eventually (ignoring `display-buffer-alist').

On a practical level: The user may want to differentiate buffer display
behavior based on the name of the shell command buffer or by some other
predicate, including disabling showing that buffer, regardless of
whether `async-shell-command-display-buffer' is set to t or nil.  I have
recently authored a package to make this easier [1] and ran into this
problem.  The example configuration in the README might shed some more
light on the expected behavior.

[1] https://github.com/elizagamedev/shell-command-x.el

-- 
Eliza

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[martin rudalics]

 >> I'm probably missing something, but how can display-buffer fail to
 >> support any action function, such as display-buffer-no-window?
 >>
 >> Martin, what am I missing here?

We may have to ask Juri, he conceived the "allow-no-window" concept.

 > I was also confused.  Based on the documentation for
 > `display-buffer-no-window', it seems that callers are supposed to
 > explicitly pass an `(allow-no-window . t)' cons pair when calling
 > `display-buffer' as a signal that they can correctly handle a return
 > value of nil.  If it's absent, `display-buffer-no-window' seems to err
 > on the side of caution, assume the caller can't handle nil, displays the
 > window anyway, and returns it like all the other display functions.

I think that's the idea, yes.

 > Technically it seems that you can add `(allow-no-window . t)' to
 > `display-buffer-alist' to always force the buffer never to appear, but
 > that doesn't seem at all like its intended use.

Maybe "force" is too strong here.  You can "force" it by adding an
'allow-no-window' entry to the alist _and_ a 'display-buffer-no-window'
action in a position that precedes any other display buffer action.

 >> That's fine, but those are your preferences.  I'd feel uncomfortable
 >> with forcing them on everyone, if we already have a way of tailoring
 >> this behavior by user customizations.
 >
 > I might not have been clear with what I meant here, sorry; I mean that
 > in my own personal config, when I run `mpv', its output appears in a
 > buffer named `*shell:mpv*' instead of `*Async Shell Command*', and I
 > have an explicit entry for it in `display-buffer-alist' so that it
 > doesn't appear via `display-buffer-no-window'.  This was functioning
 > well, except the moment I set `async-shell-command-display-buffer' to
 > nil, the buffer displayed itself the moment mpv began to write to
 > stdout.
 >
 > A minimally reproable example in `emacs -Q':
 >
 > --8<---------------cut here---------------start------------->8---
 > (setq display-buffer-alist
 >        '(("\\*Async Shell Command\\*"
 > 	 (display-buffer-no-window))))
 > (setq async-shell-command-display-buffer nil)
 > --8<---------------cut here---------------end--------------->8---
 >
 > `M-& echo foo RET' will unexpectedly show the `*Async Shell Command*'
 > buffer.

I suppose (Juri will correct me) that this snippet in 'shell-command'

                 (if async-shell-command-display-buffer
                     ;; Display buffer immediately.
                     (display-buffer buffer '(nil (allow-no-window . t))) <<<<<
                   ;; Defer displaying buffer until first process output.
                   ;; Use disposable named advice so that the buffer is
                   ;; displayed at most once per process lifetime.
                   (let ((nonce (make-symbol "nonce")))
                     (add-function :before (process-filter proc)
                                   (lambda (proc _string)
                                     (let ((buf (process-buffer proc)))
                                       (when (buffer-live-p buf)
                                         (remove-function (process-filter proc)
                                                          nonce)
                                         (display-buffer buf)))) <<<<<
                                   `((name . ,nonce)))))))

adding an 'allow-no-window' entry if and only if
'async-shell-command-display-buffer' is non-nil is responsible for the
behavior Eliza sees.  I have no idea whether adding such an entry in the
second case could cause problems.  We could give
'async-shell-command-display-buffer' a third value, say 'allow-no-window
and, if a user has set it to that value, have 'shell-command' add an
'allow-no-window' entry in the second case too.

martin

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Juri Linkov]

>>> I'm probably missing something, but how can display-buffer fail to
>>> support any action function, such as display-buffer-no-window?
>>>
>>> Martin, what am I missing here?
>
> We may have to ask Juri, he conceived the "allow-no-window" concept.

I don't remember the details why we decided to design it that way.
But now I don't see why not enable allow-no-window by default,
i.e. why not to make it opt-out instead of opt-in.

>> Technically it seems that you can add `(allow-no-window . t)' to
>> `display-buffer-alist' to always force the buffer never to appear, but
>> that doesn't seem at all like its intended use.
>
> Maybe "force" is too strong here.  You can "force" it by adding an
> 'allow-no-window' entry to the alist _and_ a 'display-buffer-no-window'
> action in a position that precedes any other display buffer action.

Indeed, it's possible to add 'allow-no-window' in customization:

  (setq display-buffer-alist
        '(("\\*Async Shell Command\\*"
           display-buffer-no-window
           (allow-no-window . t))))
  (setq async-shell-command-display-buffer nil)

> I suppose (Juri will correct me) that this snippet in 'shell-command'
>
>                 (if async-shell-command-display-buffer
>                     ;; Display buffer immediately.
>                     (display-buffer buffer '(nil (allow-no-window . t))) <<<<<
>                   ;; Defer displaying buffer until first process output.
>                   ;; Use disposable named advice so that the buffer is
>                   ;; displayed at most once per process lifetime.
>                   (let ((nonce (make-symbol "nonce")))
>                     (add-function :before (process-filter proc)
>                                   (lambda (proc _string)
>                                     (let ((buf (process-buffer proc)))
>                                       (when (buffer-live-p buf)
>                                         (remove-function (process-filter proc)
>                                                          nonce)
>                                         (display-buffer buf)))) <<<<<
>                                   `((name . ,nonce)))))))
>
> adding an 'allow-no-window' entry if and only if
> 'async-shell-command-display-buffer' is non-nil is responsible for the
> behavior Eliza sees.  I have no idea whether adding such an entry in the
> second case could cause problems.  We could give
> 'async-shell-command-display-buffer' a third value, say 'allow-no-window
> and, if a user has set it to that value, have 'shell-command' add an
> 'allow-no-window' entry in the second case too.

I think it's a plain bug that the first call of 'display-buffer'
sets 'allow-no-window' to t, but the second call doesn't.

These two 'display-buffer' calls were intended to do the same thing.
Only the second call is delayed until input arrives.

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[martin rudalics]

 > Indeed, it's possible to add 'allow-no-window' in customization:
 >
 >    (setq display-buffer-alist
 >          '(("\\*Async Shell Command\\*"
 >             display-buffer-no-window
 >             (allow-no-window . t))))
 >    (setq async-shell-command-display-buffer nil)

But it's not recommended.  We say that

      It is assumed that when a caller of ‘display-buffer’ specifies a
      non-‘nil’ ‘allow-no-window’ entry, it is also able to handle a
      ‘nil’ return value.

and 'display-buffer-alist' is not in the domain of the caller.  We just
don't disallow it either so users are free to experiment with it (and
shoot themselves in the foot in the course of things).

 > I think it's a plain bug that the first call of 'display-buffer'
 > sets 'allow-no-window' to t, but the second call doesn't.
 >
 > These two 'display-buffer' calls were intended to do the same thing.
 > Only the second call is delayed until input arrives.

Maybe the buffer display is intended to simply wake up the user.  I
would find it disruptive, though.

I'd suggest to fix it your way on master.  As for the release branch,
people can customize 'display-buffer-alist' the way you suggested above.
Eliza WDYT?

martin

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Eliza Velasquez]

On Mon, Jul 03 2023 at 08:46 +02, martin rudalics <rudalics@gmx.at> wrote:

> I'd suggest to fix it your way on master.  As for the release branch,
> people can customize 'display-buffer-alist' the way you suggested
> above.  Eliza WDYT?

Sounds good to me!

-- 
Eliza

bug#64394: [PATCH] Fix `async-shell-command-display-buffer' display

[Juri Linkov]

close 64394 30.0.50
thanks

>> I'd suggest to fix it your way on master.  As for the release branch,
>> people can customize 'display-buffer-alist' the way you suggested
>> above.  Eliza WDYT?
>
> Sounds good to me!

Thanks for the patch!
Your patch is pushed now to master.