Split AppLogic into "App logic" and "Window logic"
#14825
Conversation
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
This comment has been minimized.
This comment has been minimized.
zadjii-msft
added
Product-Terminal
Yep! |
| @@ -108,5 +108,6 @@ namespace Microsoft.Terminal.Settings.Model | |||
| void AddTheme(Theme theme); | |||
| INHERITABLE_SETTING(ThemePair, Theme); | |||
| Theme CurrentTheme { get; }; | |||
| Boolean ShouldUsePersistedLayout(); | |||
There was a problem hiding this comment.
Seems like you can outright remove this and just directly do FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout. Any reason you're not just doing that?
There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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!
| // 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; |
There was a problem hiding this comment.
(half note to self, half actual question) huh, SettingsChanged is in both AppLogic and TerminalWindow. Why both?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
| @@ -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*/) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Probably. It's Real Dead.
|
(bumping the question from my review):
|
Yep |
There was a problem hiding this comment.
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.
| @@ -108,5 +108,6 @@ namespace Microsoft.Terminal.Settings.Model | |||
| void AddTheme(Theme theme); | |||
| INHERITABLE_SETTING(ThemePair, Theme); | |||
| Theme CurrentTheme { get; }; | |||
| Boolean ShouldUsePersistedLayout(); | |||
There was a problem hiding this comment.
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!
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
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
This part ends up making more sense in the next PR, you're right that in this iteration, the abstraction is a little... pointless... |
followed by: #14843
Map
AppLogicinto "App logic" and "Window logic" #14825 <---- YOU ARE HEREAnd 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:AppLogicper process, shared across all Terminal windows.AppHost,TerminalWindow, andTerminalPagefor 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.
Detailed description
Sure doesn't! I didn't think that was something it needed to know.
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.