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

WriteCharsLegacy: Accumulated bugs #10808

Closed
DHowett opened this issue Jul 27, 2021 · 2 comments · Fixed by #15567
Closed

WriteCharsLegacy: Accumulated bugs #10808

DHowett opened this issue Jul 27, 2021 · 2 comments · Fixed by #15567
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Impact-Correctness It be wrong. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase
Milestone

Comments

@DHowett
Copy link
Member

DHowett commented Jul 27, 2021

Inserting a tab character that would put us at the right edge of the buffer inserts two tabs (one to get us to the right edge of the buffer, one more after we wrap)

Code and example
#include <windows.h>
int main() {
        CONSOLE_SCREEN_BUFFER_INFOEX csbiex{};
        csbiex.cbSize = sizeof(csbiex);
        GetConsoleScreenBufferInfoEx(GetStdHandle(STD_OUTPUT_HANDLE), &csbiex);
        SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), COORD{(SHORT)(csbiex.dwSize.X - 9), csbiex.dwCursorPosition.Y});
        WriteConsoleW(GetStdHandle(STD_OUTPUT_HANDLE), L"|\t|\n", 4, nullptr, nullptr);
        return 0;
}

image

Inserting a printable control character at the right edge of the screen (ala ^A) when only the ^ would fit emits a ^ and a space.

Example

image
(here, ^D)

Inserting a printable control character (^D) at the right edge of the screen when both ^ and D fit and then backspacing it deletes the ^, not the D:

Example

image

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Jul 27, 2021
@DHowett DHowett added Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase labels Jul 27, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 27, 2021
@DHowett DHowett added this to the Console Backlog milestone Jul 27, 2021
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 27, 2021
@DHowett DHowett self-assigned this Jul 27, 2021
@DHowett
Copy link
Member Author

DHowett commented Jul 27, 2021

Tab bug: the measurer "eats" the space before it decides that the Tab does not fit. The bulk text inserter then uses the estimated position (which was consumed by the tab we aborted inserting) to move the cursor. Then when WCL loops back around to insert the tab on the next line it measures back out at 8 characters and inserts it again.

@DHowett
Copy link
Member Author

DHowett commented Jul 27, 2021

Control char bug 1: WCL does not consider a printable control char to take up 2 spaces when it is trying to fit it up against the edge of the screen. Correct behavior: treat ^D as two characters and push the whole sequence down to the next line, or print ^ and D on separate lines and be sure to delete them both.

@zadjii-msft zadjii-msft modified the milestones: Console Backlog, 22H2 Jan 4, 2022
@lhecker lhecker self-assigned this Jul 24, 2022
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Jun 25, 2023
microsoft-github-policy-service bot pushed a commit that referenced this issue Jun 30, 2023
… Emoji support (#15567)

This is a complete rewrite of the old `WriteCharsLegacy` function
which is used when VT mode is disabled as well as for all interactive
console input handling on Windows. The previous code was almost
horrifying in some aspects as it first wrote the incoming text into a
local buffer, stripping/replacing any control characters. That's not
particular fast and never was. It's unknown why it was like that.

It also measured the width of each glyph to correctly determine the
cursor position and line wrapping. Presumably this used to work quite
well in the original console code, because it would then just copy
that local buffer into the destination text buffer, but with the
introduction of the broken and extremely slow `OutputCellIterator`
abstraction this would end up measuring all text twice and cause
disagreements between `WriteCharsLegacy`'s idea of the cursor position
and `OutputCellIterator`'s cursor position. Emoji input was basically
entirely broken. This PR fixes it by passing any incoming text
straight to the `TextBuffer` as well as by using its cursor positioning
facilities to correctly implement wrapping and backspace handling.

Backspacing over Emojis and an array of other aspects still don't work
correctly thanks to cmdline.cpp, but it works quite a lot better now.

Related to #8000
Closes #8839
Closes #10808

## Validation Steps Performed
* Printing various Unicode text ✅
* On an fgets() input line
  * Typing text works ✅
  * Inserting text works anywhere ✅
  * Ctrl+X is translated to ^X ✅
  * Null is translated to ^@ ✅
    This was tested by hardcoding the `OutputMode` to 3 instead of 7.
  * Backspace only advances to start of the input ✅
  * Backspace deletes the entire preceding tab ✅
  * Backspace doesn't delete whitespace preceding a tab ✅
  * Backspacing a force-wrapped wide glyph unwraps the line break ✅
  * Backspacing ^X deletes both glyphs ✅
  * Backspacing a force-wrapped tab deletes trailing whitespace ✅
* When executing
  ```cpp
  fputs("foo: ", stdout);
  fgets(buffer, stdin);
  ```
  pressing tab and then backspace does not delete the whitespace
  that follows after the "foo:" string (= `sOriginalXPosition`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Impact-Correctness It be wrong. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase
Projects
None yet
3 participants