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.

Sign up for GitHub

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

Jump to bottom

Implement stream writing mechanism on buffer (a.k.a. WriteCharsLegacy2ElectricBoogaloo) #780

Closed
miniksa opened this issue May 14, 2019 · 6 comments · Fixed by #14640
Closed

Implement stream writing mechanism on buffer (a.k.a. WriteCharsLegacy2ElectricBoogaloo) #780

miniksa opened this issue May 14, 2019 · 6 comments · Fixed by #14640
Assignees
@lhecker
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
22H2

Comments

@miniksa
Copy link
Member

miniksa commented May 14, 2019

Apologies if this explanation is confusing. I've mentally internalized the problem here, but I'm not sure if I can fully articulate. Here goes...

When receiving characters in a stream fashion (in either conhost or the Terminal), there are a bunch of situations that occur that run off the golden path rails of "just put this into the buffer and move the cursor forward by one".

A few major categories of these are:

  1. C0 control characters (tab, backspace, newline, carriage return, etc.)
  2. Surrogate pairs for UTF-16 (two wchar_ts are required to form one U+ codepoint)
  3. An entire run of characters required to figure out a particular cell display or run of cells.

We need to solve 1 and 2 first. Solving 3 will probably require a bit more interaction with the CustomTextLayout in the DirectWrite renderer to perform some sort of read-ahead or multiple-pass calculation over text as it flows in stream-wise to figure out the columns it should take.

The existing manifests of this sort of work are WriteCharsLegacy in the console host (which is a thousand line or more monstrosity full of multi-pass switch cases, loops, and gotos) and the stream write thing in TerminalControl (I think, might be TerminalConnection) which @zadjii-msft affectionately warned against it becoming "WriteCharsLegacy2ElectricBoogaloo" in one of his comments.

I think what needs to be done here is:

  1. Create a third entry point to the buffer object that accepts text in a stream write fashion, potentially "buffering" before it inserts into the buffer for reals (or has the ability to look-behind at what was just written) taking over the duties of the "WriteCharsLegacy" type methods into the buffer itself so at least 1 and 2 can be handled in a unified-ish manner.
  2. Teach the buffer and/or let the buffer accept some sort of loose pfn that will connect it up to the text layout engine and let it query for column counts or glyph run information as stream characters show up and adjust the behavior of their neighboring glyphs.
@miniksa miniksa added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) labels May 14, 2019
@miniksa miniksa self-assigned this May 14, 2019
@zadjii-msft zadjii-msft added this to the Windows Terminal v1.0 milestone May 14, 2019
@DHowett-MSFT DHowett-MSFT added the Product-Terminal The new Windows Terminal. label May 16, 2019
@zadjii-msft zadjii-msft added Issue-Task It's a feature request, but it doesn't really need a major design. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jun 20, 2019
@miniksa miniksa mentioned this issue Sep 5, 2019
Closed
@DHowett-MSFT DHowett-MSFT mentioned this issue Sep 5, 2019
Closed
@carlos-zamora carlos-zamora mentioned this issue Sep 17, 2019
Closed
@DHowett-MSFT DHowett-MSFT mentioned this issue Sep 23, 2019
Closed
@miniksa miniksa mentioned this issue Sep 26, 2019
Closed
miniksa added a commit that referenced this issue Sep 26, 2019
@miniksa
Patch fix for #1360 until WriteStream (#780) can be implemented.
f60566c
@miniksa miniksa mentioned this issue Sep 26, 2019
Merged
5 tasks
ghost pushed a commit that referenced this issue Oct 1, 2019
@miniksa
Patch fix for #1360 until WriteStream (#780) can be implemented. (#2924)
5d906d9 
* Patch fix for #1360 until WriteStream (#780) can be implemented.

* Add a test that hangs in the broken state and passes in the success stat. Writes a bisecting character to the right most cell in the window.

* Code format! *shakes fist at sky*

* Update src/cascadia/TerminalCore/Terminal.cpp
DHowett-MSFT pushed a commit that referenced this issue Oct 3, 2019
@miniksa @DHowett
Patch fix for #1360 until WriteStream (#780) can be implemented. (#2924)
776b9a5 
* Patch fix for #1360 until WriteStream (#780) can be implemented.

* Add a test that hangs in the broken state and passes in the success stat. Writes a bisecting character to the right most cell in the window.

* Code format! *shakes fist at sky*

