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.

Sign up for GitHub

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

Jump to bottom

Make sure RIS re-enables win32 input and focus events #15476

Merged
merged 4 commits into from Jun 2, 2023
Merged

Make sure RIS re-enables win32 input and focus events #15476

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3f9c06
Make sure RIS re-enables win32 input and focus events
j4james May 29, 2023
e5dce63
Remove the unneeded --win32input option.
j4james May 29, 2023
40e09cd
Don't worry about invalid args for now.
j4james Jun 1, 2023
f5ccf12
Remove any usage of the --win32input option.
j4james Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp
View file
Open in desktop
Expand Up @@ -1200,7 +1200,9 @@ void ConptyRoundtripTests::PassthroughHardReset()
}

// Write a Hard Reset VT sequence to the host, it should come through to the Terminal
// along with a DECSET sequence to re-enable win32 input and focus events.
expectedOutput.push_back("\033c");
expectedOutput.push_back("\033[?9001;1004h");
hostSm.ProcessString(L"\033c");

const auto termSecondView = term->GetViewport();
Expand Down
12 changes: 3 additions & 9 deletions src/host/ConsoleArguments.cpp
View file
Open in desktop
Expand Up @@ -20,7 +20,6 @@ const std::wstring_view ConsoleArguments::WIDTH_ARG = L"--width";
const std::wstring_view ConsoleArguments::HEIGHT_ARG = L"--height";
const std::wstring_view ConsoleArguments::INHERIT_CURSOR_ARG = L"--inheritcursor";
const std::wstring_view ConsoleArguments::RESIZE_QUIRK = L"--resizeQuirk";
const std::wstring_view ConsoleArguments::WIN32_INPUT_MODE = L"--win32input";
const std::wstring_view ConsoleArguments::FEATURE_ARG = L"--feature";
const std::wstring_view ConsoleArguments::FEATURE_PTY_ARG = L"pty";
const std::wstring_view ConsoleArguments::COM_SERVER_ARG = L"-Embedding";
Expand Down Expand Up @@ -515,9 +514,10 @@ void ConsoleArguments::s_ConsumeArg(_Inout_ std::vector<std::wstring>& args, _In
s_ConsumeArg(args, i);
hr = S_OK;
}
else if (arg == WIN32_INPUT_MODE)
else if (std::regex_match(arg, std::wregex(L"--[A-Za-z0-9]+")))
{
_win32InputMode = true;
// Any other alphanumeric sequence starting with -- is likely
// an unrecognized argument that we need to ignore.
s_ConsumeArg(args, i);
hr = S_OK;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if you'll be happy with using a regex here, if that's going to add bloat to conhost. It looks to me like we're already using regex searches in the TextBuffer, but I'm not sure if that actually gets linked into conhost.

Worst case, we could make this only ignore the defunct --win32input option, but I thought it would be nice to have it dropping all the unsupported args, since there was already a TODO for that a few lines down from here.

Copy link
Member

@lhecker lhecker May 30, 2023 •
edited

Choose a reason for hiding this comment

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

It's not linked into conhost as far as I know. I personally would like to replace it with ICU in the future so that we can get proper Unicode aware regex queries. ICU is also significantly faster, like in the order 10-100x. It can go through the entire 120x9001 text buffer full of enwik8 in <3ms with a worst-case query like e (or some other common ASCII character).

Is there any reason to not just check if the arg starts with -- and then ignore it? What other arguments could start with -- and not be followed by a-z0-9?

Copy link
Member

Choose a reason for hiding this comment

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

This, I am somewhat worried about. We go from explicitly rejecting unknowns to explicitly accepting them...

Since conpty.dll and openconsole.exe ship in lock-step, I would be comfortable just yanking all support for the argument and the special flag and everything. :)

After all . . . it never made it up to our public API surface on Windows!

Copy link
Member

Choose a reason for hiding this comment

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

This got me thinking, and I'd be interested in what you and Leonard and @zadjii-msft think.

This is speculative and not for this PR.

