Skip to content

Commit

Permalink
Split TermControl into a Core, Interactivity, and Control layer (#9820
Browse files Browse the repository at this point in the history
)

## 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 #1256
* Proc Model: #5000
* https://github.com/microsoft/terminal/projects/5

## PR Checklist
* [x] Closes #6842
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760249
* [x] Closes https://github.com/microsoft/terminal/projects/5#card-50760258
* [x] I work here
* [x] 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 `TypedEvent`s.
* 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.
  • Loading branch information
zadjii-msft authored Apr 27, 2021
1 parent 8d50609 commit 8910a16
Show file tree
Hide file tree
Showing 43 changed files with 3,740 additions and 1,711 deletions.
4 changes: 4 additions & 0 deletions .github/actions/spelling/dictionary/apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ CXICON
CYICON
D2DERR_SHADER_COMPILE_FAILED
dataobject
dcomp
DERR
dlldata
DONTADDTORECENT
Expand Down Expand Up @@ -127,9 +128,12 @@ wsregex
wwinmain
XDocument
XElement
xhash
xlocmes
xlocmon
xlocnum
xloctime
XParse
xstring
xtree
xutility
15 changes: 14 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,20 @@
"xloctime": "cpp",
"multi_span": "cpp",
"pointers": "cpp",
"vector": "cpp"
"vector": "cpp",
"bitset": "cpp",
"deque": "cpp",
"initializer_list": "cpp",
"list": "cpp",
"queue": "cpp",
"random": "cpp",
"regex": "cpp",
"stack": "cpp",
"xhash": "cpp",
"xtree": "cpp",
"xutility": "cpp",
"span": "cpp",
"string_span": "cpp"
},
"files.exclude": {
"**/bin/**": true,
Expand Down
7 changes: 4 additions & 3 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"-Command",
"Import-Module ${workspaceFolder}\\tools\\OpenConsole.psm1;",
"Set-MsBuildDevEnvironment;",
"$project = switch(\"${input:buildProjectChoice}\"){OpenConsole{\"Conhost\\Host_EXE\"} Terminal{\"Terminal\\CascadiaPackage\"}};",
"$project = switch(\"${input:buildProjectChoice}\"){OpenConsole{\"Conhost\\Host_EXE\"} Terminal{\"Terminal\\CascadiaPackage\"} TermControl{\"Terminal\\TerminalControl\"}};",
"$target = switch(\"${input:buildModeChoice}\"){Build{\"\"} Rebuild{\":Rebuild\"} Clean{\":Clean\"}};",
"$target = $project + $target;",
"msbuild",
Expand Down Expand Up @@ -111,10 +111,11 @@
"description": "OpenConsole or Terminal?",
"options":[
"OpenConsole",
"Terminal"
"Terminal",
"TermControl"
],
"default": "Terminal"
}

]
}
}
2 changes: 2 additions & 0 deletions OpenConsole.sln
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,9 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Microsoft.Terminal.Control.
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B} = {CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}
{CA5CAD1A-ABCD-429C-B551-8562EC954746} = {CA5CAD1A-ABCD-429C-B551-8562EC954746}
{1CF55140-EF6A-4736-A403-957E4F7430BB} = {1CF55140-EF6A-4736-A403-957E4F7430BB}
{48D21369-3D7B-4431-9967-24E81292CF62} = {48D21369-3D7B-4431-9967-24E81292CF62}
{48D21369-3D7B-4431-9967-24E81292CF63} = {48D21369-3D7B-4431-9967-24E81292CF63}
{AF0A096A-8B3A-4949-81EF-7DF8F0FEE91F} = {AF0A096A-8B3A-4949-81EF-7DF8F0FEE91F}
EndProjectSection
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Microsoft.Terminal.Control", "src\cascadia\TerminalControl\dll\TerminalControl.vcxproj", "{CA5CAD1A-F542-4635-A069-7CAEFB930070}"
Expand Down
6 changes: 3 additions & 3 deletions build/pipelines/templates/build-console-steps.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ steps:
configPath: NuGet.config
restoreSolution: OpenConsole.sln
restoreDirectory: '$(Build.SourcesDirectory)\packages'

