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

Add support for the DECRQM escape sequence #14444

Merged
18 commits merged into from
Nov 30, 2022
Merged

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Nov 27, 2022

This PR adds support for the DECRQM (Request Mode) escape sequence,
which allows applications to query the state of the various modes
supported by the terminal. It also adds support for the DECNKM mode,
which aliases the existing DECKPAM and DECKPNM operations, so they
can be queried with DECRQM too.

This is one solution for #10153 (saving and restoring the state of
bracketed paste mode), and should also help with #1040 (providing a way
for clients to determine the capabilities of the terminal).

Prior to adding DECRQM, I also did some refactoring of the mode
handling to get rid of the mode setting methods in the ITermDispatch
interface that had no need to be there. Most of them were essentially a
single line of code that could easily be executed directly from the
_ModeParamsHelper handler anyway.

As part of this refactoring I combined all the internal AdaptDispatch
modes into an enumset to allow for easier management, and made sure
all modes were correctly reset in the HardReset method (prior to this,
there were a number of modes that we weren't restoring when we should
have been).

And note that there are some differences in behavior between conhost and
Windows Terminal. In conhost, DECRQM will report bracketed paste mode
as unsupported, and in Terminal, both DECCOLM and AllowDECCOLM are
reported as unsupported. And DECCOLM is now explicitly ignored in
conpty mode, to avoid the conpty client and conhost getting out of sync.

Validation Steps Performed

I've manually confirmed that all the supported modes are reported in the
DECRQM tests in Vttest, and I have my own test scripts which I've used
to confirm that RIS is now resetting the modes correctly.

I've also added a unit test in AdapterTest that iterates through the
modes, checking the responses from DECRQM for both the set and reset
states.

I should also mention that I had to do some refactoring of the existing
tests to compensate for methods that were removed from ITermDispatch,
particularly in OutputEngineTest. In many cases, though, these tests
weren't doing much more than testing the test framework.

Copy link
Collaborator Author

@j4james j4james left a comment

Choose a reason for hiding this comment

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

Note to reviewers: I think it might make things a little easier to follow if you do a side-by-side diff, and disable whitespace. There's nothing particularly complicated, but there's some overlap of methods being removed where other methods have been added, and that seems to have confused the diff.

Comment on lines 101 to +110
void Cursor::SetBlinkingAllowed(const bool fBlinkingAllowed) noexcept
{
_fBlinkingAllowed = fBlinkingAllowed;
// GH#2642 - From what we've gathered from other terminals, when blinking is
// disabled, the cursor should remain On always, and have the visibility
// controlled by the IsVisible property. So when you do a printf "\e[?12l"
// to disable blinking, the cursor stays stuck On. At this point, only the
// cursor visibility property controls whether the user can see it or not.
// (Yes, the cursor can be On and NOT Visible)
_fIsOn = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously we had to call both SetBlinkingAllowed and SetIsOn whenever we changed the blinking state, so this just lets us do that more efficiently with a single call. The comment was copied from EnableCursorBlinking method, which was one of the places we were doing that, and which has now been removed as part of the ITermDispatch cleanup.

// - True.
bool AdaptDispatch::_DoDECCOLMHelper(const VTInt columns)
// - <none>
void AdaptDispatch::_SetColumnMode(const bool enable)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is essentially just a merge of the SetColumns method (which was removed from ITermDispatch) and the _DoDECCOLMHelper method. But this also now tracks the current state of the DECOLM mode so it can be reported from DECRQM.

// Return Value:
// - <none>
void AdaptDispatch::_SetParserMode(const StateMachine::Mode mode, const bool enable)
void AdaptDispatch::_SetAlternateScreenBufferMode(const bool enable)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just a merge of UseAlternateScreenBuffer and UseMainScreenBuffer, which have now been removed from ITermDispatch.

Comment on lines +1423 to +1425
bool AdaptDispatch::_PassThroughInputModes()
{
_terminalInput.SetInputMode(mode, enable);

// If we're a conpty, AND WE'RE IN VT INPUT MODE, always pass input mode requests
// The VT Input mode check is to work around ssh.exe v7.7, which uses VT
// output, but not Input.
// The original comment said, "Once the conpty supports these types of input,
// this check can be removed. See GH#4911". Unfortunately, time has shown
// us that SSH 7.7 _also_ requests mouse input and that can have a user interface
// impact on the actual connected terminal. We can't remove this check,
// because SSH <=7.7 is out in the wild on all versions of Windows <=2004.

return !_api.IsConsolePty() || !_api.IsVtInputEnabled();
return _api.IsConsolePty() && _api.IsVtInputEnabled();
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 wanted to get rid of _SetInputMode, and just call _terminalInput.SetInputMode directly, because in many cases we don't care about the passthrough conditions, and these two api calls are wasted. But I've kept them in this helper method, so we can easily use this when we do actually care about passthrough, and also so we don't lose the comment above.

// Return Value:
// - True if handled successfully. False otherwise.
bool AdaptDispatch::EnableCursorBlinking(const bool enable)
bool AdaptDispatch::SetKeypadMode(const bool fApplicationMode)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this isn't actually new code - the diff has just got it mixed up with the newly added RequestMode method above.

Comment on lines +1697 to +1701
TEST_METHOD(TestPrivateModes)
{
auto dispatch = std::make_unique<StatefulDispatch>();
auto pDispatch = dispatch.get();
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
StateMachine mach(std::move(engine));

mach.ProcessString(L"\x1b[?7l");
VERIFY_IS_FALSE(pDispatch->_isAutoWrapEnabled);

pDispatch->ClearState();
pDispatch->_isAutoWrapEnabled = false;

mach.ProcessString(L"\x1b[?7h");
VERIFY_IS_TRUE(pDispatch->_isAutoWrapEnabled);

pDispatch->ClearState();
}

TEST_METHOD(TestCursorBlinking)
{
auto dispatch = std::make_unique<StatefulDispatch>();
auto pDispatch = dispatch.get();
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
StateMachine mach(std::move(engine));

mach.ProcessString(L"\x1b[?12h");
VERIFY_IS_TRUE(pDispatch->_cursorBlinking);

pDispatch->ClearState();

mach.ProcessString(L"\x1b[?12l");
VERIFY_IS_FALSE(pDispatch->_cursorBlinking);

pDispatch->ClearState();
}

TEST_METHOD(TestCursorVisibility)
{
auto dispatch = std::make_unique<StatefulDispatch>();
auto pDispatch = dispatch.get();
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
StateMachine mach(std::move(engine));

mach.ProcessString(L"\x1b[?25h");
VERIFY_IS_TRUE(pDispatch->_cursorVisible);

pDispatch->ClearState();

mach.ProcessString(L"\x1b[?25l");
VERIFY_IS_FALSE(pDispatch->_cursorVisible);

pDispatch->ClearState();
}

TEST_METHOD(TestAltBufferSwapping)
{
auto dispatch = std::make_unique<StatefulDispatch>();
auto pDispatch = dispatch.get();
auto engine = std::make_unique<OutputStateMachineEngine>(std::move(dispatch));
StateMachine mach(std::move(engine));

mach.ProcessString(L"\x1b[?1049h");
VERIFY_IS_TRUE(pDispatch->_isAltBuffer);

pDispatch->ClearState();

mach.ProcessString(L"\x1b[?1049h");
VERIFY_IS_TRUE(pDispatch->_isAltBuffer);
mach.ProcessString(L"\x1b[?1049h");
VERIFY_IS_TRUE(pDispatch->_isAltBuffer);

pDispatch->ClearState();

mach.ProcessString(L"\x1b[?1049l");
VERIFY_IS_FALSE(pDispatch->_isAltBuffer);

pDispatch->ClearState();

mach.ProcessString(L"\x1b[?1049h");
VERIFY_IS_TRUE(pDispatch->_isAltBuffer);
mach.ProcessString(L"\x1b[?1049l");
VERIFY_IS_FALSE(pDispatch->_isAltBuffer);

pDispatch->ClearState();
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:modeNumber", L"{1, 3, 5, 6, 7, 12, 25, 40, 1049}")
END_TEST_METHOD_PROPERTIES()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a replacement for all the individual mode testing methods which were checking whether a particular mode change would trigger a call to a one of the mode setting methods in ITermDispatch (which have now been removed). But the code path we were testing wasn't real - it was just the StatefulDispatch mock class above, so it was kind of pointless. This test now just confirms that the _ModeParamsHelper method is triggered, which is all we really care about here.

Copy link
Member

Choose a reason for hiding this comment

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

HA. Thank you!

@j4james
Copy link
Collaborator Author

j4james commented Nov 27, 2022

Btw, if you don't like the refactoring, that's not absolutely necessary for this PR. But other than the bloat reduction, I think it also helps somewhat to see what the code is actually doing when we're not hiding it behind an unnecessary method call. This is especially helper when comparing the new RequestMode method with the old _ModeParamsHelper, so you can more easily see how the request implementations correspond with the setter implementations.

@j4james j4james marked this pull request as ready for review November 27, 2022 12:48
@DHowett
Copy link
Member

DHowett commented Nov 28, 2022

(I love your refactorings. Go wild!)

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks! If we end up going the DECRQM route in #14342 (comment), I'm excited to leverage this! 😊

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
Comment on lines 2493 to -2656
auto& cursor = _api.GetTextBuffer().GetCursor();
cursor.SetType(actualType);
cursor.SetBlinkingAllowed(fEnableBlinking);
cursor.SetIsOn(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why remove SetIsOn(true)?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. You moved it to Cursor::SetBlinkingAllowed

TEST_METHOD(RequestModeTests)
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:modeNumber", L"{1, 3, 5, 6, 8, 12, 25, 40, 66, 67, 1000, 1002, 1003, 1004, 1005, 1006, 1007, 1049, 9001}")
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, I wish there was an easy way we could just iterate over ModeParams. Should we at least mention that the supported modes are stored in DispatchTypes.hpp as ModeParams?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I've added a comment there now. And thanks for catching all those typos.

src/host/outputStream.hpp Outdated Show resolved Hide resolved
src/host/outputStream.cpp Outdated Show resolved Hide resolved
src/host/outputStream.cpp Outdated Show resolved Hide resolved
@DHowett
Copy link
Member

DHowett commented Nov 29, 2022

Alas, you'll need to merge main to get at the newly-reenabled spell checker. Sorry about that!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Outstanding as always. I like the removal of abstractions that have proven unnecessary with time and effort. 😄 Thank you!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 30, 2022
@ghost
Copy link

ghost commented Nov 30, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 4379148 into microsoft:main Nov 30, 2022
@j4james j4james deleted the feature-decrqm branch December 26, 2022 20:31
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants