-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
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.
| 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.
| <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
| Style="{StaticResource TextBlockSubHeaderStyle}" /> | ||
|
|
||
| <!-- 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 😅
| </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.
DHowett
left a comment
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
This comment has been minimized.
This comment has been minimized.
DHowett
left a comment
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.
Ideally we would have scaled-up versions of the generator icons as well. Some of them may be easy to get, others may be less easy to get. We can do this as followup work.
DHowett
left a comment
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.
Very close now!
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
| parseAndLayerFragmentFiles(fragmentExtFolder.path(), winrt::hstring{ source }); | ||
| parseAndLayerFragmentFiles(fragmentExtFolder.path(), | ||
| winrt::hstring{ source }, | ||
| rfid == FOLDERID_LocalAppData ? FragmentScope::User : FragmentScope::Machine); // scope |
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.
fwiw you could structure this as follows:
auto scope = FragmentScope::User;
for(const auto& rfid : std::array{ RFID, RFID }) {
for (...) {
something(scope);
}
// Since the outer loop only has two entries, set the scope for the next run
scope = FragmentScope::Machine;
}that lets you get rid of one pesky ternary and a comparison against one of the array entries; TBD which is more fragile
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.
Personally, I find it more fragile to set the scope once. Makes me feel a little icky haha.
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
| { | ||
| if (!_extensionPackages) | ||
| { | ||
| // Lazy load the ExtensionPackage objects |
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.
FUNNY. I bet we only call this on the CLONED one, so the cache is always empty (clone the base settings object without the extensions, get the extensions, fill the cache)
I think this is fine, fwiw, because it results in users seeing up-to-date fragment information on every settings UI load ;p
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.
Sorta. When we Copy(), we completely copy over _extensionPackages. So we should lazy load them on the clone when we open the SUI the first time. Then "discard changes" should regenerate it (as you said) because _settingsSource hasn't loaded it. But --
Yeah nevermind, you're right. We could make it static, but meh, I agree with you, it's nice to see up-to-date fragment info.
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.
When we Copy(), we completely copy over _extensionPackages.
Which is nullptr :) (I suspect you figured this out)
src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
|
Ugh, now my local VS version won't let me build the project. Updated from 17.14 (broken) to 17.14.2 (broken) too, so it looks like I can't rollback 😭. The changes I made should all be safe though, but please confirm when you come back to approve it and we can merge the PR. |
|
Reverted back to a version of VS that can build the project. Tested changes locally. We're good to go 😊 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |

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.
App extensions extract the name and icon from the extension package and display it in the UI. Dynamic profile generators extract the name and icon from the generator and display it in the UI. We prefer to use the display name for breadcrumbs when possible.
A "NEW" badge was added to the Extensions page's
NavigationViewItemto highlight that it's new. It goes away once the user visits it.Detailed Description of the Pull Request / Additional comments
FragmentSettingsrepresents a parsed json fragment extension.FragmentProfileEntryandFragmentColorSchemeEntryare used to track profiles and color schemes added/modifiedExtensionPackagebundles theFragmentSettingstogether. This is how we represent multiple JSON files in one extension.IDynamicProfileGeneratorexposes aDisplayNameandIconExtensionPackages created from app extensions extract theDisplayNameandIconfrom the extensionApplicationStateis used to track which badges have been dismissed and prevent them from appearing againstd::unordered_setis used to keep track of the dismissed badges, but we only expose a get and append function via the IDL to interact with itExtensionsViewModeloperates as the main view model for the page.FragmentProfileViewModelandFragmentColorSchemeViewModelare used to reference specific components of fragments. They also provide support for navigating to the linked profile or color scheme via the settings UI!ExtensionPackageViewModelis 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.)CurrentExtensionPackageis used to track which extension package is currently in view, if applicable (similar to how the new tab menu page works)Extensions.xamluses a lot of data templates. These are reused inItemsControls to display extension components.ExtensionPackageTemplateSelectoris used to displayExtensionPackages with metadata vs simple ones that just have a source (i.e. Git)NewInfoBadgestyle that is just an InfoBadge with "New" in it instead of a number or an icon. Based on Add "new" labels to navigation PowerToys#36939getcall to theApplicationStateconducted via theExtensionsPageViewModel. The VM is also responsible for updating the state.CascadiaSettingsSerialization.cpp. TheSettingsLoadercan be used specifically to load and create the extension objects.Validation Steps
✅ Keyboard navigation feels right
✅ Screen reader reads all info on screen properly
✅ Accessibility Insights FastPass found no issues
✅ "Discard changes" retains subpage, but removes any changes
✅ Extensions page nav item displays a badge if page hasn't been visited
✅ The badge is dismissed when the user visits the page
Follow-ups
SettingContainer: display the badge and add logic to read/writeApplicationStateappropriately (similarly to above)XPageViewModel:InfoBadge.ValueNewInfoBadgestyle