Skip to content

Commit

Permalink
Properly filter focus events from the console API (#15606)
Browse files Browse the repository at this point in the history
This is an improved fix for #13238. Instead of handling focus events in
the `TerminalInput::HandleKey` function and the need to filter them
out depending on where they came from, we simply don't call `HandleKey`
in the first place. This makes the somewhat unreliable `CameFromApi`
function unnecessary and the code a bit more robust.

This change is required because `CameFromApi` is not representable
in a `INPUT_RECORD` and I'm getting rid of `IInputEvent`.

## Validation Steps Performed
* No `[O` when exiting nvim ✅
* Mouse input in nvim works ✅
  • Loading branch information
lhecker authored Jun 27, 2023
1 parent a596004 commit 58e1380
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 76 deletions.
3 changes: 1 addition & 2 deletions src/host/input.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,7 @@ void HandleFocusEvent(const BOOL fSetFocus)

try
{
const auto EventsWritten = gci.pInputBuffer->Write(std::make_unique<FocusEvent>(!!fSetFocus));
FAIL_FAST_IF(EventsWritten != 1);
gci.pInputBuffer->WriteFocusEvent(fSetFocus);
}
catch (...)
{
Expand Down
50 changes: 50 additions & 0 deletions src/host/inputBuffer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,56 @@ size_t InputBuffer::Write(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& inEv
}
}

// This can be considered a "privileged" variant of Write() which allows FOCUS_EVENTs to generate focus VT sequences.
// If we didn't do this, someone could write a FOCUS_EVENT_RECORD with WriteConsoleInput, exit without flushing the
// input buffer and the next application will suddenly get a "\x1b[I" sequence in their input. See GH#13238.
void InputBuffer::WriteFocusEvent(bool focused) noexcept
{
if (IsInVirtualTerminalInputMode())
{
_termInput.HandleFocus(focused);
}
else
{
// This is a mini-version of Write().
const auto wasEmpty = _storage.empty();
_storage.push_back(std::make_unique<FocusEvent>(focused));
if (wasEmpty)
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
}
WakeUpReadersWaitingForData();
}
}

// Returns true when mouse input started. You should then capture the mouse and produce further events.
bool InputBuffer::WriteMouseEvent(til::point position, const unsigned int button, const short keyState, const short wheelDelta)
{
if (IsInVirtualTerminalInputMode())
{
// This magic flag is "documented" at https://msdn.microsoft.com/en-us/library/windows/desktop/ms646301(v=vs.85).aspx
// "If the high-order bit is 1, the key is down; otherwise, it is up."
static constexpr short KeyPressed{ gsl::narrow_cast<short>(0x8000) };

const TerminalInput::MouseButtonState state{
WI_IsFlagSet(OneCoreSafeGetKeyState(VK_LBUTTON), KeyPressed),
WI_IsFlagSet(OneCoreSafeGetKeyState(VK_MBUTTON), KeyPressed),
WI_IsFlagSet(OneCoreSafeGetKeyState(VK_RBUTTON), KeyPressed)
};

// GH#6401: VT applications should be able to receive mouse events from outside the
// terminal buffer. This is likely to happen when the user drags the cursor offscreen.
// 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)].
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
gci.GetActiveOutputBuffer().GetViewport().ToOrigin().Clamp(position);

return _termInput.HandleMouse(position, button, keyState, wheelDelta, state);
}

return false;
}

// Routine Description:
// - Coalesces input events and transfers them to storage queue.
// Arguments:
Expand Down
3 changes: 3 additions & 0 deletions src/host/inputBuffer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ class InputBuffer final : public ConsoleObjectHeader
size_t Write(_Inout_ std::unique_ptr<IInputEvent> inEvent);
size_t Write(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& inEvents);

void WriteFocusEvent(bool focused) noexcept;
bool WriteMouseEvent(til::point position, unsigned int button, short keyState, short wheelDelta);

bool IsInVirtualTerminalInputMode() const;
Microsoft::Console::VirtualTerminal::TerminalInput& GetTerminalInput();
void SetTerminalConnection(_In_ Microsoft::Console::Render::VtEngine* const pTtyConnection);
Expand Down
30 changes: 2 additions & 28 deletions src/interactivity/win32/windowio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,6 @@ using Microsoft::Console::Interactivity::ServiceLocator;
// Bit 29 is whether ALT was held when the message was posted.
#define WM_SYSKEYDOWN_ALT_PRESSED (0x20000000)

// This magic flag is "documented" at https://msdn.microsoft.com/en-us/library/windows/desktop/ms646301(v=vs.85).aspx
// "If the high-order bit is 1, the key is down; otherwise, it is up."
static constexpr short KeyPressed{ gsl::narrow_cast<short>(0x8000) };

// ----------------------------
// Helpers
// ----------------------------
Expand Down Expand Up @@ -119,30 +115,8 @@ bool HandleTerminalMouseEvent(const til::point cMousePosition,
const short sModifierKeystate,
const short sWheelDelta)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
// If the modes don't align, this is unhandled by default.
auto fWasHandled = false;

