Skip to content

Commit

Permalink
Process actions sync. on startup; don't dupe nonexistent profile (#5090)
Browse files Browse the repository at this point in the history
This PR has evolved to encapsulate two related fixes that I can't really
untie anymore.

#2455 - Duplicating a tab that doesn't exist anymore

This was the bug I was originally fixing in #4429. 

When the user tries to `duplicateTab` with a profile that doesn't exist
anymore (like might happen after a settings reload), don't crash.

As I was going about adding tests for this, got blocked by the fact that
the Terminal couldn't open _any_ panes while the `TerminalPage` was size
0x0. This had two theoretical solutions: 

* Fake the `TerminalPage` into thinking it had a real size in the test -
  probably possible, though I'm unsure how it would work in practice.
* Change `Pane`s to not require an `ActualWidth`, `ActualHeight` on
  initialization. 

Fortuately, the second option was something else that was already on my
backlog of bugs. 

#4618 - `wt` command-line can't consistently parse more than one arg

Presently, the Terminal just arbitrarily dispatches a bunch of handlers
to try and handle all the commands provided on the commandline. That's
lead to a bunch of reports that not all the commands will always get
executed, nor will they all get executed in the same order. 

This PR also changes the `TerminalPage` to be able to dispatch all the
commands sequentially, all at once in the startup. No longer will there
be a hot second where the commands seem to execute themselves in from of
the user - they'll all happen behind the scenes on startup. 

This involved a couple other changes areound the `TerminalPage`
* I had to make sure that panes could be opened at a 0x0 size. Now they
  use a star sizing based off the percentage of the parent they're
  supposed to consume, so that when the parent _does_ get laid out,
  they'll take the appropriate size of that parent.
* I had to do some math ahead of time to try and calculate what a
  `SplitState::Automatic` would be evaluated as, despite the fact that
  we don't actually know how big the pane will be. 
* I had to ensure that `focus-tab` commands appropriately mark a single
  tab as focused while we're in startup, without roundtripping to the
  Dispatcher thread and back

## References

#4429 - the original PR for #2455
#5047 - a follow-up task from discussion in #4429
#4953 - a PR for making panes use star sizing, which was immensly
        helpful for this PR.

## Detailed Description of the Pull Request / Additional comments

`CascadiaSettings::BuildSettings` can throw if the GUID doesn't exist.
This wraps those calls up with a try/catch.

It also adds a couple tests - a few `SettingsTests` for try/catching
this state. It also adds a XAML-y test in `TabTests` that creates a
`TerminalPage` and then performs som UI-like actions on it. This test
required a minor change to how we generate the new tab dropdown - in the
tests, `Application::Current()` is _not_ a `TerminalApp::App`, so it
doesn't have a `Logic()` to query. So wrap that in a try/catch as well.

While working on these tests, I found that we'd crash pretty agressively
for mysterious reasons if the TestHostApp became focused while the test
was running. This was due to a call in
`TSFInputControl::NotifyFocusEnter` that would callback to
`TSFInputControl::_layoutRequested`, which would crash on setting the
`MaxSize` of the canvas to a negative value. This PR includes a hotfix
for that bug as well. 

## Validation Steps Performed

* Manual testing with a _lot_ of commands in a commandline
* run the tests
* Team tested in selfhost

Closes #2455
Closes #4618
  • Loading branch information
zadjii-msft authored Mar 26, 2020
1 parent 0461a2a commit b3fa88e
Show file tree
Hide file tree
Showing 11 changed files with 879 additions and 148 deletions.
144 changes: 144 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,10 @@ namespace TerminalAppLocalTests

TEST_METHOD(TestTerminalArgsForBinding);

TEST_METHOD(FindMissingProfile);
TEST_METHOD(MakeSettingsForProfileThatDoesntExist);
TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist);

TEST_METHOD(TestLayerProfileOnColorScheme);

