Split `TermControl` into a Core, Interactivity, and Control layer #9820

zadjii-msft · 2021-04-14T17:58:16Z

Summary of the Pull Request

Brace yourselves, it's finally here. This PR does the dirty work of splitting the monolithic TermControl into three components. These components are:

ControlCore: This encapsulates the Terminal instance, the DxEngine and Renderer, and the Connection. This is intended to everything that someone might need to stand up a terminal instance in a control, but without any regard for how the UX works.
ControlInteractivity: This is a wrapper for the ControlCore, which holds the logic for things like double-click, right click copy/paste, selection, etc. This is intended to be a UI framework-independent abstraction. The methods this layer exposes can be called the same from both the WinUI TermControl and the WPF control.
TermControl: This is the UWP control. It's got a Core and Interactivity inside it, which it uses for the actual logic of the terminal itself. TermControl's main responsibility is now

By splitting into smaller pieces, it will enable us to

write unit tests for the Core and Interactivity bits, which we desparately need
Combine ControlCore and ControlInteractivity in an out-of-proc core process in the future, to enable tab tearout.

However, we're not doing that work quite yet. There's still lots of work to be done to enable that, thought this is likely the biggest portion.

Ideally, this would just be methods moved wholesale from one file to another. Unfortunately, there are a bunch of cases where that didn't work as well as expected. Especially when trying to better enforce the boundary between the classes.

We've got a couple tests here that I've added. These are partially examples, and partially things I ran into while implementing this. A bunch of things from #7001 can go in now that we have this.

This PR is gonna be a huge pain to review - 38 files with 3,730 additions and 1,661 deletions is nothing to scoff at. It will also conflict 100% with anything that's targeting TermControl. I'm hoping we can review this over the course of the next week and just be done with it, and leave plenty of runway for 1.9 bugs in post.

References

In pursuit of Add support for tab tearoff and tab merge #1256
Proc Model: Scenario: Windows Terminal 2.0 Process Model Improvements #5000
https://github.com/microsoft/terminal/projects/5

PR Checklist

Closes Introduce abstraction layer between TermControl and TerminalCore (aka Interactivity) #6842
Closes https://github.com/microsoft/terminal/projects/5#card-50760249
Closes https://github.com/microsoft/terminal/projects/5#card-50760258
I work here
Tests added/passed
[n/a] Requires documentation to be updated

Detailed Description of the Pull Request / Additional comments

I don't love the names ControlCore and ControlInteractivity. Open to other names.
I added a ICoreState interface for "properties that come from the ControlCore, but consumers of the TermControl need to know". In the future, these will all need to be handled specially, because they might involve an RPC call to retrieve the info from the core (or cache it) in the window process.
I've added more EventArgs to make more events proper TypedEvents.
I've changed how the TerminalApp layer requests updated TaskbarProgress state. It doesn't need to pump TermControl to raise a new event anymore.
Something that snuck into this branch in the very long history is the switch to DCompositionCreateSurfaceHandle for the DxEngine. @miniksa wrote this originally in 30b8335, I'm just finally committing it here. We'll need that in the future for the out-of-proc stuff.
- I reverted this in c113b65. We can revert that commit when we want to come back to it.
I've changed the acrylic handler a decent amount. But added tests!
All the ThrottledFunc things are left in TermControl. Some might be able to move down into core/interactivity, but once we figure out how to use a different kind of Dispatcher (because a UI thread won't necessarily exist for those components).
I've undoubtably messed up the merging of the locking around the appearance config stuff recently

Validation Steps Performed

I've got a rolling list in #6842 (comment) that I'm updating as I go.

This is the code as of 4703dfe I'm moving it to TermControl in a new branch, to actually try checking it out now that I think it's mostly done. There are still 14 !TODO!s but this should at least let me start testing

…part-control

…features O.o

Wadda ya know, we _can_ have TerminalCore expose types. Maybe we just got that in a recent VS / cppwinrt update, but it works finally

…Control

…rminalcontrol-into-lib # Conflicts: # src/cascadia/TerminalApp/TerminalSettings.h # src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

…le. Convert some older macro uses to TYPED_EVENT

…l' into dev/migrie/oop-terminal.control-split-control

…nteractivity

…al.control-split-control # Conflicts: # src/cascadia/TerminalApp/TerminalPage.cpp # src/cascadia/TerminalApp/TerminalPage.h # src/cascadia/TerminalControl/TermControl.cpp # src/cascadia/TerminalControl/TermControl.h # src/cascadia/TerminalControl/TermControl.idl # src/cascadia/TerminalControl/TerminalControl.def # src/cascadia/TerminalControl/TerminalControlLib.vcxproj # src/cascadia/TerminalControl/dll/Microsoft.Terminal.Control.def # src/cascadia/TerminalControl/dll/TerminalControl.def # src/cascadia/TerminalControl/dll/TerminalControl.vcxproj

…nteractivity-control UpdateHoveredCell was a MESS

…nteractivity-control

This also moved _HyperlinkHandler, and _TrySendMouseEvent Started with 29 errors, down to 24

Now we're down to 12 errors. This is starting to feel like it's coming together. Mouse scrolling is scary though.

And a bit of cleanup of things left behind by the previous commits. Down to 5 errors!

…ile I'm out lol

DHowett

I am sold. You addressed my 150 comments. Thank you for doing that, and I am excited to re-platform the WPF control on top of this.

Unresolved comments:

Adding counters to the opacity callback tests to make certain the callback is run.
Verifying floats using delta algorithm instead of ARE_EQUAL - VERIFY_IS_LESS_THAN(std::abs(1.0 - expectedOpacity), 0.001))
Was the ordering on UpdateSettings before/after UI settings (TermControl) important
Split TermControl into a Core, Interactivity, and Control layer #9820 (comment) Is this invariant something that should be promoted to a doc comment?
gonna have to do a manual test pass to ensure that modifiers and OS-level keys do not jump the terminal down or dismiss selection.
@miniksa commented on "this should be on the UI thread", "Is there any way to assert/throw if it's not on the XAML thread?"
Should the mocks WEX::Log
Maybe make Interactivity figure out focused on its own
@carlos-zamora comments on dropping an else branch in opacity adjustment (ControlCore) - Split TermControl into a Core, Interactivity, and Control layer #9820 (comment)
Same place, but question if we need SetBackgroundOpacity - Split TermControl into a Core, Interactivity, and Control layer #9820 (comment)

You could also write the following:
hostingTab.TaskbarProgressChanged({ get_weak(), &TerminalPage::_SetTaskbarProgressHandler });
Unfortunately there's no explanation in this file why get_weak() is being used in certain places instead of a simple this. If you know why a comment would probably be useful and prevent someone from breaking it in the future.

@lhecker https://github.com/microsoft/terminal/pull/9820/files#r616981656

If you make this method resistant against being called from a background thread you don't need to switch into the foreground thread in neither _RendererWarning nor _RaiseReadOnlyWarning.

@lhecker #9820 (comment)

DHowett · 2021-04-22T20:50:09Z

@carlos-zamora you are a waiter on this PR.

@miniksa You and I discussed this, so I may dismiss your review.

carlos-zamora

Alright, let's do this.

…nteractivity-control

Dustin said it was cool if I dismiss you, so 👋

ghost · 2021-04-27T15:50:42Z

Hello @zadjii-msft!

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.

## Summary of the Pull Request This PR encompasses the first three bugs we found post-#9820. ### A: Mousedown, select, SCROLL does a weird thing with endpoints that doesn't happen in stable We were using the terminal position to set the selection anchor, when we should have used the pixel position. This is fixed in 4f4df01. ### B: Trackpad scrolling down with small increments seems buggy This one's the most complicated. The touchpad sends very many small scroll deltas, less than one row at a time. The control scrollbar can store a `double`, so small deltas can accumulate. Originally, these would accumulate in the scrollbar, and we'd only read that out as an `int` in the scrollbar updater, which is throttled. In the interactivity split, there's no place for us to store that double. We immediately narrow to an `int` for `ControlInteractivity::_updateScrollbar`. So this introduces a double inside `ControlInteractivity` as a fake scrollbar, with which to accumulate to. This is fixed in 33d29fa...0fefc5b ### C: Looks like there's a selection issue when you click and drag too quickly. The diff for this one is: <table> <tr><td>1.8</td><td>main</td></tr> <tr> <td> ```c++ if (_singleClickTouchdownPos) { // Figure out if the user's moved a quarter of a cell's smaller axis away from the clickdown point auto& touchdownPoint{ *_singleClickTouchdownPos }; auto distance{ std::sqrtf(std::powf(cursorPosition.X - touchdownPoint.X, 2) + std::powf(cursorPosition.Y - touchdownPoint.Y, 2)) }; const til::size fontSize{ _actualFont.GetSize() }; const auto fontSizeInDips = fontSize.scale(til::math::rounding, 1.0f / _renderEngine->GetScaling()); if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f)) { _terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint)); // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; } } ``` </td> <td> ```c++ if (_singleClickTouchdownPos) { // Figure out if the user's moved a quarter of a cell's smaller axis away from the clickdown point auto& touchdownPoint{ *_singleClickTouchdownPos }; float dx = ::base::saturated_cast<float>(pixelPosition.x() - touchdownPoint.x()); float dy = ::base::saturated_cast<float>(pixelPosition.y() - touchdownPoint.y()); auto distance{ std::sqrtf(std::powf(dx, 2) + std::powf(dy, 2)) }; const auto fontSizeInDips{ _core->FontSizeInDips() }; if (distance >= (std::min(fontSizeInDips.width(), fontSizeInDips.height()) / 4.f)) { _core->SetSelectionAnchor(terminalPosition); // stop tracking the touchdown point _singleClickTouchdownPos = std::nullopt; } } ``` </td> </tr> </table> ```c++ _terminal->SetSelectionAnchor(_GetTerminalPosition(touchdownPoint)); ``` vs ```c++ _core->SetSelectionAnchor(terminalPosition); ``` We're now using the location of the drag event as the selection anchor, instead of the location that the user initially clicked. Oops. ## PR Checklist * [x] Checks three boxes, though I'll be shocked if they're the last. * [x] I work here * [x] Tests added/passed 🎉🎉🎉 * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments All three have tests, 🙌🙌🙌🙌 ## Validation Steps Performed Manual, and automated via tests

