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 programming: cursor handled incorrectly #4102

Closed
thautwarm opened this issue Jan 3, 2020 · 4 comments · Fixed by #4372
Closed

Curses programming: cursor handled incorrectly #4102

thautwarm opened this issue Jan 3, 2020 · 4 comments · Fixed by #4372
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@thautwarm
Copy link

Environment

Windows build number: Microsoft Windows [Version 10.0.19041.1]
Windows Terminal version : Version: 0.7.3451.0

CPython: 3.7.4
CPython package curses(Windows-curses)

Steps to reproduce

Open cmd or PowerShell inside Windows Terminal, with python run

def main():
    window = curses.initscr()
    curses.noecho()
    curses.nonl()
    curses.nocbreak()
    curses.raw()
    window.keypad(False)
    x = 0
    y = 0
    while True:
        window.refresh()
        c = window.getch()
        window.addstr(0, 0, str(c))
        if c == ord('q'):
            return
        elif c == ord('a'):
            x = 10
        elif c == ord('b'):
            y = 10
        else:
            x = 0
            y = 0
            pass

        window.move(y, x)
try:
    main()
finally:
    curses.endwin()

Expected behavior

Press a, the cursor move to (0, 10) immediately.

Actual behavior

After pressing a, the cursor doesn't move to (0, 10) immediately, i.e., the screen show doesn't change at all. Then after an another pressing, the cursor moves to (0, 10).

P.S:

  • Expected behavior is according to the package curses bundled in Python of Linux distros.
  • Things work perfectly when we use cmd or PowerShell without Windows Terminal.

zephyrproject-rtos/windows-curses#7

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jan 3, 2020
@zadjii-msft
Copy link
Member

zadjii-msft commented Jan 3, 2020

Yep that looks broken to me. Definitely something wrong in conpty, since this repros in inception (cmd.exe /c wsl.exe cmd.exe) as well. Thanks for the clear report with easy repro steps!

(future me: I put the file in %~%\dev\tmp)

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty labels Jan 3, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 3, 2020
@zadjii-msft zadjii-msft modified the milestones: 21H1, Terminal v1.0 Jan 3, 2020
@j4james
Copy link
Collaborator

j4james commented Jan 5, 2020

I expect this is the same bug as #4107.

@DHowett-MSFT
Copy link
Contributor

Nah, this one is separate from #4107. It repros in inception (#4102 (comment)), and the inbox conhost servicing interop hasn't been updated to the changeset that regressed 4107.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 6, 2020
@DHowett-MSFT DHowett-MSFT modified the milestones: Terminal v1.0, 21H1 Jan 6, 2020
zadjii-msft added a commit that referenced this issue Jan 27, 2020
zadjii-msft added a commit that referenced this issue Jan 27, 2020
(cherry picked from commit 5d54842)
@ghost ghost added the In-PR This issue has a related PR label Jan 27, 2020
@ghost ghost closed this as completed in #4372 Jan 30, 2020
ghost pushed a commit that referenced this issue Jan 30, 2020
## Summary of the Pull Request

This is a pair of related fixes to conpty. For both of these bugs, the root cause was that the cursor was getting set to Off in conpty. Without the `CursorBlinkerTimer`, the cursor would remain off, and frames that only had cursor movements would not update the cursor position in the terminal.

## References

## PR Checklist
* [x] Closes #4102 
* [x] Closes #2642
* [x] I work here
* [ ] Tests added/passed
* [n/a] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

Recall that there's a bunch of cursor state that's hard to parse without looking up:
* `Visibility` This controls whether the cursor is visible _at all_, regardless if it's been blinked on or off
* `Blinking` controls whether the blinker timer should do something, or leave the cursor alone.
* `IsOn`: When the cursor is blinking, this alternates between true and false. 

The trick here is that we only `TriggerCursorMoved` when the cursor is `On`, and there are some scenarios where the cursor is manually set to off. 

Fundamentally, these two bugs are similar cases, but they are triggered by different things:
* #2642 was caused by `DoSrvPrivateAllowCursorBlinking(false)` (`^[[?12l`) also manually turning the cursor off.
* #4102 was caused by the client calling `SetConsoleScreenBuffer` to change the active buffer. `win-curses` actually uses that API instead of the alt buffer.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 30, 2020
@ghost
Copy link

ghost commented Feb 13, 2020

🎉This issue was addressed in #4372, which has now been successfully released as Windows Terminal Preview v0.9.433.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants