Add UI for adding, renaming, and deleting a color scheme #8403

carlos-zamora · 2020-11-25T20:50:17Z

Introduces the following UI controls to the ColorSchemes page:

"Add new" button
- next to dropdown selector
- adds a new color scheme named ("Color Scheme #" where # is the number of color schemes you have)
"Rename" Button
- next to the selector
- replaces the ComboBox with a TextBox and the accept/cancel buttons appear
"Delete" button
- bottom of the page
- opens flyout, when confirmed, deletes the current color scheme and selects another one

This also adds a Delete button to the Profiles page. The Hide checkbox was moved above the Delete button.

References

#1564 - Settings UI
#6800 - Settings UI Completion Epic

Detailed Description of the Pull Request / Additional comments

Color Schemes:

Deleting a color scheme selects another one from the list available
Rename replaces the combobox with a textbox to allow editing
The Add New button creates a new color scheme named "Color Scheme X" where X is the number of schemes defined
In-box color schemes cannot be deleted

Profile:

Deleting a profile selects another one from the list available
the rename button does not exist (yet), because it needs a modification to the NavigationView's Header Template
The delete button is disabled for in-box profiles (CMD and Windows Powershell) and dynamic profiles

Validation Steps Performed

Color Schemes - Add New
✅ Creates a new color scheme named "Color Scheme X" (X being the number of color schemes)
✅ The new color scheme can be renamed/deleted/modified

Color Schemes - Rename
✅ You cannot rename an in-box color scheme
✅ The rename button has a tooltip
✅ Clicking the rename button replaces the combobox with a textbox
✅ Accept --> changes name
✅ Cancel --> does not change the name
✅ accepting/cancelling the rename operation updates the combo box appropriately

Color Schemes - Delete
✅ Clicking delete produces a flyout to confirm deletion
✅ Deleting a color scheme removes it from the list and select the one under it
✅ Deleting the last color scheme selects the last available color scheme after it's deleted
✅ In-box color schemes have the delete button disabled, and a disclaimer appears next to it

Profile- Delete
✅ Base layer presents a disclaimer at the top, and hides the delete button
✅ Dynamic and in-box profiles disable the delete button and show the appropriate disclaimer next to the disabled button
✅ Clicking delete produces a flyout to confirm deletion
✅ Regular profiles have a delete button that is styled appropriately
✅ Clicking the delete profile button opens a content dialog. Confirmation deletes the profile and navigates to the profile indexed under it (deleting the last one redirects to the last one)

Demo

Refer to this post [here](#8403 (comment).
Confirmation flyout demo: #8403 (comment)

This commit introduces a rough prototype of the Settings UI as the `TerminalSettingsEditor`. This project was added to OpenConsole.sln and deploys as a standalone app. Some databinding is configured to a fake TerminalSettingsModel (located in the ObjectModel folder). This commit will start the settings UI feature branch, which will receive a full review on merge-back into main. References #6720 - Settings UI Spec and Design References #1564 - Settings UI Feature/Epic

This PR's only purpose is to convert the TSE project into a DLL while getting rid of unnecessary files in the meantime, namely the `App.*` files and moving its `ResourceDictionary` into a file called `CommonResources.xaml`. In the process, I needed to move all the `Model` classes under `Editor` since it would have been a huge pain to try to add a Model dll alongside the Editor dll. I figured it also wouldn't matter too much since we'll be deleting the `Model` namespace in favor of using the incoming TSM. While this doesn't contain any code of putting the Settings UI in a tab, I figured it would be nice to push this particular part as its own standalone PR.

This commit introduces a rough prototype of the Settings UI as the `TerminalSettingsEditor`. This project was added to OpenConsole.sln and deploys as a standalone app. Some databinding is configured to a fake TerminalSettingsModel (located in the ObjectModel folder). This commit will start the settings UI feature branch, which will receive a full review on merge-back into main. References #6720 - Settings UI Spec and Design References #1564 - Settings UI Feature/Epic

This PR's only purpose is to convert the TSE project into a DLL while getting rid of unnecessary files in the meantime, namely the `App.*` files and moving its `ResourceDictionary` into a file called `CommonResources.xaml`. In the process, I needed to move all the `Model` classes under `Editor` since it would have been a huge pain to try to add a Model dll alongside the Editor dll. I figured it also wouldn't matter too much since we'll be deleting the `Model` namespace in favor of using the incoming TSM. While this doesn't contain any code of putting the Settings UI in a tab, I figured it would be nice to push this particular part as its own standalone PR.

…nto feature/settings-ui

…7802) This PR's goal was to add an option to the `OpenSettings` keybinding to open the Settings UI in a tab. In order to implement that, a couple of changes had to be made to `Tab`, specifically: - Introduce a tab interface named `ITab` - Create/Rename two new Tab classes that implement `ITab` called `SettingsTab` and `TerminalTab` I've also put some `TODOs` that I wanted to get some thoughts on - I'll make a follow up PR but perhaps they can be revisited when we flesh out the Settings UI more. - Does a Settings UI tab shutdown need to do anything special for cleanup? - Does a Settings UI tab need to have `GetActiveTitle`? Maybe depending on which page in the UI is open? - Technically, I can't focus a `Grid` control, so I'll need to figure out what to `Focus` when the tab is selected. - The Settings UI tab doesn't have a `TermControl`, so once focus is moved to that tab, users won't be able to `nextTab/prevTab` out of it (along with all other keybindings). References #1564, #5915

## Summary of the Pull Request Moved all strings into resources file for localization.  ## References #1564 - Epic  ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments  ## Validation Steps Performed

Added save and reset buttons. Added warning message for unsaved settings. ![save/reset buttons and warning](https://user-images.githubusercontent.com/48369326/96289952-b6221800-0f9a-11eb-9d98-bd62c17aca19.png) ## References #1564 - Settings UI Epic

This import is required to make localized resources work.

This PR's main goal was to get rid of all our ObjectModel references and replace them with references to TSM. There's a lot of places in the SUI where I wasn't able to easily drop in the TSM. Usually those settings types aren't as simple as a boolean, so they'll require some templating and finessing. For those settings, I've either commented them out for now or attempted to replace them. Here's a TLDR of what I've done in this PR. - Since `MainPage` is the entry point, it gets a `CascadiaSettings` object to hold on to, and that's the settings object the rest of the pages will bind to. - Deleted everything inside of `ObjectModel` along with their references. - Replaced the ObjectModel references in the `.xaml` files with a reference to the settings object obtained in `MainPage` - Commented out a couple of settings here and there that might need converters and/or templating. - Attempted to write out most of the templating and data binding code for `ColorSchemes.xaml`

## Summary of the Pull Request Fixes the last bit of missing localization for the settings UI  ## References  ## PR Checklist * [ ] Closes #xxx * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments  ## Validation Steps Performed

## Summary of the Pull Request Updated icons for: - Startup - Appearance - Color schemes (moved the palette icon from Appearance to here) - Profiles ![image](https://user-images.githubusercontent.com/48369326/96916583-97b99200-145c-11eb-9301-df7b824fb1da.png)  ## References  ## PR Checklist * [ ] Closes #xxx * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments  ## Validation Steps Performed

This PR fleshes out the Settings UI's ColorSchemes page so that it's able to display all your defined color schemes. It'll allow the user to modify all the colors of the scheme through either a color picker or entering a hex color in the TextBoxes. If the user enters an invalid hex code, the TextBox will automatically revert back to the original hex. Since we haven't hooked up the save button just yet, making any changes to a color scheme will modify the color scheme only for the currently open Terminal instance. References #1564

When I did the polishing I forgot to do the Advanced page under profiles. I fixed that here.

## Summary of the Pull Request Removed the following from the navigation menu: - Search bar - Home page - Keyboard page Changed landing page to Launch page ![image](https://user-images.githubusercontent.com/48369326/97476068-3afe2180-190b-11eb-9c9b-9b0c07a6974c.png)  ## References #1564  ## PR Checklist * [ ] Closes #xxx * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments  ## Validation Steps Performed

This PR will make the "Open JSON file" navigation view item in the Settings UI will open the user's settings file. Normally, `NavigationViewItems` raise their parent's `ItemInvoked` handler, but when a `NavViewItem` is in the `NavigationView.PaneFooter`, it doesn't do that. So, we have to hook up `OpenJson`'s `Tapped` and `KeyDown` handler to detect both a tap event and the enter keydown events. References #1564

## Summary of the Pull Request Fixed the resizing bug where long text buttons get cut off, hid the unsaved changes warning, and changed "save" to "apply" ![MicrosoftTeams-image (1)](https://user-images.githubusercontent.com/48369326/97494176-2f1e5980-1923-11eb-8980-604bd5a067e5.png) ![image](https://user-images.githubusercontent.com/48369326/97494207-3a718500-1923-11eb-9409-7a95ecb50536.png)  ## References #1564  ## PR Checklist * [ ] Closes #xxx * [ ] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [ ] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx  ## Detailed Description of the Pull Request / Additional comments  ## Validation Steps Performed

carlos-zamora · 2020-12-17T17:48:56Z

I’m still holding out for that flyout on the delete button asking the user to confirm it! It’s a destructive operation, doing it instantly without asking will result in data loss

Since you can see the name behind the dialog, figured it'd be fine to show not include the name of the profile/scheme in the content dialog. But I'm open to changing that. Just let me know 😊

DHowett · 2020-12-17T18:20:38Z

Can we not use a simple flyout? Content dialog seems too heavyhanded. I don’t mean “flyout”, i mean Flyout. The class.

htcfreek · 2020-12-17T18:39:32Z

I’m still holding out for that flyout on the delete button asking the user to confirm it! It’s a destructive operation, doing it instantly without asking will result in data loss

Since you can see the name behind the dialog, figured it'd be fine to show not include the name of the profile/scheme in the content dialog. But I'm open to changing that. Just let me know 😊

Should we define the "no" button as default?

carlos-zamora · 2020-12-17T19:41:27Z

Replaced the contentdialog with the flyout so it feels less aggressive. Loving it!

Demo - Flyouts

DHowett

14/30

src/cascadia/TerminalSettingsModel/Profile.idl

src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj.filters

src/cascadia/TerminalSettingsModel/Microsoft.Terminal.Settings.ModelLib.vcxproj

src/cascadia/TerminalSettingsModel/ColorScheme.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

src/cascadia/TerminalSettingsEditor/Profiles.idl

src/cascadia/TerminalSettingsEditor/Profiles.h

DHowett · 2020-12-17T20:05:28Z

src/cascadia/TerminalSettingsEditor/Profiles.cpp

@@ -64,6 +100,24 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
        {
            biButton.IsChecked(biButton.Tag().as<int32_t>() == biAlignmentVal);
        }
+
+        // Set the text disclaimer for the text box
+        // For an unknown reason, data-binding the text does not compile (using x:Bind)


every compiler error has a reason.

(don't leave comments like this.)

Removing the "for an unknown reason..." part. IIRC, the error came up in the XAML code, but I don't have XAML symbols with me so I can't look into it all the way.

src/cascadia/TerminalSettingsEditor/Profiles.cpp

I'm unblocking this because I'm unsure if I'll have a chance to get back to this tonight 😬

zadjii-msft

Okay yea, passing the event handler in the state is wack, but I don't have a better idea.

zadjii-msft · 2020-12-17T22:36:21Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

@@ -233,19 +235,12 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

    void MainPage::_Navigate(const Editor::ProfileViewModel& profile)
    {
-        auto state{ winrt::make_self<ProfilePageNavigationState>(profile, _settingsClone.GlobalSettings().ColorSchemes(), *this) };
+        auto state{ winrt::make<ProfilePageNavigationState>(profile, _settingsClone.GlobalSettings().ColorSchemes(), *this) };


woah wait why is this a make and not a make_self? Just curious

No reason, really. I've been using winrt::make for all the other navigation states so I guess to be consistent

DHowett · 2020-12-17T22:57:46Z

src/cascadia/TerminalSettingsModel/ColorScheme.cpp

+    _Foreground{ defaultFg },
+    _Background{ defaultBg },
+    _SelectionBackground{ DEFAULT_FOREGROUND },
+    _CursorColor{ cursorColor }


DHowett

Looks great. I love it.

ghost · 2020-12-17T23:07:53Z

Hello @carlos-zamora!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

) Introduces the following UI controls to the ColorSchemes page: - "Add new" button - next to dropdown selector - adds a new color scheme named ("Color Scheme #" where # is the number of color schemes you have) - "Rename" Button - next to the selector - replaces the ComboBox with a TextBox and the accept/cancel buttons appear - "Delete" button - bottom of the page - opens flyout, when confirmed, deletes the current color scheme and selects another one This also adds a Delete button to the Profiles page. The Hide checkbox was moved above the Delete button. ## References microsoft#1564 - Settings UI microsoft#6800 - Settings UI Completion Epic ## Detailed Description of the Pull Request / Additional comments **Color Schemes:** - Deleting a color scheme selects another one from the list available - Rename replaces the combobox with a textbox to allow editing - The Add New button creates a new color scheme named "Color Scheme X" where X is the number of schemes defined - In-box color schemes cannot be deleted **Profile:** - Deleting a profile selects another one from the list available - the rename button does not exist (yet), because it needs a modification to the NavigationView's Header Template - The delete button is disabled for in-box profiles (CMD and Windows Powershell) and dynamic profiles ## Validation Steps Performed **Color Schemes - Add New** ✅ Creates a new color scheme named "Color Scheme X" (X being the number of color schemes) ✅ The new color scheme can be renamed/deleted/modified **Color Schemes - Rename** ✅ You cannot rename an in-box color scheme ✅ The rename button has a tooltip ✅ Clicking the rename button replaces the combobox with a textbox ✅ Accept --> changes name ✅ Cancel --> does not change the name ✅ accepting/cancelling the rename operation updates the combo box appropriately **Color Schemes - Delete** ✅ Clicking delete produces a flyout to confirm deletion ✅ Deleting a color scheme removes it from the list and select the one under it ✅ Deleting the last color scheme selects the last available color scheme after it's deleted ✅ In-box color schemes have the delete button disabled, and a disclaimer appears next to it **Profile- Delete** ✅ Base layer presents a disclaimer at the top, and hides the delete button ✅ Dynamic and in-box profiles disable the delete button and show the appropriate disclaimer next to the disabled button ✅ Clicking delete produces a flyout to confirm deletion ✅ Regular profiles have a delete button that is styled appropriately ✅ Clicking the delete profile button opens a content dialog. Confirmation deletes the profile and navigates to the profile indexed under it (deleting the last one redirects to the last one) ## Demo Refer to this post [here](microsoft#8403 (comment). Confirmation flyout demo: microsoft#8403 (comment)

stephenanthonylewis

L

DHowett · 2021-02-09T18:34:16Z

@stephenanthonylewis Do explain why it was necessary to sign off on an already-completed pull request from three months ago three times and e-mail the entire team about it 😉

carlos-zamora and others added 30 commits September 17, 2020 18:33

Merge branch 'feature/settings-ui' of github.com:microsoft/terminal i…

b87f6d6

…nto feature/settings-ui

merging from master

09677fd

Merge branch 'master' into feature/settings-ui

e002752

Merge branch 'master' into feature/settings-ui

0ce05b5

Make sure TSE pulls in the wildcard resource targets (#7967)

8213cf1

This import is required to make localized resources work.

Merge remote-tracking branch 'origin/master' into HEAD

788e685

merge from master

f5e9d95

Add scrollviewers to settings UI (#7998)

3b04ba9

Merge remote-tracking branch 'origin/master' into HEAD

d3d128d

Merge remote-tracking branch 'origin/main' into HEAD

52b0915

Merge remote-tracking branch 'origin/main' into feature/settings-ui

15b4565

Merge remote-tracking branch 'origin/main' into feature/settings-ui

9c4d77a

Fix build for broken merge in 15b4565

b757e82

Fix spacing on profile advanced page (#8065)

22805bd

When I did the polishing I forgot to do the Advanced page under profiles. I fixed that here.

Merge branch 'main' into feature/settings-ui

78545a0

Merge branch 'main' into feature/settings-ui

1a36ca6

add confirmation dialog to delete buttons

a3d6695

apply changes from design review w/ cinnamon

cc8dec1

DHowett requested changes Dec 17, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 17, 2020

carlos-zamora added 4 commits December 17, 2020 15:59

address Dustin's PR comments

dd2ef1c

move and wrap disclaimer

a2ebc74

manually hide CS delete flyout

541f46c

accept enter for CS rename

fd8db96

carlos-zamora added 2 commits December 17, 2020 16:29

reduce standard min width to prevent cropping on resize

4436762

fix Profile page scrolling bug

556be58

carlos-zamora unassigned zadjii-msft Dec 17, 2020

zadjii-msft approved these changes Dec 17, 2020

View reviewed changes

carlos-zamora unassigned cinnamon-msft Dec 17, 2020

DHowett reviewed Dec 17, 2020

View reviewed changes

DHowett approved these changes Dec 17, 2020

View reviewed changes

carlos-zamora unassigned DHowett Dec 17, 2020

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 17, 2020

ghost merged commit 33470ad into main Dec 17, 2020

ghost deleted the dev/cazamor/sui/rename-and-delete branch December 17, 2020 23:14

stephenanthonylewis approved these changes Feb 9, 2021

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI for adding, renaming, and deleting a color scheme #8403

Add UI for adding, renaming, and deleting a color scheme #8403

carlos-zamora commented Nov 25, 2020 •

edited by DHowett

Loading

carlos-zamora commented Dec 17, 2020

DHowett commented Dec 17, 2020

htcfreek commented Dec 17, 2020

carlos-zamora commented Dec 17, 2020

DHowett left a comment

DHowett Dec 17, 2020

DHowett Dec 17, 2020

carlos-zamora Dec 17, 2020

zadjii-msft left a comment

zadjii-msft Dec 17, 2020

carlos-zamora Dec 17, 2020

DHowett Dec 17, 2020

DHowett left a comment

ghost commented Dec 17, 2020

stephenanthonylewis left a comment

DHowett commented Feb 9, 2021

Add UI for adding, renaming, and deleting a color scheme #8403

Add UI for adding, renaming, and deleting a color scheme #8403

Conversation

carlos-zamora commented Nov 25, 2020 • edited by DHowett Loading

References

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Demo

carlos-zamora commented Dec 17, 2020

DHowett commented Dec 17, 2020

htcfreek commented Dec 17, 2020

carlos-zamora commented Dec 17, 2020

Demo - Flyouts

DHowett left a comment

Choose a reason for hiding this comment

DHowett Dec 17, 2020

Choose a reason for hiding this comment

DHowett Dec 17, 2020

Choose a reason for hiding this comment

carlos-zamora Dec 17, 2020

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Dec 17, 2020

Choose a reason for hiding this comment

carlos-zamora Dec 17, 2020

Choose a reason for hiding this comment

DHowett Dec 17, 2020

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

ghost commented Dec 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

stephenanthonylewis left a comment

Choose a reason for hiding this comment

DHowett commented Feb 9, 2021

carlos-zamora commented Nov 25, 2020 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.