Skip to content

Commit

Permalink
Remove boolean success return values from TextBuffer (#15566)
Browse files Browse the repository at this point in the history
I've removed these because it made some of my new code pretty
convoluted for now good reason as most of these functions aren't
exception safe to begin with. Basically, their boolean status
is often just a pretense because they can crash or throw anyways.

Furthermore, `WriteCharsLegacy` failed to check the status code
returned by `AdjustCursorPosition` in some of its parts too.

In the future we should instead probably strive to continue
make our legacy code more exception safe.
  • Loading branch information
lhecker authored Jun 22, 2023
1 parent e594d97 commit f0291c6
Show file tree
Hide file tree
Showing 10 changed files with 178 additions and 269 deletions.
3 changes: 1 addition & 2 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -435,10 +435,9 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const til::CoordType c
return it;
}

bool ROW::SetAttrToEnd(const til::CoordType columnBegin, const TextAttribute attr)
void ROW::SetAttrToEnd(const til::CoordType columnBegin, const TextAttribute attr)
{
_attr.replace(_clampedColumnInclusive(columnBegin), _attr.size(), attr);
return true;
}

void ROW::ReplaceAttributes(const til::CoordType beginIndex, const til::CoordType endIndex, const TextAttribute& newAttr)
Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ class ROW final

void ClearCell(til::CoordType column);
OutputCellIterator WriteCells(OutputCellIterator it, til::CoordType columnBegin, std::optional<bool> wrap = std::nullopt, std::optional<til::CoordType> limitRight = std::nullopt);
bool SetAttrToEnd(til::CoordType columnBegin, TextAttribute attr);
void SetAttrToEnd(til::CoordType columnBegin, TextAttribute attr);
void ReplaceAttributes(til::CoordType beginIndex, til::CoordType endIndex, const TextAttribute& newAttr);
void ReplaceCharacters(til::CoordType columnBegin, til::CoordType width, const std::wstring_view& chars);
void ReplaceText(RowWriteState& state);
Expand Down
317 changes: 128 additions & 189 deletions src/buffer/out/textBuffer.cpp

Large diffs are not rendered by default.

12 changes: 6 additions & 6 deletions src/buffer/out/textBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,13 @@ class TextBuffer final
const std::optional<bool> setWrap = std::nullopt,
const std::optional<til::CoordType> limitRight = std::nullopt);

bool InsertCharacter(const wchar_t wch, const DbcsAttribute dbcsAttribute, const TextAttribute attr);
bool InsertCharacter(const std::wstring_view chars, const DbcsAttribute dbcsAttribute, const TextAttribute attr);
bool IncrementCursor();
bool NewlineCursor();
void InsertCharacter(const wchar_t wch, const DbcsAttribute dbcsAttribute, const TextAttribute attr);
void InsertCharacter(const std::wstring_view chars, const DbcsAttribute dbcsAttribute, const TextAttribute attr);
void IncrementCursor();
void NewlineCursor();

// Scroll needs access to this to quickly rotate around the buffer.
bool IncrementCircularBuffer(const TextAttribute& fillAttributes = {});
void IncrementCircularBuffer(const TextAttribute& fillAttributes = {});

til::point GetLastNonSpaceCharacter(std::optional<const Microsoft::Console::Types::Viewport> viewOptional = std::nullopt) const;

Expand Down Expand Up @@ -241,7 +241,7 @@ class TextBuffer final
void _SetWrapOnCurrentRow();
void _AdjustWrapOnCurrentRow(const bool fSet);
// Assist with maintaining proper buffer state for Double Byte character sequences
bool _PrepareForDoubleByteSequence(const DbcsAttribute dbcsAttribute);
void _PrepareForDoubleByteSequence(const DbcsAttribute dbcsAttribute);
bool _AssertValidDoubleByteSequence(const DbcsAttribute dbcsAttribute);
void _ExpandTextRow(til::inclusive_rect& selectionRow) const;
DelimiterClass _GetDelimiterClassAt(const til::point pos, const std::wstring_view wordDelimiters) const;
Expand Down
58 changes: 21 additions & 37 deletions src/host/_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
// - coordCursor - New location of cursor.
// - fKeepCursorVisible - TRUE if changing window origin desirable when hit right edge
// Return Value:
[[nodiscard]] NTSTATUS AdjustCursorPosition(SCREEN_INFORMATION& screenInfo,
_In_ til::point coordCursor,
const BOOL fKeepCursorVisible,
_Inout_opt_ til::CoordType* psScrollY)
void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point coordCursor, const BOOL fKeepCursorVisible, _Inout_opt_ til::CoordType* psScrollY)
{
const auto bufferSize = screenInfo.GetBufferSize().Dimensions();
if (coordCursor.x < 0)
Expand Down Expand Up @@ -71,16 +68,10 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
}
}

