Consolidate the interfaces for setting VT input modes #11384

j4james · 2021-09-30T17:45:33Z

Instead of having a separate method for setting each mouse and keyboard
mode, this PR consolidates them all into a single method which takes a
mode parameter, and stores the modes in a til::enumset rather than
having a separate bool for each mode.

This enables us to get rid of a lot of boilerplate code, and makes the
code easier to extend when we want to introduce additional modes in the
future. It'll also makes it easier to read back the state of the various
modes when implementing the DECRQM query.

Most of the complication is in the TerminalInput class, which had to
be adjusted to work with an enumset in place of all the bool fields.
For the rest, it was largely a matter of replacing calls to all the old
mode setting methods with the new SetInputMode method, and deleting a
bunch of unused code.

One thing worth mentioning is that the AdaptDispatch implementation
used to have a _ShouldPassThroughInputModeChange method that was
called after every mode change. This code has now been moved up into the
SetInputMode implementation in ConhostInternalGetSet so it's just
handled in one place. Keeping this out of the dispatch class will also
be beneficial for sharing the implementation with TerminalDispatch.

Validation

The updated interface necessitated some adjustments to the tests in
AdapterTest and MouseInputTest, but the essential structure of the
tests remains unchanged, and everything still passes.

I've also tested the keyboard and mouse modes in Vttest and confirmed
they still work at least as well as they did before (both conhost and
Windows Terminal), and I tested the alternate scroll mode manually
(conhost only).

Simplifying the ConGetSet and ITerminalApi is also part of the plan
to de-duplicate the AdaptDispatch and TerminalDispatch
implementation (#3849).

lhecker

lhecker · 2021-10-01T22:40:09Z

src/host/outputStream.cpp

+    auto& terminalInput = _io.GetActiveInputBuffer()->GetTerminalInput();
+    terminalInput.SetInputMode(mode, enabled);


Why did the old code use ServiceLocator::LocateGlobals() for this?
I'm assuming that was just some code cruft / accidental roundaboutness?

Well the original code was calling through to static functions in getset.cpp which aren't part of the ConhostInternalGetSet class, and so the only way they can access that state was through ServiceLocator::LocateGlobals. I think this was originally because the code in getset.cpp was mostly implementations of the public console APIs, and the same pattern was followed when private functions were added. This has resulted in a fair amount of unnecessary boilerplate code, which I'm hoping we can cut down on over time.

"It seemed like a good idea at the time" is the only excuse I have for this...

boilerplate
cut down over time

YES PLEASE

miniksa

Awesome stuff. I love it.

DHowett

Minor questions -- I'm not blocking signoff/merge if Michael wants to, but am generally curious. Excellent work as always!

DHowett · 2021-10-26T20:13:32Z

src/host/outputStream.cpp

+    auto& terminalInput = _io.GetActiveInputBuffer()->GetTerminalInput();
+    terminalInput.SetInputMode(mode, enabled);


boilerplate
cut down over time

YES PLEASE

DHowett · 2021-10-26T20:17:20Z

src/terminal/input/mouseInput.cpp

                }
-
+                else


This change will make us respond to a request for URXVT encoding by emitting Standard/X11, right?

I'm not sure - since that's not in the Mode enum, we might just always return false before we even get here. I don't think we ever actually supported that mode - I probably put it in the enum pre-emptively.

Yeah, we never supported the ability to select Urxvt mode, so nothing has changed here. If you try to select that mode, it'll be ignored as an unrecognised mode in the AdaptDispatch class.

DHowett · 2021-10-26T20:18:48Z

src/terminal/input/terminalInput.cpp

@@ -655,7 +662,7 @@ bool TerminalInput::HandleKey(const IInputEvent* const pInEvent)
    // Check any other key mappings (like those for the F1-F12 keys).
    // These mappings will kick in no matter which modifiers are pressed and as such
    // must be checked last, or otherwise we'd override more complex key combinations.
-    const auto mapping = _getKeyMapping(keyEvent, _ansiMode, _cursorApplicationMode, _keypadApplicationMode);
+    const auto mapping = _getKeyMapping(keyEvent, _inputMode.test(Mode::Ansi), _inputMode.test(Mode::CursorKey), _inputMode.test(Mode::Keypad));


a followup may be to consolidate the signature of _getKeyMapping to take an input mode enumset?

Yeah, that would be much better - I'm sorry I didn't spot that now. There are definitely more things I'd like to do with the input at some point, so I'm sure I'll get back to this at some point if nobody else does.

zadjii-msft

it's just... so much better 😍

ghost · 2021-10-26T21:11:59Z

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 · 2022-02-03T18:23:27Z

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

Handy links:

j4james added 6 commits September 30, 2021 15:51

Merge the TerminalInput keyboard mode methods.

c8e4b4a

Merge the ConGetSet keyboard mode methods.

04c753e

Merge the mouse encoding with the other input modes.

7aea2f6

Merge the mouse tracking with other input modes.

e577683

Merge alternate scroll with the other input modes.

a9a9875

Simplify ITerminalApi in the same way as ConGetSet.

2e32c87

lhecker approved these changes Oct 1, 2021

View reviewed changes

lhecker added the Needs-Second It's a PR that needs another sign-off label Oct 12, 2021

ghost requested review from zadjii-msft, miniksa, carlos-zamora, DHowett, leonMSFT and PankajBhojwani October 12, 2021 20:15

j4james mentioned this pull request Oct 13, 2021

Improve performance and binary size of til::enumset #11493

Merged

2 tasks

WSLUser mentioned this pull request Oct 14, 2021

Add keyboard protocol support (the kitty one) #11509

Open

miniksa approved these changes Oct 21, 2021

View reviewed changes

DHowett reviewed Oct 26, 2021

View reviewed changes

zadjii-msft approved these changes Oct 26, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 26, 2021

ghost merged commit 7b7dea0 into microsoft:main Oct 26, 2021

ghost removed the Needs-Second It's a PR that needs another sign-off label Oct 26, 2021

j4james deleted the consolidate-input-modes branch November 6, 2021 20:39

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate the interfaces for setting VT input modes #11384

Consolidate the interfaces for setting VT input modes #11384

j4james commented Sep 30, 2021 •

edited by DHowett

Loading

lhecker left a comment

lhecker Oct 1, 2021

j4james Oct 1, 2021

miniksa Oct 20, 2021

DHowett Oct 26, 2021

miniksa left a comment

DHowett left a comment

DHowett Oct 26, 2021

DHowett Oct 26, 2021

zadjii-msft Oct 26, 2021

j4james Oct 26, 2021

DHowett Oct 26, 2021

j4james Oct 26, 2021

zadjii-msft left a comment

ghost commented Oct 26, 2021

ghost commented Feb 3, 2022

		auto& terminalInput = _io.GetActiveInputBuffer()->GetTerminalInput();
		terminalInput.SetInputMode(mode, enabled);

Consolidate the interfaces for setting VT input modes #11384

Consolidate the interfaces for setting VT input modes #11384

Conversation

j4james commented Sep 30, 2021 • edited by DHowett Loading

Validation

lhecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Oct 26, 2021

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 commented Feb 3, 2022

j4james commented Sep 30, 2021 •

edited by DHowett

Loading

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.