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

Have TentativeBoundary trigger rollback #112510

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Dec 15, 2020

The problem was, certain shells (like PowerShell) timeout when you hit backspace/left arrow at the far left of the prompt, rather than return something. This meant that the cursor is never invalidated because invalidation only happens when the response from the shell is different than what we predict.

This fix makes sure that the TentativeBoundary prediction also rolls back it's internal prediction which will make sure that our internal cursor receives the update that the internal state has changed.

Additionally this adds a couple tests.

@TylerLeonhardt
Copy link
Member Author

Also, are there tests for this code somewhere? Or perhaps a place I could add new tests to?

@connor4312
Copy link
Member

connor4312 commented Dec 15, 2020

Ahh, right. Good catch. Although nothing is emitted we'd want to roll back in order to correct the cursor position.

why does the apply run the inner apply

The idea in the tenative boundary is to "buffer" the predictions and keep them from being emitted unless the tenatively proposed prediction is validated. If validation is successful, all the buffered predictions in the next generation will be instantly applied. So we still want to call apply() so that the cursor position is updated for use by those predictions.

Also, if you didn't guess by osmosis, the reason we maintain our own cursor position is that xterm writes are asynchronous, so if we looked up the cursor from the buffer each time that would cause issues since things we just wrote or predicted might not be reflected yet.

Also, are there tests for this code somewhere? Or perhaps a place I could add new tests to?

Here: https://github.com/microsoft/vscode/blob/master/src/vs/workbench/contrib/terminal/test/browser/terminalTypeahead.test.ts Tests are not super thorough right now, but fleshing them out is always good.

@TylerLeonhardt
Copy link
Member Author

Let me take a stab at writing a test or two before this goes in.

@TylerLeonhardt TylerLeonhardt marked this pull request as ready for review December 15, 2020 22:40
@TylerLeonhardt TylerLeonhardt merged commit bffa404 into microsoft:master Dec 16, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants