bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

[Kévin Le Gouguec]

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

Hi,

In Emacs 27.2, M-x run-python did two things that Emacs 28 no longer
does.  From emacs -Q:

(1) M-x run-python
    Problem 1: 27.2 makes the Python buffer current; 28 does not.

(2) Additional step for Emacs 28, to get to the *Python* buffer:
    C-x o

(3) M-x bury-buffer

(4) M-x run-python
    Problem 2: 27.2 raises the Python buffer; 28 does not.

(The latter might seem like a niche corner case; it's actually a very
convenient feature of python.el's C-c C-p, IMO)

The current code in python.el fails on two fronts, AFAICT:

(1) the set-buffer clause near the end of run-python is ineffectual,
    because, as the docstring says:

>               This function does not display the buffer, so its effect
> ends when the current command terminates.  Use ‘switch-to-buffer’ or
> ‘pop-to-buffer’ to switch buffers permanently.

(2) the (when show (display-buffer buffer)) form in
    python-shell-make-comint is not evaluated when the *Python* buffer
    already exists, because of it is guarded by (when (not
    (comint-check-proc proc-buffer-name)) …).

I've bisected this; the "faulty" commit (2020-11-09 "Make the SHOW
parameter work again in `run-python'" (0d9e2b80d8)) was itself fixing a
regression.  Trying to build on top of it, I propose the attached patch.
(No changelog entry, because I'm not sure it's the right way to solve
this)

WDYT?


In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.16.0)
 of 2021-12-06 built on amdahl30
Repository revision: 4434deaee2aa9d8c6b9631690c6376f78a9b057f
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101001
System Description: openSUSE Tumbleweed

Configured using:
 'configure --with-xwidgets --with-cairo --with-gconf --with-xinput2'

Configured features:
ACL CAIRO DBUS FREETYPE GCONF GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ
JPEG JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XINPUT2 XPM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=local
  locale-coding-system: utf-8-unix


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: run-python.patch --]
[-- Type: text/x-patch, Size: 845 bytes --]

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 47d8d1ce8e..aee89f6519 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2996,8 +2996,8 @@ python-shell-make-comint
                   (mapconcat #'identity args " ")))
             (with-current-buffer buffer
               (inferior-python-mode))
-            (when show (display-buffer buffer))
             (and internal (set-process-query-on-exit-flag process nil))))
+        (when show (pop-to-buffer proc-buffer-name))
         proc-buffer-name))))
 
 ;;;###autoload
@@ -3029,7 +3029,6 @@ run-python
          (python-shell-make-comint
           (or cmd (python-shell-calculate-command))
           (python-shell-get-process-name dedicated) show)))
-    (set-buffer buffer)
     (get-buffer-process buffer)))
 
 (defun run-python-internal ()

bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> I've bisected this; the "faulty" commit (2020-11-09 "Make the SHOW
> parameter work again in `run-python'" (0d9e2b80d8)) was itself fixing a
> regression.  Trying to build on top of it, I propose the attached patch.
> (No changelog entry, because I'm not sure it's the right way to solve
> this)
>
> WDYT?

Makes sense to me; pushed to emacs-28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Makes sense to me; pushed to emacs-28.

But then I reverted it, because it led to a test failure, so could you
have a look at that?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Makes sense to me; pushed to emacs-28.
>
> But then I reverted it, because it led to a test failure, so could you
> have a look at that?

Well great; I meant to write a test case for that eventually.

Assuming you mean python-tests--bug31398?  That's the one I'm seeing
failing with my patch.

I've banged my head on it for an hour now; I don't understand what's
going on.  The test case is checking precisely what I was trying to fix,
and for some reason the set-buffer I removed does something that the
pop-to-buffer I replaced it with doesn't.

What set-buffer does not, of course, is actually work in interactive
usage 😒

bug#52380: ERT, buffers and windows (was: bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter)

[Kévin Le Gouguec]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>
>>> Makes sense to me; pushed to emacs-28.
>>
>> But then I reverted it, because it led to a test failure, so could you
>> have a look at that?
>
> Well great; I meant to write a test case for that eventually.
>
> Assuming you mean python-tests--bug31398?  That's the one I'm seeing
> failing with my patch.
>
> I've banged my head on it for an hour now; I don't understand what's
> going on.  The test case is checking precisely what I was trying to fix,
> and for some reason the set-buffer I removed does something that the
> pop-to-buffer I replaced it with doesn't.
>
> What set-buffer does not, of course, is actually work in interactive
> usage 😒

If I amend the test as follows:

diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
index 15bda5c197..783f115fc7 100644
--- a/test/lisp/progmodes/python-tests.el
+++ b/test/lisp/progmodes/python-tests.el
@@ -5454,6 +5454,7 @@ python-tests--bug31398
   "Test for https://debbugs.gnu.org/31398 ."
   (skip-unless (executable-find python-tests-shell-interpreter))
   (let ((buffer (process-buffer (run-python nil nil 'show))))
+    (should (eq (get-buffer-window buffer) (selected-window)))
     (should (eq buffer (current-buffer)))
     (pop-to-buffer (other-buffer))
     (run-python nil nil 'show)

… then (should (eq (get-buffer-window buffer) (selected-window)))
passes, but (should (eq buffer (current-buffer))) does not.  Does that
make sense to anyone?

bug#52380: ERT, buffers and windows (was: bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter)

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Fri, 10 Dec 2021 21:09:30 +0100
> Cc: 52380@debbugs.gnu.org
> 
> If I amend the test as follows:
> 
> diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
> index 15bda5c197..783f115fc7 100644
> --- a/test/lisp/progmodes/python-tests.el
> +++ b/test/lisp/progmodes/python-tests.el
> @@ -5454,6 +5454,7 @@ python-tests--bug31398
>    "Test for https://debbugs.gnu.org/31398 ."
>    (skip-unless (executable-find python-tests-shell-interpreter))
>    (let ((buffer (process-buffer (run-python nil nil 'show))))
> +    (should (eq (get-buffer-window buffer) (selected-window)))
>      (should (eq buffer (current-buffer)))
>      (pop-to-buffer (other-buffer))
>      (run-python nil nil 'show)
> 
> … then (should (eq (get-buffer-window buffer) (selected-window)))
> passes, but (should (eq buffer (current-buffer))) does not.  Does that
> make sense to anyone?

You assume that the selected window always shows the current buffer?
That's not in general true.

bug#52380: ERT, buffers and windows

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Date: Fri, 10 Dec 2021 21:09:30 +0100
>> Cc: 52380@debbugs.gnu.org
>> 
>> If I amend the test as follows:
>> 
>> diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
>> index 15bda5c197..783f115fc7 100644
>> --- a/test/lisp/progmodes/python-tests.el
>> +++ b/test/lisp/progmodes/python-tests.el
>> @@ -5454,6 +5454,7 @@ python-tests--bug31398
>>    "Test for https://debbugs.gnu.org/31398 ."
>>    (skip-unless (executable-find python-tests-shell-interpreter))
>>    (let ((buffer (process-buffer (run-python nil nil 'show))))
>> +    (should (eq (get-buffer-window buffer) (selected-window)))
>>      (should (eq buffer (current-buffer)))
>>      (pop-to-buffer (other-buffer))
>>      (run-python nil nil 'show)
>> 
>> … then (should (eq (get-buffer-window buffer) (selected-window)))
>> passes, but (should (eq buffer (current-buffer))) does not.  Does that
>> make sense to anyone?
>
> You assume that the selected window always shows the current buffer?
> That's not in general true.

Mmm, thanks.  I see that "(elisp) Current Buffer" has things to say on
the subject:

>    When an editing command returns to the editor command loop, Emacs
> automatically calls ‘set-buffer’ on the buffer shown in the selected
> window (*note Selecting Windows::).

In the context of ERT, what would be the correct way to test that after
calling M-x run-python (≡ (run-python nil nil 'show)), the *Python*
buffer is visible, and its window is selected?

Should we just disregard (current-buffer) when writing ERT tests, and
assert things in terms of (selected-window)?  Or is there a way to make
ERT "return to the editor command loop", thereby selecting the buffer
shown in the selected window?

bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

[Kévin Le Gouguec]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Makes sense to me; pushed to emacs-28.
>
> But then I reverted it, because it led to a test failure, so could you
> have a look at that?

Based on my understanding of

- my digression on buffers and windows in my subthread on ERT,

- how buffer "currentness" relates to window "selectedness" in the
  context of a regular command loop vs during a test execution,

- what Tino attempted to fix with bug#31398,

… my inclination would be to (1) keep my patch as-is, (2) amend the test
to check (selected-window) rather than (current-buffer), and (3) add a
comment to explain what we want to test and why we do it that way[1].

We could keep the call to (set-buffer) in run-python, but AFAICT it's
redundant for user interaction: pop-to-buffer selects the window, so
when the command loop returns to the user the *Python* buffer will be
made current anyway.

Does this sound… sound?  If so, I'll submit a v2 amending
python-tests--bug31398.


[1] And optionally (4) start a thread on emacs-devel to better
    understand ERT pitfalls.  I seem to keep stumbling on them; I dimly
    remember struggling to write Elisp code that would reproduce a
    tricky issue with undo and electric-pair-mode (that was 100%
    reproducible interactively), and struggling to write font-lock tests
    because some fontification passes are not triggered unless something
    happens interactively.

    (Or something.  I'll do my research before starting this thread,
    obviously)

    I think at the very least, some documentation of these issues in the
    ERT manual would help; ideally ERT could also provide helpers to
    simulate a "regular command loop".

bug#52380: 28.0.50; [PATCH] run-python no longer focuses interpreter

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> We could keep the call to (set-buffer) in run-python, but AFAICT it's
> redundant for user interaction: pop-to-buffer selects the window, so
> when the command loop returns to the user the *Python* buffer will be
> made current anyway.
>
> Does this sound… sound?  If so, I'll submit a v2 amending
> python-tests--bug31398.

Sounds good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#52380: 28.0.50; [PATCH v2] run-python no longer focuses interpreter

[Kévin Le Gouguec]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> We could keep the call to (set-buffer) in run-python, but AFAICT it's
>> redundant for user interaction: pop-to-buffer selects the window, so
>> when the command loop returns to the user the *Python* buffer will be
>> made current anyway.
>>
>> Does this sound… sound?  If so, I'll submit a v2 amending
>> python-tests--bug31398.
>
> Sounds good to me.

Alright, here goes; after reviewing the original bug#31398 report, I
also took the liberty of adjusting the test name, commentary and
docstring.

It'd be great if the rationale bit of the commit message made it in 🙏


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-M-x-run-python-select-the-window-again.patch --]
[-- Type: text/x-patch, Size: 3605 bytes --]

From 9b61be086d9651a9f07962e20a1f5e09cd46f1f2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Sun, 12 Dec 2021 12:24:24 +0100
Subject: [PATCH] Make `M-x run-python' select the window again

Interactively, we want M-x run-python to focus the interpreter buffer.
The previous code failed in two ways:

- the call to 'display-buffer' was not reached if an interpreter
  was already running,

- set-buffer is ineffectual if the interpreter's window is not
  selected: once Emacs returns to the command loop, the current buffer
  will revert back to what the selected window contains.

* lisp/progmodes/python.el (python-shell-make-comint): Handle the SHOW
argument regardless of whether an interpreter buffer exists, and use
pop-to-buffer to select the window.
(run-python): Delegate buffer management to
'python-shell-make-comint'.

* test/lisp/progmodes/python-tests.el
(python-tests--run-python-selects-window): Rename from
'python-tests--bug31398', and adjust assertions.
---
 lisp/progmodes/python.el            |  4 ++--
 test/lisp/progmodes/python-tests.el | 18 ++++++++++++------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 47d8d1ce8e..b403de8b7a 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -2996,8 +2996,9 @@ python-shell-make-comint
                   (mapconcat #'identity args " ")))
             (with-current-buffer buffer
               (inferior-python-mode))
-            (when show (display-buffer buffer))
             (and internal (set-process-query-on-exit-flag process nil))))
+        (when show
+          (pop-to-buffer proc-buffer-name))
         proc-buffer-name))))
 
 ;;;###autoload
@@ -3029,7 +3030,6 @@ run-python
          (python-shell-make-comint
           (or cmd (python-shell-calculate-command))
           (python-shell-get-process-name dedicated) show)))
-    (set-buffer buffer)
     (get-buffer-process buffer)))
 
 (defun run-python-internal ()
diff --git a/test/lisp/progmodes/python-tests.el b/test/lisp/progmodes/python-tests.el
index 15bda5c197..2d1ccdca41 100644
--- a/test/lisp/progmodes/python-tests.el
+++ b/test/lisp/progmodes/python-tests.el
@@ -5449,15 +5449,21 @@ python-tests--python-nav-end-of-statement--infloop
                     (python-nav-end-of-statement)))
     (should (eolp))))
 
-;; After call `run-python' the buffer running the python process is current.
-(ert-deftest python-tests--bug31398 ()
-  "Test for https://debbugs.gnu.org/31398 ."
+;; Interactively, `run-python' focuses the buffer running the
+;; interpreter.
+(ert-deftest python-tests--run-python-selects-window ()
+  "Test for bug#31398.  See also bug#44421 and bug#52380."
   (skip-unless (executable-find python-tests-shell-interpreter))
-  (let ((buffer (process-buffer (run-python nil nil 'show))))
-    (should (eq buffer (current-buffer)))
+  (let* ((buffer (process-buffer (run-python nil nil 'show)))
+         (window (get-buffer-window buffer)))
+    ;; We look at `selected-window' rather than `current-buffer'
+    ;; because as `(elisp)Current buffer' says, the latter will only
+    ;; be synchronized with the former when returning to the "command
+    ;; loop"; until then, `current-buffer' can change arbitrarily.
+    (should (eq window (selected-window)))
     (pop-to-buffer (other-buffer))
     (run-python nil nil 'show)
-    (should (eq buffer (current-buffer)))))
+    (should (eq window (selected-window)))))
 
 (ert-deftest python-tests--fill-long-first-line ()
   (should
-- 
2.34.1

bug#52380: 28.0.50; [PATCH v2] run-python no longer focuses interpreter

[Lars Ingebrigtsen]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Alright, here goes; after reviewing the original bug#31398 report, I
> also took the liberty of adjusting the test name, commentary and
> docstring.

Thanks; pushed to Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no