Skip to content

Commit

Permalink
Rewrite how marks are stored & add reflow (#16937)
Browse files Browse the repository at this point in the history
This is pretty much a huge refactoring of how marks are stored in the
buffer.

Gone is the list of `ScrollMark`s in the buffer that store regions of
text as points marking the ends. Those would be nigh impossible to
reflow nicely.

Instead, we're going to use `TextAttribute`s to store the kind of output
we've got - `Prompt`, `Command`, `Output`, or, the default, `None`.
Those already reflow nicely!

But we also need to store things like, the exit code for the command.
That's why we've now added `ScrollbarData` to `ROW`s. There's really
only going to be one prompt->output on a single row. So, we only need to
store one ScrollbarData per-row. When a command ends, we can just go
update the mark on the row that started that command.

But iterating over the whole buffer to find the next/previous
prompt/command/output region sounds complicated. So, to avoid everyone
needing to do some variant of that, we've added `MarkExtents` (which is
literally just the same mark structure as before). TextBuffer can figure
out where all the mark regions are, and hand that back to callers. This
allows ControlCore to be basically unchanged.

_But collecting up all the regions for all the marks sounds expensive!
We need to update the scrollbar frequently, we can't just collect those
up every time!_ No we can't! But we also don't need to. The scrollbar
doesn't need to know where all the marks start and end and if they have
commands and this and that - no. We only need to know the rows that have
marks on them. So, we've now also got `ScrollMark` to represent just a
mark on a scrollbar at a specific row on the buffer. We can get those
quickly.

* [x] I added a bunch of tests for this. 
* [x] I played with it and it feels good, even after a reflow (finally)
* See:
  * #11000
* #15057 (I'm not marking this as closed. The stacked PR will close
this, when I move marks to Stable)
  • Loading branch information
zadjii-msft authored Apr 5, 2024
1 parent dc4026d commit c3f44f7
Show file tree
Hide file tree
Showing 28 changed files with 1,628 additions and 496 deletions.
91 changes: 91 additions & 0 deletions src/buffer/out/Marks.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/*++
Copyright (c) Microsoft Corporation
Licensed under the MIT license.
Module Name:
- marks.hpp
Abstract:
- Definitions for types that are used for "scroll marks" and shell integration
in the buffer.
- Scroll marks are identified by the existence of "ScrollbarData" on a ROW in the buffer.
- Shell integration will then also markup the buffer with special
TextAttributes, to identify regions of text as the Prompt, the Command, the
Output, etc.
- MarkExtents are used to abstract away those regions of text, so a caller
doesn't need to iterate over the buffer themselves.
--*/

#pragma once

enum class MarkCategory : uint8_t
{
Default = 0,
Error = 1,
Warning = 2,
Success = 3,
Prompt = 4
};

// This is the data that's stored on each ROW, to suggest that there's something
// interesting on this row to show in the scrollbar. Also used in conjunction
// with shell integration - when a prompt is added through shell integration,
// we'll also add a scrollbar mark as a quick "bookmark" to the start of that
// command.
struct ScrollbarData
{
MarkCategory category{ MarkCategory::Default };

// Scrollbar marks may have been given a color, or not.
std::optional<til::color> color;

// Prompts without an exit code haven't had a matching FTCS CommandEnd
// called yet. Any value other than 0 is an error.
std::optional<uint32_t> exitCode;
// Future consideration: stick the literal command as a string on here, if
// we were given it with the 633;E sequence.
};

// Helper struct for describing the bounds of a command and it's output,
// * The Prompt is between the start & end
// * The Command is between the end & commandEnd
// * The Output is between the commandEnd & outputEnd
//
// These are not actually stored in the buffer. The buffer can produce them for
// callers, to make reasoning about regions of the buffer easier.
struct MarkExtents
{
// Data from the row
ScrollbarData data;

til::point start;
til::point end; // exclusive
std::optional<til::point> commandEnd;
std::optional<til::point> outputEnd;

// MarkCategory category{ MarkCategory::Info };
// Other things we may want to think about in the future are listed in
// GH#11000

bool HasCommand() const noexcept
{
return commandEnd.has_value() && *commandEnd != end;
}
bool HasOutput() const noexcept
{
return outputEnd.has_value() && *outputEnd != *commandEnd;
}
std::pair<til::point, til::point> GetExtent() const
{
til::point realEnd{ til::coalesce_value(outputEnd, commandEnd, end) };
return std::make_pair(start, realEnd);
}
};

// Another helper, for when callers would like to know just about the data of
// the scrollbar, but don't actually need all the extents of prompts.
struct ScrollMark
{
til::CoordType row{ 0 };
ScrollbarData data;
};
43 changes: 43 additions & 0 deletions src/buffer/out/Row.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ void ROW::Reset(const TextAttribute& attr) noexcept
_lineRendition = LineRendition::SingleWidth;
_wrapForced = false;
_doubleBytePadded = false;
_promptData = std::nullopt;
_init();
}

Expand Down Expand Up @@ -1181,3 +1182,45 @@ CharToColumnMapper ROW::_createCharToColumnMapper(ptrdiff_t offset) const noexce
const auto guessedColumn = gsl::narrow_cast<til::CoordType>(clamp(offset, 0, _columnCount));
return CharToColumnMapper{ _chars.data(), _charOffsets.data(), lastChar, guessedColumn };
}

