Skip to content

Fixes #2745 - NetDriver key handling is broken on Windows Terminal v1.18+#2746

Merged
tig merged 9 commits intogui-cs:developfrom
tig:v1_fixes2745_netdriver_keyhandling
Jul 19, 2023
Merged

Fixes #2745 - NetDriver key handling is broken on Windows Terminal v1.18+#2746
tig merged 9 commits intogui-cs:developfrom
tig:v1_fixes2745_netdriver_keyhandling

Conversation

@tig
Copy link
Copy Markdown
Collaborator

@tig tig commented Jul 12, 2023 •
edited
Loading

Fixes #2745 (see also this WT issue: microsoft/terminal#15693)

This PR comments out the code in NetDriver that attempted to use ESC sequences to determine if the terminal has been resized. This code is naive and doesn't actually work: If IsTerminalWithOptions is true we send the "query size" ESC sequence every 10ms and WT 1.18+ dutifully responds. Thus all other keyboard input is swamped.

All of this code will be significantly reworked in v2 (as part of #2612).

@BDisp - You put this code in for a reason so I'm a little leery about simply disabling it as I've done here. Do you remember what scenario this may break (e.g. is it some oddball linux/docker scenario or some other terminal beyond Windows Terminal?).

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig tig added the v1 label Jul 12, 2023
@tig tig requested a review from migueldeicaza as a code owner July 12, 2023 14:52
@tig
Copy link
Copy Markdown
Collaborator Author

tig commented Jul 12, 2023

I've also pulled #2731 into this. Thanks @tznind .

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Jul 12, 2023

@BDisp - You put this code in for a reason so I'm a little leery about simply disabling it as I've done here. Do you remember what scenario this may break (e.g. is it some oddball linux/docker scenario or some other terminal beyond Windows Terminal?).

Yes, I know why I put that. It's was to use on Linux because it don't recognize Console.BufferWidth and the Console.BufferHeight and the escape sequence is querying for terminal resize changes. In Linux this is natively fast but on a pseudo terminal on Windows, even with Windows Terminal it take longer to process them. But I also realized the query only return the windows size and not the buffer size. So, that can be ignored.

@tig
Copy link
Copy Markdown
Collaborator Author

tig commented Jul 12, 2023

@BDisp - You put this code in for a reason so I'm a little leery about simply disabling it as I've done here. Do you remember what scenario this may break (e.g. is it some oddball linux/docker scenario or some other terminal beyond Windows Terminal?).

Yes, I know why I put that. It's was to use on Linux because it don't recognize Console.BufferWidth and the Console.BufferHeight and the escape sequence is querying for terminal resize changes. In Linux this is natively fast but on a pseudo terminal on Windows, even with Windows Terminal it take longer to process them. But I also realized the query only return the windows size and not the buffer size. So, that can be ignored.

My test results:

  • In WT v1.18 - Works fine

  • In Terminator via WSL and this PR works fine:
    rXRRHFH 1

  • In xterm via WSL this PR works fine:
    wSxQ079 1

  • ConEmu (not WSL)

    • Resizing does not work (but doesn't work with develop either)

@BDisp
Copy link
Copy Markdown
Collaborator

BDisp commented Jul 12, 2023

As I said, when I implemented that was to get the terminal buffer size (to use with the EnableConsoleScrolling but it only get the terminal size) and not the the terminal size. Linux read the terminfo to get the LINES and COLS and the mainloop invoke a event when their values changes and it not need the escape sequence for that, glad you removed it. On ConEmu I didn't test it yet.

@tig
Copy link
Copy Markdown
Collaborator Author

tig commented Jul 14, 2023

Last call before I merge this fix. I'm pretty sure it doesn't break anything else, but I'd feel much better if others had tested it.

BDisp
BDisp requested changes Jul 14, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@BDisp BDisp left a comment

Choose a reason for hiding this comment

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

@tig please see my review, thanks.

Comment thread Terminal.Gui/ConsoleDrivers/NetDriver.cs Outdated
@tig tig merged commit 155e533 into gui-cs:develop Jul 19, 2023
@tig tig deleted the v1_fixes2745_netdriver_keyhandling branch April 3, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BDisp BDisp BDisp requested changes

@migueldeicaza migueldeicaza Awaiting requested review from migueldeicaza

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NetDriver key handling is broken on Windows Terminal v1.18+

2 participants

@tig @BDisp