-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
Add display name and icon to SUI extensions page #18633
Conversation
src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp
Outdated
Show resolved
Hide resolved
<!-- | ||
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. | ||
--> |
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.
📝 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") |
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.
📝 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 }; |
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.
📝Drive by. We weren't using it at all.
This comment was marked as resolved.
This comment was marked as resolved.
ec88b32
to
7cdbb7c
Compare
f780523
to
cb04e7f
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.
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.
This comment was marked as outdated.
This comment was marked as outdated.
src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp
Outdated
Show resolved
Hide resolved
f650f45
to
b963d60
Compare
b963d60
to
f25e6fe
Compare
f25e6fe
into
dev/cazamor/sui/extensions-page
Summary of the Pull Request
Adds some polish to the Extensions page in the Settings UI. This includes:
References and Relevant Issues
Targets #18559
Detailed Description of the Pull Request / Additional comments
DisplayName
andIcon
toIDynamicProfileGenerator
DisplayName
andIcon
from app extensions and use themExtensionPackage
is a new WinRT class to package a bunch ofFragmentSettings
togetherExtensionPackage
s rather than dynamically adding/removingFragmentSetting
s contentsCurrentExtensionSource
withCurrentExtensionPackage
ExtensionPackageTemplateSelector
is used to displayExtensionPackage
s with metadata vs simple ones that just have a sourceValidation
✅ 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