Skip to content

Commit

Permalink
Merge the TerminalDispatch and AdaptDispatch classes (#13024)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

This PR replaces the `TerminalDispatch` class with the `AdaptDispatch` class from conhost, so we're no longer duplicating the VT functionality in two places. It also gives us a more complete VT implementation on the Terminal side, so it should work better in pass-through mode.

## References

This is essentially part two of PR #12703.

## PR Checklist
* [x] Closes #3849
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema updated.
* [x] I've discussed this with core contributors already. Issue number where discussion took place: #12662

## Detailed Description of the Pull Request / Additional comments

The first thing was to give the `ConGetSet` interface a new name, since it's now no longer specific to conhost. I went with `ITerminalApi`, since that was the equivalent interface on the terminal side, and it still seemed like a generic enough name. I also changed the way the api is managed by the `AdaptDispatch` class, so it's now stored as a reference rather than a `unique_ptr`, which more closely matches the way the `TerminalDispatch` class worked.

I then had to make sure that `AdaptDispatch` actually included all of the functionality currently in `TerminalDispatch`. That meant copying across the code for bracketed paste mode, the copy to clipboard operation, and the various ConEmu OSC operations. This also required a few new methods to the `ConGetSet`/`ITerminalApi` interface, but for now these are just stubs in conhost.

Then there were a few thing in the api interface that needed cleaning up. The `ReparentWindow` method doesn't belong there, so I've moved that into `PtySignalInputThread` class. And the `WriteInput` method was too low-level for the Terminal requirements, so I've replaced that with a `ReturnResponse` method which takes a `wstring_view`.

It was then a matter of getting the `Terminal` class to implement all the methods in the new `ITerminalApi` interface that it didn't already have. This was mostly mapping to existing functionality, but there are still a number of methods that I've had to leave as stubs for now. However, what we have is still good enough that I could then nuke the `TerminalDispatch` class from the Terminal code and replace it with `AdaptDispatch`.

One oddity that came up in testing, though, was the `AdaptDispatch` implementation of `EraseAll` would push a blank line into the scrollback when called on an empty buffer, whereas the previous terminal implementation did not. That caused problems for the conpty connection, because one of the first things it does on startup is send an `ED 2` sequence. I've now updated the `AdaptDispatch` implementation to match the behavior of the terminal implementation in that regard.

Another problem was that the terminal implementation of the color table commands had special handling for the background color to notify the application window that it needed to repaint the background. I didn't want to have to push the color table operations through the `ITerminalApi` interface, so I've instead moved the handling of the background update into the renderer, initiated by a flag on the `TriggerRefreshAll` method.

## Validation Steps Performed

Surprisingly this PR didn't require a lot of changes to get the unit tests working again. There were just a few methods used from the original `ITerminalApi` that have now been removed, and which needed an equivalent replacement. Also the updated behavior of the `EraseAll` method in conhost resulted in a change to the expected cursor position in one of the screen buffer tests.

In terms of manual testing, I've tried out all the different shells in Windows Terminal to make sure there wasn't anything obviously wrong. And I've run a bunch of the tests from _vttest_ to try and get a wider coverage of the VT functionality, and confirmed everything still works at least as well as it used to. I've also run some of my own tests to verify the operations that had to be copied from `TerminalDispatch` to `AdaptDispatch`.
  • Loading branch information
j4james authored May 4, 2022
1 parent eb5c26c commit f4e0d9f
Show file tree
Hide file tree
Showing 42 changed files with 628 additions and 2,285 deletions.
2 changes: 1 addition & 1 deletion src/buffer/out/search.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ std::pair<COORD, COORD> Search::GetFoundLocation() const noexcept
// - direction - The intended direction of the search
// Return Value:
// - Coordinate to start the search from.
COORD Search::s_GetInitialAnchor(IUiaData& uiaData, const Direction direction)
COORD Search::s_GetInitialAnchor(const IUiaData& uiaData, const Direction direction)
{
const auto& textBuffer = uiaData.GetTextBuffer();
const auto textBufferEndPosition = uiaData.GetTextBufferEndPosition();
Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/search.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class Search final
void _IncrementCoord(COORD& coord) const noexcept;
void _DecrementCoord(COORD& coord) const noexcept;

static COORD s_GetInitialAnchor(Microsoft::Console::Types::IUiaData& uiaData, const Direction dir);
static COORD s_GetInitialAnchor(const Microsoft::Console::Types::IUiaData& uiaData, const Direction dir);

static std::vector<std::vector<wchar_t>> s_CreateNeedleFromString(const std::wstring& wstr);

Expand Down
16 changes: 8 additions & 8 deletions src/cascadia/PublicTerminalCore/HwndTerminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,9 @@ HRESULT HwndTerminal::Initialize()
_terminal = std::make_unique<::Microsoft::Terminal::Core::Terminal>();
auto renderThread = std::make_unique<::Microsoft::Console::Render::RenderThread>();
auto* const localPointerToThread = renderThread.get();
const auto& renderSettings = _terminal->GetRenderSettings();
auto& renderSettings = _terminal->GetRenderSettings();
renderSettings.SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, RGB(12, 12, 12));
renderSettings.SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, RGB(204, 204, 204));
_renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get(), nullptr, 0, std::move(renderThread));
RETURN_HR_IF_NULL(E_POINTER, localPointerToThread);
RETURN_IF_FAILED(localPointerToThread->Initialize(_renderer.get()));
Expand All @@ -237,11 +239,7 @@ HRESULT HwndTerminal::Initialize()

