Skip to content

gh-146458: Fix repl height and width tracking on resize#146459

Open
GabrielvMarinho wants to merge 4 commits intopython:mainfrom
GabrielvMarinho:fix-repl-height-and-width-tracking-on-resize
Open

gh-146458: Fix repl height and width tracking on resize#146459
GabrielvMarinho wants to merge 4 commits intopython:mainfrom
GabrielvMarinho:fix-repl-height-and-width-tracking-on-resize

Conversation

@GabrielvMarinho
Copy link
Copy Markdown

@GabrielvMarinho GabrielvMarinho commented Mar 26, 2026 •
edited
Loading

On windows, the resize event should be triggering an update in the console height and width, however this is not happening.

I noticed the Unix console didn't have this problem, it was updating fine. My suggestion to fixing this is taking the resizing logic from both console implementations and let it at the reader level, that way it will update both (Unix and windows).

After adding this change, the resizing was happening:

fixed-repl-video.mp4

The tests then started failing as the Magic Mocks to getheightwidth weren't actually returning a tuple with the height and width, so I changed it by adding the side_effect keyword and sending a lambda with the values. With that addition, the tests are actually testing if the resize is happening, if you comment out the height and width resizing logic, they will fail.

@python-cla-bot
Copy link
Copy Markdown

python-cla-bot bot commented Mar 26, 2026 •
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Mar 26, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app bot mentioned this pull request Mar 26, 2026
Open
@GabrielvMarinho GabrielvMarinho force-pushed the fix-repl-height-and-width-tracking-on-resize branch from 29dca79 to 47c7453 Compare March 26, 2026 23:19
@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Mar 26, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@GabrielvMarinho GabrielvMarinho marked this pull request as ready for review March 27, 2026 00:51
@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 27, 2026

The CI is failing. Example:

======================================================================
ERROR: test_resize_bigger_on_multiline_function (test.test_pyrepl.test_unix_console.TestConsole.test_resize_bigger_on_multiline_function)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/mock.py", line 1439, in patched
    return func(*newargs, **newkeywargs)
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/test_unix_console.py", line 263, in test_resize_bigger_on_multiline_function
    _, con = handle_all_events(
             ~~~~~~~~~~~~~~~~~^
        [Event(evt="resize", data=None)],
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        prepare_reader=same_reader,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
        prepare_console=same_console,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/support.py", line 105, in handle_all_events
    reader.handle1()
    ~~~~~~~~~~~~~~^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/_pyrepl/reader.py", line 728, in handle1
    self.refresh()
    ~~~~~~~~~~~~^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/_pyrepl/reader.py", line 647, in refresh
    self.console.height, self.console.width = self.console.getheightwidth()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: not enough values to unpack (expected 2, got 0)

======================================================================
ERROR: test_resize_smaller_on_multiline_function (test.test_pyrepl.test_unix_console.TestConsole.test_resize_smaller_on_multiline_function)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/unittest/mock.py", line 1439, in patched
    return func(*newargs, **newkeywargs)
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/test_unix_console.py", line 299, in test_resize_smaller_on_multiline_function
    _, con = handle_all_events(
             ~~~~~~~~~~~~~~~~~^
        [Event(evt="resize", data=None)],
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        prepare_reader=same_reader,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
        prepare_console=same_console,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_pyrepl/support.py", line 105, in handle_all_events
    reader.handle1()
    ~~~~~~~~~~~~~~^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/_pyrepl/reader.py", line 728, in handle1
    self.refresh()
    ~~~~~~~~~~~~^^
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/_pyrepl/reader.py", line 647, in refresh
    self.console.height, self.console.width = self.console.getheightwidth()
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: not enough values to unpack (expected 2, got 0)

@GabrielvMarinho GabrielvMarinho force-pushed the fix-repl-height-and-width-tracking-on-resize branch from 47c7453 to 1e7907a Compare March 27, 2026 18:52
@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Mar 27, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@GabrielvMarinho GabrielvMarinho force-pushed the fix-repl-height-and-width-tracking-on-resize branch from 1e7907a to 42dd0de Compare March 27, 2026 20:54
@bedevere-app
Copy link
Copy Markdown

bedevere-app bot commented Mar 27, 2026

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 27, 2026

If possible, try to avoid git push --force, it makes reviews harder.

@GabrielvMarinho
Copy link
Copy Markdown
Author

GabrielvMarinho commented Mar 27, 2026

If possible, try to avoid git push --force, it makes reviews harder.

I had to git push --force some few times because i was rebasing, so its preferable to merge and do a new commit to get to the new state of the main?

@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 27, 2026

so its preferable to merge and do a new commit to get to the new state of the main?

Yes, merge is preferred. Rebase is also causing troubles in reviews.

@GabrielvMarinho
Copy link
Copy Markdown
Author

GabrielvMarinho commented Mar 27, 2026

so its preferable to merge and do a new commit to get to the new state of the main?

Yes, merge is preferred. Rebase is also causing troubles in reviews.

Thanks for the heads up, let me know if you need any help on the code review process.

vstinner
vstinner approved these changes Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. unix_console and window_console generates "resize" events. Before, the window_console didn't update the console height and width. Now, the code is generic in Reader.refresh().

@vstinner vstinner added needs backport to 3.14 bugs and security fixes needs backport to 3.13 bugs and security fixes labels Mar 27, 2026
@vstinner
Copy link
Copy Markdown
Member

vstinner commented Mar 27, 2026

You may add a NEWS entry to describe the fix.

@GabrielvMarinho
Copy link
Copy Markdown
Author

GabrielvMarinho commented Mar 27, 2026

You may add a NEWS entry to describe the fix.

I just added it, is it OK that way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

@ambv ambv Awaiting requested review from ambv ambv is a code owner

Assignees

No one assigned

Labels

awaiting merge needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@GabrielvMarinho @vstinner