Skip to content

Commit

Permalink
Introduce VTInt to represent VT parameters (#12207)
Browse files Browse the repository at this point in the history 
This commit replaces our use of `size_t` to represent VT parameters with
`int32_t`. While unsigned integers have the inherent benefit of being less
ambiguous and enjoying two's complement, our buffer coordinates use signed
integers. Since a number of VT functions need to convert their parameters
to coordinates, this commit makes the conversion easier.
The benefit of this change becomes even more apparent if one considers
that a number of places performed unsafe conversions
of their size_t parameters to int or short already.

Files that had to be modified were converted to use til
wrappers instead of COORD or SMALL_RECT wherever possible.

## References

This commit contains about 20% of the work for #4015.

## PR Checklist
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
I'm mostly relying on our unit tests here. Both OpenConsole and WT appear to work fine.
  • Loading branch information
@lhecker
lhecker authored Apr 20, 2022
1 parent 0da5bd7 commit 7af134c
Show file tree
Hide file tree
Showing 36 changed files with 575 additions and 492 deletions.
10 changes: 5 additions & 5 deletions src/cascadia/TerminalCore/ITerminalApi.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ namespace Microsoft::Terminal::Core
virtual void SetTextAttributes(const TextAttribute& attrs) = 0;

virtual Microsoft::Console::Types::Viewport GetBufferSize() = 0;
virtual void SetCursorPosition(short x, short y) = 0;
virtual COORD GetCursorPosition() = 0;
virtual void SetCursorPosition(til::point pos) = 0;
virtual til::point GetCursorPosition() = 0;
virtual void SetCursorVisibility(const bool visible) = 0;
virtual void CursorLineFeed(const bool withReturn) = 0;
virtual void EnableCursorBlinking(const bool enable) = 0;

virtual void DeleteCharacter(const size_t count) = 0;
virtual void InsertCharacter(const size_t count) = 0;
virtual void EraseCharacters(const size_t numChars) = 0;
virtual void DeleteCharacter(const til::CoordType count) = 0;
virtual void InsertCharacter(const til::CoordType count) = 0;
virtual void EraseCharacters(const til::CoordType numChars) = 0;
virtual bool EraseInLine(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) = 0;
virtual bool EraseInDisplay(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) = 0;

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ bool Terminal::SendMouseEvent(const COORD viewportPos, const unsigned int uiButt
#pragma warning(suppress : 26496) // analysis can't tell we're assigning through a reference below
auto clampedPos{ viewportPos };
_GetMutableViewport().ToOrigin().Clamp(clampedPos);
return _terminalInput->HandleMouse(clampedPos, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta, state);
return _terminalInput->HandleMouse(til::point{ clampedPos }, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta, state);
}

// Method Description:
Expand Down
10 changes: 5 additions & 5 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,14 @@ class Microsoft::Terminal::Core::Terminal final :
TextAttribute GetTextAttributes() const override;
void SetTextAttributes(const TextAttribute& attrs) override;
Microsoft::Console::Types::Viewport GetBufferSize() override;
void SetCursorPosition(short x, short y) override;
COORD GetCursorPosition() override;
void SetCursorPosition(til::point pos) override;
til::point GetCursorPosition() override;
void SetCursorVisibility(const bool visible) override;
void EnableCursorBlinking(const bool enable) override;
void CursorLineFeed(const bool withReturn) override;
void DeleteCharacter(const size_t count) override;
void InsertCharacter(const size_t count) override;
void EraseCharacters(const size_t numChars) override;
void DeleteCharacter(const til::CoordType count) override;
void InsertCharacter(const til::CoordType count) override;
void EraseCharacters(const til::CoordType numChars) override;
bool EraseInLine(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) override;
bool EraseInDisplay(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) override;
void WarningBell() override;
Expand Down
61 changes: 23 additions & 38 deletions src/cascadia/TerminalCore/TerminalApi.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,28 +41,22 @@ Viewport Terminal::GetBufferSize()
return _activeBuffer().GetSize();
}

void Terminal::SetCursorPosition(short x, short y)
void Terminal::SetCursorPosition(til::point pos)
{
const auto viewport = _GetMutableViewport();
const auto viewOrigin = viewport.Origin();
const short absoluteX = viewOrigin.X + x;
const short absoluteY = viewOrigin.Y + y;
COORD newPos{ absoluteX, absoluteY };
const til::point viewOrigin{ viewport.Origin() };
auto newPos = til::unwrap_coord(viewOrigin + pos);
viewport.Clamp(newPos);
_activeBuffer().GetCursor().SetPosition(newPos);
}