"In the future", we might want to add more types of conpty flags. Maybe. I don't love it, but maybe we will.

Mike and Michael used to prefer --real --arguments that set flags, rather than --flags 0xABCD to bulk enable all of 0xABCD. The argument was that it was easier spot in a debugger or a process list.

I still think there's value in having --flags 0xABCD, but this PR made me realize that maybe what we actually want is --flags.required 0x0001 --flags.optional 0x0002. Conforming versions of conhost can say, "I don't support flag 0x0001" if 0x0001 is too new for their blood... but they can ignore 0x0002 if they don't know what it is.

Is that YAGNI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there any reason to not just check if the arg starts with -- and then ignore it? What other arguments could start with -- and not be followed by a-z0-9?

That was actually my first attempt, but it caused this test case to fail:

terminal/src/host/ut_host/ConsoleArgumentsTests.cpp

Line 113 in c9e993a

commandline = L"conhost.exe --headless \"--vtmode bar this is the commandline\"";

So then I thought I should maybe also check that the arg contained no spaces, but that still caused this test to fail:

terminal/src/host/ut_host/ConsoleArgumentsTests.cpp

Line 182 in c9e993a

commandline = L"conhost.exe --headless\\ foo\\ --outpipe\\ bar\\ this\\ is\\ the\\ commandline";

And that's when I figured I'd need to go with a regex.

But maybe those test cases aren't really something we need to support. I don't know.

Copy link
Member

Choose a reason for hiding this comment

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

But I need to ask again, are we sure this won't break any third-party terminals?

Fffffair, and sorry I didn't answer it at first. My official stance is, "we haven't built the conpty API for use without conpty.dll"; if somebody is invoking it directly and passing their own pipes, they are allowed to keep the pieces when it breaks.

The command line args are effectively an internal interface. Technically. I know somebody's caught us out for breaking them before, but that's because they were setting "conhost --pty" as their debugger to stop certain Windows processes from starting . . .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, that's just me not understanding how it works - that seems reasonable. But do I keep the PSEUDOCONSOLE_WIN32_INPUT_MODE flag, and just not do anything with it, or should that be nuked as well?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it would be fine for you to remove that flag in its entirety. :)

Copy link
Member

Choose a reason for hiding this comment

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

That's the other half of the equation. We haven't made a publicly supported API-stable release of ConPTY's new undocked interface, so anybody using it is taking on 0.x version level support. I'm happy to be the one with whom the buck stops if they come to ask!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is done now. I've dropped the regex test for the invalid args, and removed all the --win32input command line parameters as well as the PSEUDOCONSOLE flag. Everything still appears to work.