ghost · 2021-05-25T19:09:12Z

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

Handy links:

## Summary of the Pull Request This was missed in #10051. We need to make sure that the UIA provider can immediately know about the padding in the control, not just after the settings reload. ## PR Checklist * [x] Closes #9955.e * [x] Additionally, this just closes #9955. The only remaining box in there never repro'd, so probably wasn't even root caused by #9820. I think we can close that issue for now, and reactivate if something else was broken. * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Validation Steps Performed Checked before/after in Accessibility Insights. Before the row rectangles were the full width of the control initially. Now they're properly padded.

This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method. In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654 #10051 didn't change this In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before: * [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler * [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d * [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged` ## Validation Steps Performed Print a URL, scroll the wheel: it still works. Closes #11055

This is on me. When I got rid of the `_updatePatternLocations` `ThrottledFunc` in the `TermControl`, I didn't add a matching call to `_updatePatternLocations->Run()` in this method. In #9820, in `TermControl::_ScrollPositionChanged`, there was still a call to `_updatePatternLocations->Run();`. (TermControl.cpp:1655 on the right) https://github.com/microsoft/terminal/pull/9820/files#diff-c10bb023995e88dac6c1d786129284c454c2df739ea547ce462129dc86dc2697R1654 #10051 didn't change this In #10187 I moved the `_updatePatternLocations` throttled func from termcontrol to controlcore. Places it existed before: * [x] `TermControl::_coreReceivedOutput`: already matched by ControlCore::_connectionOutputHandler * [x] `TermControl::_ScrollbarChangeHandler` -> added in c20eb9d * [x] `TermControl::_ScrollPositionChanged` -> `ControlCore::_terminalScrollPositionChanged` ## Validation Steps Performed Print a URL, scroll the wheel: it still works. Closes #11055 (cherry picked from commit 7423734)

zadjii-msft added 30 commits March 11, 2021 06:27

Move everything to a fresh branch

0bd30c2

This is the code as of 4703dfe I'm moving it to TermControl in a new branch, to actually try checking it out now that I think it's mostly done. There are still 14 !TODO!s but this should at least let me start testing

fix some build breaks, because I was only playing in TerminalControl

b0f1fa4

Merge remote-tracking branch 'origin/main' into dev/migrie/oop-tear-a…

243867a

…part-control

hey look it launches. But there's definitely a good amount of broken …

acf84f1

…features O.o

I wasn't the murderer after all

5e655ef

Hokay, so that gets the control as a lib/dll split

2402c59

its-all-coming-together.gif

24765d7

can I get a big woop woop from all my homies

54f443b

Wadda ya know, we _can_ have TerminalCore expose types. Maybe we just got that in a recent VS / cppwinrt update, but it works finally

Fix the tests

322d511

Rename Microsoft.Terminal.TerminalControl to just Microsoft.Terminal.…

8f8cf59

…Control

some dead code

eee5bee

Change the GUID, project name

04d403c

Merge remote-tracking branch 'origin/main' into dev/migrie/r/split-te…

e8d06d7

…rminalcontrol-into-lib # Conflicts: # src/cascadia/TerminalApp/TerminalSettings.h # src/cascadia/TerminalSettingsModel/TerminalSettings.cpp

