Skip to content

Add display name and icon to SUI extensions page #18633

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

carlos-zamora
Copy link
Member

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

Summary of the Pull Request

Adds some polish to the Extensions page in the Settings UI. This includes:

  • For app extensions, extract the display name and icon from the extension package and display it in the UI
  • For dynamic profile generators, add a display name and icon and display it in the UI
  • If possible, prefer to use the display name for breadcrumbs other UI

References and Relevant Issues

Targets #18559

Detailed Description of the Pull Request / Additional comments

  • Settings Model Changes:
    • adds a DisplayName and Icon to IDynamicProfileGenerator
    • extract the DisplayName and Icon from app extensions and use them
    • ExtensionPackage is a new WinRT class to package a bunch of FragmentSettings together
  • Editor changes - view models:
    • Use ExtensionPackages rather than dynamically adding/removing FragmentSettings contents
    • Replace CurrentExtensionSource with CurrentExtensionPackage
  • Editor changes - views:
    • ExtensionPackageTemplateSelector is used to display ExtensionPackages with metadata vs simple ones that just have a source
    • improve accessibility names displayed and read out

Validation

✅ 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

@carlos-zamora
Copy link
Member Author

Demo

{AE427997-B029-4685-8E0F-3FE2761CFE2B}

{91C4546E-BBB8-42C6-B3EF-E6463E1DD9AA}

Comment on lines +391 to +424
<!--
BODGY
Theoretically, you could use a ContentTemplateSelector directly. However, that doesn't work.
For some reason, we just get the object type's ToString called and the selector gets nullptr as a parameter.
Adding the template as a view model property is a workaround.
-->
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 FYI, I spent like most of today figuring out how to work around this. Frustrating 😡

@@ -119,17 +119,19 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
_extensionsVM = *extensionsVMImpl;
_extensionsViewModelChangedRevoker = _extensionsVM.PropertyChanged(winrt::auto_revoke, [=](auto&&, const PropertyChangedEventArgs& args) {
const auto settingName{ args.PropertyName() };
if (settingName == L"CurrentExtensionSource")
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 All the changes in this file are really just because I switched from CurrentExtensionSource to CurrentExtensionPackage. Honestly, it's better this way imo. It aligns better with how the New Tab Menu works as well as the other subpages for profiles

@@ -42,7 +42,6 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
WINRT_OBSERVABLE_PROPERTY(Editor::NewTabMenuViewModel, ViewModel, _PropertyChangedHandlers, nullptr);

private:
Editor::NewTabMenuEntryTemplateSelector _entryTemplateSelector{ nullptr };
Copy link
Member Author

Choose a reason for hiding this comment

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

📝Drive by. We weren't using it at all.

@carlos-zamora

This comment was marked as resolved.

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/extensions-page branch from ec88b32 to 7cdbb7c Compare February 27, 2025 18:17
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/beautify branch from f780523 to cb04e7f Compare February 27, 2025 18:18
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

I had a few nits, but they were so few and far between that I felt like it's not worth bringing up at all. I think the blurry icons are the only major issue.

@DHowett DHowett marked this pull request as draft March 21, 2025 18:34
@carlos-zamora

This comment was marked as outdated.

@carlos-zamora carlos-zamora marked this pull request as ready for review April 16, 2025 18:57
@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 16, 2025
@carlos-zamora
Copy link
Member Author

Updated icons:
image

@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 16, 2025
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/beautify branch from f650f45 to b963d60 Compare April 21, 2025 22:02
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/beautify branch from b963d60 to f25e6fe Compare April 22, 2025 22:13
@carlos-zamora carlos-zamora merged commit f25e6fe into dev/cazamor/sui/extensions-page Apr 24, 2025
11 checks passed
@carlos-zamora carlos-zamora deleted the dev/cazamor/sui/ext-page/beautify branch April 24, 2025 22:24
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.

3 participants