From da1e1a693e0358b7944e0c964d25c47fba0ce062 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Wed, 24 Mar 2021 14:26:50 -0700 Subject: [PATCH] WriteCharsLegacy: Add some notes in comments and rename WC_ECHO (#9605) This commit clarifies some things inside WriteCharsLegacy by adding comments and renaming parameter and enum names. It does not change any logic. `WC_ECHO` was used extensively to mean only one thing: whether to print a control character like `\x18` (Ctrl+X>) as `^X`. It's been renamed to make that abundantly clear. --- src/host/CommandNumberPopup.cpp | 4 ++-- src/host/_stream.cpp | 38 +++++++++++++++++++++++++-------- src/host/_stream.h | 6 +++--- src/host/cmdline.cpp | 24 ++++++++++----------- src/host/cmdline.h | 2 +- src/host/misc.cpp | 8 +++---- src/host/misc.h | 2 +- src/host/readDataCooked.cpp | 10 ++++----- 8 files changed, 57 insertions(+), 37 deletions(-) diff --git a/src/host/CommandNumberPopup.cpp b/src/host/CommandNumberPopup.cpp index 6b819350a..f0b03365c 100644 --- a/src/host/CommandNumberPopup.cpp +++ b/src/host/CommandNumberPopup.cpp @@ -42,7 +42,7 @@ void CommandNumberPopup::_handleNumber(COOKED_READ_DATA& cookedReadData, const w &CharsToWrite, &NumSpaces, cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr)); cookedReadData.ScreenInfo().SetAttributes(realAttributes); try @@ -73,7 +73,7 @@ void CommandNumberPopup::_handleBackspace(COOKED_READ_DATA& cookedReadData) noex &CharsToWrite, &NumSpaces, cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr)); cookedReadData.ScreenInfo().SetAttributes(realAttributes); _pop(); diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp index daef2575d..a416de795 100644 --- a/src/host/_stream.cpp +++ b/src/host/_stream.cpp @@ -317,9 +317,9 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; // - pcb - On input, number of bytes to write. On output, number of bytes written. // - pcSpaces - On output, the number of spaces consumed by the written characters. // - dwFlags - -// WC_DESTRUCTIVE_BACKSPACE backspace overwrites characters. -// WC_KEEP_CURSOR_VISIBLE change window origin desirable when hit rt. edge -// WC_ECHO if called by Read (echoing characters) +// WC_DESTRUCTIVE_BACKSPACE backspace overwrites characters. +// WC_KEEP_CURSOR_VISIBLE change window origin desirable when hit rt. edge +// WC_PRINTABLE_CONTROL_CHARS if control characters should be expanded (as in, to "^X") // Return Value: // Note: // - This routine does not process tabs and backspace properly. That code will be implemented as part of the line editing services. @@ -392,9 +392,13 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; { #pragma prefast(suppress : 26019, "Buffer is taken in multiples of 2. Validation is ok.") const wchar_t Char = *lpString; + // WCL-NOTE: We believe RealUnicodeChar to be identical to Char, because we believe pwchRealUnicode + // WCL-NOTE: to be identical to lpString. They are incremented in lockstep, never separately, and lpString + // WCL-NOTE: is initialized from pwchRealUnicode. const wchar_t RealUnicodeChar = *pwchRealUnicode; if (IS_GLYPH_CHAR(RealUnicodeChar) || fUnprocessed) { + // WCL-NOTE: This operates on a single code unit instead of a whole codepoint. It will mis-measure surrogate pairs. if (IsGlyphFullWidth(Char)) { if (i < (LOCAL_BUFFER_SIZE - 1) && XPosition < (coordScreenBufferSize.X - 1)) @@ -426,7 +430,7 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; switch (RealUnicodeChar) { case UNICODE_BELL: - if (dwFlags & WC_ECHO) + if (dwFlags & WC_PRINTABLE_CONTROL_CHARS) { goto CtrlChar; } @@ -470,11 +474,13 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; default: // if char is ctrl char, write ^char. - if ((dwFlags & WC_ECHO) && (IS_CONTROL_CHAR(RealUnicodeChar))) + if ((dwFlags & WC_PRINTABLE_CONTROL_CHARS) && (IS_CONTROL_CHAR(RealUnicodeChar))) { CtrlChar: if (i < (LOCAL_BUFFER_SIZE - 1)) { + // WCL-NOTE: We do not properly measure that there is space for two characters + // WCL-NOTE: left on the screen. *LocalBufPtr = (WCHAR)'^'; LocalBufPtr++; XPosition++; @@ -515,6 +521,10 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; } else { + // WCL-NOTE: We should never hit this. + // WCL-NOTE: 1. Normal characters are handled via the early check for IS_GLYPH_CHAR + // WCL-NOTE: 2. Control characters are handled via the CtrlChar label (if WC_PRINTABLE_CONTROL_CHARS is on) + // WCL-NOTE: And if they are control characters they will trigger the C1_CNTRL check above. *LocalBufPtr = Char; } } @@ -536,6 +546,7 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; CursorPosition = cursor.GetPosition(); // Make sure we don't write past the end of the buffer. + // WCL-NOTE: This check uses a code unit count instead of a column count. That is incorrect. if (i > gsl::narrow_cast(coordScreenBufferSize.X) - CursorPosition.X) { i = gsl::narrow_cast(coordScreenBufferSize.X) - CursorPosition.X; @@ -551,6 +562,9 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; // The number of "spaces" or "cells" we have consumed needs to be reported and stored for later // when/if we need to erase the command line. TempNumSpaces += itEnd.GetCellDistance(it); + // WCL-NOTE: We are using the "estimated" X position delta instead of the actual delta from + // WCL-NOTE: the iterator. It is not clear why. If they differ, the cursor ends up in the + // WCL-NOTE: wrong place (typically inside another character). CursorPosition.X = XPosition; // enforce a delayed newline if we're about to pass the end and the WC_DELAY_EOL_WRAP flag is set. @@ -570,6 +584,8 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY); } + // WCL-NOTE: If we have processed the entire input string during our "fast one-line print" handler, + // WCL-NOTE: we are done as there is nothing more to do. Neat! if (*pcb == BufferSize) { if (nullptr != pcSpaces) @@ -582,6 +598,10 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; } else if (*pcb >= BufferSize) { + // WCL-NOTE: This case looks like it is never encountered, but it *is* if WC_PRINTABLE_CONTROL_CHARS is off. + // WCL-NOTE: If the string is entirely nonprinting control characters, there will be + // WCL-NOTE: no output in the buffer (LocalBuffer; i == 0) but we will have processed + // WCL-NOTE: "every" character. We can just bail out and report the number of spaces consumed. FAIL_FAST_IF(!(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT))); // this catches the case where the number of backspaces == the number of characters. @@ -730,7 +750,7 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; pwchBuffer + 1 - pwchBufferBackupLimit, gsl::narrow_cast(coordScreenBufferSize.X) - sOriginalXPosition, sOriginalXPosition, - dwFlags & WC_ECHO)) + dwFlags & WC_PRINTABLE_CONTROL_CHARS)) { CursorPosition.X = coordScreenBufferSize.X - 1; CursorPosition.Y = (SHORT)(cursor.GetPosition().Y - 1); @@ -898,9 +918,9 @@ constexpr unsigned int LOCAL_BUFFER_SIZE = 100; // - pcb - On input, number of bytes to write. On output, number of bytes written. // - pcSpaces - On output, the number of spaces consumed by the written characters. // - dwFlags - -// WC_DESTRUCTIVE_BACKSPACE backspace overwrites characters. -// WC_KEEP_CURSOR_VISIBLE change window origin (viewport) desirable when hit rt. edge -// WC_ECHO if called by Read (echoing characters) +// WC_DESTRUCTIVE_BACKSPACE backspace overwrites characters. +// WC_KEEP_CURSOR_VISIBLE change window origin (viewport) desirable when hit rt. edge +// WC_PRINTABLE_CONTROL_CHARS if control characters should be expanded (as in, to "^X") // Return Value: // Note: // - This routine does not process tabs and backspace properly. That code will be implemented as part of the line editing services. diff --git a/src/host/_stream.h b/src/host/_stream.h index 11641fb08..349cb206a 100644 --- a/src/host/_stream.h +++ b/src/host/_stream.h @@ -57,9 +57,9 @@ Arguments: bytes written. NumSpaces - On output, the number of spaces consumed by the written characters. dwFlags - - WC_DESTRUCTIVE_BACKSPACE backspace overwrites characters. - WC_KEEP_CURSOR_VISIBLE change window origin desirable when hit rt. edge - WC_ECHO if called by Read (echoing characters) + WC_DESTRUCTIVE_BACKSPACE backspace overwrites characters. + WC_KEEP_CURSOR_VISIBLE change window origin desirable when hit rt. edge + WC_PRINTABLE_CONTROL_CHARS if control characters should be expanded (as in, to "^X") Return Value: diff --git a/src/host/cmdline.cpp b/src/host/cmdline.cpp index 25efcfc62..6f09fb28d 100644 --- a/src/host/cmdline.cpp +++ b/src/host/cmdline.cpp @@ -256,7 +256,7 @@ void RedrawCommandLine(COOKED_READ_DATA& cookedReadData) &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY); FAIL_FAST_IF_NTSTATUS_FAILED(Status); @@ -297,7 +297,7 @@ void SetCurrentCommandLine(COOKED_READ_DATA& cookedReadData, _In_ SHORT Index) / &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; } @@ -461,7 +461,7 @@ void CommandLine::_processHistoryCycling(COOKED_READ_DATA& cookedReadData, &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; } @@ -496,7 +496,7 @@ void CommandLine::_setPromptToOldestCommand(COOKED_READ_DATA& cookedReadData) &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; } @@ -532,7 +532,7 @@ void CommandLine::_setPromptToNewestCommand(COOKED_READ_DATA& cookedReadData) &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; } @@ -559,7 +559,7 @@ void CommandLine::DeletePromptAfterCursor(COOKED_READ_DATA& cookedReadData) noex &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr)); } } @@ -586,7 +586,7 @@ COORD CommandLine::_deletePromptBeforeCursor(COOKED_READ_DATA& cookedReadData) n &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr)); } return cookedReadData.OriginalCursorPosition(); @@ -878,7 +878,7 @@ COORD CommandLine::_moveCursorRight(COOKED_READ_DATA& cookedReadData) noexcept &CharsToWrite, &NumSpaces, cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; cookedReadData.VisibleCharCount() += NumSpaces; @@ -921,7 +921,7 @@ void CommandLine::_insertCtrlZ(COOKED_READ_DATA& cookedReadData) noexcept &CharsToWrite, &NumSpaces, cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; cookedReadData.VisibleCharCount() += NumSpaces; @@ -971,7 +971,7 @@ void CommandLine::_fillPromptWithPreviousCommandFragment(COOKED_READ_DATA& cooke &cchCount, &NumSpaces, cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; cookedReadData.VisibleCharCount() += NumSpaces; @@ -1018,7 +1018,7 @@ COORD CommandLine::_cycleMatchingCommandHistoryToPrompt(COOKED_READ_DATA& cooked &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); cookedReadData.OriginalCursorPosition().Y += ScrollY; cursorPosition.Y += ScrollY; @@ -1073,7 +1073,7 @@ COORD CommandLine::DeleteFromRightOfCursor(COOKED_READ_DATA& cookedReadData) noe &cookedReadData.BytesRead(), &cookedReadData.VisibleCharCount(), cookedReadData.OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr)); } diff --git a/src/host/cmdline.h b/src/host/cmdline.h index 8339e72c2..3285952b1 100644 --- a/src/host/cmdline.h +++ b/src/host/cmdline.h @@ -133,7 +133,7 @@ void RedrawCommandLine(COOKED_READ_DATA& cookedReadData); // Values for WriteChars(), WriteCharsLegacy() dwFlags #define WC_DESTRUCTIVE_BACKSPACE 0x01 #define WC_KEEP_CURSOR_VISIBLE 0x02 -#define WC_ECHO 0x04 +#define WC_PRINTABLE_CONTROL_CHARS 0x04 // This is no longer necessary. The buffer will always be Unicode. We don't need to perform special work to check if we're in a raster font // and convert the entire buffer to match (and all insertions). diff --git a/src/host/misc.cpp b/src/host/misc.cpp index 3a71861f1..a89c982b0 100644 --- a/src/host/misc.cpp +++ b/src/host/misc.cpp @@ -115,7 +115,7 @@ BOOL CheckBisectStringW(_In_reads_bytes_(cBytes) const WCHAR* pwchBuffer, // - pwchBuffer - Pointer to Unicode string buffer. // - cWords - Number of Unicode string. // - cBytes - Number of bisect position by byte counts. -// - fEcho - TRUE if called by Read (echoing characters) +// - fPrintableControlChars - TRUE if control characters are being expanded (to ^X) // Return Value: // - TRUE - Bisected character. // - FALSE - Correctly. @@ -124,7 +124,7 @@ BOOL CheckBisectProcessW(const SCREEN_INFORMATION& ScreenInfo, _In_ size_t cWords, _In_ size_t cBytes, _In_ SHORT sOriginalXPosition, - _In_ BOOL fEcho) + _In_ BOOL fPrintableControlChars) { if (WI_IsFlagSet(ScreenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT)) { @@ -162,7 +162,7 @@ BOOL CheckBisectProcessW(const SCREEN_INFORMATION& ScreenInfo, switch (Char) { case UNICODE_BELL: - if (fEcho) + if (fPrintableControlChars) goto CtrlChar; break; case UNICODE_BACKSPACE: @@ -179,7 +179,7 @@ BOOL CheckBisectProcessW(const SCREEN_INFORMATION& ScreenInfo, break; } default: - if (fEcho) + if (fPrintableControlChars) { CtrlChar: if (cBytes < 2) diff --git a/src/host/misc.h b/src/host/misc.h index f598894ac..11eefe92e 100644 --- a/src/host/misc.h +++ b/src/host/misc.h @@ -36,7 +36,7 @@ BOOL CheckBisectProcessW(const SCREEN_INFORMATION& ScreenInfo, _In_ size_t cWords, _In_ size_t cBytes, _In_ SHORT sOriginalXPosition, - _In_ BOOL fEcho); + _In_ BOOL fPrintableControlChars); int ConvertToOem(const UINT uiCodePage, _In_reads_(cchSource) const WCHAR* const pwchSource, diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp index 77fdf3cdb..60a817f15 100644 --- a/src/host/readDataCooked.cpp +++ b/src/host/readDataCooked.cpp @@ -533,7 +533,7 @@ bool COOKED_READ_DATA::ProcessInput(const wchar_t wchOrig, &NumToWrite, &NumSpaces, _originalCursorPosition.X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY); if (NT_SUCCESS(status)) { @@ -615,7 +615,7 @@ bool COOKED_READ_DATA::ProcessInput(const wchar_t wchOrig, &NumToWrite, nullptr, _originalCursorPosition.X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr); if (!NT_SUCCESS(status)) { @@ -716,7 +716,7 @@ bool COOKED_READ_DATA::ProcessInput(const wchar_t wchOrig, // write the new command line to the screen NumToWrite = _bytesRead; - DWORD dwFlags = WC_DESTRUCTIVE_BACKSPACE | WC_ECHO; + DWORD dwFlags = WC_DESTRUCTIVE_BACKSPACE | WC_PRINTABLE_CONTROL_CHARS; if (wch == UNICODE_CARRIAGERETURN) { dwFlags |= WC_KEEP_CURSOR_VISIBLE; @@ -786,7 +786,7 @@ bool COOKED_READ_DATA::ProcessInput(const wchar_t wchOrig, &NumToWrite, nullptr, _originalCursorPosition.X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, nullptr); if (!NT_SUCCESS(status)) { @@ -848,7 +848,7 @@ size_t COOKED_READ_DATA::Write(const std::wstring_view wstr) &bytesInserted, &NumSpaces, OriginalCursorPosition().X, - WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_ECHO, + WC_DESTRUCTIVE_BACKSPACE | WC_KEEP_CURSOR_VISIBLE | WC_PRINTABLE_CONTROL_CHARS, &ScrollY)); OriginalCursorPosition().Y += ScrollY; VisibleCharCount() += NumSpaces;