Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move TerminalSettings from TermApp to TerminalSettingsModel #9318

Merged
9 commits merged into from
Mar 15, 2021
Merged

Move TerminalSettings from TermApp to TerminalSettingsModel #9318

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cae4ec7
Move TerminalSettings from TermApp to TSM
carlos-zamora Mar 1, 2021
1e968fd
polish PR for review
carlos-zamora Mar 4, 2021
187a510
Merge branch 'main' into dev/cazamor/sui/move-term-sets
carlos-zamora Mar 4, 2021
fc26768
fix tests (verified)
carlos-zamora Mar 6, 2021
e2fb1e7
Merge branch 'main' into dev/cazamor/sui/move-term-sets
carlos-zamora Mar 8, 2021
0eaf930
add comment for BuildSettings
carlos-zamora Mar 9, 2021
3de9894
address Dustin's PR comments
carlos-zamora Mar 11, 2021
1321d76
a bit more polish
carlos-zamora Mar 12, 2021
f3ac13a
Merge branch 'main' into dev/cazamor/sui/move-term-sets
carlos-zamora Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ namespace winrt::TerminalApp::implementation
if (const auto scheme = _settings.GlobalSettings().ColorSchemes().TryLookup(realArgs.SchemeName()))
{
auto controlSettings = activeControl.Settings().as<TerminalSettings>();
controlSettings->ApplyColorScheme(scheme);
controlSettings.ApplyColorScheme(scheme);
activeControl.UpdateSettings();
args.Handled(true);
}
Expand Down Expand Up @@ -658,9 +658,8 @@ namespace winrt::TerminalApp::implementation
newTerminalArgs = NewTerminalArgs();
}

auto [profileGuid, settings] = TerminalSettings::BuildSettings(_settings,
newTerminalArgs,
*_bindings);
const auto profileGuid{ _settings.GetProfileForArgs(newTerminalArgs) };
const auto settings{ TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings) };

// Manually fill in the evaluated profile.
newTerminalArgs.Profile(::Microsoft::Console::Utils::GuidToString(profileGuid));
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/AppLogic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ namespace winrt::TerminalApp::implementation
}

// Use the default profile to determine how big of a window we need.
const auto [_, settings] = TerminalSettings::BuildSettings(_settings, nullptr, nullptr);
const auto settings{ TerminalSettings::BuildSettings(_settings, nullptr, nullptr) };

auto proposedSize = TermControl::GetProposedDimensions(settings, dpi);

