Skip to content

gh-134210: handle signals in _curses.window.getch #134326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
May 27, 2025

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented May 20, 2025

@vstinner
Copy link
Member

vstinner commented May 20, 2025

I tried this change with the script:

from curses import wrapper
import ctypes

def main(window):
    window.nodelay(True)
    return window.getch()

print(wrapper(main))

Before (without this change), it displays -1.

After (with this change), it fails with the exception: _curses.error: wgetch() (called by getch()): no input.

So this change is backward incompatible. Is it deliberate?

@picnixz
Copy link
Member Author

picnixz commented May 20, 2025

Hum. No it's not meant to be broken!

Only the signals should be handled, but otherwise we shouldn't raise an exception.

@picnixz
Copy link
Member Author

picnixz commented May 20, 2025

Now I am bit conflicted. Ideally, we should return -1 for everything or raise an exception as in the other functions. I don't know why it was historically chosen to return -1 for getch but not for get_wch in no-delay mode.

However, since I don't want to break stuff, I don't know how we can make a smooth transition, except by raising a warning before returning -1.

@picnixz
Copy link
Member Author

picnixz commented May 24, 2025

For now, let's keep the same behavior (namely, -1 for getch, and exception for get_wch and getkey). The functions have been there at least since Python 3.4 and I prefer not to change anything unless a more pressing issue exists. At least, signal handlers will be properly called.

@picnixz picnixz removed the skip news label May 24, 2025
@picnixz picnixz requested a review from vstinner May 24, 2025 11:12
@picnixz
Copy link
Member Author

picnixz commented May 24, 2025

I could backport this as it's half a bugfix and half a feature. I'll only backport this to 3.14 as the layout of 3.13 is very different for curses (there was a conversion from static types to heap types and I don't think it made it to 3.13)

@picnixz picnixz requested a review from vstinner May 25, 2025 12:53
Copy link
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

@vstinner
Copy link
Member

You might backport the change to 3.13 and 3.14 bugfix branches.

@picnixz picnixz self-assigned this May 26, 2025
@picnixz picnixz merged commit 51762b6 into python:main May 27, 2025
39 checks passed
@picnixz picnixz added the needs backport to 3.14 bugs and security fixes label May 27, 2025
@miss-islington-app
Copy link

Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@picnixz picnixz deleted the fix/curses/getch-signals-134210 branch May 27, 2025 08:51
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 27, 2025
…34326)

(cherry picked from commit 51762b6)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented May 27, 2025

GH-134783 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label May 27, 2025
picnixz added a commit to picnixz/cpython that referenced this pull request May 27, 2025
…honGH-134326)

(cherry picked from commit 51762b6)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented May 27, 2025

GH-134784 is a backport of this pull request to the 3.13 branch.

picnixz added a commit that referenced this pull request May 27, 2025
#134783)

gh-134210: handle signals in `_curses.window.getch` (GH-134326)
(cherry picked from commit 51762b6)

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
picnixz added a commit that referenced this pull request May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants