Skip to content

Commit

Permalink
ConPTY: Flush unhandled sequences to the buffer (#17741)
Browse files Browse the repository at this point in the history
#17510 made it so that VT requests like DA1 are passed through to the
hosting terminal and so conhost stopped responding to them on its own.
But since our input parser doesn't support proper passthrough (yet),
it swallowed the response coming from the terminal.

To solve this issue, this PR repurposes the existing boolean return
values to indicate to the parser whether the current sequence should
be flushed to the dispatcher as-is. The output parser always returns
true (success) and leaves its pass-through handler empty, while the
input parser returns false for sequences it doesn't expect.

## Validation Steps Performed
* Launch cmd
* Press `Ctrl+[`, `[`, `c`, `Enter` (= `^[[c` = DA1 request)
* DA1 response is visible ✅
  • Loading branch information
lhecker committed Aug 22, 2024
1 parent cbb4a0a commit 6dd9c46
Show file tree
Hide file tree
Showing 25 changed files with 1,201 additions and 1,767 deletions.
4 changes: 0 additions & 4 deletions src/host/VtInputThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCur

auto dispatch = std::make_unique<InteractDispatch>();
auto engine = std::make_unique<InputStateMachineEngine>(std::move(dispatch), inheritCursor);

// we need this callback to be able to flush an unknown input sequence to the app
engine->SetFlushToInputQueueCallback([this] { return _pInputStateMachine->FlushToTerminal(); });

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

Expand Down
13 changes: 0 additions & 13 deletions src/inc/til.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,19 +96,6 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"

// These sit outside the namespace because they sit outside for WIL too.

// Inspired from RETURN_IF_WIN32_BOOL_FALSE
// WIL doesn't include a RETURN_BOOL_IF_FALSE, and RETURN_IF_WIN32_BOOL_FALSE
// will actually return the value of GLE.
#define RETURN_BOOL_IF_FALSE(b) \
do \
{ \
const bool __boolRet = wil::verify_bool(b); \
if (!__boolRet) \
{ \
return __boolRet; \
} \
} while (0, 0)

// Due to a bug (DevDiv 441931), Warning 4297 (function marked noexcept throws
// exception) is detected even when the throwing code is unreachable, such as
// the end of scope after a return, in function-level catch.
Expand Down
6 changes: 2 additions & 4 deletions src/terminal/adapter/DispatchTypes.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ namespace Microsoft::Console::VirtualTerminal
}

template<typename T>
bool for_each(const T&& predicate) const
void for_each(const T&& predicate) const
{
auto params = _params;

Expand All @@ -291,12 +291,10 @@ namespace Microsoft::Console::VirtualTerminal
params = defaultParameters;
}

auto success = true;
for (const auto& v : params)
{
success = predicate(v) && success;
predicate(v);
}
return success;
}

private:
Expand Down
5 changes: 4 additions & 1 deletion src/terminal/adapter/FontBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,10 @@ void FontBuffer::AddSixelData(const wchar_t ch)
bool FontBuffer::FinalizeSixelData()
{
// If the charset ID hasn't been initialized this isn't a valid update.
RETURN_BOOL_IF_FALSE(_charsetIdInitialized);
if (!_charsetIdInitialized)
{
return false;
}

// Flush the current line to make sure we take all the used positions
// into account when calculating the font dimensions.
Expand Down
20 changes: 6 additions & 14 deletions src/terminal/adapter/IInteractDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,13 @@ namespace Microsoft::Console::VirtualTerminal
virtual ~IInteractDispatch() = default;
#pragma warning(pop)

virtual bool WriteInput(const std::span<const INPUT_RECORD>& inputEvents) = 0;

virtual bool WriteCtrlKey(const INPUT_RECORD& event) = 0;

virtual bool WriteString(const std::wstring_view string) = 0;

virtual bool WindowManipulation(const DispatchTypes::WindowManipulationType function,
const VTParameter parameter1,
const VTParameter parameter2) = 0;

virtual bool MoveCursor(const VTInt row,
const VTInt col) = 0;

virtual bool IsVtInputEnabled() const = 0;

virtual bool FocusChanged(const bool focused) const = 0;
virtual void WriteInput(const std::span<const INPUT_RECORD>& inputEvents) = 0;
virtual void WriteCtrlKey(const INPUT_RECORD& event) = 0;
virtual void WriteString(std::wstring_view string) = 0;
virtual void WindowManipulation(DispatchTypes::WindowManipulationType function, VTParameter parameter1, VTParameter parameter2) = 0;
virtual void MoveCursor(VTInt row, VTInt col) = 0;
virtual void FocusChanged(bool focused) = 0;
};
}
224 changes: 112 additions & 112 deletions src/terminal/adapter/ITermDispatch.hpp

Large diffs are not rendered by default.

39 changes: 12 additions & 27 deletions src/terminal/adapter/InteractDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,10 @@ InteractDispatch::InteractDispatch() :
// to be read by the client.
// Arguments:
// - inputEvents: a collection of IInputEvents
// Return Value:
// - True.
bool InteractDispatch::WriteInput(const std::span<const INPUT_RECORD>& inputEvents)
void InteractDispatch::WriteInput(const std::span<const INPUT_RECORD>& inputEvents)
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.GetActiveInputBuffer()->Write(inputEvents);
return true;
}

