Improve Profile.Icon UI in settings #17965
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Demo
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
FWIW should we rename "hidden" to the more obvious "no icon"? Hidden suggests that there is an icon, but that it is hidden. |
carlos-zamora
added
the
Needs-Discussion
|
Feedback from team sync (10/7):
|
Demo
|
carlos-zamora
removed
the
Needs-Discussion
| @@ -98,6 +127,58 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
| _defaultAppearanceViewModel.IsDefault(true); | |||
| } | |||
|
|
|||
| void ProfileViewModel::_UpdateBuiltInIcons() | |||
| { | |||
| _BuiltInIcons = single_threaded_vector<IInspectable>(); | |||
There was a problem hiding this comment.
fwiw right now we do this once per profile when we could do it once per settings UI
There was a problem hiding this comment.
Nah, _BuiltInIcons is static so it's a shared instance across all ProfileVMs.
And we only call it twice:
terminal/src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
Lines 116 to 119 in bb32896
terminal/src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp
Lines 165 to 168 in bb32896
Both of which are wrapped in a null-check.
Confirmed w/ breakpoint in _UpdateBuiltInIcons() and poking around SUI (and closing/reopening SUI).
There was a problem hiding this comment.
Oh jeez you're a step ahead of me!
microsoft-github-policy-service
bot
requested review from
zadjii-msft,
PankajBhojwani,
DHowett and
lhecker
microsoft-github-policy-service
bot
removed
the
Needs-Second
| for (auto& [val, name] : s_SegoeFluentIcons) | ||
| { | ||
| _BuiltInIcons.Append(make<EnumEntry>(hstring{ name }, box_value(val))); | ||
| } |
There was a problem hiding this comment.
I'd recommend the "first construct a std::vector, then move it into an IVector" approach here given that you append a lot of items.
BTW it's a funny thought but technically it's completely 100% possible to create a custom HSTRING that does not the need a heap allocation. The HSTRING struct is part of the ABI.
There was a problem hiding this comment.
| Grid.Column="1" | ||
| MaxWidth="Infinity" | ||
| HorizontalAlignment="Stretch" | ||
| FontFamily="Segoe UI, Segoe Fluent Icons, Segoe MDL2 Assets" |
There was a problem hiding this comment.
pretty sure we've got a resource for this: https://github.com/search?q=repo%3Amicrosoft%2Fterminal%20SymbolThemeFontFamily&type=code
DHowett
merged commit 460b180
into
dev/cazamor/SUI/nullable-color-picker
Summary of the Pull Request
Improves the UI for the Profile.Icon setting by adding an "Icon Type" combo box. This allows the user to pick from multiple options:
Additionally, the rendered icon is displayed in the setting container. If "none", "none" is presented to the user (localized).
✅ Verified as accessible using Accessibility Insights
#10000