Introduce a ContentManager helper #14851

zadjii-msft · 2023-02-14T15:42:41Z

Summary

Thus we come to the introduction of a new servant, the ContentManager, a singular entity that serves at the behest of the emperor. It is its charge to keep track of all TermControl instances created by the windows, for each window must seek its blessing before calling forth such an instance.
With the aid of the ContentManager, the TermControl shall now be traced by the hand of fate through the use of unique identifying marks, known as GUIDs. Yet, its purpose remains yet unknown, for it is merely a waypoint upon the journey yet to come.
This act of bridging also brings a change to the handling of events within the TermControl. This change shall see the addition of a revoker, similar to the manner in which the AppHost hath employed it, to the TermControl. Additionally, there is a new layer of indirection between the ControlCore and the App layer, making ready for the day when the TermControl may be repositioned and re-parented with ease.
Consider this but a trivial act, a mere shadow of things yet to come, for its impact shall be felt but briefly, like the passing of a gentle breeze.

Related to #5000
Related to #1256

Detailed description

This PR is another small bridge PR between the big work in #14843, and the PR that will enable panes to move between windows.

This introduces a new class, called ContentManager. This is a global singleton object, owned by the emperor. Whenever a window wants to instantiate a new TermControl, it must ask the ContentManager to give it one. This allows the ContentManager to track each "content" by GUID. That's it. We don't do anything with them in this PR by itself, we just track them.

This also includes a small change to the way TermControl events are handled. It adds an AppHost-like revoker struct, and weak_ref's all the handlers. We also add a layer of indirection between the ControlCore's raising of events and the App layer's handling. This will make reparenting content easier in the future.

This is a pretty trivial change which shouldn't have any major side effects. Consider it exposition of the things to come. It's intentionally small to try and keep the reviews more managable.

…ing AppLogic that I hate so I'm gonna start over

This reverts commit a5255ba.

We'll need this for #5000, for ainulindale. This refactoring will be annoying enough as it is so we may as well do it as a first, separate PR.

…ns though, so it immediately exits

…ndale # Conflicts: # src/cascadia/WindowsTerminal/AppHost.h

…XAML island is started. I think this can work in the parent at least

…It launches and crashes immediately. We'll keep shuffling code.

At this point, I determined that I would need to make some big changes to AppHost and decided that it was time to commit before moving on.

…pressive It exits itself after 30s, but hey it worked

(cherry picked from commit 57d1dd4)

Doesn't work with multiple windows open, but doesn't do _nothing_ (cherry picked from commit 427a4a5)

…ut this doesn't fix the crash (cherry picked from commit 700aadc)

TerminalPage is the thing that ends up expanding iterable Command. It does this largely with copies - it makes a new map, a new vector, copies the Commands over, and does the work there before setting up the cmdpal. Except, it's not making a copy of the Commands, it's making a copy of the vector, with winrt objects all pointing at the Command objects that are ultimately owned by CascadiaSettings. This doesn't matter if there's only one TerminalPage - we'll only ever do that once. If there's many, on different threads, then one tpage will end up expanding the subcommands of one Command while another tpage is ALSO iterating on those subcommands. Hence why I'm getting `hresult_changed_state`s (cherry picked from commit 2122eec)

…aquenta

37 TODOs left

36 TODOs

…ispatcher. 35 TODOs left

33 TODOs

32 TODOs

30 TODOs left

…lindale

…to dev/migrie/oop/3/valaquenta

src/cascadia/TerminalApp/ContentManager.h

src/cascadia/TerminalApp/ContentManager.cpp

src/cascadia/TerminalControl/ControlInteractivity.cpp

src/cascadia/TerminalApp/ContentManager.cpp

src/cascadia/TerminalApp/AppLogic.h

src/cascadia/TerminalApp/ContentManager.cpp

DHowett · 2023-03-21T19:44:30Z

src/cascadia/TerminalApp/ContentManager.cpp