- task: 333b11bd-d341-40d9-afcf-b32d5ce6f23b@2
displayName: Restore NuGet packages for extraneous build actions
inputs:
Expand Down Expand Up @@ -96,7 +96,7 @@ steps:
displayName: 'Upload converted test logs'
inputs:
testResultsFormat: 'xUnit' # Options: JUnit, NUnit, VSTest, xUnit, cTest
testResultsFiles: '**/onBuildMachineResults.xml'
testResultsFiles: '**/onBuildMachineResults.xml'
#searchFolder: '$(System.DefaultWorkingDirectory)' # Optional
#mergeTestResults: false # Optional
#failTaskOnFailedTests: false # Optional
Expand Down Expand Up @@ -147,4 +147,4 @@ steps:
displayName: 'Publish All Build Artifacts'
inputs:
PathtoPublish: '$(Build.ArtifactStagingDirectory)'
ArtifactName: 'drop'
ArtifactName: 'drop'
3 changes: 1 addition & 2 deletions src/cascadia/TerminalApp/Pane.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@
// - Mike Griese (zadjii-msft) 16-May-2019

#pragma once
#include <winrt/Microsoft.Terminal.Control.h>
#include <winrt/TerminalApp.h>

#include "../../cascadia/inc/cppwinrt_utils.h"

enum class Borders : int
Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalApp/SettingsTab.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ Author(s):
#pragma once
#include "TabBase.h"
#include "SettingsTab.g.h"
#include <winrt/TerminalApp.h>
#include <winrt/Microsoft.Terminal.Settings.Editor.h>
#include "../../cascadia/inc/cppwinrt_utils.h"

namespace winrt::TerminalApp::implementation
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalApp/TabManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ namespace winrt::TerminalApp::implementation
{
// Possibly update the icon of the tab.
page->_UpdateTabIcon(*tab);

// Update the taskbar progress as well. We'll raise our own
// SetTaskbarProgress event here, to get tell the hosting
// application to re-query this value from us.
page->_SetTaskbarProgressHandlers(*page, nullptr);
}
});

Expand Down
61 changes: 36 additions & 25 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -996,9 +996,6 @@ namespace winrt::TerminalApp::implementation

term.OpenHyperlink({ this, &TerminalPage::_OpenHyperlinkHandler });

// Add an event handler for when the terminal wants to set a progress indicator on the taskbar
term.SetTaskbarProgress({ this, &TerminalPage::_SetTaskbarProgressHandler });

term.HidePointerCursor({ get_weak(), &TerminalPage::_HidePointerCursorHandler });
term.RestorePointerCursor({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });

Expand Down Expand Up @@ -1053,6 +1050,11 @@ namespace winrt::TerminalApp::implementation
}
});

// Add an event handler for when the terminal or tab wants to set a
// progress indicator on the taskbar
hostingTab.TaskbarProgressChanged({ get_weak(), &TerminalPage::_SetTaskbarProgressHandler });
term.SetTaskbarProgress({ get_weak(), &TerminalPage::_SetTaskbarProgressHandler });

// TODO GH#3327: Once we support colorizing the NewTab button based on
// the color of the tab, we'll want to make sure to call
// _ClearNewTabButtonColor here, to reset it to the default (for the
Expand Down Expand Up @@ -1154,7 +1156,7 @@ namespace winrt::TerminalApp::implementation
{
// The magic value of WHEEL_PAGESCROLL indicates that we need to scroll the entire page
realRowsToScroll = _systemRowsToScroll == WHEEL_PAGESCROLL ?
terminalTab->GetActiveTerminalControl().GetViewHeight() :
terminalTab->GetActiveTerminalControl().ViewHeight() :
_systemRowsToScroll;
}
else
Expand Down Expand Up @@ -1295,7 +1297,7 @@ namespace winrt::TerminalApp::implementation
if (const auto terminalTab{ _GetFocusedTabImpl() })
{
const auto control = _GetActiveControl();
const auto termHeight = control.GetViewHeight();
const auto termHeight = control.ViewHeight();
auto scrollDelta = _ComputeScrollDelta(scrollDirection, termHeight);
terminalTab->Scroll(scrollDelta);
}
Expand Down Expand Up @@ -1647,29 +1649,37 @@ namespace winrt::TerminalApp::implementation
return false;
}

