Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> branch: emacs-29
> commit bdb0bc2b4e44a7d40369e10e3de825d58fe46825
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     Fix tex-mode display-buffer issues
>     
>     * lisp/window.el (display-tex-shell-buffer-action): New defcustom.

This results in a test error:

--8<---------------cut here---------------start------------->8---
# make -C test test-custom-opts
...
The following options might have problems:
variable: display-tex-shell-buffer-action
   value: display-buffer-in-previous-window
    type: (cons :tag "Action" (choice :tag "Action functions" (choice :tag "Function" (const :tag "--" ignore) (const display-buffer-reuse-window) (const display-buffer-pop-up-window) (const display-buffer-same-window) (const display-buffer-pop-up-frame) (const display-buffer-full-frame) (const display-buffer-in-child-frame) (const display-buffer-below-selected) (const display-buffer-at-bottom) (const display-buffer-in-previous-window) (const display-buffer-use-least-recent-window) (const display-buffer-use-some-window) (const display-buffer-use-some-frame) (function :tag "Other function")) (repeat :tag "List of functions" (choice :tag "Function" (const :tag "--" ignore) (const display-buffer-reuse-window) (const display-buffer-pop-up-window) (const display-buffer-same-window) (const display-buffer-pop-up-frame) (const display-buffer-full-frame) (const display-buffer-in-child-frame) (const display-buffer-below-selected) (const display-buffer-at-bottom) (const display-buffer-in-previous-window) (const display-buffer-use-least-recent-window) (const display-buffer-use-some-window) (const display-buffer-use-some-frame) (function :tag "Other function")))) (alist :tag "Action arguments" :key-type symbol :value-type (sexp :tag "Value")))
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 12 Jun 2023 14:21:07 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Hi Eli,
> 
> > branch: emacs-29
> > commit bdb0bc2b4e44a7d40369e10e3de825d58fe46825
> > Author: Eli Zaretskii <eliz@gnu.org>
> > Commit: Eli Zaretskii <eliz@gnu.org>
> >
> >     Fix tex-mode display-buffer issues
> >     
> >     * lisp/window.el (display-tex-shell-buffer-action): New defcustom.
> 
> This results in a test error:
> 
> --8<---------------cut here---------------start------------->8---
> # make -C test test-custom-opts
> ...
> The following options might have problems:
> variable: display-tex-shell-buffer-action
>    value: display-buffer-in-previous-window
>     type: (cons :tag "Action" (choice :tag "Action functions" (choice :tag "Function" (const :tag "--" ignore) (const display-buffer-reuse-window) (const display-buffer-pop-up-window) (const display-buffer-same-window) (const display-buffer-pop-up-frame) (const display-buffer-full-frame) (const display-buffer-in-child-frame) (const display-buffer-below-selected) (const display-buffer-at-bottom) (const display-buffer-in-previous-window) (const display-buffer-use-least-recent-window) (const display-buffer-use-some-window) (const display-buffer-use-some-frame) (function :tag "Other function")) (repeat :tag "List of functions" (choice :tag "Function" (const :tag "--" ignore) (const display-buffer-reuse-window) (const display-buffer-pop-up-window) (const display-buffer-same-window) (const display-buffer-pop-up-frame) (const display-buffer-full-frame) (const display-buffer-in-child-frame) (const display-buffer-below-selected) (const display-buffer-at-bottom) (const display-buffer-in-previous-window) (const display-buffer-use-least-recent-window) (const display-buffer-use-some-window) (const display-buffer-use-some-frame) (function :tag "Other function")))) (alist :tag "Action arguments" :key-type symbol :value-type (sexp :tag "Value")))
> --8<---------------cut here---------------end--------------->8---

Sorry, I have no idea what this failure means.  In particular, the
value is correct for the defcustom, and works in practice, and also
fits the type of the defcustom (AFAICT).

The checks that cus-test.el does are not documented in any meaningful
way, and its output is not useful.  I didn't even know we had such a
test, and never ran it myself.

So I'm sorry, but I cannot make heads or tails out of this test
failure and its diagnostics.  Feel free to fix this, or ignore, or
anything else.

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Eli Zaretskii]