_renderEngine = std::move(dxEngine);

_terminal->SetBackgroundCallback([](auto) {});

_terminal->Create(COORD{ 80, 25 }, 1000, *_renderer);
_terminal->SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, RGB(12, 12, 12));
_terminal->SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, RGB(204, 204, 204));
_terminal->SetWriteInputCallback([=](std::wstring_view input) noexcept { _WriteTextToConnection(input); });
localPointerToThread->EnablePainting();

Expand Down Expand Up @@ -788,15 +786,17 @@ void _stdcall TerminalSetTheme(void* terminal, TerminalTheme theme, LPCWSTR font
{
auto lock = publicTerminal->_terminal->LockForWriting();

publicTerminal->_terminal->SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, theme.DefaultForeground);
publicTerminal->_terminal->SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, theme.DefaultBackground);
auto& renderSettings = publicTerminal->_terminal->GetRenderSettings();
renderSettings.SetColorTableEntry(TextColor::DEFAULT_FOREGROUND, theme.DefaultForeground);
renderSettings.SetColorTableEntry(TextColor::DEFAULT_BACKGROUND, theme.DefaultBackground);

publicTerminal->_renderEngine->SetSelectionBackground(theme.DefaultSelectionBackground, theme.SelectionBackgroundAlpha);

// Set the font colors
for (size_t tableIndex = 0; tableIndex < 16; tableIndex++)
{
// It's using gsl::at to check the index is in bounds, but the analyzer still calls this array-to-pointer-decay
[[gsl::suppress(bounds .3)]] publicTerminal->_terminal->SetColorTableEntry(tableIndex, gsl::at(theme.ColorTable, tableIndex));
[[gsl::suppress(bounds .3)]] renderSettings.SetColorTableEntry(tableIndex, gsl::at(theme.ColorTable, tableIndex));
}
}

Expand Down
30 changes: 9 additions & 21 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto pfnTabColorChanged = std::bind(&ControlCore::_terminalTabColorChanged, this, std::placeholders::_1);
_terminal->SetTabColorChangedCallback(pfnTabColorChanged);

auto pfnBackgroundColorChanged = std::bind(&ControlCore::_terminalBackgroundColorChanged, this, std::placeholders::_1);
_terminal->SetBackgroundCallback(pfnBackgroundColorChanged);

auto pfnScrollPositionChanged = std::bind(&ControlCore::_terminalScrollPositionChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3);
_terminal->SetScrollPositionChangedCallback(pfnScrollPositionChanged);

Expand Down Expand Up @@ -128,6 +125,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto& renderSettings = _terminal->GetRenderSettings();
_renderer = std::make_unique<::Microsoft::Console::Render::Renderer>(renderSettings, _terminal.get(), nullptr, 0, std::move(renderThread));

_renderer->SetBackgroundColorChangedCallback([this]() { _rendererBackgroundColorChanged(); });