auto Status = STATUS_SUCCESS;

if (coordCursor.y >= bufferSize.height)
{
// At the end of the buffer. Scroll contents of screen buffer so new position is visible.
FAIL_FAST_IF(!(coordCursor.y == bufferSize.height));
if (!StreamScrollRegion(screenInfo))
{
Status = STATUS_NO_MEMORY;
}
StreamScrollRegion(screenInfo);

if (nullptr != psScrollY)
{
Expand All @@ -90,28 +81,21 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
}

const auto cursorMovedPastViewport = coordCursor.y > screenInfo.GetViewport().BottomInclusive();
if (SUCCEEDED_NTSTATUS(Status))

// if at right or bottom edge of window, scroll right or down one char.
if (cursorMovedPastViewport)
{
// if at right or bottom edge of window, scroll right or down one char.
if (cursorMovedPastViewport)
{
til::point WindowOrigin;
WindowOrigin.x = 0;
WindowOrigin.y = coordCursor.y - screenInfo.GetViewport().BottomInclusive();
Status = screenInfo.SetViewportOrigin(false, WindowOrigin, true);
}
til::point WindowOrigin;
WindowOrigin.x = 0;
WindowOrigin.y = coordCursor.y - screenInfo.GetViewport().BottomInclusive();
LOG_IF_FAILED(screenInfo.SetViewportOrigin(false, WindowOrigin, true));
}

if (SUCCEEDED_NTSTATUS(Status))
if (fKeepCursorVisible)
{
if (fKeepCursorVisible)
{
screenInfo.MakeCursorVisible(coordCursor);
}
Status = screenInfo.SetCursorPosition(coordCursor, !!fKeepCursorVisible);
screenInfo.MakeCursorVisible(coordCursor);
}

return Status;
LOG_IF_FAILED(screenInfo.SetCursorPosition(coordCursor, !!fKeepCursorVisible));
}

// Routine Description:
Expand Down Expand Up @@ -180,7 +164,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
CursorPosition.x = 0;
CursorPosition.y++;

Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);

CursorPosition = cursor.GetPosition();
}
Expand Down Expand Up @@ -372,7 +356,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
// WCL-NOTE: wrong place (typically inside another character).
CursorPosition.x = XPosition;

Status = AdjustCursorPosition(screenInfo, CursorPosition, WI_IsFlagSet(dwFlags, WC_KEEP_CURSOR_VISIBLE), psScrollY);
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!
Expand Down Expand Up @@ -501,7 +485,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
CursorPosition.x -= 1;
TempNumSpaces -= 1;

Status = AdjustCursorPosition(screenInfo, CursorPosition, dwFlags & WC_KEEP_CURSOR_VISIBLE, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, dwFlags & WC_KEEP_CURSOR_VISIBLE, psScrollY);
if (dwFlags & WC_DESTRUCTIVE_BACKSPACE)
{
try
Expand All @@ -523,7 +507,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
CursorPosition.x = 0;
OutputDebugStringA(("CONSRV: Ignoring backspace to previous line\n"));
}
Status = AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
if (dwFlags & WC_DESTRUCTIVE_BACKSPACE)
{
try
Expand All @@ -548,7 +532,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
// on the prev row if it was set
textBuffer.GetRowByOffset(CursorPosition.y).SetWrapForced(false);

Status = AdjustCursorPosition(screenInfo, CursorPosition, dwFlags & WC_KEEP_CURSOR_VISIBLE, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, dwFlags & WC_KEEP_CURSOR_VISIBLE, psScrollY);
}
}
// Notify accessibility to read the backspaced character.
Expand Down Expand Up @@ -598,7 +582,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
}
CATCH_LOG();

Status = AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
break;
}
case UNICODE_CARRIAGERETURN:
Expand All @@ -609,7 +593,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
pwchBuffer++;
CursorPosition.x = 0;
CursorPosition.y = cursor.GetPosition().y;
Status = AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
break;
}
case UNICODE_LINEFEED:
Expand All @@ -632,7 +616,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
textBuffer.GetRowByOffset(cursor.GetPosition().y).SetWrapForced(false);
}

Status = AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, (dwFlags & WC_KEEP_CURSOR_VISIBLE) != 0, psScrollY);
break;
}
default:
Expand Down Expand Up @@ -678,7 +662,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
// is too wide to fit on the current line).
Row.SetDoubleBytePadded(true);

