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

Fix chunked soft fonts not working #16349

Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 21, 2023

This changeset fixes an issue caused by #15991 where "chunked" escape
sequences would get corrupted. The fix is to simply not flush eagerly
anymore. I tried my best to keep the input lag reduction from #15991,
but unfortunately this isn't possible for console APIs.

Closes #16079

Validation Steps Performed

  • type ascii.com produces soft font ASCII characters ✅

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-VT Virtual Terminal sequence support Product-Conpty For console issues specifically related to conpty labels Nov 21, 2023
@DHowett
Copy link
Member

DHowett commented Nov 21, 2023

Does this meaningfully change the contract for Cork in a way that callers need to know about?

@lhecker
Copy link
Member Author

lhecker commented Nov 21, 2023

No, I believe this is about as (un)safe to use as it was before. Previously, the buffer would get flushed implicitly due to VtIo calling PaintFrame and calling VtEngine::Cork would've broken this design. Now it's still about the same, maybe even a bit safer.

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit bdf2f6f into main Nov 21, 2023
17 checks passed
@microsoft-github-policy-service microsoft-github-policy-service bot deleted the dev/lhecker/16079-corked-fixup branch November 21, 2023 20:51
@lhecker lhecker modified the milestone: Terminal v1.19 Dec 6, 2023
DHowett pushed a commit that referenced this pull request Dec 6, 2023
This changeset fixes an issue caused by #15991 where "chunked" escape
sequences would get corrupted. The fix is to simply not flush eagerly
anymore. I tried my best to keep the input lag reduction from #15991,
but unfortunately this isn't possible for console APIs.

Closes #16079

## Validation Steps Performed
* `type ascii.com` produces soft font ASCII characters ✅

(cherry picked from commit bdf2f6f)
Service-Card-Id: 91283205
Service-Version: 1.19
github-merge-queue bot pushed a commit that referenced this pull request Mar 28, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769
DHowett pushed a commit that referenced this pull request Apr 16, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769

(cherry picked from commit 1ede023)
Service-Card-Id: 91965217
Service-Version: 1.20
DHowett pushed a commit that referenced this pull request Apr 16, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769

(cherry picked from commit 1ede023)
Service-Card-Id: 91965216
Service-Version: 1.19
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 AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty
Projects
Status: To Consider
Development

Successfully merging this pull request may close these issues.

"Chunked" soft fonts are no longer working correctly
3 participants