Move TerminalSettings from TermApp to TerminalSettingsModel #9318
Conversation
| Model::TerminalSettings MakeChild() { return *CreateChild(); } | ||
| void SetParent(Model::TerminalSettings parent) | ||
| { | ||
| ClearParents(); | ||
| com_ptr<TerminalSettings> parentImpl; | ||
| parentImpl.attach(get_self<TerminalSettings>(parent)); | ||
| InsertParent(0, parentImpl); | ||
| } |
There was a problem hiding this comment.
These are basically just exposing functions from IInheritable, except exposing them as projected types. Unfortunately, that's why the name of these has to be something similar, but not the same thing.
I could update IInheritable to return projected types, then expose these functions in the IDL. But it feels really weird to expose inheritance outside of TSM. I guess TerminalSettings gets to be special in that way?
There was a problem hiding this comment.
TerminalSettings should always have only one parent. So SetParent makes sense as a name, even though the contents look a bit weird (because IInheritable is designed to handle multiple parents)
| @@ -732,7 +732,8 @@ namespace winrt::TerminalApp::implementation | |||
| void TerminalPage::_OpenNewTab(const NewTerminalArgs& newTerminalArgs) | |||
| try | |||
| { | |||
| auto [profileGuid, settings] = TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings); | |||
There was a problem hiding this comment.
wow crazy how many times we were actually using the guid in the return value of this function too. Shame WinRT won't let us return the pair. Shame.
There was a problem hiding this comment.
I really don't love that this responsibility moved into TSM but ok.
| @@ -0,0 +1,135 @@ | |||
| /*++ | |||
There was a problem hiding this comment.
wow I hate that the github diff for this cannot figure out the file was renamed
There was a problem hiding this comment.
for future reviewers:
git diff --word-diff main:src/cascadia/TerminalApp/TerminalSettings.h origin/dev/cazamor/sui/move-term-sets:src/cascadia/TerminalSettingsModel/TerminalSettings.h
that'll give you the diff for this file by words. Spoiler: it's just renaming namespaces
ghost
requested review from
src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
Outdated
Show resolved
Hide resolved
| @@ -732,7 +732,8 @@ namespace winrt::TerminalApp::implementation | |||
| void TerminalPage::_OpenNewTab(const NewTerminalArgs& newTerminalArgs) | |||
| try | |||
| { | |||
| auto [profileGuid, settings] = TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings); | |||
There was a problem hiding this comment.
I really don't love that this responsibility moved into TSM but ok.
| @@ -1829,7 +1830,7 @@ namespace winrt::TerminalApp::implementation | |||
|
|
|||
| try | |||
| { | |||
| TerminalApp::TerminalSettings controlSettings; | |||
| TerminalSettings controlSettings; | |||
There was a problem hiding this comment.
This constructs an empty TerminalSettings. Is that intended? Make sure it's intended.
There was a problem hiding this comment.
Looks like it isn't. I've changed it to TerminalSettings controlSettings{nullptr}.
This should've been happening before though so the change shouldn't do anything (other than avoiding the construction of an unused TerminalSettings). Right?
There was a problem hiding this comment.
Be very, very sure none of the control flows below this initialization relied on it not being null. I can imagine a world where the conditionals fail but then we initialize a terminal with the "default settings".
There was a problem hiding this comment.
So here's all the possibilities:
splitMode == Duplicate
current_guidfound- ✔️
controlSettingsis set toCreateWithProfileByID(never null)
- ✔️
current_guidnot foundprofileFoundstays asfalse, process to step 2
splitMode != Duplicate(soprofileFoundstays as false)
- ✔️
controlSettingsis set toCreateWithNewTerminalArgs(based onCreateWithProfileByID, so never null)
We could rewrite this to be more like this?
// NOTE: we could
const auto current_guid{ focusedTab->GetFocusedProfile() };
if (splitMode == SplitType::Duplicate && current_guid)
{
// set realGuid and controlSettings
// update working directory
}
else
{
// set realGuid and controlSettings
}That way it would be more clear that we're always setting realGuid and controlSettings. Thoughts?
There was a problem hiding this comment.
As long as it is provable that there is no codepath that tries to initialize with a null settings, I am okay accepting this.
ghost
added
Needs-Author-Feedback
|
Fix the conflicts and let's go! |
DHowett
changed the title
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 (
|
This accomplishes the first step towards embedding a preview on the Profiles/ColorSchemes page, by moving the
TerminalSettingsobject over to the Terminal Settings Model project. We'll leverage this in a later PR to construct an embedded terminal in the settings UI.TerminalSettingshad to see a few more functions exposed in the IDL(including some inheritance stuff).
Refresh the JSON to make TerminalSettings do it's thing across all the
open terminals.
References #9122 - Terminal Preview
References #6800 - SUI Epic