oh! IT CAUGHT AN ISSUE! YAY!

f8e969e

Bite-sized refactor: Move all the args in TermControl to their own fi…

8c6c7ec

…le. Convert some older macro uses to TYPED_EVENT

Hey, lets clean this up while we're at it

2f532bd

Merge remote-tracking branch 'origin/dev/migrie/oop-tear-apart-contro…

093e8ac

…l' into dev/migrie/oop-terminal.control-split-control

Reorganize these files to be a little less painful to read

c035e69

Move isReadOnly to the core as well, through that really belongs in i…

801b296

…nteractivity

Ooh, do the font size event too

6ab8339

Start moving things to a shared middle bit too

c29ffba

Move _UpdateSystemParameterSettings, 30 errors remain

72c35a8

I suppose this should have been in the parent commit

541d151

Merge remote-tracking branch 'origin/main' into dev/migrie/oop-core-i…

2035fcd

…nteractivity-control UpdateHoveredCell was a MESS

I think this needs to go in main

bbf605b

Merge remote-tracking branch 'origin/main' into dev/migrie/oop-core-i…

ad88c6a

…nteractivity-control

Move PointerPressed to ControlInteractivity

ed79dab

This also moved _HyperlinkHandler, and _TrySendMouseEvent Started with 29 errors, down to 24

Move more mouse methods

618b207

Now we're down to 12 errors. This is starting to feel like it's coming together. Mouse scrolling is scary though.

Move around the clipboard handling methods

e85bdff

And a bit of cleanup of things left behind by the previous commits. Down to 5 errors!

This is all the major blockers IMO. not like anyone will review it wh…

6d5351e

…ile I'm out lol

DHowett reviewed Apr 22, 2021

View reviewed changes

DHowett approved these changes Apr 22, 2021

View reviewed changes

This was referenced Apr 23, 2021

[PoC] Allow broadcasting keys to all panes in tab #9222

Closed

Introduce search status #8588

Closed

Dragging a tab makes the window slowly scroll up for a while even after not dragging the tab anymore #9109

Closed

zadjii-msft added 2 commits April 23, 2021 11:06

Leonard is right, this is better

efbec1f

LOAD BEARING COMMIT

0f62cea

zadjii-msft mentioned this pull request Apr 26, 2021

Interactivity bugs post-#9820 #9955

Closed

5 tasks

carlos-zamora approved these changes Apr 27, 2021

View reviewed changes

zadjii-msft added 2 commits April 27, 2021 05:18

Merge remote-tracking branch 'origin/main' into dev/migrie/oop-core-i…

88742d6

…nteractivity-control

an update from main

9731636

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 27, 2021

ghost merged commit 8910a16 into main Apr 27, 2021

ghost deleted the dev/migrie/oop-core-interactivity-control branch April 27, 2021 15:50

zadjii-msft mentioned this pull request Apr 28, 2021

First three interactivity fixes #9980

Merged

3 tasks

ghost mentioned this pull request May 25, 2021

Introduce abstraction layer between TermControl and TerminalCore (aka Interactivity) #6842

Closed

DHowett mentioned this pull request May 27, 2021

Win32 mouse coordinates wrong when viewport scrolled #10190

Closed

zadjii-msft mentioned this pull request Aug 4, 2021

Initialize the padding for the Control UIA provider #10874

Merged

4 tasks

This was referenced Aug 26, 2021

1.11: scrolling at all breaks the URL pattern buffer (no URLs work) #11055

Closed

Update pattern locations again after scrolling #11059

Merged

zadjii-msft mentioned this pull request Mar 1, 2022

Region invalidation malfunctioning after text buffer has begun to scroll #11214

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split `TermControl` into a Core, Interactivity, and Control layer #9820

Split `TermControl` into a Core, Interactivity, and Control layer #9820

zadjii-msft commented Apr 14, 2021 •

edited

Loading

DHowett left a comment

DHowett commented Apr 22, 2021

carlos-zamora left a comment

ghost commented Apr 27, 2021

ghost commented May 25, 2021

Split TermControl into a Core, Interactivity, and Control layer #9820

Split TermControl into a Core, Interactivity, and Control layer #9820

Conversation

zadjii-msft commented Apr 14, 2021 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Apr 22, 2021

carlos-zamora left a comment

Choose a reason for hiding this comment

ghost commented Apr 27, 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 May 25, 2021

Split `TermControl` into a Core, Interactivity, and Control layer #9820

Split `TermControl` into a Core, Interactivity, and Control layer #9820

zadjii-msft commented Apr 14, 2021 •

edited

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.