7395: do not clear text selection upon PrintScreen

[Don-Vito]

Summary of the Pull Request

When handling SendKey, preserve selection upon PrintScreen (VK_SNAPSHOT)

References

PR Checklist

• Closes PrintScreen keyboard key removes current text selection #7395
• CLA signed.
• Tests added/passed
• Documentation updated.
• Schema updated.
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

The explicitly ignores the VK_SNAPSHOT. Probably, additional keys should not alter the selection.

Validation Steps Performed

• Manually, took snapshot with content selected and checked that the selection is not changed. Verified that the snapshot is taken (and even contains the selection 😄)
• Manually, checked that the old behavior is preserved for modifier and non-modifier keys

[Don-Vito]

After looking for several hours at the area I believe that this one is merely a patch, and the area requires some sort of refactor. For instance, the media buttons remove the selection as well - in addition to scrolling to the bottom (as reported in #5366).

In #5366, @DHowett-MSFT mentioned that to prevent the scrolling behavior we need to run it only upon input. The same should probably apply to clearing text selection as well (besides few cases, like vk_escape, where we will want to clear the selection, even without passing a sequence to connection).

I would like to work on the refactor that will probably cleanup the behavior for Terminal updates (e.g., scrolling to bottom, clearing selection) upon different kinds of events (e.g., clipboard, key), but I would like to align on the vision beforehands.

Currently the logic is somewhat scattered between the Terminal and TermControl, e.g.:

• TrySnapOnInput is invoked in TermControl upon clipboard event, but in Terminal upon key event
• Some keys are handled in Terminal::SendKeyEvent, some in TermControl::_TrySendKey

My suggestion is:

• Move all input related logic (including mouse, clipboard, keys, etc.) from Terminal to TermControl
• TermControl will hold TerminalInput, Terminal and Connection
• TermContol will work directly with TerminalInput (probably the callback mechanism can be replaced with a call)

E.g., upon key event:

• TermControl will get the event
• TermControl will apply the business logic (e.g., understanding that clear selection is required) and invoke relevant methods in Terminal to update the UI
• TermControl will invoke TerminalInput to obtain the correct sequence
• TermControll will send the sequence to Connection.

WDYT?

[DHowett]

I'd caution you against too extensive of refactoring that moves logic into TermControl today. Not because I don't think it's the right thing to do, but because there's actually two TermControls (the one in WpfTerminalControl and the one that's used in Cascadia/Terminal.)[1] @lhecker and @zadjii-msft have worked extensively in this area and can give more correct or fine-grained guidance. 😄

[1]: I have a workitem in the backlog that suggests we should unify the interactivity layer (mouse, keyboard input, translation of pixel positions to coordinates, etc.) between the two and make it shared logic; this would likely live in TerminalCore or in another type that consumes TerminalCore.

[Don-Vito]

I'd caution you against too extensive of refactoring that moves logic into TermControl today. Not because I don't think it's the right thing to do, but because there's actually two TermControls (the one in WpfTerminalControl and the one that's used in Cascadia/Terminal.)[1] @lhecker and @zadjii-msft have worked extensively in this area and can give more correct or fine-grained guidance. 😄

[1]: I have a workitem in the backlog that suggests we should unify the interactivity layer (mouse, keyboard input, translation of pixel positions to coordinates, etc.) between the two and make it shared logic; this would likely live in TerminalCore or in another type that consumes TerminalCore.

@DHowett - thanks for the reply! I must admit I am too new in the project to understand the implications of the suggested refactoring on WpfTerminalControl. I will learn 😄

Regarding this PR, I suggest to consider it as a short term mitigation for clearing text selection upon Print Screen (which is painful).

In parallel, I have created a separate PR (#7896), that hopefully addresses this and #5366 by delaying the decision on scrolling / clearing selection until the input is sent to TermControl.

[lhecker]

@Don-Vito In my personal opinion the entire input handling should be abstracted out of the two TermControls, Terminal, as well as TerminalInput. Each of those classes has certain bits of the input handlings, until it's finally turned into a VT sequence.

If it was hoisted out of those, we could make them behave like a pipe of sorts: You put key and character events into the front and you get VT strings out of the end / returned from the pipe invocation. Each area of concern (filtering, buffering, ...) could be a seperate class, all of which share a common, abstract interface. The pipe could be constructed by simply concatenating the list of filters, mappers, etc. Since those classes are small we could theoretically also develop them one by one and integrate them directly into the existing code without constructing a "pipe".

We could then also construct accurate unit tests for each class. E.g. we could ensure that a hypothetical altGrAliasing-filter-class (for the same-named altGrAliasing-feature) works correctly, without being hindered by other logic that might obstruct our tests. No need for mocks in that case!

Alternatively we could also just keep the entire logic within a single class (i.e. merge the 4 classes listed above).
It would be a simple and acceptable approach in my eyes, even if the code might have a lot of if/else.
Regardless I'd recommend against moving the logic into the TermControl classes or into the Terminal/TerminalInput classes. Those classes are already large enough IMHO. 😄 Hoisting it into a seperate class sounds like a good idea though.

[Don-Vito]

@Don-Vito In my personal opinion the entire input handling should be abstracted out of the two TermControls, Terminal, as well as TerminalInput. Each of those classes has certain bits of the input handlings, until it's finally turned into a VT sequence.

If it was hoisted out of those, we could make them behave like a pipe of sorts: You put key and character events into the front and you get VT strings out of the end / returned from the pipe invocation. Each area of concern (filtering, buffering, ...) could be a seperate class, all of which share a common, abstract interface. The pipe could be constructed by simply concatenating the list of filters, mappers, etc. Since those classes are small we could theoretically also develop them one by one and integrate them directly into the existing code without constructing a "pipe".

We could then also construct accurate unit tests for each class. E.g. we could ensure that a hypothetical altGrAliasing-filter-class (for the same-named altGrAliasing-feature) works correctly, without being hindered by other logic that might obstruct our tests. No need for mocks in that case!

Alternatively we could also just keep the entire logic within a single class (i.e. merge the 4 classes listed above).
It would be a simple and acceptable approach in my eyes, even if the code might have a lot of if/else.
Regardless I'd recommend against moving the logic into the TermControl classes or into the Terminal/TerminalInput classes. Those classes are already large enough IMHO. 😄 Hoisting it into a seperate class sounds like a good idea though.

@lhecker - what you say make perfect sense to me, and I would glad to collaborate (not sure I can raise it alone still).
Some thoughts about the design pattern you suggest:

• Creating a pipe you describe is a good way to isolate functionality.
• However, then you pay a price when debugging the code - instead of going through the code flow you start traversing lists 😄.
• I would recommend this pure pipe approach only if the list should be created dynamically (especially if the list can be altered during the traversing)
• A simpler approach might be creating an orchesrtator that explicitly calls each filter or mapper.

@lhecker - if it something you are planning to work, I will be glad to assist. By now I am just putting this PR to Review, so at least we mitigate the PrintScreen issue for now 😄

[zadjii-msft]

Alright so I'm gonna approve this PR now, but highly caution against more broad refactorings for the input in Terminal and TermControl for the time being. As part of the work that needs to be done for #5000, we're going to need to actually separate those two classes more, so much that the UI will be in one process, and the Terminal in another altogether. For more details, see the design and implementation plan (see steps 6-15). I'm not totally confident how the cards will fall as a part of that refactoring, but I want to make sure that all the future work in this area takes that plan into account.

My suggestion is:

• Move all input related logic (including mouse, clipboard, keys, etc.) from Terminal to TermControl
• TermControl will hold TerminalInput, Terminal and Connection
• TermContol will work directly with TerminalInput (probably the callback mechanism can be replaced with a call)

If we were to abstract that into a class sitting between TermControl and Terminal, that would probably help us with the aformentioned plans, because then we could move all the logic at once to the right process, regardless of which process that ends up being. That at least seems sensible to my 5am brain 😛

[lhecker]

@Don-Vito The C++20 ranges library would be a good fit, stylistically at least, for many things in this project, as far as I can see it. Unfortunately - as far as I know - it suffers from a very poor compilation performance. As such creating an orchestrator sounds like the perfect solution to me.
But I mean what is an orchestrator anyways, if not simply a list of calls. *ba dum tss* 😄😄😄

Apart from the general idea / intention of abstracting the input handling away, I haven't really thought about this any further yet.
I'm not really planning anything yet either. There are a lot of open questions... For instance:

• How can we signal the caller that VT sequences were generated? We can't simple return the VT string, because...
• What if multiple existing modules need feedback for a single keystroke? A single keystroke, can simultaneously generate VT sequences, clear the current selection, scroll up/down, etc... Each of those actions must be fed back into TermControl.
• ...

I believe doing this involves a lot of small PRs, each of which abstracts away one aray of concern in the current TermControl -> Terminal -> TerminalInput -> Terminal (callback) -> TerminalControl (callback) pipeline. For instance here's some code that buffers surrogate pairs.
To be honest though, I feel like my understanding of the Terminal architecture is still too limited, in order to be comfortable making such changes without immediate assistance from the maintainers. I think I'll be able to submit more time to this project starting early next year though and finally get to know all the ins and outs of this project. In the meantime, @zadjii-msft (among others) knows about all the things regarding input handling and can comment on such grander changes much better than me. 🙂

[DHowett]

Thanks for doing this

[ghost]

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

Handy links:

• Release Notes
• Store Download

[ghost]

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

Handy links:

• Release Notes
• Store Download