Skip to content
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

Implement the rest of the FTCS marks #14341

Merged
19 commits merged into from
Dec 1, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2022,7 +2022,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

// The version of this that only accepts a ScrollMark will automatically
// set the start & end to the cursor position.
_terminal->AddMark(m, m.start, m.end);
_terminal->AddMark(m, m.start, m.end, true);
}
void ControlCore::ClearMark() { _terminal->ClearMark(); }
void ControlCore::ClearAllMarks() { _terminal->ClearAllMarks(); }
Expand Down
76 changes: 65 additions & 11 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -804,11 +804,34 @@ bool Terminal::SendCharEvent(const wchar_t ch, const WORD scanCode, const Contro
// Then treat this line like it's a prompt mark.
if (_autoMarkPrompts && vkey == VK_RETURN && !_inAltBuffer())
{
DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
// Don't set the color - we'll automatically use the DEFAULT_FOREGROUND
// color for any MarkCategory::Prompt marks without one set.
AddMark(mark);
// * If we have a _currentPrompt:
// - Then we did know that the prompt started, (we may have also
// already gotten a CommandStart sequence). The user has pressed
// enter, and we're treating that like the prompt has now ended.
// - Perform a FTCS_COMMAND_EXECUTED, so that we start marking this
// as output.
// - This enables CMD to have full FTCS support, even though there's
// no point in CMD to insert a "preexec" hook
// * Else: We don't have a prompt. We don't know anything else, but we
// can set the whole line as the prompt, no command, and start the
// command_executed now.

if (_currentPrompt)
{
OutputStart();
Copy link
Member

@lhecker lhecker Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Both branches end up calling OutputStart at the end.

}
else
{
DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
// Don't set the color - we'll automatically use the DEFAULT_FOREGROUND
// color for any MarkCategory::Prompt marks without one set.

AddMark(mark); // without parameters, this will act as if it came
// from the connection itself, and set _currentPromptState accordingly

OutputStart();
}
}

// Unfortunately, the UI doesn't give us both a character down and a
Expand Down Expand Up @@ -1251,8 +1274,20 @@ void Terminal::_AdjustCursorPosition(const til::point proposedPosition)
{
for (auto& mark : _scrollMarks)
{
// Move the mark up
mark.start.y -= rowsPushedOffTopOfBuffer;

// If the mark had sub-regions, then move those pointers too
if (mark.commandEnd.has_value())
{
(*mark.commandEnd).y -= rowsPushedOffTopOfBuffer;
}
if (mark.outputEnd.has_value())
{
(*mark.outputEnd).y -= rowsPushedOffTopOfBuffer;
}
}

_scrollMarks.erase(std::remove_if(_scrollMarks.begin(),
_scrollMarks.end(),
[](const VirtualTerminal::DispatchTypes::ScrollMark& m) { return m.start.y < 0; }),
Expand Down Expand Up @@ -1542,9 +1577,11 @@ void Terminal::_updateUrlDetection()
}
}

// NOTE: This is the version of AddMark that comes from the UI. The VT api call into this too.
void Terminal::AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark,
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
const til::point& start,
const til::point& end)
const til::point& end,
const bool fromUi)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that using indices would be better than using .back() and adding .insert().
Rust exposes this problem much more obviously, as you can't "borrow" foreign data easily there without introducing lifetime issues. A common solution is to instead save a numeric index (or similar) of where the data is stored at. In this case you could store a size_t _currentPromptMarkIndex which stores the index of the mark inside _scrollMarks, so that you can safely retrieve it from _scrollMarks at a later time.
It's basically exactly like the _currentPrompt pointer since it's still sort of a (relative) memory location, but unlike it, it's much easier to prove that it doesn't reference invalid data (by checking that _currentPromptMarkIndex < .size()). This way you don't need to insert() user marks at the beginning either, allowing you to remove this boolean parameter. Such a numeric index doesn't fix any "stateful" bugs of course, but it does prevent memory lifetime issues, as was the case with the raw pointer.

{
if (_inAltBuffer())
{
Expand All @@ -1555,11 +1592,21 @@ void Terminal::AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes:
m.start = start;
m.end = end;

_scrollMarks.push_back(m);
if (fromUi)
{
_scrollMarks.insert(_scrollMarks.begin(), m);
}
else
{
_scrollMarks.push_back(m);
}

// Tell the control that the scrollbar has somehow changed. Used as a
// workaround to force the control to redraw any scrollbar marks
_NotifyScrollEvent();

// DON'T set _currentPrompt. The VT impl will do that for you. We don't want
// UI-driven marks to set that.
}

void Terminal::ClearMark()
Expand All @@ -1576,13 +1623,19 @@ void Terminal::ClearMark()
start = til::point{ GetSelectionAnchor() };
end = til::point{ GetSelectionEnd() };
}
auto inSelection = [&start, &end](const DispatchTypes::ScrollMark& m) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I just realized that the two positions were for a selection!)