COORD Terminal::GetCursorPosition()
til::point Terminal::GetCursorPosition()
{
const auto absoluteCursorPos = _activeBuffer().GetCursor().GetPosition();
const til::point absoluteCursorPos{ _activeBuffer().GetCursor().GetPosition() };
const auto viewport = _GetMutableViewport();
const auto viewOrigin = viewport.Origin();
const short relativeX = absoluteCursorPos.X - viewOrigin.X;
const short relativeY = absoluteCursorPos.Y - viewOrigin.Y;
COORD newPos{ relativeX, relativeY };

const til::point viewOrigin{ viewport.Origin() };
// TODO assert that the coord is > (0, 0) && <(view.W, view.H)
return newPos;
return absoluteCursorPos - viewOrigin;
}

// Method Description:
Expand Down Expand Up @@ -97,21 +91,17 @@ void Terminal::CursorLineFeed(const bool withReturn)
// - count, the number of characters to delete
// Return value:
// - <none>
void Terminal::DeleteCharacter(const size_t count)
void Terminal::DeleteCharacter(const til::CoordType count)
{
SHORT dist;
THROW_IF_FAILED(SizeTToShort(count, &dist));
const auto cursorPos = _activeBuffer().GetCursor().GetPosition();
const auto copyToPos = cursorPos;
const COORD copyFromPos{ cursorPos.X + dist, cursorPos.Y };
const til::point copyFromPos{ cursorPos.X + count, cursorPos.Y };
const auto viewport = _GetMutableViewport();

const auto sourceWidth = viewport.RightExclusive() - copyFromPos.X;
SHORT width;
THROW_IF_FAILED(UIntToShort(sourceWidth, &width));

// Get a rectangle of the source
auto source = Viewport::FromDimensions(copyFromPos, width, 1);
auto source = Viewport::FromDimensions(til::unwrap_coord(copyFromPos), gsl::narrow<short>(sourceWidth), 1);

// Get a rectangle of the target
const auto target = Viewport::FromDimensions(copyToPos, source.Dimensions());
Expand All @@ -137,28 +127,23 @@ void Terminal::DeleteCharacter(const size_t count)
// - count, the number of spaces to insert
// Return value:
// - <none>
void Terminal::InsertCharacter(const size_t count)
void Terminal::InsertCharacter(const til::CoordType count)
{
// NOTE: the code below is _extremely_ similar to DeleteCharacter
// We will want to use this same logic and implement a helper function instead
// that does the 'move a region from here to there' operation
// TODO: GitHub issue #2163
SHORT dist;
THROW_IF_FAILED(SizeTToShort(count, &dist));
const auto cursorPos = _activeBuffer().GetCursor().GetPosition();
const auto copyFromPos = cursorPos;
const COORD copyToPos{ cursorPos.X + dist, cursorPos.Y };
const til::point copyToPos{ cursorPos.X + count, cursorPos.Y };
const auto viewport = _GetMutableViewport();
const auto sourceWidth = viewport.RightExclusive() - copyFromPos.X;
SHORT width;
THROW_IF_FAILED(UIntToShort(sourceWidth, &width));

// Get a rectangle of the source
auto source = Viewport::FromDimensions(copyFromPos, width, 1);
const auto sourceOrigin = source.Origin();
auto source = Viewport::FromDimensions(copyFromPos, gsl::narrow<short>(sourceWidth), 1);

// Get a rectangle of the target
const auto target = Viewport::FromDimensions(copyToPos, source.Dimensions());
const auto target = Viewport::FromDimensions(til::unwrap_coord(copyToPos), source.Dimensions());
const auto walkDirection = Viewport::DetermineWalkDirection(source, target);

auto sourcePos = source.GetWalkOrigin(walkDirection);
Expand All @@ -170,16 +155,16 @@ void Terminal::InsertCharacter(const size_t count)
const auto data = OutputCell(*(_activeBuffer().GetCellDataAt(sourcePos)));
_activeBuffer().Write(OutputCellIterator({ &data, 1 }), targetPos);
} while (source.WalkInBounds(sourcePos, walkDirection) && target.WalkInBounds(targetPos, walkDirection));
const auto eraseIter = OutputCellIterator(UNICODE_SPACE, _activeBuffer().GetCurrentAttributes(), dist);
const auto eraseIter = OutputCellIterator(UNICODE_SPACE, _activeBuffer().GetCurrentAttributes(), count);
_activeBuffer().Write(eraseIter, cursorPos);
}