TEST_METHOD(ValidateKeybindingsWarnings);
Expand Down Expand Up @@ -2094,6 +2098,146 @@ namespace TerminalAppLocalTests
}
}

void SettingsTests::FindMissingProfile()
{
// Test that CascadiaSettings::FindProfile returns null for a GUID that
// doesn't exist
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"profiles": [
{
"name" : "profile0",
"guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}"
},
{
"name" : "profile1",
"guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}"
}
]
})" };
const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
auto settings = CascadiaSettings::FromJson(settingsJsonObj);

const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");

const Profile* const profile1 = settings->FindProfile(guid1);
const Profile* const profile2 = settings->FindProfile(guid2);
const Profile* const profile3 = settings->FindProfile(guid3);

VERIFY_IS_NOT_NULL(profile1);
VERIFY_IS_NOT_NULL(profile2);
VERIFY_IS_NULL(profile3);

VERIFY_ARE_EQUAL(L"profile0", profile1->GetName());
VERIFY_ARE_EQUAL(L"profile1", profile2->GetName());
}

void SettingsTests::MakeSettingsForProfileThatDoesntExist()
{
// Test that MakeSettings throws when the GUID doesn't exist
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"profiles": [
{
"name" : "profile0",
"guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"historySize": 1
},
{
"name" : "profile1",
"guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"historySize": 2
}
]
})" };
const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
auto settings = CascadiaSettings::FromJson(settingsJsonObj);

const auto guid1 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-1111-49a3-80bd-e8fdd045185c}");
const auto guid2 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-2222-49a3-80bd-e8fdd045185c}");
const auto guid3 = Microsoft::Console::Utils::GuidFromString(L"{6239a42c-3333-49a3-80bd-e8fdd045185c}");

try
{
auto terminalSettings = settings->BuildSettings(guid1);
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(1, terminalSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
}

try
{
auto terminalSettings = settings->BuildSettings(guid2);
VERIFY_ARE_NOT_EQUAL(nullptr, terminalSettings);
VERIFY_ARE_EQUAL(2, terminalSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
}

VERIFY_THROWS(auto terminalSettings = settings->BuildSettings(guid3), wil::ResultException, L"This call to BuildSettings should fail");

try
{
const auto [guid, termSettings] = settings->BuildSettings(nullptr);
VERIFY_ARE_NOT_EQUAL(nullptr, termSettings);
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
}
}

void SettingsTests::MakeSettingsForDefaultProfileThatDoesntExist()
{
// Test that MakeSettings _doesnt_ throw when we load settings with a
// defaultProfile that's not in the list, we validate the settings, and
// then call MakeSettings(nullopt). The validation should ensure that
// the default profile is something reasonable
const std::string settingsString{ R"(
{
"defaultProfile": "{6239a42c-3333-49a3-80bd-e8fdd045185c}",
"profiles": [
{
"name" : "profile0",
"guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}",
"historySize": 1
},
{
"name" : "profile1",
"guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}",
"historySize": 2
}
]
})" };
const auto settingsJsonObj = VerifyParseSucceeded(settingsString);
auto settings = CascadiaSettings::FromJson(settingsJsonObj);
settings->_ValidateSettings();

VERIFY_ARE_EQUAL(2u, settings->_warnings.size());
VERIFY_ARE_EQUAL(2u, settings->_profiles.size());
VERIFY_ARE_EQUAL(settings->_globals.GetDefaultProfile(), settings->_profiles.at(0).GetGuid());
try
{
const auto [guid, termSettings] = settings->BuildSettings(nullptr);
VERIFY_ARE_NOT_EQUAL(nullptr, termSettings);
VERIFY_ARE_EQUAL(1, termSettings.HistorySize());
}
catch (...)
{
VERIFY_IS_TRUE(false, L"This call to BuildSettings should succeed");
}
}

void SettingsTests::TestLayerProfileOnColorScheme()
{
Log::Comment(NoThrowString().Format(
Expand Down
Loading

0 comments on commit b3fa88e

Please sign in to comment.