Skip to content
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

curses.get_wch() returns keypad codes incorrectly #59989

Closed
simpkins mannequin opened this issue Aug 26, 2012 · 18 comments
Closed

curses.get_wch() returns keypad codes incorrectly #59989

simpkins mannequin opened this issue Aug 26, 2012 · 18 comments
Labels
release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@simpkins
Copy link
Mannequin

simpkins mannequin commented Aug 26, 2012

BPO 15785
Nosy @loewis, @akuchling, @birkenfeld, @jcea, @cben, @pitrou, @vstinner, @bitdancer
Files
  • curses_get_wch-2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2012-09-08.05:49:49.618>
    created_at = <Date 2012-08-26.10:09:38.430>
    labels = ['type-bug', 'library', 'release-blocker']
    title = 'curses.get_wch() returns keypad codes incorrectly'
    updated_at = <Date 2012-09-09.09:18:56.906>
    user = 'https://bugs.python.org/simpkins'

    bugs.python.org fields:

    activity = <Date 2012-09-09.09:18:56.906>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-09-08.05:49:49.618>
    closer = 'georg.brandl'
    components = ['Library (Lib)']
    creation = <Date 2012-08-26.10:09:38.430>
    creator = 'simpkins'
    dependencies = []
    files = ['27038']
    hgrepos = []
    issue_num = 15785
    keywords = ['patch']
    message_count = 18.0
    messages = ['169165', '169171', '169172', '169219', '169248', '169272', '169311', '169312', '169313', '169314', '169316', '169318', '169319', '169321', '169322', '169632', '170030', '170090']
    nosy_count = 16.0
    nosy_names = ['loewis', 'akuchling', 'georg.brandl', 'jcea', 'cben', 'pitrou', 'vstinner', 'gpolo', 'Arfrever', 'r.david.murray', 'inigoserna', 'phep', 'zeha', 'python-dev', 'Nicholas.Cole', 'simpkins']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue15785'
    versions = ['Python 3.3', 'Python 3.4']

    @simpkins
    Copy link
    Mannequin Author

    simpkins mannequin commented Aug 26, 2012

    The curses.get_wch() function does not check if wget_wch() returned OK or KEY_CODE_YES. In either case, it simply returns the character code.

    This makes get_wch() unusable when keypad is enabled, because the caller cannot distinguish function key or arrow key presses from real unicode code points. For example, get_wch() returns 259 both for an up arrow press and for the input character 'ă'.

    It seems like this API needs to be redesigned somehow to allow terminal keypad codes to be distinguished from unicode input.

    @simpkins simpkins mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 26, 2012
    @vstinner
    Copy link
    Member

    Oh, right. I agree that the current implementation of window.get_wch() is useless. I missed completly key codes.

    Attached patch changes the API of get_wch() from get_wch()->key:int to get_wch()->(is_key_code: bool, key: str). Examples: (True, 'KEY_UP'), (False, 'é').

    Problem: unget_wch() test fails. "unget_wch('é'); is_key_code, ch = win.get_wch()" returns is_key_code=True, ch=''. I don't see how to specify to unget_wch() if the argument is a key code or not... It's maybe an issue in unget_wch() API that cannot be fixed in Python?

    @vstinner
    Copy link
    Member

    I consider this issue as a release blocker because the bug requires to change the API (of a function added to Python 3.3).

    If this issue cannot be fixed before Python 3.3 final, I prefer to drop the function until a better implementation is written.

    @birkenfeld
    Copy link
    Member

    Please get a review from another developer before I consider this for rc2.

    @simpkins
    Copy link
    Mannequin Author

    simpkins mannequin commented Aug 28, 2012

    • Get a wide character as (is_key_code, key). *is_key_code* is True for
    • function keys, keypad keys and so, in this case, *key* is a multibyte string
    • containing the key name. Otherwise, *key* is a single character
    • corresponding to the key.

    The curses module already exposes the integer KEY_* constants. I think the
    API would be easier to use if it simply returned the integer keycode constant
    rather than returning the human-readable name returned by keyname().

    I suspect most callers will want to compare the keycode against one of these
    KEY_* constants to see what type of key was pressed so they can take action on
    specific keys. Comparing the return value against one of the curses.KEY_*
    constants seems easier than having to compare it to the result of
    curses.keyname(curses.KEY_*)

    The curses module also already exposes the keyname() function if callers do
    want to get the human-readable string for an integer keycode.

    If the function returned either a single-character unicode string or an integer
    keycode, this would also make it possible to completely drop the is_key_code
    part of the return value. (Callers could simply check the type of the return
    value to see if it is a keycode.)

    @vstinner
    Copy link
    Member

    If the function returned either a single-character unicode string or an
    integer
    keycode, this would also make it possible to completely drop the
    is_key_code
    part of the return value. (Callers could simply check the type of the
    return
    value to see if it is a keycode.)

    I tried to mimic the getkey() function, but I like your idea. In many cases
    you don't have to check explicitly the type. Example: if key == "q":
    quit(), or if key == curses.KEY_UP: move(1). It works also if the key is
    used as a key of a dictionary: key => callback. And yes, keyname() can be
    used to mimic manually getkey() behaviour.

    It does not solve unget_wch() issue, but I propose to drop the
    unget_wch()+get_wch() test on non-ASCII keys because it looks like a bug in
    the curses library.

    @vstinner
    Copy link
    Member

    New patch fixing the issue with a better API: window.get_wch()->int or str depending on the key (int for key codes, str for other keys).

    Oh, I realized that test_curses does also fail on my laptop without the patch. My laptop is running Fedora 11 which uses libncurses 5.7. This version is old and has known bugs. So the "unget_wch" issue is maybe unrelated.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 28, 2012

    Perhaps you should simply remove the new function, and re-add it in 3.4 when you've thought it out a bit more.

    @vstinner
    Copy link
    Member

    window.get_wch() has been added by the issue bpo-6755, and curses.unget_wch() by bpo-12567. Copy the nosy of the issue bpo-6755.

    @vstinner
    Copy link
    Member

    New patch with a test for this specific issue.

    (It's not easy to me to write a patch because my version of libncurses doesn't work with integers bigger than 128...)

    @vstinner
    Copy link
    Member

    Perhaps you should simply remove the new function, and re-add it in 3.4 when you've thought it out a bit more.

    Python 3 forces somehow to use Unicode, and the Unicode support of the curses module in Python 3.2 is incomplete or broken (see . Many issues have been fixed in Python 3.3. It would be a regression to remove get_wch(), it's an important function to use the curses module with Python 3.

    Changes between Python 3.2 and 3.3:
    http://docs.python.org/dev/whatsnew/3.3.html#curses

    @vstinner
    Copy link
    Member

    bitdancer tested the patch version 3 for me and it fails: unget_wch(KEY_UP) inserts the character U+0103 (259, à) instead of KEY_UP in get_wch() buffer. unget_wch() cannot be used to insert key codes, only classic keys like letters.

    So I removed the patch version 3 and restored patch version 2 which only changes the get_wch() API and adapts the existing unit test.

    @vstinner
    Copy link
    Member

    @simpkins: can you please try my patch?

    @vstinner
    Copy link
    Member

    bitdancer proposes (on IRC) a better doc for get_wch():

    "Get a wide character. Return a character for most keys, or an integer for function keys, keypad keys, and other special keys."

    We may also replace "and so on" by "and other special keys" in getkey() definition.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 28, 2012

    New changeset c58789634d22 by Victor Stinner in branch 'default':
    Issue bpo-15785: Modify window.get_wch() API of the curses module: return a
    http://hg.python.org/cpython/rev/c58789634d22

    @vstinner
    Copy link
    Member

    vstinner commented Sep 1, 2012

    @georg.brandl: Can you please include the important fix c58789634d22 into Python 3.3 final?

    @birkenfeld
    Copy link
    Member

    Now picked into 3.3.0 release clone in 23377e88487b.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2012

    New changeset 23377e88487b by Victor Stinner in branch 'default':
    Issue bpo-15785: Modify window.get_wch() API of the curses module: return a
    http://hg.python.org/cpython/rev/23377e88487b

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants