GNU bug report logs - #52380
28.0.50; [PATCH] run-python no longer focuses interpreter

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 52380 in the body.
You can then email your comments to 52380 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: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>
Subject: 28.0.50; [PATCH] run-python no longer focuses interpreter
Date: Thu, 09 Dec 2021 00:10:28 +0100

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

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

[run-python.patch (text/x-patch, inline)]

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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH] run-python no longer focuses
 interpreter
Date: Fri, 10 Dec 2021 13:06:11 +0100

Kévin Le Gouguec <kevin.legouguec <at> 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

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH] run-python no longer focuses
 interpreter
Date: Fri, 10 Dec 2021 13:08:04 +0100

Lars Ingebrigtsen <larsi <at> 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

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

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH] run-python no longer focuses
 interpreter
Date: Fri, 10 Dec 2021 20:26:34 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Lars Ingebrigtsen <larsi <at> 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 😒

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

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 52380 <at> debbugs.gnu.org
Subject: ERT, buffers and windows (was: bug#52380: 28.0.50; [PATCH]
 run-python no longer focuses interpreter)
Date: Fri, 10 Dec 2021 21:09:30 +0100

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

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> Lars Ingebrigtsen <larsi <at> 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?

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: larsi <at> gnus.org, 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: ERT, buffers and windows (was: bug#52380: 28.0.50;
 [PATCH] run-python no longer focuses interpreter)
Date: Fri, 10 Dec 2021 22:30:50 +0200

> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Date: Fri, 10 Dec 2021 21:09:30 +0100
> Cc: 52380 <at> 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.

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

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: ERT, buffers and windows
Date: Fri, 10 Dec 2021 21:58:16 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
>> Date: Fri, 10 Dec 2021 21:09:30 +0100
>> Cc: 52380 <at> 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?

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

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH] run-python no longer focuses
 interpreter
Date: Sat, 11 Dec 2021 23:23:15 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Lars Ingebrigtsen <larsi <at> 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".

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH] run-python no longer focuses
 interpreter
Date: Sun, 12 Dec 2021 07:40:08 +0100

Kévin Le Gouguec <kevin.legouguec <at> 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

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

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH v2] run-python no longer focuses
 interpreter
Date: Sun, 12 Dec 2021 15:03:43 +0100

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

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Kévin Le Gouguec <kevin.legouguec <at> 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 🙏

[0001-Make-M-x-run-python-select-the-window-again.patch (text/x-patch, attachment)]

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: 52380 <at> debbugs.gnu.org
Subject: Re: bug#52380: 28.0.50; [PATCH v2] run-python no longer focuses
 interpreter
Date: Mon, 13 Dec 2021 05:17:17 +0100

Kévin Le Gouguec <kevin.legouguec <at> 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

Previous Next

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