void Terminal::EraseCharacters(const size_t numChars)
void Terminal::EraseCharacters(const til::CoordType numChars)
{
const auto absoluteCursorPos = _activeBuffer().GetCursor().GetPosition();
const auto viewport = _GetMutableViewport();
const short distanceToRight = viewport.RightExclusive() - absoluteCursorPos.X;
const short fillLimit = std::min(static_cast<short>(numChars), distanceToRight);
const auto distanceToRight = viewport.RightExclusive() - absoluteCursorPos.X;
const auto fillLimit = std::min(numChars, distanceToRight);
const auto eraseIter = OutputCellIterator(UNICODE_SPACE, _activeBuffer().GetCurrentAttributes(), fillLimit);
_activeBuffer().Write(eraseIter, absoluteCursorPos);
}
Expand All @@ -198,7 +183,7 @@ bool Terminal::EraseInLine(const ::Microsoft::Console::VirtualTerminal::Dispatch
{
const auto cursorPos = _activeBuffer().GetCursor().GetPosition();
const auto viewport = _GetMutableViewport();
COORD startPos = { 0 };
til::point startPos;
startPos.Y = cursorPos.Y;
// nlength determines the number of spaces we need to write
DWORD nlength = 0;
Expand All @@ -224,7 +209,7 @@ bool Terminal::EraseInLine(const ::Microsoft::Console::VirtualTerminal::Dispatch
const auto eraseIter = OutputCellIterator(UNICODE_SPACE, _activeBuffer().GetCurrentAttributes(), nlength);

// Explicitly turn off end-of-line wrap-flag-setting when erasing cells.
_activeBuffer().Write(eraseIter, startPos, false);
_activeBuffer().Write(eraseIter, til::unwrap_coord(startPos), false);
return true;
}

Expand Down Expand Up @@ -275,7 +260,7 @@ bool Terminal::EraseInDisplay(const DispatchTypes::EraseType eraseType)
short sNewTop = coordLastChar.Y + 1;

// Increment the circular buffer only if the new location of the viewport would be 'below' the buffer
const short delta = (sNewTop + viewport.Height()) - (_activeBuffer().GetSize().Height());
const auto delta = (sNewTop + viewport.Height()) - (_activeBuffer().GetSize().Height());
for (auto i = 0; i < delta; i++)
{
_activeBuffer().IncrementCircularBuffer();
Expand All @@ -297,7 +282,7 @@ bool Terminal::EraseInDisplay(const DispatchTypes::EraseType eraseType)
// and we have to make sure we erase that text
const auto eraseStart = viewport.Height();
const auto eraseEnd = _activeBuffer().GetLastNonSpaceCharacter(viewport).Y;
for (SHORT i = eraseStart; i <= eraseEnd; i++)
for (auto i = eraseStart; i <= eraseEnd; i++)
{
_activeBuffer().GetRowByOffset(i).Reset(_activeBuffer().GetCurrentAttributes());
}
Expand All @@ -316,7 +301,7 @@ bool Terminal::EraseInDisplay(const DispatchTypes::EraseType eraseType)
// Move the viewport, adjust the scroll bar if needed, and restore the old cursor position
_mutableViewport = Viewport::FromExclusive(newWin);
Terminal::_NotifyScrollEvent();
SetCursorPosition(relativeCursor.X, relativeCursor.Y);
SetCursorPosition(til::point{ relativeCursor });

return true;
}
Expand Down
52 changes: 20 additions & 32 deletions src/cascadia/TerminalCore/TerminalDispatch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,10 @@ void TerminalDispatch::PrintString(const std::wstring_view string)
_terminalApi.PrintString(string);
}

bool TerminalDispatch::CursorPosition(const size_t line,
const size_t column)
bool TerminalDispatch::CursorPosition(VTInt line,
VTInt column)
{
SHORT x{ 0 };
SHORT y{ 0 };

THROW_IF_FAILED(SizeTToShort(column, &x));
THROW_IF_FAILED(SizeTToShort(line, &y));

THROW_IF_FAILED(ShortSub(x, 1, &x));
THROW_IF_FAILED(ShortSub(y, 1, &y));

_terminalApi.SetCursorPosition(x, y);
_terminalApi.SetCursorPosition({ column - 1, line - 1 });
return true;
}

Expand All @@ -57,27 +48,24 @@ bool TerminalDispatch::EnableCursorBlinking(const bool enable)
return true;
}

bool TerminalDispatch::CursorForward(const size_t distance)
bool TerminalDispatch::CursorForward(const VTInt distance)
{
const auto cursorPos = _terminalApi.GetCursorPosition();
const COORD newCursorPos{ cursorPos.X + gsl::narrow<short>(distance), cursorPos.Y };
_terminalApi.SetCursorPosition(newCursorPos.X, newCursorPos.Y);
_terminalApi.SetCursorPosition({ cursorPos.X + distance, cursorPos.Y });
return true;
}

