Skip to content

Commit

Permalink
Fix loading of fragments that update multiple profiles
Browse files Browse the repository at this point in the history
  • Loading branch information
@lhecker
lhecker committed Oct 24, 2021
1 parent 670ae2b commit bac197c
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 26 deletions.
33 changes: 33 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@
#include "../TerminalSettingsModel/CascadiaSettings.h"
#include "JsonTestClass.h"
#include "TestUtils.h"

#include <defaults.h>
#include <userDefaults.h>

using namespace Microsoft::Console;
using namespace WEX::Logging;
Expand Down Expand Up @@ -70,6 +72,7 @@ namespace SettingsModelLocalTests
TEST_METHOD(TestCloneInheritanceTree);
TEST_METHOD(TestValidDefaults);
TEST_METHOD(TestInheritedCommand);
TEST_METHOD(LoadFragmentsWithMultipleUpdates);

private:
static winrt::com_ptr<implementation::CascadiaSettings> createSettings(const std::string_view& userJSON)
Expand Down Expand Up @@ -1979,4 +1982,34 @@ namespace SettingsModelLocalTests
VERIFY_IS_NULL(actualKeyChord);
}
}

// This test ensures GH#11597 doesn't regress.
void DeserializationTests::LoadFragmentsWithMultipleUpdates()
{
static constexpr std::wstring_view fragmentSource{ L"fragment" };
static constexpr std::string_view fragmentJson{ R"({
"profiles": [
{
"updates": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}",
"cursorShape": "filledBox"
},
{
"updates": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}",
"cursorShape": "filledBox"
},
{
"guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}",
"commandline": "cmd.exe"
}
]
})" };

implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
loader.MergeInboxIntoUserSettings();
loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
loader.FinalizeLayering();

VERIFY_IS_FALSE(loader.duplicateProfile);
VERIFY_ARE_EQUAL(3u, loader.userSettings.profiles.size());
}
}
3 changes: 2 additions & 1 deletion src/cascadia/TerminalSettingsModel/CascadiaSettings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void ApplyRuntimeInitialSettings();
void MergeInboxIntoUserSettings();
void FindFragmentsAndMergeIntoUserSettings();
void MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content);
void FinalizeLayering();
bool DisableDeletedProfiles();

Expand All @@ -82,7 +83,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _parseFragment(const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings);
static JsonSettings _parseJson(const std::string_view& content);
static winrt::com_ptr<implementation::Profile> _parseProfile(const OriginTag origin, const winrt::hstring& source, const Json::Value& profileJson);
void _appendProfile(winrt::com_ptr<implementation::Profile>&& profile, ParsedSettings& settings);
void _appendProfile(winrt::com_ptr<Profile>&& profile, const winrt::guid& guid, ParsedSettings& settings);
static void _addParentProfile(const winrt::com_ptr<implementation::Profile>& profile, ParsedSettings& settings);
void _executeGenerator(const IDynamicProfileGenerator& generator);

Expand Down
62 changes: 37 additions & 25 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,26 +207,6 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings()
{
const auto content = ReadUTF8File(fragmentExt.path());
_parseFragment(source, content, fragmentSettings);

for (const auto& fragmentProfile : fragmentSettings.profiles)
{
if (const auto updates = fragmentProfile->Updates(); updates != winrt::guid{})
{
if (const auto it = userSettings.profilesByGuid.find(updates); it != userSettings.profilesByGuid.end())
{
it->second->InsertParent(0, fragmentProfile);
}
}
else
{
_addParentProfile(fragmentProfile, userSettings);
}
}

for (const auto& kv : fragmentSettings.globals->ColorSchemes())
{
userSettings.globals->AddColorScheme(kv.Value());
}
}
CATCH_LOG();
}
Expand Down Expand Up @@ -289,6 +269,15 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings()
}
}

// See FindFragmentsAndMergeIntoUserSettings.
// This function does the same, but for a single given JSON blob and source
// and at the time of writing is used for unit tests only.
void SettingsLoader::MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content)
{
ParsedSettings fragmentSettings;
_parseFragment(source, content, fragmentSettings);
}

// Call this method before passing SettingsLoader to the CascadiaSettings constructor.
// It layers all remaining objects onto each other (those that aren't covered
// by MergeInboxIntoUserSettings/FindFragmentsAndMergeIntoUserSettings).
Expand Down Expand Up @@ -475,7 +464,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
// GH#9962: Discard Guid-less, Name-less profiles.
if (profile->HasGuid())
{
_appendProfile(std::move(profile), settings);
_appendProfile(std::move(profile), profile->Guid(), settings);
}
}
}
Expand Down Expand Up @@ -517,14 +506,37 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str
auto profile = _parseProfile(OriginTag::Fragment, source, profileJson);
// GH#9962: Discard Guid-less, Name-less profiles, but...
// allow ones with an Updates field, as those are special for fragments.
if (profile->HasGuid() || profile->Updates() != winrt::guid{})
// We need to make sure to only call Guid() if HasGuid() is true,
// as Guid() will dynamically generate a return value otherwise.
const auto guid = profile->HasGuid() ? profile->Guid() : profile->Updates();
if (guid != winrt::guid{})
{
_appendProfile(std::move(profile), settings);
_appendProfile(std::move(profile), guid, settings);
}
}
CATCH_LOG()
}
}

for (const auto& fragmentProfile : settings.profiles)
{
if (const auto updates = fragmentProfile->Updates(); updates != winrt::guid{})
{
if (const auto it = userSettings.profilesByGuid.find(updates); it != userSettings.profilesByGuid.end())
{
it->second->InsertParent(0, fragmentProfile);
}
}
else
{
_addParentProfile(fragmentProfile, userSettings);
}
}

for (const auto& kv : settings.globals->ColorSchemes())
{
userSettings.globals->AddColorScheme(kv.Value());
}
}

SettingsLoader::JsonSettings SettingsLoader::_parseJson(const std::string_view& content)
Expand Down Expand Up @@ -564,11 +576,11 @@ winrt::com_ptr<Profile> SettingsLoader::_parseProfile(const OriginTag origin, co

// Adds a profile to the ParsedSettings instance. Takes ownership of the profile.
// It ensures no duplicate GUIDs are added to the ParsedSettings instance.
void SettingsLoader::_appendProfile(winrt::com_ptr<Profile>&& profile, ParsedSettings& settings)
void SettingsLoader::_appendProfile(winrt::com_ptr<Profile>&& profile, const winrt::guid& guid, ParsedSettings& settings)
{
// FYI: The static_cast ensures we don't move the profile into
// `profilesByGuid`, even though we still need it later for `profiles`.
if (settings.profilesByGuid.emplace(profile->Guid(), static_cast<const winrt::com_ptr<Profile>&>(profile)).second)
if (settings.profilesByGuid.emplace(guid, static_cast<const winrt::com_ptr<Profile>&>(profile)).second)
{
settings.profiles.emplace_back(profile);
}
Expand Down

0 comments on commit bac197c

Please sign in to comment.