Split `AppLogic` into "App logic" and "Window logic" by zadjii-msft · Pull Request #14825 · microsoft/terminal

zadjii-msft · 2023-02-10T22:58:14Z

followed by: #14843

_{Map

Split AppLogic into "App logic" and "Window logic" #14825 <---- YOU ARE HERE
One process to rule them all #14843
Introduce a ContentManager helper #14851
Add support for moving panes and tabs between windows #14866
Enable dragging tabs between windows #14901
Enable tearing out tabs to create new windows #14935}

And so begins the first chapter in the epic tale of the Terminal's tab tear-out. This commit, though humble in its nature, shall mark the beginning of a grand journey.
This initial offering, though small in its scope, doth serve to divide the code that currently resides within TerminalPage and AppLogic, moving it unto a new entity known as TerminalWindow. In the ages to come, these classes shall take on separate responsibilities, each with their own purpose.
The AppLogic shall hold sway over the entire process, shared among all Terminal windows, while the AppHost, TerminalWindow, and TerminalPage shall rule over each individual window.
This pull request prepares the way for the future, moving state that pertains to the individual windows into the TerminalWindow. This is a task of great labor, for it requires moving much code, but the end result shall bring greater organization to the codebase.
And so the stage is set, for in the next pull request, the Process Model v3 shall be revealed, unifying all Terminal windows into a single process, a grand accomplishment indeed.

courtesy of G.P.T. Tolkien

Or, as I wrote it originally.

This is the first of the commits in the long saga which will culminate in tab tear-out for the Terminal.

This the most functionally trivial of the PRs. It mostly just splits up code that's currently in TerminalPage & AppLogic, and moves it into a new class TerminalWindow. In the future, these classes will separate responsibility as such:

There will be one AppLogic per process, shared across all Terminal windows.
There will be one AppHost, TerminalWindow, and TerminalPage for each individual window in the process.

This PR prepares for that by moving some state that's applicable to individual windows into TerminalWindow. This is almost exclusively a code moving PR. There should be minimal functional changes.

In the next PR, we'll introduce the actual "Process Model v3", merging all Terminal windows into a single terminal process.

Related to #5000. See #5000 (comment) for my current todo list.
Related to #1256.

These commits are all artificially broken down pieces. Honestly, I don't want to really merge them till they're all ready, so we know that the work e2e. This my feigned attempt to break it into digestable PRs.

Lightly manually tested, things seem to still all work? Most of this code was actually written in deeper branches, it was only today I realized it all needed to come back to this branch.

The window persistence fishy-ness of the subsequent PR isn't present here. So that's something.
Localtests still pass

Detailed description

Q: Does AppLogic not keep track of its windows?

Sure doesn't! I didn't think that was something it needed to know.

Q: Why does TerminalWindow (per-window) have access to the commandline args (per-process)

It's because it's not per process. Commandline args are per-window. Consider - you launch the Terminal, then run a wt -w -1 -- foo. That makes its own window. In this process, yes. But that new window has its own commandline args, separate from the ones that started the original process.

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

…word

lhecker

So the idea is to have 1 AppLogic and N WindowLogic instances right?

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/AppLogic.h

zadjii-msft · 2023-02-23T22:55:20Z

So the idea is to have 1 AppLogic and N WindowLogic instances right?

Yep!

…word

carlos-zamora

(Assuming the local tests are passing) are they still passing with this change?

carlos-zamora · 2023-02-27T21:46:00Z

src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl

        void AddTheme(Theme theme);
        INHERITABLE_SETTING(ThemePair, Theme);
        Theme CurrentTheme { get; };
+        Boolean ShouldUsePersistedLayout();


Seems like you can outright remove this and just directly do FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout. Any reason you're not just doing that?

Meh, that check was being used in a bunch of places, so I thought it too brittle to have the same logic everywhere. Thought I should just put it in once place.

In like, the next branch, I'm also changing this to FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout && !IsolatedMode(), so it seems like it will make more sense in the next one 🤷

does the settings model know about Isolated Mode? If it can be specified via command line, I would venture the answer is "no"... and therefore the check for it would almost certainly not belong in TSM!

carlos-zamora · 2023-02-27T22:36:54Z

src/cascadia/TerminalApp/AppLogic.idl

-        // information.
-        Windows.Foundation.IAsyncOperation<Windows.UI.Xaml.Controls.ContentDialogResult> ShowDialog(Windows.UI.Xaml.Controls.ContentDialog dialog);
-        void DismissDialog();
+        event Windows.Foundation.TypedEventHandler<Object, SettingsLoadEventArgs> SettingsChanged;


(half note to self, half actual question) huh, SettingsChanged is in both AppLogic and TerminalWindow. Why both?

IIRC in like one commit, it becomes this:

EDIT: Removed, this was a mild fact after all. Next comment is better

So the emperor listens to the AppLogic, then tells the windows the settings changed, then they each let their pages know that the settings changed.

That ~~might be~~ is a mild fact, lemme double check the next branch

sequenceDiagram participant Emperor participant AppLogic participant AppHost participant TerminalWindow participant TerminalPage Note Right of AppLogic: AL::ReloadSettings AppLogic ->> Emperor: raise SettingsChanged Note left of Emperor: E::...GlobalHotkeys Note left of Emperor: E::...NotificationIcon AppLogic ->> TerminalWindow: raise SettingsChanged<br>(to each window) AppLogic ->> TerminalWindow: AppLogic ->> TerminalWindow: Note right of TerminalWindow: TW::UpdateSettingsHandler Note right of TerminalWindow: TW::UpdateSettings TerminalWindow ->> TerminalPage: SetSettings TerminalWindow ->> AppHost: raise SettingsChanged Note right of AppHost: AH::_HandleSettingsChanged

Loading

Awesome. And out of curiosity, does this mean there's only one copy of the settings model around that everybody queries? Or do we have one for each TerminalWindow?

Ah, here's a relevant excerpt from the next PR:

The emperor can do that - there's only ever one emperor. It can also own a singular copy of the settings model, and hand out references to each other thread.

so yeah, guess that answers that. Nice!

src/cascadia/TerminalApp/AppLogic.idl

src/cascadia/TerminalApp/TerminalWindow.h

src/cascadia/TerminalApp/TerminalWindow.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/TerminalApp/SettingsLoadEventArgs.h

carlos-zamora · 2023-02-28T01:15:12Z

src/cascadia/TerminalApp/App.cpp

@@ -77,22 +77,7 @@ namespace winrt::TerminalApp::implementation
    /// <param name="e">Details about the launch request and process.</param>
    void App::OnLaunched(const LaunchActivatedEventArgs& /*e*/)


Not necessarily as a part of this PR, but can we just outright remove this function and all the other pure UWP stuff that's left over?

Probably. It's Real Dead.

carlos-zamora · 2023-02-28T19:23:55Z

(bumping the question from my review):

(Assuming the local tests are passing) are they still passing with this change?

zadjii-msft · 2023-02-28T19:45:30Z

(bumping the question from my review):

(Assuming the local tests are passing) are they still passing with this change?

Yep

DHowett

Just some light reading!

I find the overall roundaboutness a little bit hard to digest. App Host creates AppLogic and AppLogic creates a Window but AppHost needs to know about Window; AppHost asks Window for properties that Window asks AppLogic for; AppLogic reaches down into the Settings to get them, but only sometimes because sometimes Window reaches into Settings to get them. 🤷🏻

Blocking because of some specific questions about who/what/why and some safety concerns.

src/cascadia/LocalTests_TerminalApp/TabTests.cpp

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/TerminalApp/TerminalWindow.cpp

DHowett · 2023-03-09T15:34:45Z

src/cascadia/TerminalSettingsModel/GlobalAppSettings.idl

        void AddTheme(Theme theme);
        INHERITABLE_SETTING(ThemePair, Theme);
        Theme CurrentTheme { get; };
+        Boolean ShouldUsePersistedLayout();


does the settings model know about Isolated Mode? If it can be specified via command line, I would venture the answer is "no"... and therefore the check for it would almost certainly not belong in TSM!

This removes all the weirdness around the way that TerminalPage needs to track the number of open windows. Instead of TerminalPage persisting empty state when the last tab closes, it lets the AppHost know that the last tab was closed due to _closing the tab_ (as opposed to closing the window / quitting). This gives AppHost an opportunity to persist empty state for that case, because _it_ knows how many windows there are. This could basically be its own PR. Probably worth xlinking this commit to #9800

zadjii-msft · 2023-03-09T19:11:33Z

App Host creates AppLogic and AppLogic creates a Window but AppHost needs to know about Window

This part ends up making more sense in the next PR, you're right that in this iteration, the abstraction is a little... pointless...

…od amount

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

DHowett

Merge after nits. Thanks!

…word

zadjii-msft added 7 commits January 30, 2023 14:52

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 remote-tracking branch 'origin/main' into dev/migrie/oop/3/fore…

cfa6108

…word

let's do it

e40575b

This comment has been minimized.

Sign in to view

zadjii-msft added 2 commits February 13, 2023 09:26

bwahahahahaha

33685d9

spel

a4f19a9

This was referenced Feb 13, 2023

Scenario: Windows Terminal 2.0 Process Model Improvements #5000

Closed

One process to rule them all #14843

Merged

zadjii-msft added 3 commits February 13, 2023 11:48

this was usnused

82224bc

get the tests to build, at least

d9d4d2e

fix the tests

118bffa

zadjii-msft added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-Windowing Window frame, quake mode, tearout labels Feb 14, 2023

zadjii-msft mentioned this pull request Feb 21, 2023

Stop rendering when the terminal has been minimized/control has been hidden #1989

Open

lhecker reviewed Feb 21, 2023

View reviewed changes

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

lhecker reviewed Feb 21, 2023

View reviewed changes

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

zadjii-msft added 2 commits February 23, 2023 16:56

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

273f2a8

…word

std::vectors are better

c79f27c

lhecker approved these changes Feb 27, 2023

View reviewed changes

carlos-zamora reviewed Feb 28, 2023

View reviewed changes

zadjii-msft added 3 commits February 28, 2023 11:22

Lots of PR nits

b589d09

the last nits, I think

0199aba

well that is an underwhelming codeformat commit. Thanks VS

ada3f42

zadjii-msft requested a review from carlos-zamora February 28, 2023 18:04

zadjii-msft mentioned this pull request Mar 6, 2023

Process Model v3 / Tab tear-out ship list #14957

Open

6 tasks

zadjii-msft assigned DHowett Mar 9, 2023

DHowett requested changes Mar 9, 2023

View reviewed changes

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

zadjii-msft added 2 commits March 9, 2023 12:29

PR nits

6dead99

save this for later, we should only need ot look it up once

1b59eb9

zadjii-msft mentioned this pull request Mar 9, 2023

Opening a new terminal window does not restore the previous window's size and position on the screen #12633

Open

zadjii-msft added 5 commits March 9, 2023 13:51

to wit, also make sure that there is a tablayout

39a9450

Redo how ownership of WindowProperties works. This is cleaner by a go…

44b238e

…od amount

fix the tests too

073350e

this is why I don't use VS

ca511c9

I don't trust you phyllis

f70775a

zadjii-msft requested a review from DHowett March 10, 2023 16:05

This was referenced Mar 13, 2023

Introduce a ContentManager helper #14851

Merged

Add support for moving panes and tabs between windows #14866

Merged

Enable dragging tabs between windows #14901

Merged

Enable tearing out tabs to create new windows #14935

Merged

A hotfix for the selfhost build

6aec80b

DHowett reviewed Mar 16, 2023

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved

DHowett reviewed Mar 16, 2023

View reviewed changes

src/cascadia/WindowsTerminal/AppHost.cpp Outdated Show resolved Hide resolved

DHowett approved these changes Mar 16, 2023

View reviewed changes

zadjii-msft added 2 commits March 17, 2023 08:29

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

0808f94

…word

last nits, let's do this

5b3dc08

zadjii-msft merged commit 5b434dc into main Mar 17, 2023

zadjii-msft deleted the dev/migrie/oop/3/foreword branch March 17, 2023 14:17

		@@ -77,22 +77,7 @@ namespace winrt::TerminalApp::implementation
		/// <param name="e">Details about the launch request and process.</param>
		void App::OnLaunched(const LaunchActivatedEventArgs& /e/)

Conversation

zadjii-msft commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Detailed description

Uh oh!

This comment has been minimized.

lhecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft commented Feb 23, 2023

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora commented Feb 28, 2023

Uh oh!

zadjii-msft commented Feb 28, 2023

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DHowett Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Mar 9, 2023

Uh oh!

Uh oh!

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

zadjii-msft commented Feb 10, 2023 •

edited

Loading

DHowett Mar 9, 2023 •

edited

Loading

zadjii-msft Feb 28, 2023 •

edited

Loading

DHowett Mar 9, 2023 •

edited

Loading