bool TerminalDispatch::CursorBackward(const size_t distance)
bool TerminalDispatch::CursorBackward(const VTInt distance)
{
const auto cursorPos = _terminalApi.GetCursorPosition();
const COORD newCursorPos{ cursorPos.X - gsl::narrow<short>(distance), cursorPos.Y };
_terminalApi.SetCursorPosition(newCursorPos.X, newCursorPos.Y);
_terminalApi.SetCursorPosition({ cursorPos.X - distance, cursorPos.Y });
return true;
}

bool TerminalDispatch::CursorUp(const size_t distance)
bool TerminalDispatch::CursorUp(const VTInt distance)
{
const auto cursorPos = _terminalApi.GetCursorPosition();
const COORD newCursorPos{ cursorPos.X, cursorPos.Y + gsl::narrow<short>(distance) };
_terminalApi.SetCursorPosition(newCursorPos.X, newCursorPos.Y);
_terminalApi.SetCursorPosition({ cursorPos.X, cursorPos.Y + distance });
return true;
}

Expand All @@ -99,7 +87,7 @@ bool TerminalDispatch::LineFeed(const DispatchTypes::LineFeedType lineFeedType)
}
}

bool TerminalDispatch::EraseCharacters(const size_t numChars)
bool TerminalDispatch::EraseCharacters(const VTInt numChars)
{
_terminalApi.EraseCharacters(numChars);
return true;
Expand All @@ -114,7 +102,7 @@ bool TerminalDispatch::WarningBell()
bool TerminalDispatch::CarriageReturn()
{
const auto cursorPos = _terminalApi.GetCursorPosition();
_terminalApi.SetCursorPosition(0, cursorPos.Y);
_terminalApi.SetCursorPosition({ 0, cursorPos.Y });
return true;
}

Expand All @@ -134,13 +122,13 @@ bool TerminalDispatch::HorizontalTabSet()
return true;
}

bool TerminalDispatch::ForwardTab(const size_t numTabs)
bool TerminalDispatch::ForwardTab(const VTInt numTabs)
{
const auto width = _terminalApi.GetBufferSize().Dimensions().X;
const auto cursorPosition = _terminalApi.GetCursorPosition();
auto column = cursorPosition.X;
const auto row = cursorPosition.Y;
auto tabsPerformed = 0u;
VTInt tabsPerformed = 0;
_InitTabStopsForWidth(width);
while (column + 1 < width && tabsPerformed < numTabs)
{
Expand All @@ -151,17 +139,17 @@ bool TerminalDispatch::ForwardTab(const size_t numTabs)
}
}

_terminalApi.SetCursorPosition(column, row);
_terminalApi.SetCursorPosition({ column, row });
return true;
}

bool TerminalDispatch::BackwardsTab(const size_t numTabs)
bool TerminalDispatch::BackwardsTab(const VTInt numTabs)
{
const auto width = _terminalApi.GetBufferSize().Dimensions().X;
const auto cursorPosition = _terminalApi.GetCursorPosition();
auto column = cursorPosition.X;
const auto row = cursorPosition.Y;
auto tabsPerformed = 0u;
VTInt tabsPerformed = 0;
_InitTabStopsForWidth(width);
while (column > 0 && tabsPerformed < numTabs)
{
Expand All @@ -172,7 +160,7 @@ bool TerminalDispatch::BackwardsTab(const size_t numTabs)
}
}

_terminalApi.SetCursorPosition(column, row);
_terminalApi.SetCursorPosition({ column, row });
return true;
}

Expand Down Expand Up @@ -266,7 +254,7 @@ bool TerminalDispatch::EraseInLine(const DispatchTypes::EraseType eraseType)
// - count, the number of characters to delete
// Return Value:
// - True.
bool TerminalDispatch::DeleteCharacter(const size_t count)
bool TerminalDispatch::DeleteCharacter(const VTInt count)
{
_terminalApi.DeleteCharacter(count);
return true;
Expand All @@ -278,7 +266,7 @@ bool TerminalDispatch::DeleteCharacter(const size_t count)
// - count, the number of spaces to add
// Return Value:
// - True.
bool TerminalDispatch::InsertCharacter(const size_t count)
bool TerminalDispatch::InsertCharacter(const VTInt count)
{
_terminalApi.InsertCharacter(count);
return true;
Expand Down Expand Up @@ -803,7 +791,7 @@ bool TerminalDispatch::CursorSaveState()
// TODO GH#3849: When de-duplicating this, the AdaptDispatch version of this
// is more elaborate.
const auto attributes = _terminalApi.GetTextAttributes();
COORD coordCursor = _terminalApi.GetCursorPosition();
const auto coordCursor = _terminalApi.GetCursorPosition();
// The cursor is given to us by the API as relative to current viewport top.

// VT is also 1 based, not 0 based, so correct by 1.
Expand Down
Loading

0 comments on commit 7af134c

Please sign in to comment.