// Method Description:
Expand All @@ -51,19 +48,16 @@ bool InteractDispatch::WriteInput(const std::span<const INPUT_RECORD>& inputEven
// client application.
// Arguments:
// - event: The key to send to the host.
bool InteractDispatch::WriteCtrlKey(const INPUT_RECORD& event)
void InteractDispatch::WriteCtrlKey(const INPUT_RECORD& event)
{
HandleGenericKeyEvent(event, false);
return true;
}

// Method Description:
// - Writes a string of input to the host.
// Arguments:
// - string : a string to write to the console.
// Return Value:
// - True.
bool InteractDispatch::WriteString(const std::wstring_view string)
void InteractDispatch::WriteString(const std::wstring_view string)
{
if (!string.empty())
{
Expand All @@ -77,7 +71,6 @@ bool InteractDispatch::WriteString(const std::wstring_view string)

WriteInput(keyEvents);
}
return true;
}

//Method Description:
Expand All @@ -90,9 +83,7 @@ bool InteractDispatch::WriteString(const std::wstring_view string)
// - function - An identifier of the WindowManipulation function to perform
// - parameter1 - The first optional parameter for the function
// - parameter2 - The second optional parameter for the function
// Return value:
// True if handled successfully. False otherwise.
bool InteractDispatch::WindowManipulation(const DispatchTypes::WindowManipulationType function,
void InteractDispatch::WindowManipulation(const DispatchTypes::WindowManipulationType function,
const VTParameter parameter1,
const VTParameter parameter2)
{
Expand All @@ -103,20 +94,20 @@ bool InteractDispatch::WindowManipulation(const DispatchTypes::WindowManipulatio
{
case DispatchTypes::WindowManipulationType::DeIconifyWindow:
_api.ShowWindow(true);
return true;
break;
case DispatchTypes::WindowManipulationType::IconifyWindow:
_api.ShowWindow(false);
return true;
break;
case DispatchTypes::WindowManipulationType::RefreshWindow:
_api.GetBufferAndViewport().buffer.TriggerRedrawAll();
return true;
break;
case DispatchTypes::WindowManipulationType::ResizeWindowInCharacters:
// TODO:GH#1765 We should introduce a better `ResizeConpty` function to
// ConhostInternalGetSet, that specifically handles a conpty resize.
_api.ResizeWindow(parameter2.value_or(0), parameter1.value_or(0));
return true;
break;
default:
return false;
break;
}
}

Expand All @@ -126,9 +117,7 @@ bool InteractDispatch::WindowManipulation(const DispatchTypes::WindowManipulatio
//Arguments:
// - row: The row to move the cursor to.
// - col: The column to move the cursor to.
// Return value:
// - True.
bool InteractDispatch::MoveCursor(const VTInt row, const VTInt col)
void InteractDispatch::MoveCursor(const VTInt row, const VTInt col)
{
// First retrieve some information about the buffer
const auto viewport = _api.GetBufferAndViewport().viewport;
Expand All @@ -142,7 +131,7 @@ bool InteractDispatch::MoveCursor(const VTInt row, const VTInt col)
// Finally, attempt to set the adjusted cursor position back into the console.
const auto api = gsl::not_null{ ServiceLocator::LocateGlobals().api };
auto& info = ServiceLocator::LocateGlobals().getConsoleInformation().GetActiveOutputBuffer();
return SUCCEEDED(api->SetConsoleCursorPositionImpl(info, coordCursor));
LOG_IF_FAILED(api->SetConsoleCursorPositionImpl(info, coordCursor));
}

// Routine Description:
Expand All @@ -164,9 +153,7 @@ bool InteractDispatch::IsVtInputEnabled() const
// - Used to call ConsoleControl(ConsoleSetForeground,...).
// Arguments:
// - focused: if the terminal is now focused
// Return Value:
// - true always.
bool InteractDispatch::FocusChanged(const bool focused) const
void InteractDispatch::FocusChanged(const bool focused)
{
auto& g = ServiceLocator::LocateGlobals();
auto& gci = g.getConsoleInformation();
Expand Down Expand Up @@ -231,6 +218,4 @@ bool InteractDispatch::FocusChanged(const bool focused) const
gci.pInputBuffer->WriteFocusEvent(focused);
}
// Does nothing outside of ConPTY. If there's a real HWND, then the HWND is solely in charge.

return true;
}
15 changes: 6 additions & 9 deletions src/terminal/adapter/InteractDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,14 @@ namespace Microsoft::Console::VirtualTerminal
public:
InteractDispatch();

bool WriteInput(const std::span<const INPUT_RECORD>& inputEvents) override;
bool WriteCtrlKey(const INPUT_RECORD& event) override;
bool WriteString(const std::wstring_view string) override;
bool WindowManipulation(const DispatchTypes::WindowManipulationType function,
const VTParameter parameter1,
const VTParameter parameter2) override; // DTTERM_WindowManipulation
bool MoveCursor(const VTInt row, const VTInt col) override;

bool IsVtInputEnabled() const override;

bool FocusChanged(const bool focused) const override;
void WriteInput(const std::span<const INPUT_RECORD>& inputEvents) override;
void WriteCtrlKey(const INPUT_RECORD& event) override;
void WriteString(std::wstring_view string) override;
void WindowManipulation(DispatchTypes::WindowManipulationType function, VTParameter parameter1, VTParameter parameter2) override;
void MoveCursor(VTInt row, VTInt col) override;
void FocusChanged(bool focused) override;

private:
ConhostInternalGetSet _api;
Expand Down
Loading

0 comments on commit 6dd9c46

Please sign in to comment.