> Date: Mon, 12 Jun 2023 16:11:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Sorry, I have no idea what this failure means.  In particular, the
> value is correct for the defcustom, and works in practice, and also
> fits the type of the defcustom (AFAICT).
> 
> The checks that cus-test.el does are not documented in any meaningful
> way, and its output is not useful.  I didn't even know we had such a
> test, and never ran it myself.

Btw, running "make test-custom-opts" in test/ gives me a different
error:

  make[1]: Entering directory `/d/gnu/git/emacs/branch/test'
  Running 1 tests (2023-06-12 16:19:27+0300, selector `(not (or (tag :unstable) (tag :nativecomp)))')
  Loading d:/gnu/git/emacs/branch/admin/cus-test.el (source)...
  Running cus-test-load-libs
  Finding files...
  Test check-for-wrong-custom-opts backtrace:
    signal(error ("find exited with status 1"))
    apply(signal (error ("find exited with status 1")))
    (setq value-7 (apply fn-5 args-6))
    (unwind-protect (setq value-7 (apply fn-5 args-6)) (setq form-descri
    (if (unwind-protect (setq value-7 (apply fn-5 args-6)) (setq form-de
    (let (form-description-9) (if (unwind-protect (setq value-7 (apply f
    (let ((value-7 'ert-form-evaluation-aborted-8)) (let (form-descripti
    (let* ((fn-5 #'null) (args-6 (condition-case err (let ((signal-hook-
    (closure (t) nil (let* ((fn-0 #'file-readable-p) (args-1 (condition-
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name check-for-wrong-custom-opts :document
    ert-run-or-rerun-test(#s(ert--stats :selector (not (or (tag :unstabl
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "misc/test-custom-opts.el"
    command-line()
    normal-top-level()
  Test check-for-wrong-custom-opts condition:
      (error "find exited with status 1")
     FAILED  1/1  check-for-wrong-custom-opts (0.406250 sec) at misc/test-custom-opts.el:33

  Ran 1 tests, 0 results as expected, 1 unexpected (2023-06-12 16:19:28+0300, 1.453125 sec)

  1 unexpected results:
     FAILED  check-for-wrong-custom-opts

So there's more to this than meets the eye, and some code (but which
one?) which runs 'find' is probably doing that in a non-portable
fashion.  (I needed to run admin/cus-tests.el by hand to get the error
which you quote.)

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Robert Pluim]

>>>>> On Mon, 12 Jun 2023 16:11:03 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Michael Albinus <michael.albinus@gmx.de>
    >> Cc: Eli Zaretskii <eliz@gnu.org>
    >> Date: Mon, 12 Jun 2023 14:21:07 +0200
    >> 
    >> Eli Zaretskii <eliz@gnu.org> writes:
    >> 
    >> Hi Eli,
    >> 
    >> > branch: emacs-29
    >> > commit bdb0bc2b4e44a7d40369e10e3de825d58fe46825
    >> > Author: Eli Zaretskii <eliz@gnu.org>
    >> > Commit: Eli Zaretskii <eliz@gnu.org>
    >> >
    >> >     Fix tex-mode display-buffer issues
    >> >     
    >> >     * lisp/window.el (display-tex-shell-buffer-action): New defcustom.
    >> 
    >> This results in a test error:
    >> 
    >> --8<---------------cut here---------------start------------->8---
    >> # make -C test test-custom-opts
    >> ...
    >> The following options might have problems:
    >> variable: display-tex-shell-buffer-action
    >> value: display-buffer-in-previous-window
    >> type: (cons :tag "Action" (choice :tag "Action functions" (choice :tag "Function" (const :tag "--" ignore) (const display-buffer-reuse-window) (const display-buffer-pop-up-window) (const display-buffer-same-window) (const display-buffer-pop-up-frame) (const display-buffer-full-frame) (const display-buffer-in-child-frame) (const display-buffer-below-selected) (const display-buffer-at-bottom) (const display-buffer-in-previous-window) (const display-buffer-use-least-recent-window) (const display-buffer-use-some-window) (const display-buffer-use-some-frame) (function :tag "Other function")) (repeat :tag "List of functions" (choice :tag "Function" (const :tag "--" ignore) (const display-buffer-reuse-window) (const display-buffer-pop-up-window) (const display-buffer-same-window) (const display-buffer-pop-up-frame) (const display-buffer-full-frame) (const display-buffer-in-child-frame) (const display-buffer-below-selected) (const display-buffer-at-bottom) (const display-buffer-in-previous-window) (const display-buffer-use-least-recent-window) (const display-buffer-use-some-window) (const display-buffer-use-some-frame) (function :tag "Other function")))) (alist :tag "Action arguments" :key-type symbol :value-type (sexp :tag "Value")))
    >> --8<---------------cut here---------------end--------------->8---

    Eli> Sorry, I have no idea what this failure means.  In particular, the
    Eli> value is correct for the defcustom, and works in practice, and also
    Eli> fits the type of the defcustom (AFAICT).

Are you sure?

(defconst display-buffer--action-custom-type
  `(cons :tag "Action"
	 (choice :tag "Action functions"
		 ,display-buffer--action-function-custom-type
		 (repeat
		  :tag "List of functions"
		  ,display-buffer--action-function-custom-type))
	 (alist :tag "Action arguments"
		:key-type symbol
		:value-type (sexp :tag "Value")))
  "Custom type for `display-buffer' actions.")

(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
  "`display-buffer' action for displaying TeX shell buffers."
  :type display-buffer--action-custom-type

Did you mean this instead:

(defconst display-buffer--action-function-custom-type
  '(choice :tag "Function"
	   (const :tag "--" ignore) ; default for insertion
	   (const display-buffer-reuse-window)

Robert
-- 

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  emacs-devel@gnu.org
> Date: Mon, 12 Jun 2023 16:01:46 +0200
> 
>     Eli> Sorry, I have no idea what this failure means.  In particular, the
>     Eli> value is correct for the defcustom, and works in practice, and also
>     Eli> fits the type of the defcustom (AFAICT).
> 
> Are you sure?

Sure about which part?

Once again, if you see the problem, feel free to fix it, and thanks.

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Robert Pluim]

>>>>> On Mon, 12 Jun 2023 17:35:35 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Michael Albinus <michael.albinus@gmx.de>,  emacs-devel@gnu.org
    >> Date: Mon, 12 Jun 2023 16:01:46 +0200
    >> 
    Eli> Sorry, I have no idea what this failure means.  In particular, the
    Eli> value is correct for the defcustom, and works in practice, and also
    Eli> fits the type of the defcustom (AFAICT).
    >> 
    >> Are you sure?

    Eli> Sure about which part?

About it fitting the type of the defcustom. Since the current value
works for you, it appears the code is more lenient than the defcustom
type indicates.

    Eli> Once again, if you see the problem, feel free to fix it, and thanks.

The minimal fix would be:

diff --git a/lisp/window.el b/lisp/window.el
index 78b0787abdb..16f16a75418 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -8776,7 +8776,7 @@ display-comint-buffer-action
   :group 'windows
   :group 'comint)
 
-(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
+(defcustom display-tex-shell-buffer-action '(display-buffer-in-previous-window)
   "`display-buffer' action for displaying TeX shell buffers."
   :type display-buffer--action-custom-type
   :risky t

Although if we went by analogy to `display-comint-buffer-action', it
would have to be this:

diff --git a/lisp/window.el b/lisp/window.el
index 78b0787abdb..0213023e495 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -7575,6 +7575,13 @@ display-buffer--same-window-action
 Specifies to call `display-buffer-same-window'.")
 (put 'display-buffer--same-window-action 'risky-local-variable t)
 
+(defvar display-buffer--in-previous-window-action
+  '(display-buffer-in-previous-window
+    (inhibit-same-window . t))
+  "A `display-buffer' action for displaying in the previous window.
+Specifies to call `display-buffer-in-previous-window'.")
+(put 'display-buffer--in-previous-window-action 'risky-local-variable t)
+
 (defvar display-buffer--other-frame-action
   '((display-buffer-reuse-window
      display-buffer-pop-up-frame)
@@ -8776,7 +8783,7 @@ display-comint-buffer-action
   :group 'windows
   :group 'comint)
 
-(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
+(defcustom display-tex-shell-buffer-action display-buffer--in-previous-window-action
   "`display-buffer' action for displaying TeX shell buffers."
   :type display-buffer--action-custom-type
   :risky t


Robert
-- 

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: michael.albinus@gmx.de,  emacs-devel@gnu.org
> Date: Mon, 12 Jun 2023 17:57:00 +0200
> 
> The minimal fix would be:
> 
> diff --git a/lisp/window.el b/lisp/window.el
> index 78b0787abdb..16f16a75418 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -8776,7 +8776,7 @@ display-comint-buffer-action
>    :group 'windows
>    :group 'comint)
>  
> -(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
> +(defcustom display-tex-shell-buffer-action '(display-buffer-in-previous-window)
>    "`display-buffer' action for displaying TeX shell buffers."
>    :type display-buffer--action-custom-type
>    :risky t

Fine by me, please install.

> Although if we went by analogy to `display-comint-buffer-action', it
> would have to be this:
> 
> diff --git a/lisp/window.el b/lisp/window.el
> index 78b0787abdb..0213023e495 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -7575,6 +7575,13 @@ display-buffer--same-window-action
>  Specifies to call `display-buffer-same-window'.")
>  (put 'display-buffer--same-window-action 'risky-local-variable t)
>  
> +(defvar display-buffer--in-previous-window-action
> +  '(display-buffer-in-previous-window
> +    (inhibit-same-window . t))
> +  "A `display-buffer' action for displaying in the previous window.
> +Specifies to call `display-buffer-in-previous-window'.")
> +(put 'display-buffer--in-previous-window-action 'risky-local-variable t)
> +
>  (defvar display-buffer--other-frame-action
>    '((display-buffer-reuse-window
>       display-buffer-pop-up-frame)
> @@ -8776,7 +8783,7 @@ display-comint-buffer-action
>    :group 'windows
>    :group 'comint)
>  
> -(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
> +(defcustom display-tex-shell-buffer-action display-buffer--in-previous-window-action
>    "`display-buffer' action for displaying TeX shell buffers."
>    :type display-buffer--action-custom-type
>    :risky t

Why do we need an extra variable here?

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Robert Pluim]

>>>>> On Mon, 12 Jun 2023 19:40:24 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: michael.albinus@gmx.de,  emacs-devel@gnu.org
    >> Date: Mon, 12 Jun 2023 17:57:00 +0200
    >> 
    >> The minimal fix would be:
    >> 
    >> diff --git a/lisp/window.el b/lisp/window.el
    >> index 78b0787abdb..16f16a75418 100644
    >> --- a/lisp/window.el
    >> +++ b/lisp/window.el
    >> @@ -8776,7 +8776,7 @@ display-comint-buffer-action
    >> :group 'windows
    >> :group 'comint)
    >> 
    >> -(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
    >> +(defcustom display-tex-shell-buffer-action '(display-buffer-in-previous-window)
    >> "`display-buffer' action for displaying TeX shell buffers."
    >> :type display-buffer--action-custom-type
    >> :risky t

    Eli> Fine by me, please install.

Done, although we should consider having that action have
'(inhibit-same-window . t)' in it. Maybe on master.

    >> -(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
    >> +(defcustom display-tex-shell-buffer-action display-buffer--in-previous-window-action
    >> "`display-buffer' action for displaying TeX shell buffers."
    >> :type display-buffer--action-custom-type
    >> :risky t

    Eli> Why do we need an extra variable here?

We donʼt, it just looks nicer than stuffing a multi-line constant in
the default value field.

Robert
-- 

Re: emacs-29 bdb0bc2b4e4: Fix tex-mode display-buffer issues

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: michael.albinus@gmx.de,  emacs-devel@gnu.org
> Date: Mon, 12 Jun 2023 19:10:02 +0200
> 
> >>>>> On Mon, 12 Jun 2023 19:40:24 +0300, Eli Zaretskii <eliz@gnu.org> said:
> 
>     Eli> Fine by me, please install.
> 
> Done, although we should consider having that action have
> '(inhibit-same-window . t)' in it. Maybe on master.

Definitely on master.

>     >> -(defcustom display-tex-shell-buffer-action 'display-buffer-in-previous-window
>     >> +(defcustom display-tex-shell-buffer-action display-buffer--in-previous-window-action
>     >> "`display-buffer' action for displaying TeX shell buffers."
>     >> :type display-buffer--action-custom-type
>     >> :risky t
> 
>     Eli> Why do we need an extra variable here?
> 
> We donʼt, it just looks nicer than stuffing a multi-line constant in
> the default value field.

I guess "nicer" is in the eyes of the beholder.  To me, it just makes
the whole arrangement harder to read and understand.