+                                                    TerminalConnection::ITerminalConnection connection)
+    {
+        auto content = ControlInteractivity{ settings, unfocusedAppearance, connection };
+        content.Closed({ get_weak(), &ContentManager::_closedHandler });


qq on weak: is the content manager going away ~~after~~ before the control a real concern?

But is there any benefit to using this over get_weak() in our UI code? Because get_weak() does have the benefit of being safer to use if lifetimes aren't easily visible.

I'm cool with leaning towards safety here. Who knows, maybe there's a "the window closes, so we start tearing down, but the controls only get released after the rest of the app does" situation. I suppose that's covered by our PeekMessage loop in App::Close, but still, this can't hurt, right?

That's all fair; there is some overhead inherent in get_weak, mostly the construction and maintenance of a proxy weak ref object and the need to resolve the weak every time it's dispatched... but that is likely to be small.

DHowett · 2023-03-21T19:45:12Z

src/cascadia/TerminalApp/ContentManager.cpp

+        auto content = ControlInteractivity{ settings, unfocusedAppearance, connection };
+        content.Closed({ get_weak(), &ContentManager::_closedHandler });
+
+        _content.emplace(content.Id(), content);


I don't love that Id has to leak across the boundary like this; it is another member that somebody in the future consuming ControlInteractivity needs to think about

Is there a way to scope this knowledge to ContentManager itself?

I mean, we could stash the id just within ContentManager. But then, when we want to detach content, we'd have to go reverse lookup the key for the content we want to detach. Sure that's just an O(N=number of controls) lookup, but that still feels blechy

src/cascadia/TerminalApp/ContentManager.cpp

src/cascadia/TerminalApp/TerminalPage.idl

src/cascadia/TerminalControl/TermControl.cpp

src/cascadia/TerminalControl/ControlInteractivity.cpp

DHowett · 2023-03-22T16:55:24Z

src/cascadia/TerminalApp/AppLogic.h

@@ -101,7 +101,7 @@ namespace winrt::TerminalApp::implementation
        winrt::com_ptr<LanguageProfileNotifier> _languageProfileNotifier;
        wil::unique_folder_change_reader_nothrow _reader;

-        TerminalApp::ContentManager _contentManager{ *winrt::make_self<implementation::ContentManager>() };
+        TerminalApp::ContentManager _contentManager{ winrt::make<implementation::ContentManager>() };


Want to know something even more fun? Since these are in the same module, if you just do TerminalApp::ContentManager _contentManager; it will use a fast path to call winrt::make<...> directly. You can treat the projected type as "law"! I forgot about this until I saw it with make in.

You don't need to make this change, I just think it's v.cool. Check it out:

In Foo.g.cpp

// WARNING: Please don't edit this file. It was generated by C++/WinRT v2.0.210825.3 void* winrt_make_TerminalApp_Foo() { return winrt::detach_abi(winrt::make<winrt::TerminalApp::factory_implementation::Foo>()); } WINRT_EXPORT namespace winrt::TerminalApp { Foo::Foo() : Foo(make<TerminalApp::implementation::Foo>()) { } }

It provides a linkable constructor for TerminalApp::Foo, and doesn't even generate the one that calls RoActivateInstance in winrt/whatever.h

DHowett · 2023-03-22T16:59:42Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

@@ -46,7 +46,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        _lastMouseClickPos{},
        _selectionNeedsToBeCopied{ false }
    {
-        _id = ControlInteractivity::_nextId++;
+        _id = _nextId.fetch_add(1, std::memory_order_relaxed);


Editor's note: you can have up to 18,446,744,073,709,551,615 terminal panes in the same process before this becomes a problem; it is likely that we will crash before you get to there :)

Due to a bad merge a few commits back. This event should have had a revoker. Probably regressed in #14851

zadjii-msft added 30 commits January 30, 2023 12:37

I definitely want all of this. But I started down a path for refactor…

581acd4

…ing AppLogic that I hate so I'm gonna start over

I don't think I want any of these

a5255ba

Revert "I don't think I want any of these"

e6220b7

This reverts commit a5255ba.

Start splitting AppLogic into AppLogic and Window logic

936c01f

We'll need this for #5000, for ainulindale. This refactoring will be annoying enough as it is so we may as well do it as a first, separate PR.

this is dangerously close to compiling

439b21f

It doesn't crash on launch. That's something. There's no startupActio…

99bc280

…ns though, so it immediately exits

it launches

2195515

I think the todo's that are left, we can move on without them for now.

5116ca1

Merge branch 'dev/migrie/oop/3/foreword' into dev/migrie/oop/3/ainuli…

ef7e2ed

…ndale # Conflicts: # src/cascadia/WindowsTerminal/AppHost.h

[TO PARENT] Move the page ctor call, so that it can happen after the …

af14c2b

…XAML island is started. I think this can work in the parent at least

Use the single AppLogic for all windows. Remove the _app references. …

d456210

…It launches and crashes immediately. We'll keep shuffling code.

it runs!

f5b030c

wow it just... worked

2c4613a

Start rewriting WindowManager to facilitate the new process model

7a3e2e0

At this point, I determined that I would need to make some big changes to AppHost and decided that it was time to commit before moving on.

This does successfully get a window on the screen, which is pretty im…

f655296

…pressive It exits itself after 30s, but hey it worked

it's working

274d62d

notes

e623299

I literally think this is the whole thing

1b4f1ef

[ainulindale] Clean tear down the App when the process exits

47336c0

(cherry picked from commit 57d1dd4)

[ainulindale] "fix" hot reload

9924e23

Doesn't work with multiple windows open, but doesn't do _nothing_ (cherry picked from commit 427a4a5)

[ainulindale] This I'm confident fixes some of the x-thread errors, b…

f06e484

…ut this doesn't fix the crash (cherry picked from commit 700aadc)

Merge branch 'dev/migrie/oop/3/ainulindale' into dev/migrie/oop/3/val…

f27db59

…aquenta

Don't try to snap on create if we're not prepared to snap

2332f0c

37 TODOs left

I guess this is just dead now

4e7da2e

36 TODOs

Move the initialization of the showHide Throttler after we init the d…

23c4d4c

…ispatcher. 35 TODOs left

This one was dead code, 34 TODOs left

761bd6a

I guess we need to just hardcode these sizes. Sad, but okay.

93e9dc5

33 TODOs

UWP is dead

d5396d1

32 TODOs

Plumb initial load result into the window on startup

07ff418

30 TODOs left

Merge remote-tracking branch 'origin/main' into dev/migrie/oop/3/ainu…

1e4cc33

…lindale

Base automatically changed from dev/migrie/oop/3/ainulindale to main March 17, 2023 22:59

zadjii-msft added 2 commits March 20, 2023 11:34

Merge remote-tracking branch 'origin/dev/migrie/oop/3/ainulindale' in…

5a420d8

…to dev/migrie/oop/3/valaquenta

Merge branch 'main' into dev/migrie/oop/3/valaquenta

24da827

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Mar 20, 2023

microsoft-github-policy-service bot requested review from PankajBhojwani, carlos-zamora, DHowett and lhecker March 20, 2023 18:37

lhecker reviewed Mar 20, 2023

View reviewed changes

src/cascadia/TerminalApp/ContentManager.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/ContentManager.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalControl/ControlInteractivity.cpp Outdated Show resolved Hide resolved

microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Mar 20, 2023

zadjii-msft added 3 commits March 21, 2023 06:54

easier nits

b02b935

fix the tests

f73ec9a

why not just use uints?

3659744

lhecker reviewed Mar 21, 2023

View reviewed changes

src/cascadia/TerminalApp/ContentManager.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/ContentManager.cpp Outdated Show resolved Hide resolved

I don't know how to stl

3af4d94

lhecker approved these changes Mar 21, 2023

View reviewed changes

DHowett requested changes Mar 21, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 21, 2023

lhecker reviewed Mar 22, 2023

View reviewed changes

src/cascadia/TerminalControl/ControlInteractivity.cpp Outdated Show resolved Hide resolved

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 22, 2023

notes from PR

697b420

zadjii-msft assigned DHowett Mar 22, 2023

DHowett reviewed Mar 22, 2023

View reviewed changes

DHowett approved these changes Mar 22, 2023

View reviewed changes

DHowett merged commit f3a722e into main Mar 22, 2023

DHowett deleted the dev/migrie/oop/3/valaquenta branch March 22, 2023 17:11

zadjii-msft mentioned this pull request Mar 31, 2023

Fix a crash in the rclick context menu #15079

Merged

zadjii-msft added a commit that referenced this pull request Mar 31, 2023

Fix a crash in the rclick context menu (#15079)

bbd4d1b

Due to a bad merge a few commits back. This event should have had a revoker. Probably regressed in #14851

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a ContentManager helper #14851

Introduce a ContentManager helper #14851

zadjii-msft commented Feb 14, 2023 •

edited by DHowett

Loading

DHowett Mar 21, 2023 •

edited

Loading

lhecker Mar 22, 2023 •

edited

Loading

zadjii-msft Mar 22, 2023

DHowett Mar 22, 2023

DHowett Mar 21, 2023

DHowett Mar 21, 2023

zadjii-msft Mar 22, 2023

DHowett Mar 22, 2023

DHowett Mar 22, 2023

Introduce a ContentManager helper #14851

Introduce a ContentManager helper #14851

Conversation

zadjii-msft commented Feb 14, 2023 • edited by DHowett Loading

Summary

Detailed description

DHowett Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

lhecker Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

zadjii-msft Mar 22, 2023

Choose a reason for hiding this comment

DHowett Mar 22, 2023

Choose a reason for hiding this comment

DHowett Mar 21, 2023

Choose a reason for hiding this comment

DHowett Mar 21, 2023

Choose a reason for hiding this comment

zadjii-msft Mar 22, 2023

Choose a reason for hiding this comment

DHowett Mar 22, 2023

Choose a reason for hiding this comment

In Foo.g.cpp

DHowett Mar 22, 2023

Choose a reason for hiding this comment

zadjii-msft commented Feb 14, 2023 •

edited by DHowett

Loading

DHowett Mar 21, 2023 •

edited

Loading

lhecker Mar 22, 2023 •

edited

Loading

In `Foo.g.cpp`