-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Add Extensions page to Settings UI #18559
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.
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -59,7 +59,7 @@ | |||
<IconSourceElement Grid.Column="0" | |||
Width="16" | |||
Height="16" | |||
IconSource="{x:Bind mtu:IconPathConverter.IconSourceWUX(Icon), Mode=OneTime}" /> | |||
IconSource="{x:Bind mtu:IconPathConverter.IconSourceWUX(EvaluatedIcon), Mode=OneTime}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Admittedly, a drive by. Happy to pull this out into a separate PR if desired.
@@ -1199,7 +1199,7 @@ | |||
<Setter Property="HorizontalAlignment" Value="Stretch" /> | |||
<Setter Property="CornerRadius" Value="{ThemeResource ControlCornerRadius}" /> | |||
<Setter Property="VerticalAlignment" Value="Stretch" /> | |||
<Setter Property="HorizontalContentAlignment" Value="Left" /> | |||
<Setter Property="HorizontalContentAlignment" Value="Stretch" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Had to update this style a bit to enable functionality for showing "Disabled" on the right side when an extension was disabled.
The way I made that work is that we have a 2-column grid. We need the grid to take up the entire horizontal content, so we need to set HorizontalContentAlignmen to stretch
<!-- TODO CARLOS: Icon --> | ||
<!-- TODO GH #18281: Icon --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 This one's on me 😅
@@ -227,6 +227,45 @@ | |||
</Setter> | |||
</Style> | |||
|
|||
<!-- A basic setting container displaying immutable text as content --> | |||
<Style x:Key="SettingContainerWithTextContent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Used to display the Scope for an extension. Also allowed me to apply the SecondaryTextBlockStyle to the text shown on the right side (aka the value of Scope)
for (const auto&& entry : fragExt.ModifiedProfilesView()) | ||
{ | ||
// Ensure entry successfully modifies a profile before creating and registering the object | ||
if (const auto& deducedProfile = _settings.FindProfile(entry.ProfileGuid())) | ||
{ | ||
auto vm = winrt::make<FragmentProfileViewModel>(entry, fragExt, deducedProfile); | ||
currentProfilesModified.push_back(vm); | ||
if (extensionEnabled) | ||
{ | ||
profilesModifiedTotal.push_back(vm); | ||
} | ||
} | ||
} | ||
|
||
for (const auto&& entry : fragExt.NewProfilesView()) | ||
{ | ||
// Ensure entry successfully points to a profile before creating and registering the object. | ||
// The profile may have been removed by the user. | ||
if (const auto& deducedProfile = _settings.FindProfile(entry.ProfileGuid())) | ||
{ | ||
auto vm = winrt::make<FragmentProfileViewModel>(entry, fragExt, deducedProfile); | ||
currentProfilesAdded.push_back(vm); | ||
if (extensionEnabled) | ||
{ | ||
profilesAddedTotal.push_back(vm); | ||
} | ||
} | ||
} | ||
|
||
for (const auto&& entry : fragExt.ColorSchemesView()) | ||
{ | ||
for (const auto& schemeVM : _colorSchemesPageVM.AllColorSchemes()) | ||
{ | ||
if (schemeVM.Name() == entry.ColorSchemeName()) | ||
{ | ||
auto vm = winrt::make<FragmentColorSchemeViewModel>(entry, fragExt, schemeVM); | ||
currentColorSchemesAdded.push_back(vm); | ||
if (extensionEnabled) | ||
{ | ||
colorSchemesAddedTotal.push_back(vm); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only displaying the ones that were added successfully and point to something that already exists.
Long-term, do we want to do more here? Perhaps show the ones that weren't added successfully? I'm just not sure what the next steps would be and how we would want to approach this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stake: I want to be the second reviewer on this :)
ec88b32
to
7cdbb7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed everything inside TerminalSettingsModel
I'm wondering if there's something we can do to make all of this cheaper. We're now storing JSON blobs - sometimes multiple times (parsed and regenerated at different nesting levels) - for every fragment and every generated profile... for a scenario that 9/10 launches aren't going to experience (most people probably do not enter the settings UI, right?)
@@ -8,6 +8,12 @@ import "DefaultTerminal.idl"; | |||
|
|||
namespace Microsoft.Terminal.Settings.Model | |||
{ | |||
enum FragmentScope | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭App?
String Source { get; }; | ||
FragmentScope Scope { get; }; | ||
String Json { get; }; | ||
String JsonSource { get; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 what's a JsonSource vs a Source and a Json?
{ | ||
String ColorSchemeName { get; }; | ||
String Json { get; }; | ||
Boolean Conflict { get; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 conflict, with what?
// copy fragment extensions | ||
{ | ||
std::vector<Model::FragmentSettings> fragmentExtensions; | ||
fragmentExtensions.reserve(_fragmentExtensions.Size()); | ||
for (const auto& fragment : _fragmentExtensions) | ||
{ | ||
fragmentExtensions.emplace_back(get_self<FragmentSettings>(fragment)->Copy()); | ||
} | ||
settings->_fragmentExtensions = winrt::single_threaded_vector(std::move(fragmentExtensions)); | ||
} | ||
|
||
// copy dynamic profile generators | ||
{ | ||
std::vector<Model::FragmentSettings> dynamicProfileGenerators; | ||
dynamicProfileGenerators.reserve(_dynamicProfileGeneratorExtensions.Size()); | ||
for (const auto& fragment : _dynamicProfileGeneratorExtensions) | ||
{ | ||
dynamicProfileGenerators.emplace_back(get_self<FragmentSettings>(fragment)->Copy()); | ||
} | ||
settings->_dynamicProfileGeneratorExtensions = winrt::single_threaded_vector(std::move(dynamicProfileGenerators)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, so, these are notionally immutable parts of the settings environment... right?
Since they're fully immutable, is there a strong need for us to deep clone them? The settings UI can't edit them after all, and they never get Saved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same applies below
_scope{ scope }, | ||
_modifiedProfiles{ winrt::single_threaded_vector<Model::FragmentProfileEntry>() }, | ||
_newProfiles{ winrt::single_threaded_vector<Model::FragmentProfileEntry>() }, | ||
_colorSchemes{ winrt::single_threaded_vector<Model::FragmentColorSchemeEntry>() } {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will allocate three blank vectors for every kind of fragment and dynamic profile generator, the majority of which will never populate more than one of them.
if (_ignoredNamespaces.contains(std::wstring_view{ packageName })) | ||
{ | ||
continue; | ||
} | ||
|
||
// Likewise, getting the public folder from an extension is an async operation. | ||
auto foundFolder = extractValueFromTaskWithoutMainThreadAwait(ext.GetPublicFolderAsync()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slight and maybe meaningful behavior difference: now we must read the extension's files, which goes through this async WinRT call. Disabling the profile source used to disable all of the async logic here, now it doesn't. Food for thought, not a complaint.
@@ -345,7 +349,7 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings() | |||
void SettingsLoader::MergeFragmentIntoUserSettings(const winrt::hstring& source, const std::string_view& content) | |||
{ | |||
ParsedSettings fragmentSettings; | |||
_parseFragment(source, content, fragmentSettings); | |||
_parseFragment(source, content, fragmentSettings, FragmentScope::User, hstring{ L"filename.json" }, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this dummy filename now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and why are we constructing an entire hstring to then pass as a wstring_view
?
auto profile = _parseProfile(OriginTag::Fragment, source, profileJson); | ||
if (const auto guid = profile->Guid(); profile->HasGuid() && guid != winrt::guid{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto profile = _parseProfile(OriginTag::Fragment, source, profileJson); | |
if (const auto guid = profile->Guid(); profile->HasGuid() && guid != winrt::guid{}) | |
const auto guid = profile->HasGuid() ? profile->Guid() : profile->Updates(); | |
const auto destinationSet = profile->HasGuid() ? fragmentSettings->NewProfiles() : fragmentSettings->ModifiedProfiles(); | |
if (guid != winrt::guid{}) | |
{ | |
if (applyToSettings) | |
{ | |
_appendProfile(std::move(profile), guid, settings); | |
} | |
destinationSet.Append(winrt::make<FragmentProfileEntry>(guid, hstring{ til::u8u16(Json::writeString(styledWriter, profileJson)) })); | |
} |
No point in introducing more common subexpressions if we don't have to!
I could not delete the below code with my suggestion, so take this as a deletion too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also keeps the layout of the original code more intact, and avoids reading guid
until we have actually checked HasGuid
:P
_addUserProfileParent(fragmentProfile); | ||
if (!_addOrMergeUserColorScheme(fragmentColorScheme)) | ||
{ | ||
// Color scheme wasn't added because it conflicted with a non-user created scheme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only captures the case where it was conflicted with another FRAGMENT color scheme. It doesn't know if it failed because it conflicted a built-in color scheme. Or, do those not conflict?
wait, i am confused. :P
for (const auto& entry : profileEntries) | ||
{ | ||
generatorExtension->NewProfiles().Append(entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is inefficient; can you just reach in and make a new single_threaded_observable_vector
out of profileEntries
?
Summary of the Pull Request
Adds an Extensions page to the Settings UI. This lets you enable/disable extensions and see how they affect your settings (i.e. adding/modifying profiles and adding color schemes). This page is specifically designed for fragment extensions and dynamic profile generators, but can be expanded on in the future as we develop a more advanced extensions model.
Detailed Description of the Pull Request / Additional comments
FragmentSettings
represents a parsed json fragment extension.FragmentProfileEntry
andFragmentColorSchemeEntry
are used to track profiles and color schemes added/modifiedExtensionsViewModel
operates as the main view model for the page.FragmentProfileViewModel
andFragmentColorSchemeViewModel
are used to reference specific components of fragments. They also provide support for navigating to the linked profile or color scheme via the settings UI!ExtensionPackageViewModel
is a VM for a group of extensions exposed by a single source. This is mainly needed because a single source can have multiple JSON fragments in it. This is used for the navigators on the main page. Can be extended to provide additional information (i.e. package logo, package name, etc.)Extensions.xaml
uses a lot of data templates. These are reused inItemsControl
s to display extension components.Follow-ups
AppExtension