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

Refactor TerminalInput to return strings #15611

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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: 0 additions & 2 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
#include "../buffer/out/search.h"
#include "../buffer/out/TextColor.h"

#include <til/ticket_lock.h>
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is a little less bad if you suppress whitespace changes: https://github.com/microsoft/terminal/pull/15611/files?w=1


namespace ControlUnitTests
{
class ControlCoreTests;
Expand Down
56 changes: 22 additions & 34 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,35 +21,9 @@ using namespace Microsoft::Console::VirtualTerminal;

using PointTree = interval_tree::IntervalTree<til::point, size_t>;

static std::wstring _KeyEventsToText(std::deque<std::unique_ptr<IInputEvent>>& inEventsToWrite)
{
std::wstring wstr = L"";
for (const auto& ev : inEventsToWrite)
{
if (ev->EventType() == InputEventType::KeyEvent)
{
const auto& k = static_cast<KeyEvent&>(*ev);
const auto wch = k.GetCharData();
wstr += wch;
}
}
return wstr;
}

#pragma warning(suppress : 26455) // default constructor is throwing, too much effort to rearrange at this time.
Terminal::Terminal()
{
auto passAlongInput = [&](std::deque<std::unique_ptr<IInputEvent>>& inEventsToWrite) {
if (!_pfnWriteInput)
{
return;
}
const auto wstr = _KeyEventsToText(inEventsToWrite);
_pfnWriteInput(wstr);
};

_terminalInput = std::make_unique<TerminalInput>(passAlongInput);

_renderSettings.SetColorAlias(ColorAlias::DefaultForeground, TextColor::DEFAULT_FOREGROUND, RGB(255, 255, 255));
_renderSettings.SetColorAlias(ColorAlias::DefaultBackground, TextColor::DEFAULT_BACKGROUND, RGB(0, 0, 0));
}
Expand All @@ -64,7 +38,7 @@ void Terminal::Create(til::size viewportSize, til::CoordType scrollbackLines, Re
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 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));

Expand Down Expand Up @@ -111,7 +85,7 @@ void Terminal::UpdateSettings(ICoreSettings settings)
_trimBlockSelection = settings.TrimBlockSelection();
_autoMarkPrompts = settings.AutoMarkPrompts();

_terminalInput->ForceDisableWin32InputMode(settings.ForceVTInput());
_terminalInput.ForceDisableWin32InputMode(settings.ForceVTInput());

if (settings.TabColor() == nullptr)
{
Expand Down Expand Up @@ -540,7 +514,7 @@ void Terminal::TrySnapOnInput()
// - true, if we are tracking mouse input. False, otherwise
bool Terminal::IsTrackingMouseInput() const noexcept
{
return _terminalInput->IsTrackingMouseInput();
return _terminalInput.IsTrackingMouseInput();
}

// Routine Description:
Expand All @@ -554,7 +528,7 @@ bool Terminal::IsTrackingMouseInput() const noexcept
bool Terminal::ShouldSendAlternateScroll(const unsigned int uiButton,
const int32_t delta) const noexcept
{
return _terminalInput->ShouldSendAlternateScroll(uiButton, ::base::saturated_cast<short>(delta));
return _terminalInput.ShouldSendAlternateScroll(uiButton, ::base::saturated_cast<short>(delta));
}

// Method Description:
Expand Down Expand Up @@ -741,7 +715,7 @@ bool Terminal::SendKeyEvent(const WORD vkey,
}

const KeyEvent keyEv{ keyDown, 1, vkey, sc, ch, states.Value() };
return _terminalInput->HandleKey(&keyEv);
return _handleTerminalInputResult(_terminalInput.HandleKey(&keyEv));
}

// Method Description:
Expand All @@ -765,7 +739,7 @@ bool Terminal::SendMouseEvent(til::point viewportPos, const unsigned int uiButto
// We shouldn't throw away perfectly good events when they're offscreen, so we just
// clamp them to be within the range [(0, 0), (W, H)].
_GetMutableViewport().ToOrigin().Clamp(viewportPos);
return _terminalInput->HandleMouse(viewportPos, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta, state);
return _handleTerminalInputResult(_terminalInput.HandleMouse(viewportPos, uiButton, GET_KEYSTATE_WPARAM(states.Value()), wheelDelta, state));
}

// Method Description:
Expand Down Expand Up @@ -818,7 +792,7 @@ bool Terminal::SendCharEvent(const wchar_t ch, const WORD scanCode, const Contro
}

const KeyEvent keyDown{ true, 1, vkey, scanCode, ch, states.Value() };
return _terminalInput->HandleKey(&keyDown);
return _handleTerminalInputResult(_terminalInput.HandleKey(&keyDown));
}

// Method Description:
Expand All @@ -831,7 +805,7 @@ bool Terminal::SendCharEvent(const wchar_t ch, const WORD scanCode, const Contro
// - none
void Terminal::FocusChanged(const bool focused) noexcept
{
_terminalInput->HandleFocus(focused);
_handleTerminalInputResult(_terminalInput.HandleFocus(focused));
}

// Method Description:
Expand Down Expand Up @@ -955,6 +929,20 @@ catch (...)
return UNICODE_INVALID;
}

[[maybe_unused]] bool Terminal::_handleTerminalInputResult(TerminalInput::OutputType&& out) const
{
if (out)
{
const auto& str = *out;
if (_pfnWriteInput && !str.empty())
{
_pfnWriteInput(str);
}
return true;
}
return false;
}

// Method Description:
// - It's possible for a single scan code on a keyboard to
// produce different key codes depending on the keyboard state.
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalCore/Terminal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ class Microsoft::Terminal::Core::Terminal final :