Expand Down
5 changes: 1 addition & 4 deletions src/cascadia/TerminalApp/Pane.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,10 +644,7 @@ void Pane::UpdateSettings(const TerminalSettings& settings, const GUID& profile)
{
// Update the parent of the control's settings object (and not the object itself) so
// that any overrides made by the control don't get affected by the reload
auto child = winrt::get_self<winrt::TerminalApp::implementation::TerminalSettings>(_control.Settings());
auto parent = winrt::get_self<winrt::TerminalApp::implementation::TerminalSettings>(settings);
child->ClearParents();
child->InsertParent(0, parent->get_strong());
_control.Settings().as<TerminalSettings>().SetParent(settings);
_control.UpdateSettings();
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/Pane.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class Pane : public std::enable_shared_from_this<Pane>
void ClearActive();
void SetActive();

void UpdateSettings(const winrt::TerminalApp::TerminalSettings& settings,
void UpdateSettings(const winrt::Microsoft::Terminal::Settings::Model::TerminalSettings& settings,
const GUID& profile);
void ResizeContent(const winrt::Windows::Foundation::Size& newSize);
void Relayout();
Expand Down
7 changes: 0 additions & 7 deletions src/cascadia/TerminalApp/TerminalAppLib.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,6 @@
</ClInclude>
<ClInclude Include="Pane.h" />
<ClInclude Include="ColorHelper.h" />
<ClInclude Include="TerminalSettings.h">
<DependentUpon>TerminalSettings.idl</DependentUpon>
</ClInclude>
<ClInclude Include="pch.h" />
<ClInclude Include="ShortcutActionDispatch.h">
<DependentUpon>ShortcutActionDispatch.idl</DependentUpon>
Expand Down Expand Up @@ -205,9 +202,6 @@
<ClCompile Include="Pane.LayoutSizeNode.cpp" />
<ClCompile Include="ColorHelper.cpp" />
<ClCompile Include="DebugTapConnection.cpp" />
<ClCompile Include="TerminalSettings.cpp">
<DependentUpon>TerminalSettings.idl</DependentUpon>
</ClCompile>
<ClCompile Include="pch.cpp">
<PrecompiledHeader>Create</PrecompiledHeader>
</ClCompile>
Expand Down Expand Up @@ -287,7 +281,6 @@
</Midl>
<Midl Include="FilteredCommand.idl" />
<Midl Include="EmptyStringVisibilityConverter.idl" />
<Midl Include="TerminalSettings.idl" />
</ItemGroup>
<!-- ========================= Misc Files ======================== -->
<ItemGroup>
Expand Down
11 changes: 1 addition & 10 deletions src/cascadia/TerminalApp/TerminalAppLib.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@
<ClCompile Include="ColorHelper.cpp" />
<ClCompile Include="DebugTapConnection.cpp" />
<ClCompile Include="Utils.cpp" />
<ClCompile Include="TerminalSettings.cpp">
<Filter>settings</Filter>
</ClCompile>
<ClCompile Include="Jumplist.cpp" />
<ClCompile Include="Tab.cpp">
<Filter>tab</Filter>
Expand Down Expand Up @@ -70,10 +67,7 @@
<ClInclude Include="Commandline.h" />
<ClInclude Include="DebugTapConnection.h" />
<ClInclude Include="ColorHelper.h" />
<ClInclude Include="TerminalSettings.h">
<Filter>settings</Filter>
</ClInclude>
<ClInclude Include="Jumplist.h" />
<ClInclude Include="Jumplist.h" />
<ClInclude Include="Tab.h">
<Filter>tab</Filter>
</ClInclude>
Expand Down Expand Up @@ -113,9 +107,6 @@
<Filter>settings</Filter>
</Midl>
<Midl Include="IDirectKeyListener.idl" />
<Midl Include="TerminalSettings.idl">
<Filter>settings</Filter>
</Midl>
<Midl Include="ITab.idl">
<Filter>tab</Filter>
</Midl>
Expand Down
24 changes: 13 additions & 11 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,8 @@ namespace winrt::TerminalApp::implementation
void TerminalPage::_OpenNewTab(const NewTerminalArgs& newTerminalArgs)
try
{
auto [profileGuid, settings] = TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't love that this responsibility moved into TSM but ok.

const auto profileGuid{ _settings.GetProfileForArgs(newTerminalArgs) };
const auto settings{ TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings) };

_CreateNewTabFromSettings(profileGuid, settings);

Expand Down Expand Up @@ -776,7 +777,7 @@ namespace winrt::TerminalApp::implementation
// currently displayed, it will be shown.
// Arguments:
// - settings: the TerminalSettings object to use to create the TerminalControl with.
void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, TerminalApp::TerminalSettings settings)
void TerminalPage::_CreateNewTabFromSettings(GUID profileGuid, TerminalSettings settings)
{
// Initialize the new tab

Expand All @@ -799,7 +800,7 @@ namespace winrt::TerminalApp::implementation

// Give term control a child of the settings so that any overrides go in the child
// This way, when we do a settings reload we just update the parent and the overrides remain
TermControl term{ *(winrt::get_self<TerminalSettings>(settings)->CreateChild()), connection };
TermControl term{ settings.MakeChild(), connection };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved

auto newTabImpl = winrt::make_self<TerminalTab>(profileGuid, term);

Expand Down Expand Up @@ -882,7 +883,7 @@ namespace winrt::TerminalApp::implementation

if (debugConnection) // this will only be set if global debugging is on and tap is active
{
TermControl newControl{ *(winrt::get_self<TerminalSettings>(settings)->CreateChild()), debugConnection };
TermControl newControl{ settings.MakeChild(), debugConnection };
_RegisterTerminalEvents(newControl, *newTabImpl);
// Split (auto) with the debug tap.
newTabImpl->SplitPane(SplitState::Automatic, 0.5f, profileGuid, newControl);
Expand All @@ -901,7 +902,7 @@ namespace winrt::TerminalApp::implementation
// Return value:
// - the desired connection
TerminalConnection::ITerminalConnection TerminalPage::_CreateConnectionFromSettings(GUID profileGuid,
TerminalApp::TerminalSettings settings)
TerminalSettings settings)
{
const auto profile = _settings.FindProfile(profileGuid);

Expand Down Expand Up @@ -1250,7 +1251,7 @@ namespace winrt::TerminalApp::implementation
const auto& profileGuid = terminalTab->GetFocusedProfile();
if (profileGuid.has_value())
{
const auto settings{ winrt::make<TerminalSettings>(_settings, profileGuid.value(), *_bindings) };
const TerminalSettings settings{ _settings, profileGuid.value(), *_bindings };
const auto workingDirectory = terminalTab->GetActiveTerminalControl().WorkingDirectory();
const auto validWorkingDirectory = !workingDirectory.empty();
if (validWorkingDirectory)
Expand Down Expand Up @@ -1810,7 +1811,7 @@ namespace winrt::TerminalApp::implementation

try
{
TerminalApp::TerminalSettings controlSettings;
TerminalSettings controlSettings;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructs an empty TerminalSettings. Is that intended? Make sure it's intended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here's all the possibilities:

  1. splitMode == Duplicate
  • current_guid found
    • ✔️ controlSettings is set to CreateWithProfileByID (never null)
  • current_guid not found
    • profileFound stays as false, process to step 2
  1. splitMode != Duplicate (so profileFound stays as false)
  • ✔️ controlSettings is set to CreateWithNewTerminalArgs (based on CreateWithProfileByID, 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it is provable that there is no codepath that tries to initialize with a null settings, I am okay accepting this.

GUID realGuid;
bool profileFound = false;

Expand All @@ -1820,7 +1821,7 @@ namespace winrt::TerminalApp::implementation
if (current_guid)
{
profileFound = true;
controlSettings = { winrt::make<TerminalSettings>(_settings, current_guid.value(), *_bindings) };
controlSettings = { _settings, current_guid.value(), *_bindings };
const auto workingDirectory = focusedTab->GetActiveTerminalControl().WorkingDirectory();
const auto validWorkingDirectory = !workingDirectory.empty();
if (validWorkingDirectory)
Expand All @@ -1844,7 +1845,8 @@ namespace winrt::TerminalApp::implementation
}
if (!profileFound)
{
std::tie(realGuid, controlSettings) = TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings);
realGuid = _settings.GetProfileForArgs(newTerminalArgs);
controlSettings = TerminalSettings::BuildSettings(_settings, newTerminalArgs, *_bindings);
}

const auto controlConnection = _CreateConnectionFromSettings(realGuid, controlSettings);
Expand All @@ -1865,7 +1867,7 @@ namespace winrt::TerminalApp::implementation
return;
}

TermControl newControl{ *(winrt::get_self<TerminalSettings>(controlSettings)->CreateChild()), controlConnection };
TermControl newControl{ controlSettings.MakeChild(), controlConnection };

// Hookup our event handlers to the new terminal
_RegisterTerminalEvents(newControl, *focusedTab);
Expand Down Expand Up @@ -2519,7 +2521,7 @@ namespace winrt::TerminalApp::implementation
{
// This can throw an exception if the profileGuid does
// not belong to an actual profile in the list of profiles.
auto settings{ winrt::make<TerminalSettings>(_settings, profileGuid, *_bindings) };
TerminalSettings settings{ _settings, profileGuid, *_bindings };

for (auto tab : _tabs)
{
Expand Down
5 changes: 2 additions & 3 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include "TerminalPage.g.h"
#include "TerminalTab.h"
#include "AppKeyBindings.h"
#include "TerminalSettings.h"

#include <winrt/Microsoft.Terminal.TerminalControl.h>

Expand Down Expand Up @@ -152,8 +151,8 @@ namespace winrt::TerminalApp::implementation
void _CreateNewTabFlyout();
void _OpenNewTabDropdown();
void _OpenNewTab(const Microsoft::Terminal::Settings::Model::NewTerminalArgs& newTerminalArgs);
void _CreateNewTabFromSettings(GUID profileGuid, TerminalApp::TerminalSettings settings);
winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection _CreateConnectionFromSettings(GUID profileGuid, TerminalApp::TerminalSettings settings);
void _CreateNewTabFromSettings(GUID profileGuid, Microsoft::Terminal::Settings::Model::TerminalSettings settings);
winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection _CreateConnectionFromSettings(GUID profileGuid, Microsoft::Terminal::Settings::Model::TerminalSettings settings);

bool _displayingCloseDialog{ false };
void _SettingsButtonOnClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
Expand Down
132 changes: 0 additions & 132 deletions src/cascadia/TerminalApp/TerminalSettings.h
View file
Open in desktop

This file was deleted.

Loading