GNU bug report logs - #64394
[PATCH] Fix `async-shell-command-display-buffer' display

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 64394 in the body.
You can then email your comments to 64394 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eliza Velasquez <eliza <at> eliza.sh>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix `async-shell-command-display-buffer' display
Date: Fri, 30 Jun 2023 18:00:33 -0700

[Message part 1 (text/plain, inline)]

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'

[0001-Fix-async-shell-command-display-buffer-display.patch (text/patch, attachment)]

[Message part 3 (text/plain, inline)]

-- 
Eliza

Message #8 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eliza Velasquez <eliza <at> eliza.sh>, martin rudalics <rudalics <at> gmx.at>
Cc: 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Sat, 01 Jul 2023 10:24:05 +0300

> From: Eliza Velasquez <eliza <at> 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.

Message #11 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Eliza Velasquez <eliza <at> eliza.sh>
To: Eli Zaretskii <eliz <at> gnu.org>, martin rudalics <rudalics <at> gmx.at>
Cc: 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Sat, 01 Jul 2023 00:52:32 -0700

On Sat, Jul 01 2023 at 10:24 +03, Eli Zaretskii <eliz <at> 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

Message #14 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eliza Velasquez <eliza <at> eliza.sh>
Cc: rudalics <at> gmx.at, 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Sat, 01 Jul 2023 11:12:14 +0300

> From: Eliza Velasquez <eliza <at> eliza.sh>
> Cc: 64394 <at> 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?

Message #17 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Eliza Velasquez <eliza <at> eliza.sh>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: rudalics <at> gmx.at, 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Sat, 01 Jul 2023 01:42:53 -0700

On Sat, Jul 01 2023 at 11:12 +03, Eli Zaretskii <eliz <at> 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

Message #20 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Eliza Velasquez <eliza <at> eliza.sh>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 64394 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Sun, 2 Jul 2023 09:09:37 +0200

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

Message #23 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: martin rudalics <rudalics <at> gmx.at>
Cc: Eliza Velasquez <eliza <at> eliza.sh>, Eli Zaretskii <eliz <at> gnu.org>,
 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Sun, 02 Jul 2023 21:03:58 +0300

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

Message #26 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Juri Linkov <juri <at> linkov.net>
Cc: Eliza Velasquez <eliza <at> eliza.sh>, Eli Zaretskii <eliz <at> gnu.org>,
 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Mon, 3 Jul 2023 08:46:49 +0200

> 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

Message #29 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Eliza Velasquez <eliza <at> eliza.sh>
To: martin rudalics <rudalics <at> gmx.at>, Juri Linkov <juri <at> linkov.net>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Mon, 03 Jul 2023 18:18:09 -0700

On Mon, Jul 03 2023 at 08:46 +02, martin rudalics <rudalics <at> 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

Message #32 received at 64394 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Eliza Velasquez <eliza <at> eliza.sh>
Cc: martin rudalics <rudalics <at> gmx.at>, Eli Zaretskii <eliz <at> gnu.org>,
 64394 <at> debbugs.gnu.org
Subject: Re: bug#64394: [PATCH] Fix `async-shell-command-display-buffer'
 display
Date: Tue, 04 Jul 2023 20:54:58 +0300

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.