RenderSettings _renderSettings;
std::unique_ptr<::Microsoft::Console::VirtualTerminal::StateMachine> _stateMachine;
std::unique_ptr<::Microsoft::Console::VirtualTerminal::TerminalInput> _terminalInput;
::Microsoft::Console::VirtualTerminal::TerminalInput _terminalInput;

std::optional<std::wstring> _title;
std::wstring _startingTitle;
Expand Down Expand Up @@ -408,6 +408,7 @@ class Microsoft::Terminal::Core::Terminal final :
static WORD _VirtualKeyFromCharacter(const wchar_t ch) noexcept;
static wchar_t _CharacterFromKeyEvent(const WORD vkey, const WORD scanCode, const ControlKeyStates states) noexcept;

[[maybe_unused]] bool _handleTerminalInputResult(::Microsoft::Console::VirtualTerminal::TerminalInput::OutputType&& out) const;
void _StoreKeyEvent(const WORD vkey, const WORD scanCode) noexcept;
WORD _TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept;

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ void Terminal::UseAlternateScreenBuffer(const TextAttribute& attrs)

// GH#3321: Make sure we let the TerminalInput know that we switched
// buffers. This might affect how we interpret certain mouse events.
_terminalInput->UseAlternateScreenBuffer();
_terminalInput.UseAlternateScreenBuffer();

// Update scrollbars
_NotifyScrollEvent();
Expand Down Expand Up @@ -277,7 +277,7 @@ void Terminal::UseMainScreenBuffer()

// GH#3321: Make sure we let the TerminalInput know that we switched
// buffers. This might affect how we interpret certain mouse events.
_terminalInput->UseMainScreenBuffer();
_terminalInput.UseMainScreenBuffer();

// Update scrollbars
_NotifyScrollEvent();
Expand Down
37 changes: 20 additions & 17 deletions src/host/inputBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,8 @@ using namespace Microsoft::Console;
// - A new instance of InputBuffer
InputBuffer::InputBuffer() :
InputMode{ INPUT_BUFFER_DEFAULT_INPUT_MODE },
WaitQueue{},
_pTtyConnection(nullptr),
_termInput(std::bind(&InputBuffer::_HandleTerminalInputCallback, this, std::placeholders::_1))
_pTtyConnection(nullptr)
{
// The _termInput's constructor takes a reference to this object's _HandleTerminalInputCallback.
// We need to use std::bind to create a reference to that function without a reference to this InputBuffer

// initialize buffer header
fInComposition = false;
}
Expand Down Expand Up @@ -689,7 +684,10 @@ void InputBuffer::WriteFocusEvent(bool focused) noexcept
{
if (IsInVirtualTerminalInputMode())
{
_termInput.HandleFocus(focused);
if (const auto out = _termInput.HandleFocus(focused))
{
_HandleTerminalInputCallback(*out);
Comment on lines +687 to +689
Copy link
Member

Choose a reason for hiding this comment

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

this pattern is different from the one in TerminalCore, since it takes *out instead of out. Should we converge?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... No, I don't think so. I feel like the _WriteBuffer loop makes more sense this way.

}
}
else
{
Expand Down Expand Up @@ -726,7 +724,11 @@ bool InputBuffer::WriteMouseEvent(til::point position, const unsigned int button
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.GetActiveOutputBuffer().GetViewport().ToOrigin().Clamp(position);

return _termInput.HandleMouse(position, button, keyState, wheelDelta, state);
if (const auto out = _termInput.HandleMouse(position, button, keyState, wheelDelta, state))
{
_HandleTerminalInputCallback(*out);
return true;
}
}

return false;
Expand Down Expand Up @@ -765,10 +767,9 @@ void InputBuffer::_WriteBuffer(_Inout_ std::deque<std::unique_ptr<IInputEvent>>&
inEvents.pop_front();
if (vtInputMode)
{
// GH#11682: TerminalInput::HandleKey can handle both KeyEvents and Focus events seamlessly
const auto handled = _termInput.HandleKey(inEvent.get());
if (handled)
if (const auto out = _termInput.HandleKey(inEvent.get()))
{
_HandleTerminalInputCallback(*out);
eventsWritten++;
continue;
}
Expand Down Expand Up @@ -994,16 +995,18 @@ bool InputBuffer::IsInVirtualTerminalInputMode() const
// - inEvents - Series of input records to insert into the buffer
// Return Value:
// - <none>
void InputBuffer::_HandleTerminalInputCallback(std::deque<std::unique_ptr<IInputEvent>>& inEvents)
void InputBuffer::_HandleTerminalInputCallback(const TerminalInput::StringType& text)
{
try
{
// add all input events to the storage queue
while (!inEvents.empty())
if (text.empty())
{
auto inEvent = std::move(inEvents.front());
inEvents.pop_front();
_storage.push_back(std::move(inEvent));
return;
}

for (const auto& wch : text)
{
_storage.push_back(std::make_unique<KeyEvent>(true, 1ui16, 0ui16, 0ui16, wch, 0));
Copy link
Member

Choose a reason for hiding this comment

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

now for THIS we are gonna heap alloc every time but WHATEVER I HATE IT TOO

}

if (!_vtInputShouldSuppress)
Expand Down
2 changes: 1 addition & 1 deletion src/host/inputBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class InputBuffer final : public ConsoleObjectHeader
bool _CoalesceRepeatedKeyPressEvents(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& inEvents);
void _HandleConsoleSuspensionEvents(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& inEvents);

void _HandleTerminalInputCallback(_In_ std::deque<std::unique_ptr<IInputEvent>>& inEvents);
void _HandleTerminalInputCallback(const Microsoft::Console::VirtualTerminal::TerminalInput::StringType& text);

#ifdef UNIT_TESTING
friend class InputBufferTests;
Expand Down
Loading