Resolve issue where first input gets ignored on new conpty with a ding sound#271556
Resolve issue where first input gets ignored on new conpty with a ding sound#271556anthonykim1 merged 10 commits intomainfrom
Conversation
|
I see pauseInputBarrier was introduced here: https://github.com/microsoft/vscode/pull/225336/files but I don't think its being used there anymore. |
|
|
||
| this._register(this._processManager.onProcessData(e => this._onProcessData(e))); | ||
| this._register(xterm.raw.onData(async data => { | ||
| await this._pauseInputEventBarrier?.wait(); |
There was a problem hiding this comment.
Now the approach is we remove the wait from onData and explicitly put this._processManager.write('\x1b[?61;4c');. I'm thinking this would allow this._processManager.write('\x1b[?61;4c'); to be sent later.
| queueMicrotask(() => { | ||
| this._processManager.write('\x1b[?61;4c'); | ||
| }); |
There was a problem hiding this comment.
Instead of this, a method for the onData listener and have this call that would be better. That will also mean that we get logs in the Terminal channel for this (one of the reasons it was hard to debug), and any future changes to that listener will also behave properly.
There was a problem hiding this comment.
sounds good!
Do you like the name _sendDataToProcess or should it be _writeDataToProcess? c53bef2
There was a problem hiding this comment.
_handleOnData is what I'd probably have gone with, but these are fine names too.
There was a problem hiding this comment.
Pull Request Overview
Fixes incorrect ordering of terminal negotiation sequences that caused ConPTY to misinterpret the user's first input as a cursor position response and emit a bell.
- Removes the pauseInputEvents barrier mechanism and introduces a helper _sendDataToProcess for consistent write + event firing.
- Refactors CSI handler for final 'c' to use the new helper when responding with ESC [?61;4c.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/vs/workbench/contrib/terminal/browser/terminalInstance.ts | Removes pause input barrier, adds _sendDataToProcess, and changes CSI 'c' handler logic to use it. |
| src/vs/workbench/contrib/terminal/browser/terminal.ts | Updates ITerminalInstance interface by removing pauseInputEvents API no longer used. |
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
Just the async thing needs to change
Resolves: #243584
Updated summary:
There were two request I think in
\e[6n\e[c.We were sending
\e[?61;4c\e[2;1R, which is wrong.The one ending with 4c, was supposed to come later.
Because the order was wrong, ConPTY "ate" the first input from user thinking that must've been the response to their cursor position request, and made a ding sound because it was invalid.