return (m.start >= start && m.start <= end) ||
(m.end >= start && m.end <= end);
};

if (_currentPrompt && inSelection(*_currentPrompt))
{
_currentPrompt = nullptr;
}

_scrollMarks.erase(std::remove_if(_scrollMarks.begin(),
_scrollMarks.end(),
[&start, &end](const auto& m) {
return (m.start >= start && m.start <= end) ||
(m.end >= start && m.end <= end);
}),
inSelection),
_scrollMarks.end());

// Tell the control that the scrollbar has somehow changed. Used as a
Expand All @@ -1591,6 +1644,7 @@ void Terminal::ClearMark()
}
void Terminal::ClearAllMarks() noexcept
{
_currentPrompt = nullptr;
_scrollMarks.clear();
// Tell the control that the scrollbar has somehow changed. Used as a
// workaround to force the control to redraw any scrollbar marks
Expand Down
15 changes: 14 additions & 1 deletion src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ class Microsoft::Terminal::Core::Terminal final :
const std::vector<Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark>& GetScrollMarks() const noexcept;
void AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark,
const til::point& start,
const til::point& end);
const til::point& end,
const bool fromUi);

#pragma region ITerminalApi
// These methods are defined in TerminalApi.cpp
Expand Down Expand Up @@ -134,6 +135,9 @@ class Microsoft::Terminal::Core::Terminal final :
void UseMainScreenBuffer() override;

void AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) override;
void CommandStart() override;
void OutputStart() override;
void CommandFinished(std::optional<unsigned int> error) override;

bool IsConsolePty() const noexcept override;
bool IsVtInputEnabled() const noexcept override;
Expand Down Expand Up @@ -399,6 +403,15 @@ class Microsoft::Terminal::Core::Terminal final :
std::optional<KeyEventCodes> _lastKeyEventCodes;

std::vector<Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark> _scrollMarks;
enum PromptState : uint32_t
{
None = 0,
Prompt = 1,
Command = 2,
Output = 3
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
};
PromptState _currentPromptState{ PromptState::None };
Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark* _currentPrompt{ nullptr };
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved

static WORD _ScanCodeFromVirtualKey(const WORD vkey) noexcept;
static WORD _VirtualKeyFromScanCode(const WORD scanCode) noexcept;
Expand Down
98 changes: 96 additions & 2 deletions src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,104 @@ void Terminal::UseMainScreenBuffer()
CATCH_LOG();
}

void Terminal::AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark)
// NOTE: This is the version of AddMark that comes from VT
void Terminal::AddMark(const DispatchTypes::ScrollMark& mark)
{
const til::point cursorPos{ _activeBuffer().GetCursor().GetPosition() };
AddMark(mark, cursorPos, cursorPos);
AddMark(mark, cursorPos, cursorPos, false);

if (mark.category == DispatchTypes::MarkCategory::Prompt)
{
_currentPromptState = PromptState::Prompt;
}
}

void Terminal::CommandStart()
{
const til::point cursorPos{ _activeBuffer().GetCursor().GetPosition() };

if ((_currentPromptState == Prompt) &&
(_scrollMarks.size() > 0))
{
// We were in the right state, and there's a previous mark to work
// with.
//
//We can just do the work below safely.
}
else
{
// If there was no last mark, or we're in a weird state,
// then abandon the current state, and just insert a new Prompt mark that
// start's & ends here, and got to State::Command.

DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
AddMark(mark, cursorPos, cursorPos, false);
}
_scrollMarks.back().end = cursorPos;
_currentPromptState = PromptState::Command;
}

void Terminal::OutputStart()
{
const til::point cursorPos{ _activeBuffer().GetCursor().GetPosition() };

if ((_currentPromptState == Command) &&
(_scrollMarks.size() > 0))
{
// We were in the right state, and there's a previous mark to work
// with.
//
//We can just do the work below safely.
}
else
{
// If there was no last mark, or we're in a weird state,
// then abandon the current state, and just insert a new Prompt mark that
// start's & ends here, and the command ends here, and go to State::Output.

DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
AddMark(mark, cursorPos, cursorPos, false);
}
_scrollMarks.back().commandEnd = cursorPos;
_currentPromptState = PromptState::Output;
}

