Improve Profile.Icon UI in settings by carlos-zamora · Pull Request #17965 · microsoft/terminal

carlos-zamora · 2024-09-26T22:25:26Z

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:

None: sets the icon to "none" which is interpreted as no icon
Built-in Icon: presents a combo box that enumerates the Segoe MDL 2 assets
Emoji: presents a text box with a hint to open the emoji picker
File: presents a text box to input the path of the image to use

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

carlos-zamora · 2024-09-26T22:28:33Z

Demo

…rom spellcheck

DHowett · 2024-10-01T20:50:45Z

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 · 2024-10-10T16:46:00Z

Feedback from team sync (10/7):

text boxes: expand that width out further (test narrow window width)
- controls: remove headers
- stack panel orientation vertical --> horizontal
"hidden" --> "none"
- display the translated version
"custom icon" --> "file'
a11y insights fastpass

carlos-zamora · 2024-10-10T20:04:39Z

Demo

DHowett · 2024-10-10T23:05:28Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp


+    void ProfileViewModel::_UpdateBuiltInIcons()
+    {
+        _BuiltInIcons = single_threaded_vector<IInspectable>();


fwiw right now we do this once per profile when we could do it once per settings UI

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

if (!_BuiltInIcons)

{

_UpdateBuiltInIcons();

}

terminal/src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

Lines 165 to 168 in bb32896

if (!_BuiltInIcons)

{

_UpdateBuiltInIcons();

}

Both of which are wrapped in a null-check.

Confirmed w/ breakpoint in _UpdateBuiltInIcons() and poking around SUI (and closing/reopening SUI).

Oh jeez you're a step ahead of me!

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

lhecker · 2024-10-15T22:35:11Z

src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp

+        for (auto& [val, name] : s_SegoeFluentIcons)
+        {
+            _BuiltInIcons.Append(make<EnumEntry>(hstring{ name }, box_value(val)));
+        }


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.

https://learn.microsoft.com/en-us/windows/win32/api/winstring/nf-winstring-windowscreatestringreference eh

zadjii-msft · 2024-10-21T10:56:16Z

src/cascadia/TerminalSettingsEditor/Profiles_Base.xaml

+                                 Grid.Column="1"
+                                 MaxWidth="Infinity"
+                                 HorizontalAlignment="Stretch"
+                                 FontFamily="Segoe UI, Segoe Fluent Icons, Segoe MDL2 Assets"


pretty sure we've got a resource for this: https://github.com/search?q=repo%3Amicrosoft%2Fterminal%20SymbolThemeFontFamily&type=code

## 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)

Improve Profile.Icon UI in settings

6bdace7

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

split fluent icon names

9b03fd9

This comment has been minimized.

Sign in to view

Fix a few more words that make sense; exclude SegoeFluentIconList.h f…

941a792

…rom spellcheck

carlos-zamora added the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 3, 2024

carlos-zamora mentioned this pull request Oct 8, 2024

Add New Tab Menu Customization to Settings UI #18015

Merged

4 tasks

carlos-zamora mentioned this pull request Oct 10, 2024

Feature request: option to remove profile icon completely #17943

Closed

apply feedback from design review

bb32896

carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 10, 2024

Fix icon preview when targetting exe

39059c4

DHowett approved these changes Oct 10, 2024

View reviewed changes

remove redundant Icon notification

f407ab1

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Oct 10, 2024

microsoft-github-policy-service bot requested review from DHowett, PankajBhojwani, lhecker and zadjii-msft October 10, 2024 23:35

microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Oct 10, 2024

DHowett approved these changes Oct 10, 2024

View reviewed changes

lhecker approved these changes Oct 15, 2024

View reviewed changes

zadjii-msft approved these changes Oct 21, 2024

View reviewed changes

DHowett merged commit 460b180 into dev/cazamor/SUI/nullable-color-picker Oct 22, 2024

DHowett deleted the dev/cazamor/sui/icon branch October 22, 2024 21:32

carlos-zamora mentioned this pull request Nov 21, 2024

Add nullable colors and improve Profile.Icon in settings UI #17870

Merged

6 tasks

carlos-zamora mentioned this pull request Dec 3, 2024

New Tab Menu Customization SUI Follow-ups #18281

Open

6 tasks

Conversation

carlos-zamora commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

Uh oh!

This comment was marked as resolved.

Uh oh!

carlos-zamora commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demo

Uh oh!

This comment has been minimized.

This comment has been minimized.

DHowett commented Oct 1, 2024

Uh oh!

carlos-zamora commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlos-zamora commented Oct 10, 2024

Demo

Uh oh!

DHowett Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhecker Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

DHowett Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carlos-zamora commented Sep 26, 2024 •

edited

Loading

carlos-zamora commented Sep 26, 2024 •

edited

Loading

carlos-zamora commented Oct 10, 2024 •

edited

Loading