Add New Tab Menu Customization to Settings UI #18015

carlos-zamora · 2024-10-08T22:46:47Z

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 Improve Profile.Icon UI in settings #17965)

src/cascadia/TerminalSettingsEditor/NewTabMenuViewModel.cpp

src/cascadia/TerminalSettingsEditor/NewTabMenuViewModel.h

carlos-zamora · 2024-10-08T22:49:01Z

Demo

Top-level view

"Move to folder" dialog

Edit Folder view

lhecker

TBH nothing in this PR was a problem for me, although there are some nits. I'm mostly curious how it'll feel in practice (once it's in Canary). 🙂

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalSettingsEditor/ViewModelHelpers.h

src/cascadia/TerminalSettingsEditor/NewTabMenuViewModel.cpp

src/cascadia/TerminalSettingsEditor/NewTabMenu.cpp

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

PankajBhojwani

Played around with the branch, it's super cool! Mostly just nits but a couple blockers particularly around the repeated revoking/setting up of the same event handler

PankajBhojwani · 2024-11-15T18:31:56Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

+                else
+                {
+                    // If we don't have a current folder, we're at the root of the NTM
+                    contentFrame().Navigate(xaml_typename<Editor::NewTabMenu>(), _newTabMenuPageVM);


Nit: can the navigate call can be moved out of the if/else block? It's the same call either way

PankajBhojwani · 2024-11-15T19:16:24Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

+            // Reset the current folder entry and page BEFORE setting up the event handling
+            _newTabMenuPageVM.CurrentFolder(nullptr);
+
+            _SetupNTMEventHandling();


related to the other comment - we shouldn't need this call if we don't revoke the handler in _PreNavigateHelper

PankajBhojwani · 2024-11-15T19:27:50Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

+        _PreNavigateHelper();
+
+        _SetupNTMEventHandling();


Same here, we're revoking the handler in _PreNavigateHelper and then immediately setting it back up in _SetupNTMEventHandling

PankajBhojwani · 2024-11-15T19:34:20Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

@@ -301,6 +317,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
    void MainPage::_PreNavigateHelper()
    {
        _profileViewModelChangedRevoker.revoke();
+        _ntmViewModelChangedRevoker.revoke();


_newTabMenuPageVM doesn't change, so why are we revoking our event handler for it and re-setting up the event handler? See the way we set up the event handling for _colorSchemesPageVM - we should only need to set up all the event handling once and not repeatedly revoke it and set it up again

PankajBhojwani · 2024-11-15T19:36:45Z

src/cascadia/TerminalSettingsEditor/MainPage.idl

@@ -19,7 +19,8 @@ namespace Microsoft.Terminal.Settings.Editor
        Profile_Appearance,
        Profile_Terminal,
        Profile_Advanced,
-        ColorSchemes_Edit
+        ColorSchemes_Edit,
+        NTM_Folder


nit: can we call this NewTabMenu_Folder so it's consistent with ColorSchemes_Edit and more readable?

PankajBhojwani · 2024-11-15T21:36:36Z

src/cascadia/TerminalSettingsEditor/NewTabMenuViewModel.cpp

+        });
+
+        // Clear CurrentFolder to reset the view
+        _CurrentFolder = nullptr;


Instead of clearing CurrentFolder, can we first check whether the previous CurrentFolder exists and then reset that as the current folder? Similar to the way the ColorSchemesPageVM resets the current scheme if that scheme still exists

PankajBhojwani · 2024-11-15T22:08:24Z

src/cascadia/TerminalSettingsEditor/NewTabMenuViewModel.idl

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+import "ProfileViewModel.idl";


PankajBhojwani · 2024-11-15T22:13:24Z

src/cascadia/TerminalSettingsEditor/NewTabMenuViewModel.cpp

+
+    bool NewTabMenuViewModel::IsFolderView() const noexcept
+    {
+        return CurrentView() != _rootEntries;


nit: probably more readable if this is return _CurrentFolder != nullptr? Took me a bit to understand what this was used for haha

PankajBhojwani · 2024-11-15T22:21:48Z

src/cascadia/TerminalSettingsModel/FolderEntry.cpp

+#include "ProfileEntry.h"
+#include "SeparatorEntry.h"
+#include "RemainingProfilesEntry.h"
+#include "MatchProfilesEntry.h"
+#include "ActionEntry.h"


Do we need these?

PankajBhojwani · 2024-11-15T22:24:13Z

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

+#include "FolderEntry.h"
+#include "ProfileEntry.h"
+#include "SeparatorEntry.h"
+#include "RemainingProfilesEntry.h"
+#include "MatchProfilesEntry.h"
+#include "ActionEntry.h"


Same here, do we need these?

carlos-zamora · 2024-11-19T22:07:02Z

carlos-zamora added 5 commits September 16, 2024 17:15

Add New Tab Menu Customization to Settings UI

0e304d8

[broken] drag and drop don't work

2cef0c1

Add nested folder support (and serialization)

cda72b6

Add ability to modify current folder properties

577ca8b

last bit of polish

4d5616c

github-advanced-security bot found potential problems Oct 8, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

carlos-zamora added 3 commits October 8, 2024 16:51

Add support for action entries

99d2d39

Display icons for actions and folders

a0a84f9

accessibility pass

4b85165

carlos-zamora marked this pull request as ready for review October 9, 2024 22:44

lhecker reviewed Oct 11, 2024

View reviewed changes

carlos-zamora added 3 commits October 28, 2024 14:32

make NewTabMenuEntry::Copy virtual

78de36b

use std::vector; remove extra resources; remove redundant var

8fd6d9f

Merge branch 'main' into dev/cazamor/SUI/newTabMenu

0944064

This comment was marked as resolved.

Sign in to view

Merge branch 'main' into dev/cazamor/SUI/newTabMenu

4574b3d

lhecker approved these changes Nov 4, 2024

View reviewed changes

thanks Leonard!

babcd9f

PankajBhojwani requested changes Nov 15, 2024

View reviewed changes

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 Nov 15, 2024

microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add New Tab Menu Customization to Settings UI #18015

Add New Tab Menu Customization to Settings UI #18015

carlos-zamora commented Oct 8, 2024 •

edited

Loading

carlos-zamora commented Oct 8, 2024

This comment has been minimized.

lhecker left a comment

This comment was marked as resolved.

PankajBhojwani left a comment

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

PankajBhojwani Nov 15, 2024

carlos-zamora commented Nov 19, 2024 •

edited

Loading

Add New Tab Menu Customization to Settings UI #18015

Are you sure you want to change the base?

Add New Tab Menu Customization to Settings UI #18015

Conversation

carlos-zamora commented Oct 8, 2024 • edited Loading

Summary of the Pull Request

Verification

Follow-ups

carlos-zamora commented Oct 8, 2024

Demo

Top-level view

"Move to folder" dialog

Edit Folder view

This comment has been minimized.

lhecker left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

PankajBhojwani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Nov 19, 2024 • edited Loading

Feedback from Bug Bash (11/19)

carlos-zamora commented Oct 8, 2024 •

edited

Loading

carlos-zamora commented Nov 19, 2024 •

edited

Loading