Status = AdjustCursorPosition(screenInfo, CursorPosition, dwFlags & WC_KEEP_CURSOR_VISIBLE, psScrollY);
AdjustCursorPosition(screenInfo, CursorPosition, dwFlags & WC_KEEP_CURSOR_VISIBLE, psScrollY);
continue;
}
break;
Expand Down
5 changes: 1 addition & 4 deletions src/host/_stream.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,7 @@ Routine Description:
Return Value:
--*/
[[nodiscard]] NTSTATUS AdjustCursorPosition(SCREEN_INFORMATION& screenInfo,
_In_ til::point coordCursor,
const BOOL fKeepCursorVisible,
_Inout_opt_ til::CoordType* psScrollY);
void AdjustCursorPosition(SCREEN_INFORMATION& screenInfo, _In_ til::point coordCursor, const BOOL fKeepCursorVisible, _Inout_opt_ til::CoordType* psScrollY);

/*++
Routine Description:
Expand Down
6 changes: 2 additions & 4 deletions src/host/cmdline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,7 @@ void RedrawCommandLine(COOKED_READ_DATA& cookedReadData)
{
CursorPosition.x++;
}
Status = AdjustCursorPosition(cookedReadData.ScreenInfo(), CursorPosition, TRUE, nullptr);
FAIL_FAST_IF_NTSTATUS_FAILED(Status);
AdjustCursorPosition(cookedReadData.ScreenInfo(), CursorPosition, TRUE, nullptr);
}
}

Expand Down Expand Up @@ -1298,8 +1297,7 @@ til::point CommandLine::DeleteFromRightOfCursor(COOKED_READ_DATA& cookedReadData

if (UpdateCursorPosition && cookedReadData.IsEchoInput())
{
Status = AdjustCursorPosition(cookedReadData.ScreenInfo(), cursorPosition, true, nullptr);
FAIL_FAST_IF_NTSTATUS_FAILED(Status);
AdjustCursorPosition(cookedReadData.ScreenInfo(), cursorPosition, true, nullptr);
}

return STATUS_SUCCESS;
Expand Down
35 changes: 16 additions & 19 deletions src/host/output.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -299,33 +299,30 @@ static void _ScrollScreen(SCREEN_INFORMATION& screenInfo, const Viewport& source
// - screenInfo - reference to screen buffer info.
// Return Value:
// - true if we succeeded in scrolling the buffer, otherwise false (if we're out of memory)
bool StreamScrollRegion(SCREEN_INFORMATION& screenInfo)
void StreamScrollRegion(SCREEN_INFORMATION& screenInfo)
{
// Rotate the circular buffer around and wipe out the previous final line.
auto& buffer = screenInfo.GetTextBuffer();
auto fSuccess = buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes());
if (fSuccess)
buffer.IncrementCircularBuffer(buffer.GetCurrentAttributes());

// Trigger a graphical update if we're active.
if (screenInfo.IsActiveScreenBuffer())
{
// Trigger a graphical update if we're active.
if (screenInfo.IsActiveScreenBuffer())
{
til::point coordDelta;
coordDelta.y = -1;
til::point coordDelta;
coordDelta.y = -1;

auto pNotifier = ServiceLocator::LocateAccessibilityNotifier();
if (pNotifier)
{
// Notify accessibility that a scroll has occurred.
pNotifier->NotifyConsoleUpdateScrollEvent(coordDelta.x, coordDelta.y);
}
auto pNotifier = ServiceLocator::LocateAccessibilityNotifier();
if (pNotifier)
{
// Notify accessibility that a scroll has occurred.
pNotifier->NotifyConsoleUpdateScrollEvent(coordDelta.x, coordDelta.y);
}

if (ServiceLocator::LocateGlobals().pRender != nullptr)
{
ServiceLocator::LocateGlobals().pRender->TriggerScroll(&coordDelta);
}
if (ServiceLocator::LocateGlobals().pRender != nullptr)
{
ServiceLocator::LocateGlobals().pRender->TriggerScroll(&coordDelta);
}
}
return fSuccess;
}

// Routine Description:
Expand Down
2 changes: 1 addition & 1 deletion src/host/output.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void ScrollRegion(SCREEN_INFORMATION& screenInfo,

VOID SetConsoleWindowOwner(const HWND hwnd, _Inout_opt_ ConsoleProcessHandle* pProcessData);

bool StreamScrollRegion(SCREEN_INFORMATION& screenInfo);
void StreamScrollRegion(SCREEN_INFORMATION& screenInfo);

// For handling process handle state, not the window state itself.
void CloseConsoleProcessState();
7 changes: 1 addition & 6 deletions src/host/readDataCooked.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -757,12 +757,7 @@ bool COOKED_READ_DATA::ProcessInput(const wchar_t wchOrig,
// adjust cursor position for WriteChars
_originalCursorPosition.y += ScrollY;
CursorPosition.y += ScrollY;
status = AdjustCursorPosition(_screenInfo, CursorPosition, TRUE, nullptr);
if (FAILED_NTSTATUS(status))
{
_bytesRead = 0;
return true;
}
AdjustCursorPosition(_screenInfo, CursorPosition, TRUE, nullptr);
}
}
}
Expand Down

0 comments on commit f0291c6

Please sign in to comment.