* Update src/cascadia/TerminalCore/Terminal.cpp
DHowett-MSFT pushed a commit that referenced this issue Oct 4, 2019
@DHowett
Revert "Patch fix for #1360 until WriteStream (#780) can be implement…
351d326 
…ed. (#2924)"

This reverts commit 776b9a5.
@zadjii-msft zadjii-msft mentioned this issue Oct 4, 2019
Closed
This was referenced Oct 6, 2019
Closed
Closed
This was referenced Oct 14, 2019
Closed
Closed
DHowett-MSFT pushed a commit that referenced this issue Oct 15, 2019
@DHowett
Revert "Patch fix for #1360 until WriteStream (#780) can be implement…
6a35f5f 
…ed. (#2924)"

This reverts commit 5d906d9.
This was referenced Oct 22, 2019
Closed
Closed
@skyline75489 skyline75489 mentioned this issue Oct 29, 2019
Closed
5 tasks
@DHowett-MSFT DHowett-MSFT mentioned this issue Oct 29, 2019
Closed
@reli-msft reli-msft mentioned this issue Nov 16, 2019
Closed
@Bill-Stewart
Copy link

Good on the "...2 Electric Boogaloo" reference...we don't want it breakin'...

@miniksa miniksa modified the milestones: Terminal v1.0, Terminal v1.x Apr 15, 2020
donno2048 added a commit to donno2048/terminal that referenced this issue Sep 28, 2020
@donno2048
Remove unneeded VT-specific control character handling (#4289)
4377953 
## Summary of the Pull Request

This PR removes all of the VT-specific functionality from the `WriteCharsLegacy` function that dealt with control characters, since those controls are now handled in the state machine when in VT mode. It also removes most of the control character handling from the `Terminal::_WriteBuffer` method for the same reason.

## References

This is a followup to PR #4171

## PR Checklist
* [x] Closes #3971
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: microsoft/terminal#780 (comment)

## Detailed Description of the Pull Request / Additional comments

There are four changes to the `WriteCharsLegacy` implementation:

1. The `TAB` character had special case handling in VT mode which is now no longer required. This fixes a bug in the Python REPL editor (when run from a cmd shell in Windows Terminal), which would prevent you tabbing past the end of the line. It also fixes #3971.

2. Following on from point 1, the `WC_NONDESTRUCTIVE_TAB` flag could also now be removed. It only ever applied in VT mode, in which case the `TAB` character isn't handled in `WriteCharsLegacy`, so there isn't a need for a non-destructive version.

3. There used to be special case handling for a `BS` character at the beginning of the line when in VT mode, and that is also no longer required. This fixes an edge-case bug which would prevent a glyph being output for code point 8 when `ENABLE_PROCESSED_OUTPUT` was disabled. 

4. There was quite a lot of special case handling for control characters in the "end-of-line wrap" implementation, which is no longer required. This fixes a bug which would prevent "low ASCII" characters from wrapping when output at the end of a line.

Then in the `Terminal::_WriteBuffer` implementation, I've simply removed all control character handling, except for `LF`. The Terminal is always in VT mode, so the control characters are always handled by the state machine. The exception for the `LF` character is simply because it doesn't have a proper implementation yet, so it still passes the character through to `_WriteBuffer`. That will get cleaned up eventually, but I thought that could wait for a later PR.

Finally, with the removal of the VT mode handling in `WriteCharsLegacy`, there was no longer a need for the `SCREEN_INFORMATION::InVTMode` method to be publicly accessible. That has now been made private.

## Validation Steps Performed

I've only tested manually, making sure the conhost and Windows Terminal still basically work, and confirming that the above-mentioned bugs are fixed by these changes.
@miniksa miniksa mentioned this issue Oct 21, 2020
Closed
@j4james j4james mentioned this issue Nov 29, 2020
Closed
@zadjii-msft zadjii-msft modified the milestones: Terminal v2.0, 22H2 Jan 4, 2022
@j4james j4james mentioned this issue Feb 3, 2022
Closed
@carlos-zamora carlos-zamora mentioned this issue Feb 8, 2022
Closed
@zadjii-msft zadjii-msft mentioned this issue Feb 8, 2022
Open
31 tasks
@miniksa miniksa removed their assignment Mar 31, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H2, Terminal v1.16 Jun 21, 2022
@lhecker lhecker modified the milestones: Terminal v1.17, 22H2 Nov 5, 2022
This was referenced Jan 3, 2023
Open
Merged
@ghost ghost added the In-PR This issue has a related PR label Jan 6, 2023
@ghost ghost closed this as completed in #14640 Jan 18, 2023
@ghost ghost closed this as completed in a1865b9 Jan 18, 2023
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Jan 18, 2023
@ghost
Copy link

ghost commented Jan 24, 2023

🎉This issue was addressed in #14640, which has now been successfully released as Windows Terminal Preview v1.17.1023.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
22H2
Development

Successfully merging a pull request may close this issue.

Merge the PrintString functionality into AdaptDispatch j4james/terminal
9 participants
@DHowett @lhecker @j4james @DHowett-MSFT @miniksa @zadjii-msft @Bill-Stewart @cinnamon-msft @reli-msft