_renderer->SetRendererEnteredErrorStateCallback([weakThis = get_weak()]() {
if (auto strongThis{ weakThis.get() })
{
Expand Down Expand Up @@ -1153,21 +1152,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_TabColorChangedHandlers(*this, nullptr);
}

// Method Description:
// - Called for the Terminal's BackgroundColorChanged callback. This will
// re-raise a new winrt TypedEvent that can be listened to.
// - The listeners to this event will re-query the control for the current
// value of BackgroundColor().
// Arguments:
// - <unused>
// Return Value:
// - <none>
void ControlCore::_terminalBackgroundColorChanged(const COLORREF /*color*/)
{
// Raise a BackgroundColorChanged event
_BackgroundColorChangedHandlers(*this, nullptr);
}

// Method Description:
// - Update the position and size of the scrollbar to match the given
// viewport top, viewport height, and buffer size.
Expand Down Expand Up @@ -1360,6 +1344,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_SwapChainChangedHandlers(*this, nullptr);
}

void ControlCore::_rendererBackgroundColorChanged()
{
_BackgroundColorChangedHandlers(*this, nullptr);
}

void ControlCore::BlinkAttributeTick()
{
auto lock = _terminal->LockForWriting();
Expand Down Expand Up @@ -1525,7 +1514,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
if (clearType == Control::ClearBufferType::Scrollback || clearType == Control::ClearBufferType::All)
{
_terminal->EraseInDisplay(::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType::Scrollback);
_terminal->EraseScrollback();
}

if (clearType == Control::ClearBufferType::Screen || clearType == Control::ClearBufferType::All)
Expand Down Expand Up @@ -1684,8 +1673,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation

_renderEngine->SetSelectionBackground(til::color{ _settings->SelectionBackground() });

_renderer->TriggerRedrawAll();
_BackgroundColorChangedHandlers(*this, nullptr);
_renderer->TriggerRedrawAll(true);
}

bool ControlCore::HasUnfocusedAppearance() const
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void _terminalWarningBell();
void _terminalTitleChanged(std::wstring_view wstr);
void _terminalTabColorChanged(const std::optional<til::color> color);
void _terminalBackgroundColorChanged(const COLORREF color);
void _terminalScrollPositionChanged(const int viewTop,
const int viewHeight,
const int bufferSize);
Expand All @@ -277,6 +276,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
#pragma region RendererCallbacks
void _rendererWarning(const HRESULT hr);
void _renderEngineSwapChainChanged();
void _rendererBackgroundColorChanged();
#pragma endregion

void _raiseReadOnlyWarning();
Expand Down
80 changes: 0 additions & 80 deletions src/cascadia/TerminalCore/ITerminalApi.hpp

This file was deleted.

58 changes: 35 additions & 23 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@

#include "pch.h"
#include "Terminal.hpp"
#include "../../terminal/adapter/adaptDispatch.hpp"
#include "../../terminal/parser/OutputStateMachineEngine.hpp"
#include "TerminalDispatch.hpp"
#include "../../inc/unicode.hpp"
#include "../../types/inc/utils.hpp"
#include "../../types/inc/colorTable.hpp"
Expand Down Expand Up @@ -50,19 +50,6 @@ Terminal::Terminal() :
_taskbarProgress{ 0 },
_trimBlockSelection{ false }
{
auto dispatch = std::make_unique<TerminalDispatch>(*this);
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));

_stateMachine = std::make_unique<StateMachine>(std::move(engine));

// Until we have a true pass-through mode (GH#1173), the decision as to
// whether C1 controls are interpreted or not is made at the conhost level.
// If they are being filtered out, then we will simply never receive them.
// But if they are being accepted by conhost, there's a chance they may get
// passed through in some situations, so it's important that our state
// machine is always prepared to accept them.
_stateMachine->SetParserMode(StateMachine::Mode::AcceptC1, true);

auto passAlongInput = [&](std::deque<std::unique_ptr<IInputEvent>>& inEventsToWrite) {
if (!_pfnWriteInput)
{
Expand All @@ -87,6 +74,18 @@ void Terminal::Create(COORD viewportSize, SHORT scrollbackLines, Renderer& rende
const TextAttribute attr{};
const UINT cursorSize = 12;
_mainBuffer = std::make_unique<TextBuffer>(bufferSize, attr, cursorSize, true, renderer);

auto dispatch = std::make_unique<AdaptDispatch>(*this, renderer, _renderSettings, *_terminalInput);
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
_stateMachine = std::make_unique<StateMachine>(std::move(engine));

// Until we have a true pass-through mode (GH#1173), the decision as to
// whether C1 controls are interpreted or not is made at the conhost level.
// If they are being filtered out, then we will simply never receive them.
// But if they are being accepted by conhost, there's a chance they may get
// passed through in some situations, so it's important that our state
// machine is always prepared to accept them.
_stateMachine->SetParserMode(StateMachine::Mode::AcceptC1, true);
}

// Method Description:
Expand Down Expand Up @@ -218,6 +217,28 @@ void Terminal::UpdateAppearance(const ICoreAppearance& appearance)
_defaultCursorShape = cursorShape;
}

void Terminal::SetCursorStyle(const DispatchTypes::CursorStyle cursorStyle)
{
auto& engine = reinterpret_cast<OutputStateMachineEngine&>(_stateMachine->Engine());
engine.Dispatch().SetCursorStyle(cursorStyle);
}

void Terminal::EraseScrollback()
{
auto& engine = reinterpret_cast<OutputStateMachineEngine&>(_stateMachine->Engine());
engine.Dispatch().EraseInDisplay(DispatchTypes::EraseType::Scrollback);
}

bool Terminal::IsXtermBracketedPasteModeEnabled() const
{
return _bracketedPasteMode;
}

std::wstring_view Terminal::GetWorkingDirectory()
{
return _workingDirectory;
}

// Method Description:
// - Resize the terminal as the result of some user interaction.
// Arguments:
Expand Down Expand Up @@ -1271,15 +1292,6 @@ void Terminal::SetCursorPositionChangedCallback(std::function<void()> pfn) noexc
_pfnCursorPositionChanged.swap(pfn);
}

// Method Description:
// - Allows setting a callback for when the background color is changed
// Arguments:
// - pfn: a function callback that takes a color
void Terminal::SetBackgroundCallback(std::function<void(const til::color)> pfn) noexcept
{
_pfnBackgroundColorChanged.swap(pfn);
}

// Method Description:
// - Allows settings a callback for settings the taskbar progress indicator
// Arguments:
Expand Down
Loading

0 comments on commit f4e0d9f

Please sign in to comment.