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 Adds customization for the New Tab Menu to the settings UI. - Settings Model changes: - The Settings UI generally works by creating a copy of the entire settings model objects on which we apply the changes to. Turns out, we completely left the NewTabMenu out of that process. So I went ahead and implemented it. - `FolderEntry` - `FolderEntry` exposes `Entries()` (used by the new tab menu to figure out what to actually render) and `RawEntries()` (the actual JSON data deserialized into settings model objects). I went ahead and exposed `RawEntries()` since we'll need to apply changes to it to then serialize. - View Model: - `NewTabMenuViewModel` is the main view model that interacts with the page. It maintains the current view of items and applies changes to the settings model. - `NewTabMenuEntryViewModel` and all of the other `_EntryViewModel` classes are wrappers for the settings model NTM entries. - `FolderTreeViewEntry` encapsulates `FolderEntryViewModel`. It allows us to construct a `TreeView` of just folders. - View changes and additions: - Added FontIconGlyph to the SettingContainer - Added a New Tab Menu item to the navigation view - Adding entries: a stack of SettingContainers is used here. We use the new `FontIconGlyph` to make this look nice! - Reordering entries: drag and drop is supported! This might not work in admin mode though, and we can't drag and drop into folders. Buttons were added to make this keyboard accessible. - To move entries into a folder, a button was added which then displays a TreeView of all folders. - Multiple entries can be moved to a folder or deleted at once! - Breadcrumbs are used for folders - When a folder is entered, additional controls are displayed to customize that folder. ## Verification - ✅ a11y pass - ✅ keyboard accessible - scenarios: - ✅ add entries (except actions) - ✅ changes propagated to settings model (aka "saving works") - ✅ reorder entries - ✅ move entries to an existing folder - ✅ delete multiple entries - ✅ delete individual entries - ✅ display entries (including actions) ## Follow-ups - [ ] add support for adding and editing action entries - [ ] when we discard changes or save, it would be cool if we could stay on the same page - [ ] allow customizing the folder entry _before_ adding it (current workaround is to add it, then edit it) - [ ] improve UI for setting icon (reuse UI from #17965)
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