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

Fix issues with running Python's curses module on Windows #6978

Closed
ulfalizer opened this issue Apr 7, 2018 · 13 comments
Closed

Fix issues with running Python's curses module on Windows #6978

ulfalizer opened this issue Apr 7, 2018 · 13 comments
Assignees
Labels
area: Configuration System bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@ulfalizer
Copy link
Collaborator

As part of #5847, I'm looking into getting the standard Python curses module running on Windows via the Python wheels provided on https://www.lfd.uci.edu/~gohlke/pythonlibs/#curses, which seem to be widely used.

The wheels mostly work fine, with the biggest issue being broken support for terminal resizing in the windows console. I've rebuilt them and found the issue, and will do a write-up (including build instructions) and email the wheel author later.

I also submitted a related issue to the PDCurses GitHub page. PDCurses is the curses implementation used by the wheels.

@ulfalizer
Copy link
Collaborator Author

CC @carlescufi

@carlescufi
Copy link
Member

Does this mean that, for the time being, we need to maintain a fork of both PDCurses and also the wheel itself? This seems a bit onerous. Once you have it working with curses it would be really good to see how much work it is to use a native Python module to achieve the same. Depending on the effort required we can then take a decision at the TSC level on this matter.

@cgohlke
Copy link

cgohlke commented Apr 7, 2018

I updated the wheels for curses yesterday. Please try them.

@carlescufi
Copy link
Member

@cgohlke thanks for chiming in and for the updated wheel. Is there a chance this will ever live in a Git (GitHub or elsewhere) repo complete with the necessary files to actually rebuild the wheel? We're a bit weary of relying on a package whose sources don't seem to contain the build files (Makefiles or anything else required).

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Apr 7, 2018

@cgohlke
Great to hear, and thanks for making those wheels available!

Since you're here, I solved the resizing issue by calling resize_term(0, 0) + erase() on receipt of KEY_RESIZE, which seems to be what PDCurses expects from looking at the demos (see e.g. demos\worm.c). That gets you ncurses-compatible behavior, with the windows automatically resized.

No new KEY_RESIZEs will be generated until you call resize_term() either, because of the following code in wincon/pdckbd.c:

if (!SP->resized)
{
    SP->resized = TRUE;
    return KEY_RESIZE;
}

Have you thought of enabling some of the HAVE_* macros by the way, for functions that are available in PDCurses?

@carlescufi
I rebuilt the wheel with this command after building PDCurses (with nmake -f Makefile.vc WIDE=y UTF8=y, in the wincon\ directory):

setup.py bdist_wheel build_ext --include-dirs=..\pdcurses-3.6 --library-dirs=..\pdcurses-3.6\wincon

I patched setup.py from the wheel to use setuptools, since I'm not sure how to build a wheel without it.

@cgohlke
Copy link

cgohlke commented Apr 7, 2018

I enabled all possible features:

define_macros = [
    ('PDC_WIDE', None),
    ('HAVE_NCURSESW', None),
    ('HAVE_TERM_H', None),
    ('HAVE_CURSES_IS_TERM_RESIZED', None),
    ('HAVE_CURSES_RESIZETERM', None),
    ('HAVE_CURSES_TYPEAHEAD', None),
    ('HAVE_CURSES_HAS_KEY', None),
    ('HAVE_CURSES_FILTER', None),
    ('HAVE_CURSES_WCHGAT', None),
    ('HAVE_CURSES_USE_ENV', None),
    ('WINDOW_HAS_FLAGS', None),
    ('NCURSES_MOUSE_VERSION', 2),
    ('_ISPAD', 0x10),
    ('is_term_resized', 'is_termresized'),
    ('resizeterm', 'resize_term'),
]

On Python >= 3.5, unicode should now work.

@ulfalizer
Copy link
Collaborator Author

@cgohlke
Nice, then it could be solved with a manual resize_term() call.

For ncurses-compatible behavior for KEY_RESIZE, I also added a resize_term(0, 0) in PyCursesWindow_GetCh() when rtn == KEY_RESIZE. Not sure if there's more to a proper fix.

@ulfalizer
Copy link
Collaborator Author

To clarify the issue:
On PDCurses, you need to call resize_term() to receive new KEY_RESIZE events (meaning KEY_RESIZE is effectively broken for programs that don't call resize_term()). On ncurses, you get a KEY_RESIZE every time you resize the terminal, regardless of whether you call resize_term(). ncurses also automatically resizes windows.

Calling resize_term(0, 0) after receiving KEY_RESIZE gives you similar behavior on PDCurses.

@MaureenHelm MaureenHelm added bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug labels Apr 13, 2018
@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Apr 19, 2018

Tried out the most recent menuconfig and wheel versions on Windows. Colors and Unicode work fine, but resizing is problematic.

I debugged an issue related to KEY_RESIZE and get_wch() (needed for Unicode support) in PDCurses and submitted a fix I think would work.

Even with that, calling resizeterm() causes a segfault, and calling resizeterm() is mandatory on PDCurses as far as I can tell to get resizing working.

Pretty sure I can get that debugged as well, but I wonder if I should spend time on it now or later. Besides resizing, the menuconfig is fully functional on Windows now at least.

Edit: Just to clarify: You don't get a segfault when resizing on Windows unless you try to implement proper resizing, which requires calling resizeterm() when using PDCurses.

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Apr 23, 2018

@cgohlke
I tried the latest version of the curses wheels. Resizing works fine now!

There's just one compatibility issue I noticed:

ncurses' resizeterm() is meant to be called from a SIGWINCH handler (the documentation doesn't make this super clear). It ungetch()'s KEY_RESIZE (see this code), meaning the code below gets stuck in a loop with ncurses, handling KEY_RESIZE over and over and ignoring other keys:

while True:
    c = win.get_wch()

    if c == curses.KEY_RESIZE:
        curses.resizeterm(*stdscr.getmaxyx())

    elif c == ...

Instead of binding PDCurses' resize_term() to curses.resizeterm(), I think it would be better to bind it to curses.resize_term() (i.e., just have HAVE_CURSES_RESIZE_TERM and get rid of the resizeterm -> resize_term renaming). The code above will then work for both ncurses and PDCurses after doing s/curses.resizeterm/curses.resize_term/.

Those functions seem closer in spirit too, as PDCurses is more manual when it comes to dealing with resizing.

The implementations of PyCurses_ResizeTerm() and PyCurses_Resize_Term() in _cursesmodule.c only differ in which function is called, so no other changes should be needed.

@cgohlke
Copy link

cgohlke commented Apr 23, 2018

Instead of binding PDCurses' resize_term() to curses.resizeterm(), I think it would be better to bind it to curses.resize_term() (i.e., just have HAVE_CURSES_RESIZE_TERM and get rid of the resizeterm -> resize_term renaming).

I made those changes.

How about using PDCurses' is_termresized for curses.is_term_resized? Is that safe?

@ulfalizer
Copy link
Collaborator Author

@cgohlke
Thanks!

Looks like it's safe.

ncurses resizes the terminal automatically inside getch() before returning KEY_RESIZE when it's configured to handle SIGWINCH itself (see the notes at the end of resizeterm(3NCURSES)), while PDCurses needs the manual resize_term() call, so is_term(_)resized() might hit at slightly different times. It seems to mean the same thing for both of them though.

The ncurses author mentions the is_termresized() spelling here.

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Apr 24, 2018

All issues seem to be sorted out for now, so closing. Thanks again to @cgohlke.

A related pull request is at #7174.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Configuration System bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

No branches or pull requests

4 participants