const std::optional<ScrollbarData>& ROW::GetScrollbarData() const noexcept
{
return _promptData;
}
void ROW::SetScrollbarData(std::optional<ScrollbarData> data) noexcept
{
_promptData = data;
}

void ROW::StartPrompt() noexcept
{
if (!_promptData.has_value())
{
// You'd be tempted to write:
//
// _promptData = ScrollbarData{
// .category = MarkCategory::Prompt,
// .color = std::nullopt,
// .exitCode = std::nullopt,
// };
//
// But that's not very optimal! Read this thread for a breakdown of how
// weird std::optional can be some times:
//
// https://github.com/microsoft/terminal/pull/16937#discussion_r1553660833

_promptData.emplace(MarkCategory::Prompt);
}
}

void ROW::EndOutput(std::optional<unsigned int> error) noexcept
{
if (_promptData.has_value())
{
_promptData->exitCode = error;
if (error.has_value())
{
_promptData->category = *error == 0 ? MarkCategory::Success : MarkCategory::Error;
}
}
}
8 changes: 8 additions & 0 deletions src/buffer/out/Row.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "LineRendition.hpp"
#include "OutputCell.hpp"
#include "OutputCellIterator.hpp"
#include "Marks.hpp"

class ROW;
class TextBuffer;
Expand Down Expand Up @@ -167,6 +168,11 @@ class ROW final
auto AttrBegin() const noexcept { return _attr.begin(); }
auto AttrEnd() const noexcept { return _attr.end(); }

const std::optional<ScrollbarData>& GetScrollbarData() const noexcept;
void SetScrollbarData(std::optional<ScrollbarData> data) noexcept;
void StartPrompt() noexcept;
void EndOutput(std::optional<unsigned int> error) noexcept;

#ifdef UNIT_TESTING
friend constexpr bool operator==(const ROW& a, const ROW& b) noexcept;
friend class RowTests;
Expand Down Expand Up @@ -299,6 +305,8 @@ class ROW final
bool _wrapForced = false;
// Occurs when the user runs out of text to support a double byte character and we're forced to the next line
bool _doubleBytePadded = false;

std::optional<ScrollbarData> _promptData = std::nullopt;
};

#ifdef UNIT_TESTING
Expand Down
3 changes: 2 additions & 1 deletion src/buffer/out/TextAttribute.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

// Keeping TextColor compact helps us keeping TextAttribute compact,
// which in turn ensures that our buffer memory usage is low.
static_assert(sizeof(TextAttribute) == 16);
static_assert(sizeof(TextAttribute) == 18);
static_assert(alignof(TextAttribute) == 2);
// Ensure that we can memcpy() and memmove() the struct for performance.
static_assert(std::is_trivially_copyable_v<TextAttribute>);
Expand Down Expand Up @@ -434,4 +434,5 @@ void TextAttribute::SetStandardErase() noexcept
{
_attrs = CharacterAttributes::Normal;
_hyperlinkId = 0;
_markKind = MarkKind::None;
}
31 changes: 27 additions & 4 deletions src/buffer/out/TextAttribute.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ enum class UnderlineStyle
Max = DashedUnderlined
};

// We only need a few bits, but uint8_t apparently doesn't satisfy std::has_unique_object_representations_v
enum class MarkKind : uint16_t
{
None = 0,
Prompt = 1,
Command = 2,
Output = 3,
};

class TextAttribute final
{
public:
Expand All @@ -46,7 +55,8 @@ class TextAttribute final
_foreground{},
_background{},
_hyperlinkId{ 0 },
_underlineColor{}
_underlineColor{},
_markKind{ MarkKind::None }
{
}

Expand All @@ -55,7 +65,8 @@ class TextAttribute final
_foreground{ gsl::at(s_legacyForegroundColorMap, wLegacyAttr & FG_ATTRS) },
_background{ gsl::at(s_legacyBackgroundColorMap, (wLegacyAttr & BG_ATTRS) >> 4) },
_hyperlinkId{ 0 },
_underlineColor{}
_underlineColor{},
_markKind{ MarkKind::None }
{
}

Expand All @@ -66,7 +77,8 @@ class TextAttribute final
_foreground{ rgbForeground },
_background{ rgbBackground },
_hyperlinkId{ 0 },
_underlineColor{ rgbUnderline }
_underlineColor{ rgbUnderline },
_markKind{ MarkKind::None }
{
}

Expand All @@ -75,7 +87,8 @@ class TextAttribute final
_foreground{ foreground },
_background{ background },
_hyperlinkId{ hyperlinkId },
_underlineColor{ underlineColor }
_underlineColor{ underlineColor },
_markKind{ MarkKind::None }
{
}

Expand Down Expand Up @@ -135,6 +148,15 @@ class TextAttribute final
return _attrs;
}

constexpr void SetMarkAttributes(const MarkKind attrs) noexcept
{
_markKind = attrs;
}
constexpr MarkKind GetMarkAttributes() const noexcept
{
return _markKind;
}

bool IsHyperlink() const noexcept;

TextColor GetForeground() const noexcept;
Expand Down Expand Up @@ -202,6 +224,7 @@ class TextAttribute final
TextColor _foreground; // sizeof: 4, alignof: 1
TextColor _background; // sizeof: 4, alignof: 1
TextColor _underlineColor; // sizeof: 4, alignof: 1
MarkKind _markKind; // sizeof: 2, alignof: 1

#ifdef UNIT_TESTING
friend class TextBufferTests;
Expand Down
Loading

0 comments on commit c3f44f7

Please sign in to comment.