Skip to content

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Feb 11, 2025

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 NavigationViewItem to highlight that it's new. It goes away once the user visits it.

Detailed Description of the Pull Request / Additional comments

  • Settings Model changes:
    • FragmentSettings represents a parsed json fragment extension.
    • FragmentProfileEntry and FragmentColorSchemeEntry are used to track profiles and color schemes added/modified
    • ExtensionPackage bundles the FragmentSettings together. This is how we represent multiple JSON files in one extension.
    • IDynamicProfileGenerator exposes a DisplayName and Icon
    • ExtensionPackages created from app extensions extract the DisplayName and Icon from the extension
    • ApplicationState is used to track which badges have been dismissed and prevent them from appearing again
    • a std::unordered_set is used to keep track of the dismissed badges, but we only expose a get and append function via the IDL to interact with it
  • Editor changes - view models:
    • ExtensionsViewModel operates as the main view model for the page.
    • FragmentProfileViewModel and FragmentColorSchemeViewModel 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.)
    • CurrentExtensionPackage is used to track which extension package is currently in view, if applicable (similar to how the new tab menu page works)
  • Editor changes - views:
    • Extensions.xaml uses a lot of data templates. These are reused in ItemsControls to display extension components.
    • ExtensionPackageTemplateSelector is used to display ExtensionPackages with metadata vs simple ones that just have a source (i.e. Git)
    • Added a NewInfoBadge style that is just an InfoBadge with "New" in it instead of a number or an icon. Based on Add "new" labels to navigation PowerToys#36939
    • The visibility is bound to a get call to the ApplicationState conducted via the ExtensionsPageViewModel. The VM is also responsible for updating the state.
  • Lazy loading extension objects
    • Since most instances of Terminal won't actually open the settings UI, it doesn't make sense to create all the extension objects upon startup. Instead, we defer creating those objects until the user actually navigates to the Extensions page. This is most of the work that happened in CascadiaSettingsSerialization.cpp. The SettingsLoader can 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

  • Streamline a process for adding extensions from the new page
  • Long-term, we can reuse the InfoBadge system and make the following minor changes:
    • SettingContainer: display the badge and add logic to read/write ApplicationState appropriately (similarly to above)
    • XPageViewModel:
      • count all the badges that will be displayed and expose/bind that to InfoBadge.Value
      • If a whole page is new, we can just style the badge using the NewInfoBadge style

@carlos-zamora

This comment was marked as outdated.

@carlos-zamora

This comment was marked as resolved.

@carlos-zamora carlos-zamora marked this pull request as ready for review February 19, 2025 20:18
@carlos-zamora
Copy link
Member Author

Demo

Extensions Page v0.2

@@ -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}" />
Copy link
Member Author

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" />
Copy link
Member Author

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 -->
Copy link
Member Author

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"
Copy link
Member Author

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)

Comment on lines 130 to 184
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);
}
}
}
}
Copy link
Member Author

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.

lhecker
lhecker previously approved these changes Feb 25, 2025
Copy link
Member

@DHowett DHowett left a 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 :)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 27, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 27, 2025
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/extensions-page branch from ec88b32 to 7cdbb7c Compare February 27, 2025 18:17
Copy link
Member

@DHowett DHowett left a 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
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭App?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatcha mean? Like, add App as a possible scope? FragmentScope currently has User and Machine to align with what the docs say about applications installed from the web. Would App align with Microsoft store apps? Would we present it as App to the user in the Settings UI?

{
String ColorSchemeName { get; };
String Json { get; };
Boolean Conflict { get; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 conflict, with what?

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());
Copy link
Member

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.

_addUserProfileParent(fragmentProfile);
if (!_addOrMergeUserColorScheme(fragmentColorScheme))
{
// Color scheme wasn't added because it conflicted with a non-user created scheme.
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting! You're right, settings.colorSchemes only contains the ones generated by fragments at this point. But this is also the only time we attempt to add this color scheme in.

Honestly, I suspect that there could be a bug somewhere here about adding conflicting color schemes, but we're purposefully not changing any logic here. We're just marking it so that we can do something with that information later, but even then, I'm never actually retrieving Conflict anywhere in the SUI/code.

Should I just back out the Conflict() change altogether then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yeah, I'm going to undo the whole Conflict thing. We're not using it and that's an edge case to handle separately and better tbh.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 18, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 21, 2025
const auto guid = profile->HasGuid() ? profile->Guid() : profile->Updates();
auto destinationSet = profile->HasGuid() ? newProfiles : modifiedProfiles;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto destinationSet = profile->HasGuid() ? newProfiles : modifiedProfiles;
auto destinationSet = profile->HasGuid() ? &newProfiles : &modifiedProfiles;

_addUserProfileParent(fragmentProfile);
if (!_addOrMergeUserColorScheme(fragmentColorScheme))
{
// Color scheme wasn't added because it conflicted with a non-user created scheme.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yeah, I'm going to undo the whole Conflict thing. We're not using it and that's an edge case to handle separately and better tbh.

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Apr 24, 2025

Latest changes merged in some of the other branches and PRs:

This is now ready to go 😊

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants