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

Allow windows created by console apps to appear above the Terminal #12799

Merged
merged 12 commits into from
Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
21 changes: 21 additions & 0 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1696,6 +1696,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
}

// Method Description:
// - When the control gains focus, it needs to tell ConPTY about this.
// Usually, these sequences are reserved for applications that
// specifically request SET_FOCUS_EVENT_MOUSE, ?1004h. ConPTY uses this
// sequence REGARDLESS to communicate if the control was focused or not.
// - Even if a client application disables this mode, the Terminal & conpty
// should always request this from the hosting terminal (and just ignore
// internally to ConPTY).
// - Full support for this sequence is tracked in GH#11682.
// - This is related to work done for GH#2988.
void ControlCore::GotFocus()
{
_connection.WriteInput(L"\x1b[I");
}

// See GotFocus.
void ControlCore::LostFocus()
{
_connection.WriteInput(L"\x1b[O");
}

bool ControlCore::_isBackgroundTransparent()
{
// If we're:
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void PasteText(const winrt::hstring& hstr);
bool CopySelectionToClipboard(bool singleLine, const Windows::Foundation::IReference<CopyFormat>& formats);

void GotFocus();
void LostFocus();

void ToggleShaderEffects();
void AdjustOpacity(const double adjustment);
void ResumeRendering();
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalControl/ControlInteractivity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
THROW_IF_FAILED(_uiaEngine->Enable());
}

_core->GotFocus();

_updateSystemParameterSettings();
}

Expand All @@ -120,6 +122,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
THROW_IF_FAILED(_uiaEngine->Disable());
}

_core->LostFocus();
}

// Method Description
Expand Down
4 changes: 3 additions & 1 deletion src/host/CursorBlinker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo) const noexcept
auto& cursor = buffer.GetCursor();
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto* const pAccessibilityNotifier = ServiceLocator::LocateAccessibilityNotifier();
const bool inConpty{ gci.IsInVtIoMode() };

if (!WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS))
// GH#2988: ConPTY can now be focused, but it doesn't need to do any of this work either.
if (inConpty || !WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS))
Copy link
Member

Choose a reason for hiding this comment

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

Plz file a followup to make sure that this timer doesn't even get scheduled? For every hidden OpenConsole window, we are burning a timer every 250-500ms to... check some booleans that will never be true.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • CONSOLE_INFORMATION creates _blinker as a stack member
  • CursorBlinker ctor does CreateThreadpoolTimer
  • We don't start the timer till the call to SetCaretTimer
  • SetCaretTimer is called in two places:
    • CursorBlinker::FocusStart, which is only called in WM_SETFOCUS in wndproc.cpp, so that's never hit for conpty
    • CursorBlinker::SettingsChanged, which is conveniently only called WM_SETTINGCHANGE. So also never for conpty.

It's probably coincidence that the blinker was never started for conpty, but I'm not terribly afraid of this regressing any time soon

{
goto DoScroll;
}
Expand Down
17 changes: 16 additions & 1 deletion src/server/IoDispatchers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,22 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API
return pReceiveMsg;
}

gci.ProcessHandleList.ModifyConsoleProcessFocus(WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS));
// For future code archeologists: GH#2988
//
// Here, the console calls ConsoleControl(ConsoleSetForeground,...) with a
// flag depending on if the console is focused or not. This is surprisingly
// load bearing. This allows windows spawned by console processes to bring
// themselves to the foreground _when the console is focused_.
// (Historically, this is also called in the WndProc, when focus changes).
//
// Notably, before 2022, ConPTY was _never_ focused, so windows could never
// bring themselves to the foreground when run from a ConPTY console. We're
// not blanket granting the SetForeground right to all console apps when run
// in ConPTY. It's the responsibility of the hosting terminal emulator to
// always tell ConPTY when a particular instance is focused.
const bool hasFocus{ WI_IsFlagSet(gci.Flags, CONSOLE_HAS_FOCUS) };
const auto grantSetForeground{ hasFocus };
gci.ProcessHandleList.ModifyConsoleProcessFocus(grantSetForeground);
Comment on lines +472 to +474
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change needed? It's equivalent to the previous code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I suppose the code change bits aren't needed anymore. They were artifacts of debugging.

Copy link
Member

Choose a reason for hiding this comment

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

should we revert it and leave the comment?


// Create the handles.

Expand Down
2 changes: 2 additions & 0 deletions src/terminal/adapter/IInteractDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,7 @@ namespace Microsoft::Console::VirtualTerminal
const size_t col) = 0;

virtual bool IsVtInputEnabled() const = 0;

virtual bool FocusChanged(const bool focused) const = 0;
};
}
29 changes: 29 additions & 0 deletions src/terminal/adapter/InteractDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,3 +181,32 @@ bool InteractDispatch::IsVtInputEnabled() const
{
return _pConApi->IsVtInputEnabled();
}

// Method Description:
// - Inform the console that the window is focused. This is used by ConPTY.
// Terminals can send ConPTY a FocusIn/FocusOut sequence on the input pipe,
// which will end up here. This will update the console's internal tracker if
// it's focused or not, as to match the end-terminal's state.
// - Used to call ConsoleControl(ConsoleSetForeground,...).
// - Full support for this sequence is tracked in GH#11682.
// Arguments:
// - focused: if the terminal is now focused
// Return Value:
// - true always.
bool InteractDispatch::FocusChanged(const bool focused) const
{
// When we do GH#11682, we should make sure that ConPTY requests this mode
// from the terminal when it starts up, and ConPTY never unsets that flag.
// It should only ever internally disable the events from flowing to the
// client application.

auto& g = ServiceLocator::LocateGlobals();
auto& gci = g.getConsoleInformation();
WI_UpdateFlag(gci.Flags, CONSOLE_HAS_FOCUS, focused);
gci.ProcessHandleList.ModifyConsoleProcessFocus(focused);

// Theoretically, this could be propagated as a focus event as well, to the
// input buffer. That should be considered when implementing GH#11682.

return true;
}
2 changes: 2 additions & 0 deletions src/terminal/adapter/InteractDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ namespace Microsoft::Console::VirtualTerminal

bool IsVtInputEnabled() const override;

bool FocusChanged(const bool focused) const override;

private:
std::unique_ptr<ConGetSet> _pConApi;
};
Expand Down
13 changes: 12 additions & 1 deletion src/terminal/parser/InputStateMachineEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -362,9 +362,14 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter
// win32-input-mode, and if they did, then we'll just translate the
// INPUT_RECORD back to the same sequence we say here later on, when the
// client reads it.
//
// Focus events in conpty are special, so don't flush those through either.
// See GH#12799, GH#12900 for details
if (_pDispatch->IsVtInputEnabled() &&
_pfnFlushToInputQueue &&
id != CsiActionCodes::Win32KeyboardInput)
id != CsiActionCodes::Win32KeyboardInput &&
id != CsiActionCodes::FocusIn &&
id != CsiActionCodes::FocusOut)
{
return _pfnFlushToInputQueue();
}
Expand Down Expand Up @@ -426,6 +431,12 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter
case CsiActionCodes::DTTERM_WindowManipulation:
success = _pDispatch->WindowManipulation(parameters.at(0), parameters.at(1), parameters.at(2));
break;
case CsiActionCodes::FocusIn:
success = _pDispatch->FocusChanged(true);
break;
case CsiActionCodes::FocusOut:
success = _pDispatch->FocusChanged(false);
break;
case CsiActionCodes::Win32KeyboardInput:
{
// Use WriteCtrlKey here, even for keys that _aren't_ control keys,
Expand Down
2 changes: 2 additions & 0 deletions src/terminal/parser/InputStateMachineEngine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ namespace Microsoft::Console::VirtualTerminal
ArrowLeft = VTID("D"),
Home = VTID("H"),
End = VTID("F"),
FocusIn = VTID("I"),
FocusOut = VTID("O"),
MouseDown = VTID("<M"),
MouseUp = VTID("<m"),
Generic = VTID("~"), // Used for a whole bunch of possible keys
Expand Down
7 changes: 7 additions & 0 deletions src/terminal/parser/ut_parser/InputEngineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,8 @@ class Microsoft::Console::VirtualTerminal::TestInteractDispatch final : public I

virtual bool IsVtInputEnabled() const override;

virtual bool FocusChanged(const bool focused) const override;

private:
std::function<void(std::deque<std::unique_ptr<IInputEvent>>&)> _pfnWriteInputCallback;
TestState* _testState; // non-ownership pointer
Expand Down Expand Up @@ -399,6 +401,11 @@ bool TestInteractDispatch::IsVtInputEnabled() const
return true;
}

bool TestInteractDispatch::FocusChanged(const bool /*focused*/) const
{
return false;
}

void InputEngineTest::C0Test()
{
auto pfn = std::bind(&TestState::TestInputCallback, &testState, std::placeholders::_1);
Expand Down