void TerminalPage::_ControlNoticeRaisedHandler(const IInspectable /*sender*/, const Microsoft::Terminal::Control::NoticeEventArgs eventArgs)
// Important! Don't take this eventArgs by reference, we need to extend the
// lifetime of it to the other side of the co_await!
winrt::fire_and_forget TerminalPage::_ControlNoticeRaisedHandler(const IInspectable /*sender*/,
const Microsoft::Terminal::Control::NoticeEventArgs eventArgs)
{
winrt::hstring message = eventArgs.Message();
auto weakThis = get_weak();
co_await winrt::resume_foreground(Dispatcher());
if (auto page = weakThis.get())
{
winrt::hstring message = eventArgs.Message();

winrt::hstring title;
winrt::hstring title;

switch (eventArgs.Level())
{
case NoticeLevel::Debug:
title = RS_(L"NoticeDebug"); //\xebe8
break;
case NoticeLevel::Info:
title = RS_(L"NoticeInfo"); // \xe946
break;
case NoticeLevel::Warning:
title = RS_(L"NoticeWarning"); //\xe7ba
break;
case NoticeLevel::Error:
title = RS_(L"NoticeError"); //\xe783
break;
}
switch (eventArgs.Level())
{
case NoticeLevel::Debug:
title = RS_(L"NoticeDebug"); //\xebe8
break;
case NoticeLevel::Info:
title = RS_(L"NoticeInfo"); // \xe946
break;
case NoticeLevel::Warning:
title = RS_(L"NoticeWarning"); //\xe7ba
break;
case NoticeLevel::Error:
title = RS_(L"NoticeError"); //\xe783
break;
}

_ShowControlNoticeDialog(title, message);
page->_ShowControlNoticeDialog(title, message);
}
}

void TerminalPage::_ShowControlNoticeDialog(const winrt::hstring& title, const winrt::hstring& message)
Expand Down Expand Up @@ -1707,8 +1717,9 @@ namespace winrt::TerminalApp::implementation
// Arguments:
// - sender (not used)
// - eventArgs: the arguments specifying how to set the progress indicator
void TerminalPage::_SetTaskbarProgressHandler(const IInspectable /*sender*/, const IInspectable /*eventArgs*/)
winrt::fire_and_forget TerminalPage::_SetTaskbarProgressHandler(const IInspectable /*sender*/, const IInspectable /*eventArgs*/)
{
co_await resume_foreground(Dispatcher());
_SetTaskbarProgressHandlers(*this, nullptr);
}

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -266,11 +266,11 @@ namespace winrt::TerminalApp::implementation
void _ShowCouldNotOpenDialog(winrt::hstring reason, winrt::hstring uri);
bool _CopyText(const bool singleLine, const Windows::Foundation::IReference<Microsoft::Terminal::Control::CopyFormat>& formats);

void _SetTaskbarProgressHandler(const IInspectable sender, const IInspectable eventArgs);
winrt::fire_and_forget _SetTaskbarProgressHandler(const IInspectable sender, const IInspectable eventArgs);

void _PasteText();

void _ControlNoticeRaisedHandler(const IInspectable sender, const Microsoft::Terminal::Control::NoticeEventArgs eventArgs);
winrt::fire_and_forget _ControlNoticeRaisedHandler(const IInspectable sender, const Microsoft::Terminal::Control::NoticeEventArgs eventArgs);
void _ShowControlNoticeDialog(const winrt::hstring& title, const winrt::hstring& message);

fire_and_forget _LaunchSettings(const Microsoft::Terminal::Settings::Model::SettingsTarget target);
Expand Down
Loading

0 comments on commit 8910a16

Please sign in to comment.