fix: handle partial reads in queryTerminal#616
Closed
MartinodF wants to merge 2 commits into
Closed
Conversation
Nothing guarantees that we will read a complete sequence from in, and when we don't, we should not pass an incomplete one to filter or continue while discarding those bytes.
6 tasks
Contributor
|
Thank you so much for your contribution here @MartinodF! We accidentally closed your PR after merging v2-exp. Could you please re-open your changes and, we will take a look as soon as possible. |
2 tasks
Contributor
Author
|
@aymanbagabas since I'm not allowed to re-open this PR, I re-submitted it as #619 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is broken?
This all started with
lipgloss.HasDarkBackgroundtiming out most (80%?) of the time when running in Windows Terminal.After ignoring the problem for a few weeks, I finally decided to figure out what was happening:
queryBackgroundColorsends an OSC 11 and a CSI DA1 request one after the other and is supposed to receive back (at least) one response of each type.The code (with a couple of debug statements) running in the VSCode integrated terminal:
The same code in Windows Terminal:
As you can see, the terminal response is being received in chunks. Lipgloss assumes that somehow we will always end the read on a sequence boundary, and:
filter()regardless of the current parser state.This causes
filter()to:What am I changing?
The patch makes sure that we only ever send complete sequences to
filter(), while maintaining the parser state in between reads so that we can successfully complete parsing a sequence even if it's split inconveniently.If you have any better ideas on how to implement this, please feel free to propose them :)
Side note, in all terminals I use the CSI DA1 request causes the terminal to return the background and foreground colors in addition to the DA1 response. I feel like maybe there's an opportunity to simplify
queryBackgroundColor, but I don't know how this behaves in other terminal emulators.CONTRIBUTING.md.