maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix layering of fragment profiles (#11325)

Browse source 
This commit fixes layering of fragment profiles without an update key.
The previous CascadiaSettings deserializer first assembled all builtin
profiles and only then parsed the user's settings.json file.
This meant that even though fragment profiles were added to `_allProfiles`
unconditionally, they did get layered properly with user profiles regardless,
as user profiles were always properly layered.

The new CascadiaSettings approach since 168d28b was a direct translation of this
approach but this is incorrect: As the new approach reads user profiles first,
all inbox profiles, including fragments, must equally use proper layering,
instead of adding profiles unconditionally.

While this commit fixes the bug it maintains a regression:
Duplicate fragment profile GUIDs will not be detected and instead fragments with
identical GUID will all be added as parents to a single user profile.
I considered to fix this regression, but felt that this new behavior is better
than the old one, since a user often can't directly control installed fragments,
and is unlikely to occur in practice. This simplifies the implementation.

## PR Checklist
* [x] Closes #11323
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Fragment layering works ✔️
This commit is contained in:
Leonard Hecker 2021-09-24 18:21:27 +02:00 committed by GitHub
parent 9708a75131
commit 2be394f421
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 25 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/cascadia/TerminalSettingsModel/CascadiaSettings.h
View file

@ -71,6 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
gsl::span<const winrt::com_ptr<implementation::Profile>> _getNonUserOriginProfiles() const;
void _parse(const OriginTag origin, const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings);
void _appendProfile(winrt::com_ptr<implementation::Profile>&& profile, ParsedSettings& settings);
static void _addParentProfile(const winrt::com_ptr<implementation::Profile>& profile, ParsedSettings& settings);
void _executeGenerator(const IDynamicProfileGenerator& generator);
std::unordered_set<std::wstring_view> _ignoredNamespaces;

41
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file

@ -178,22 +178,7 @@ void SettingsLoader::MergeInboxIntoUserSettings()
{
for (const auto& profile : inboxSettings.profiles)
{
if (const auto [it, inserted] = userSettings.profilesByGuid.emplace(profile->Guid(), profile); !inserted)
{
// If inserted is false, we got a matching user profile with identical GUID.
// --> The generated profile is a parent of the existing user profile.
it->second->InsertParent(profile);
}
else
{
// If inserted is true, then this is a generated profile that doesn't exist in the user's settings.
// While emplace() has already created an appropriate entry in .profilesByGuid, we still need to
// add it to .profiles (which is basically a sorted list of .profilesByGuid's values).
//
// When a user modifies a profile they shouldn't modify the (static/constant)
// inbox profile of course. That's why we need to call CreateChild here.
userSettings.profiles.emplace_back(CreateChild(profile));
}
_addParentProfile(profile, userSettings);
}
}
@ -229,7 +214,7 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings()
}
else
{
_appendProfile(CreateChild(fragmentProfile), userSettings);
_addParentProfile(fragmentProfile, userSettings);
}
}
@ -549,6 +534,28 @@ void SettingsLoader::_appendProfile(winrt::com_ptr<Profile>&& profile, ParsedSet
}
}
// If the given ParsedSettings instance contains a profile with the given profile's GUID,
// the profile is added as a parent. Otherwise a new child profile is created.
void SettingsLoader::_addParentProfile(const winrt::com_ptr<implementation::Profile>& profile, ParsedSettings& settings)
{
if (const auto [it, inserted] = settings.profilesByGuid.emplace(profile->Guid(), profile); !inserted)
{
// If inserted is false, we got a matching user profile with identical GUID.
// --> The generated profile is a parent of the existing user profile.
it->second->InsertParent(profile);
}
else
{
// If inserted is true, then this is a generated profile that doesn't exist in the user's settings.
// While emplace() has already created an appropriate entry in .profilesByGuid, we still need to
// add it to .profiles (which is basically a sorted list of .profilesByGuid's values).
//
// When a user modifies a profile they shouldn't modify the (static/constant)
// inbox profile of course. That's why we need to call CreateChild here.
settings.profiles.emplace_back(CreateChild(profile));
}
}
// As the name implies it executes a generator.
// Generated profiles are added to .inboxSettings. Used by GenerateProfiles().
void SettingsLoader::_executeGenerator(const IDynamicProfileGenerator& generator)