-
Notifications
You must be signed in to change notification settings - Fork 9k
Rewrite how marks are stored & add reflow #16937
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
Changes from all commits
80a0ffb
f88ee30
0bfe603
ed79c94
ec1eb90
ef86ff0
65f7b34
d0aaf82
0b39a61
3aa16d9
c28f8c0
c6e5934
e8aa067
73cd1b3
68f8994
f1ff258
bc5a638
5f0933e
f78dec3
6afd08f
ca50d96
ad3e599
43b64f4
43d6fe9
9f792fa
e573896
11d82a2
3cf6a3c
e0fa1c3
4da4d1f
5617282
351208b
dc21f31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,6 +230,7 @@ void ROW::Reset(const TextAttribute& attr) noexcept | |
| _lineRendition = LineRendition::SingleWidth; | ||
| _wrapForced = false; | ||
| _doubleBytePadded = false; | ||
| _promptData = std::nullopt; | ||
| _init(); | ||
| } | ||
|
|
||
|
|
@@ -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()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gut check me -
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, technically, yes. But I think the one caller of this ( |
||
| { | ||
| _promptData->exitCode = error; | ||
| if (error.has_value()) | ||
| { | ||
| _promptData->category = *error == 0 ? MarkCategory::Success : MarkCategory::Error; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| #include "LineRendition.hpp" | ||
| #include "OutputCell.hpp" | ||
| #include "OutputCellIterator.hpp" | ||
| #include "Marks.hpp" | ||
|
|
||
| class ROW; | ||
| class TextBuffer; | ||
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lhecker this will grow ROW by 12+ bytes (considering padding for alignment after @zadjii-msft if you want to save the cost of this in conhost, you can use the preprocessor version of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something else we discussed: struct ScrollbarData
{
til::color color;
uint32_t exitCode = 0;
MarkCategory category{ MarkCategory::Default };
bool valid = false;
bool hasColor = false;
bool hasExitCode = false;
};
struct ROW {
// ...
ScrollbarData _promptData;
};to avoid some overhead
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly I think
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I'd agree that The current
|
||
| }; | ||
|
|
||
| #ifdef UNIT_TESTING | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
_promptDatadoesn't get reset inReset(), which is called during buffer circling. Due to this I believe this code will start failing to work once the buffer is full.If I'm correct, then simply adding it to
Reset()should fix it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is my only remaining important concern. The memory layout is IMO not optimal for perf, but we can noodle on that later.)