// Virtual terminal input mode
if (IsInVirtualTerminalInputMode())
{
const TerminalInput::MouseButtonState state{
WI_IsFlagSet(OneCoreSafeGetKeyState(VK_LBUTTON), KeyPressed),
WI_IsFlagSet(OneCoreSafeGetKeyState(VK_MBUTTON), KeyPressed),
WI_IsFlagSet(OneCoreSafeGetKeyState(VK_RBUTTON), KeyPressed)
};

// GH#6401: VT applications should be able to receive mouse events from outside the
// terminal buffer. This is likely to happen when the user drags the cursor offscreen.
// 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)].
auto clampedPosition{ cMousePosition };
const auto clampViewport{ gci.GetActiveOutputBuffer().GetViewport().ToOrigin() };
clampViewport.Clamp(clampedPosition);
fWasHandled = gci.GetActiveInputBuffer()->GetTerminalInput().HandleMouse(clampedPosition, uiButton, sModifierKeystate, sWheelDelta, state);
}

return fWasHandled;
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
return gci.pInputBuffer->WriteMouseEvent(cMousePosition, uiButton, sModifierKeystate, sWheelDelta);
}

void HandleKeyEvent(const HWND hWnd,
Expand Down
2 changes: 1 addition & 1 deletion src/terminal/adapter/InteractDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ bool InteractDispatch::FocusChanged(const bool focused) const

WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, shouldActuallyFocus);
gci.ProcessHandleList.ModifyConsoleProcessFocus(shouldActuallyFocus);
gci.pInputBuffer->Write(std::make_unique<FocusEvent>(focused));
gci.pInputBuffer->WriteFocusEvent(focused);
}
// Does nothing outside of ConPTY. If there's a real HWND, then the HWND is solely in charge.

Expand Down
26 changes: 8 additions & 18 deletions src/terminal/adapter/ut_adapter/inputTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -328,14 +328,9 @@ void InputTest::TestFocusEvents()
auto inputEvent = IInputEvent::Create(irTest);
VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT from API was NOT handled.");
}
{
auto inputEvent = std::make_unique<FocusEvent>(false);
VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was NOT handled.");
}
{
auto inputEvent = std::make_unique<FocusEvent>(true);
VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was NOT handled.");
}

VERIFY_ARE_EQUAL(false, pInput->HandleFocus(false), L"Verify FocusEvent from any other source was NOT handled.");
VERIFY_ARE_EQUAL(false, pInput->HandleFocus(true), L"Verify FocusEvent from any other source was NOT handled.");

Log::Comment(L"Enable focus event handling");

Expand All @@ -351,16 +346,11 @@ void InputTest::TestFocusEvents()
auto inputEvent = IInputEvent::Create(irTest);
VERIFY_ARE_EQUAL(false, pInput->HandleKey(inputEvent.get()), L"Verify FOCUS_EVENT from API was NOT handled.");
}
{
s_expectedInput = L"\x1b[O";
auto inputEvent = std::make_unique<FocusEvent>(false);
VERIFY_ARE_EQUAL(true, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was handled.");
}
{
s_expectedInput = L"\x1b[I";
auto inputEvent = std::make_unique<FocusEvent>(true);
VERIFY_ARE_EQUAL(true, pInput->HandleKey(inputEvent.get()), L"Verify FocusEvent from any other source was handled.");
}

s_expectedInput = L"\x1b[O";
VERIFY_ARE_EQUAL(true, pInput->HandleFocus(false), L"Verify FocusEvent from any other source was handled.");
s_expectedInput = L"\x1b[I";
VERIFY_ARE_EQUAL(true, pInput->HandleFocus(true), L"Verify FocusEvent from any other source was handled.");
}

void InputTest::TerminalInputModifierKeyTests()
Expand Down
16 changes: 0 additions & 16 deletions src/terminal/input/terminalInput.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -523,22 +523,6 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)
return false;
}

// GH#11682: If this was a focus event, we can handle this. Steal the
// focused state, and return true if we're actually in focus event mode.
if (pInEvent->EventType() == InputEventType::FocusEvent)
{
const auto& focusEvent = *static_cast<const FocusEvent* const>(pInEvent);

// BODGY
// GH#13238 - Filter out focus events that came from the API.
if (focusEvent.CameFromApi())
{
return false;
}

return HandleFocus(focusEvent.GetFocus());
}

// On key presses, prepare to translate to VT compatible sequences
if (pInEvent->EventType() != InputEventType::KeyEvent)
{
Expand Down
13 changes: 2 additions & 11 deletions src/types/inc/IInputEvent.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -515,14 +515,12 @@ class FocusEvent : public IInputEvent
{
public:
constexpr FocusEvent(const FOCUS_EVENT_RECORD& record) :
_focus{ !!record.bSetFocus },
_cameFromApi{ true }
_focus{ !!record.bSetFocus }
{
}

constexpr FocusEvent(const bool focus) :
_focus{ focus },
_cameFromApi{ false }
_focus{ focus }
{
}

Expand All @@ -542,15 +540,8 @@ class FocusEvent : public IInputEvent

void SetFocus(const bool focus) noexcept;

// BODGY - see FocusEvent.cpp for details.
constexpr bool CameFromApi() const noexcept
{
return _cameFromApi;
}

private:
bool _focus;
bool _cameFromApi;

#ifdef UNIT_TESTING
friend std::wostream& operator<<(std::wostream& stream, const FocusEvent* const pFocusEvent);
Expand Down

0 comments on commit 58e1380

Please sign in to comment.