void Terminal::CommandFinished(std::optional<unsigned int> error)
{
const til::point cursorPos{ _activeBuffer().GetCursor().GetPosition() };
auto category = DispatchTypes::MarkCategory::Prompt;
if (error.has_value())
{
category = *error == 0u ?
DispatchTypes::MarkCategory::Success :
DispatchTypes::MarkCategory::Error;
}

if ((_currentPromptState == Output) &&
(_scrollMarks.size() > 0))
{
// We were in the right state, and there's a previous mark to work
// with.
//
//We can just do the work below safely.
}
else
{
// If there was no last mark, or we're in a weird state, then abandon
// the current state, and just insert a new Prompt mark that start's &
// ends here, and the command ends here, AND the output ends here. and
// go to State::Output.

DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
AddMark(mark, cursorPos, cursorPos, false);
_scrollMarks.back().commandEnd = cursorPos;
}
_scrollMarks.back().outputEnd = cursorPos;
_scrollMarks.back().category = category;
_currentPromptState = PromptState::None;
}

// Method Description:
Expand Down
15 changes: 15 additions & 0 deletions src/host/outputStream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -486,3 +486,18 @@ void ConhostInternalGetSet::AddMark(const Microsoft::Console::VirtualTerminal::D
{
// Not implemented for conhost.
}

void ConhostInternalGetSet::CommandStart()
{
// Not implemented for conhost.
}

void ConhostInternalGetSet::OutputStart()
{
// Not implemented for conhost.
}

void ConhostInternalGetSet::CommandFinished(std::optional<unsigned int> /*error*/)
{
// Not implemented for conhost.
}
3 changes: 3 additions & 0 deletions src/host/outputStream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal::
void NotifyAccessibilityChange(const til::rect& changedRect) override;

void AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) override;
void CommandStart() override;
void OutputStart() override;
void CommandFinished(std::optional<unsigned int> error) override;

private:
Microsoft::Console::IIoProvider& _io;
Expand Down
3 changes: 3 additions & 0 deletions src/terminal/adapter/DispatchTypes.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,9 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes
std::optional<til::color> color;
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
Expand Down
3 changes: 3 additions & 0 deletions src/terminal/adapter/ITerminalApi.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,8 @@ namespace Microsoft::Console::VirtualTerminal
virtual void NotifyAccessibilityChange(const til::rect& changedRect) = 0;

virtual void AddMark(const Microsoft::Console::VirtualTerminal::DispatchTypes::ScrollMark& mark) = 0;
virtual void CommandStart() = 0;
virtual void OutputStart() = 0;
virtual void CommandFinished(std::optional<unsigned int> error) = 0;
};
}
52 changes: 45 additions & 7 deletions src/terminal/adapter/adaptDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2665,14 +2665,52 @@ bool AdaptDispatch::DoFinalTermAction(const std::wstring_view string)
}

const auto action = til::at(parts, 0);

if (action == L"A") // FTCS_PROMPT
if (action.size() == 1)
{
// Simply just mark this line as a prompt line.
DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
_api.AddMark(mark);
return true;
switch (action[0])
{
case L'A': // FTCS_PROMPT
{
// Simply just mark this line as a prompt line.
DispatchTypes::ScrollMark mark;
mark.category = DispatchTypes::MarkCategory::Prompt;
_api.AddMark(mark);
return true;
}
case L'B': // FTCS_COMMAND_START
{
_api.CommandStart();
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
case L'C': // FTCS_COMMAND_EXECUTED
{
_api.OutputStart();
return true;
}
case L'D': // FTCS_COMMAND_FINISHED
{
std::optional<unsigned int> error = std::nullopt;
if (parts.size() >= 2)
{
const auto errorString = til::at(parts, 1);

// If we fail to parse the code, then it was gibberish, or it might
// have just started with "-". Either way, let's just treat it as an
// error and move on.
//
// We know that "0" will be successfully parsed, and that's close enough.
unsigned int parsedError = 0;
error = Utils::StringToUint(errorString, parsedError) ? parsedError :
static_cast<unsigned int>(-1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am moderately horrified about this actually -- why is this better than UINT_MAX or whatever? Do we really want to use -1 to indicate.. uh, something? But, if we really want -1 shouldn't it be signed?

}
_api.CommandFinished(error);
return true;
}
default:
{
return false;
}
}
}

// When we add the rest of the FTCS sequences (GH#11000), we should add a
Expand Down