Expand All @@ -528,8 +528,6 @@ void ConsoleArguments::s_ConsumeArg(_Inout_ std::vector<std::wstring>& args, _In
break;
}
// TODO: handle the rest of the possible params (MSFT:13271366, MSFT:13631640)
// TODO: handle invalid args
// e.g. "conhost --foo bar" should not make the clientCommandline "--foo bar"
else
{
// If we encounter something that doesn't match one of our other
Expand Down Expand Up @@ -677,10 +675,6 @@ bool ConsoleArguments::IsResizeQuirkEnabled() const
{
return _resizeQuirk;
}
bool ConsoleArguments::IsWin32InputModeEnabled() const
{
return _win32InputMode;
}

#ifdef UNIT_TESTING
// Method Description:
Expand Down
3 changes: 0 additions & 3 deletions src/host/ConsoleArguments.hpp
View file
Open in desktop
Expand Up @@ -55,7 +55,6 @@ class ConsoleArguments
short GetHeight() const;
bool GetInheritCursor() const;
bool IsResizeQuirkEnabled() const;
bool IsWin32InputModeEnabled() const;

#ifdef UNIT_TESTING
void EnableConptyModeForTests();
Expand All @@ -74,7 +73,6 @@ class ConsoleArguments
static const std::wstring_view HEIGHT_ARG;
static const std::wstring_view INHERIT_CURSOR_ARG;
static const std::wstring_view RESIZE_QUIRK;
static const std::wstring_view WIN32_INPUT_MODE;
static const std::wstring_view FEATURE_ARG;
static const std::wstring_view FEATURE_PTY_ARG;
static const std::wstring_view COM_SERVER_ARG;
Expand Down Expand Up @@ -144,7 +142,6 @@ class ConsoleArguments
DWORD _signalHandle;
bool _inheritCursor;
bool _resizeQuirk{ false };
bool _win32InputMode{ false };

[[nodiscard]] HRESULT _GetClientCommandline(_Inout_ std::vector<std::wstring>& args,
const size_t index,
Expand Down
6 changes: 1 addition & 5 deletions src/host/VtIo.cpp
View file
Open in desktop
Expand Up @@ -71,7 +71,6 @@ VtIo::VtIo() :
{
_lookingForCursorPosition = pArgs->GetInheritCursor();
_resizeQuirk = pArgs->IsResizeQuirkEnabled();
_win32InputMode = pArgs->IsWin32InputModeEnabled();
_passthroughMode = pArgs->IsPassthroughMode();

// If we were already given VT handles, set up the VT IO engine to use those.
Expand Down Expand Up @@ -269,10 +268,7 @@ bool VtIo::IsUsingVt() const
// win32-input-mode from them. This will enable the connected terminal to
// send us full INPUT_RECORDs as input. If the terminal doesn't understand
// this sequence, it'll just ignore it.
if (_win32InputMode)
{
LOG_IF_FAILED(_pVtRenderEngine->RequestWin32Input());
}
LOG_IF_FAILED(_pVtRenderEngine->RequestWin32Input());

// MSFT: 15813316
// If the terminal application wants us to inherit the cursor position,
Expand Down
1 change: 0 additions & 1 deletion src/host/VtIo.hpp
View file
Open in desktop
Expand Up @@ -67,7 +67,6 @@ namespace Microsoft::Console::VirtualTerminal
bool _lookingForCursorPosition;

bool _resizeQuirk{ false };
bool _win32InputMode{ false };
bool _passthroughMode{ false };
bool _closeEventSent{ false };

Expand Down
3 changes: 3 additions & 0 deletions src/renderer/vt/state.cpp
View file
Open in desktop
Expand Up @@ -541,6 +541,9 @@ void VtEngine::SetTerminalCursorTextPosition(const til::point cursor) noexcept
// - S_OK if we succeeded, else an appropriate HRESULT for failing to allocate or write.
HRESULT VtEngine::RequestWin32Input() noexcept
{
// It's important that any additional modes set here are also mirrored in
// the AdaptDispatch::HardReset method, since that needs to re-enable them
// in the connected terminal after passing through an RIS sequence.
RETURN_IF_FAILED(_RequestWin32Input());
RETURN_IF_FAILED(_RequestFocusEventMode());
RETURN_IF_FAILED(_Flush());
Expand Down
20 changes: 15 additions & 5 deletions src/terminal/adapter/adaptDispatch.cpp
View file
Open in desktop
Expand Up @@ -3055,11 +3055,21 @@ bool AdaptDispatch::HardReset()
_macroBuffer = nullptr;
}

// GH#2715 - If all this succeeded, but we're in a conpty, return `false` to
// make the state machine propagate this RIS sequence to the connected
// terminal application. We've reset our state, but the connected terminal
// might need to do more.
return !_api.IsConsolePty();
// If we're in a conpty, we need flush this RIS sequence to the connected
// terminal application, but we also need to follow that up with a DECSET
// sequence to re-enable the modes that we require (namely win32 input mode
// and focus event mode). It's important that this is kept in sync with the
// VtEngine::RequestWin32Input method which requests the modes on startup.
if (_api.IsConsolePty())
{
auto& stateMachine = _api.GetStateMachine();
if (stateMachine.FlushToTerminal())
{
auto& engine = stateMachine.Engine();
engine.ActionPassThroughString(L"\033[?9001;1004h");
}
}
return true;
}

// Routine Description:
Expand Down