From fa5eb5f75e2c9804257b807760e58da675811e40 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 11 Nov 2020 11:36:29 -0500 Subject: [PATCH] conflict --- .github/actions/spell-check/expect/expect.txt | 1 - custom.props | 2 +- doc/cascadia/Json-Utility-API.md | 17 +- doc/cascadia/profiles.schema.json | 11 +- .../LocalTests_SettingsModel/CommandTests.cpp | 13 +- .../DeserializationTests.cpp | 4 +- .../KeyBindingsTests.cpp | 12 +- .../LocalTests_TerminalApp/TabTests.cpp | 63 ------- src/cascadia/TerminalApp/CommandPalette.cpp | 106 ++++-------- src/cascadia/TerminalApp/CommandPalette.h | 14 +- src/cascadia/TerminalApp/TabHeaderControl.cpp | 28 ++++ src/cascadia/TerminalApp/TabHeaderControl.h | 25 +++ src/cascadia/TerminalApp/TabHeaderControl.idl | 12 ++ .../TerminalApp/TabHeaderControl.xaml | 29 ++++ .../TerminalApp/TerminalAppLib.vcxproj | 22 ++- .../TerminalAppLib.vcxproj.filters | 29 +--- src/cascadia/TerminalApp/TerminalTab.cpp | 19 +-- src/cascadia/TerminalApp/TerminalTab.h | 2 + .../TerminalApp/dll/TerminalApp.vcxproj | 1 + .../TerminalConnection/AzureConnection.cpp | 23 +-- .../TerminalConnection/AzureConnection.h | 2 - .../TerminalSettingsModel/IInheritable.h | 50 +++--- .../TerminalSettingsModel/JsonUtils.h | 158 ++++++------------ .../TerminalSettingsModel/Profile.cpp | 25 ++- src/cascadia/ut_app/JsonUtilsTests.cpp | 141 ++-------------- 25 files changed, 307 insertions(+), 502 deletions(-) create mode 100644 src/cascadia/TerminalApp/TabHeaderControl.cpp create mode 100644 src/cascadia/TerminalApp/TabHeaderControl.h create mode 100644 src/cascadia/TerminalApp/TabHeaderControl.idl create mode 100644 src/cascadia/TerminalApp/TabHeaderControl.xaml diff --git a/.github/actions/spell-check/expect/expect.txt b/.github/actions/spell-check/expect/expect.txt index 55298a2bdec..f1513698f84 100644 --- a/.github/actions/spell-check/expect/expect.txt +++ b/.github/actions/spell-check/expect/expect.txt @@ -2277,7 +2277,6 @@ tcome tcommandline tcommands tcon -TDelegated TDP TEAMPROJECT tearoff diff --git a/custom.props b/custom.props index 38d0c629d53..d19d1086adc 100644 --- a/custom.props +++ b/custom.props @@ -5,7 +5,7 @@ true 2020 1 - 6 + 5 Windows Terminal diff --git a/doc/cascadia/Json-Utility-API.md b/doc/cascadia/Json-Utility-API.md index 422a550bf2b..465f3239adb 100644 --- a/doc/cascadia/Json-Utility-API.md +++ b/doc/cascadia/Json-Utility-API.md @@ -8,20 +8,19 @@ return a JSON value coerced into the specified type. When reading into existing storage, it returns a boolean indicating whether that storage was modified. If the JSON value cannot be converted to the specified type, an exception will be generated. -For non-nullable type conversions (most POD types), `null` is considered to be an invalid type. ```c++ std::string one; std::optional two; JsonUtils::GetValue(json, one); -// one is populated or an exception is thrown. +// one is populated or unchanged. JsonUtils::GetValue(json, two); -// two is populated, nullopt or an exception is thrown +// two is populated, nullopt or unchanged auto three = JsonUtils::GetValue(json); -// three is populated or an exception is thrown +// three is populated or zero-initialized auto four = JsonUtils::GetValue>(json); // four is populated or nullopt @@ -226,14 +225,14 @@ auto v = JsonUtils::GetValue(json, conv); -|json type invalid|json null|valid -|-|-|- -`T`|❌ exception|❌ exception|✔ converted +`T`|❌ exception|🔵 unchanged|✔ converted `std::optional`|❌ exception|🟨 `nullopt`|✔ converted ### GetValue<T>() (returning) -|json type invalid|json null|valid -|-|-|- -`T`|❌ exception|❌ exception|✔ converted +`T`|❌ exception|🟨 `T{}` (zero value)|✔ converted `std::optional`|❌ exception|🟨 `nullopt`|✔ converted ### GetValueForKey(T&) (type-deducing) @@ -243,14 +242,14 @@ a "key not found" state. The remaining three cases are the same. val type|key not found|_json type invalid_|_json null_|_valid_ -|-|-|-|- -`T`|🔵 unchanged|_❌ exception_|_❌ exception_|_✔ converted_ -`std::optional`|🔵 unchanged|_❌ exception_|_🟨 `nullopt`_|_✔ converted_ +`T`|🔵 unchanged|_❌ exception_|_🔵 unchanged_|_✔ converted_ +`std::optional`|_🔵 unchanged_|_❌ exception_|_🟨 `nullopt`_|_✔ converted_ ### GetValueForKey<T>() (return value) val type|key not found|_json type invalid_|_json null_|_valid_ -|-|-|-|- -`T`|🟨 `T{}` (zero value)|_❌ exception_|_❌ exception_|_✔ converted_ +`T`|🟨 `T{}` (zero value)|_❌ exception_|_🟨 `T{}` (zero value)_|_✔ converted_ `std::optional`|🟨 `nullopt`|_❌ exception_|_🟨 `nullopt`_|_✔ converted_ ### Future Direction diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 6f7ad6e6385..526836a396a 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -753,16 +753,7 @@ }, "backgroundImage": { "description": "Sets the file location of the image to draw over the window background.", - "oneOf": [ - { - "type": ["string", null] - }, - { - "enum": [ - "desktopWallpaper" - ] - } - ] + "type": ["string", "null"] }, "backgroundImageAlignment": { "default": "center", diff --git a/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp b/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp index b3a0951f78d..b50dd6059d8 100644 --- a/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/CommandTests.cpp @@ -144,6 +144,7 @@ namespace SettingsModelLocalTests // of from keybindings. const std::string commands0String{ R"([ + { "name": "command0", "command": { "action": "splitPane", "split": null } }, { "name": "command1", "command": { "action": "splitPane", "split": "vertical" } }, { "name": "command2", "command": { "action": "splitPane", "split": "horizontal" } }, { "name": "command4", "command": { "action": "splitPane" } }, @@ -156,8 +157,18 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(0u, commands.Size()); auto warnings = implementation::Command::LayerJson(commands, commands0Json); VERIFY_ARE_EQUAL(0u, warnings.size()); - VERIFY_ARE_EQUAL(4u, commands.Size()); + VERIFY_ARE_EQUAL(5u, commands.Size()); + { + auto command = commands.Lookup(L"command0"); + VERIFY_IS_NOT_NULL(command); + VERIFY_IS_NOT_NULL(command.Action()); + VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, command.Action().Action()); + const auto& realArgs = command.Action().Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle()); + } { auto command = commands.Lookup(L"command1"); VERIFY_IS_NOT_NULL(command); diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index ce7e3aff018..f99a9531bd8 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -1410,14 +1410,14 @@ namespace SettingsModelLocalTests } void DeserializationTests::TestProfileBackgroundImageWithDesktopWallpaper() { - const winrt::hstring expectedBackgroundImagePath{ L"desktopWallpaper" }; + const winrt::hstring expectedBackgroundImagePath{ winrt::to_hstring("DesktopWallpaper") }; const std::string settingsJson{ R"( { "profiles": [ { "name": "profile0", - "backgroundImage": "desktopWallpaper" + "backgroundImage": "DesktopWallpaper" } ] })" }; diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index 6f74fed4d6b..78ac3ada249 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -322,6 +322,7 @@ namespace SettingsModelLocalTests void KeyBindingsTests::TestSplitPaneArgs() { const std::string bindings0String{ R"([ + { "keys": ["ctrl+c"], "command": { "action": "splitPane", "split": null } }, { "keys": ["ctrl+d"], "command": { "action": "splitPane", "split": "vertical" } }, { "keys": ["ctrl+e"], "command": { "action": "splitPane", "split": "horizontal" } }, { "keys": ["ctrl+g"], "command": { "action": "splitPane" } }, @@ -334,8 +335,17 @@ namespace SettingsModelLocalTests VERIFY_IS_NOT_NULL(keymap); VERIFY_ARE_EQUAL(0u, keymap->_keyShortcuts.size()); keymap->LayerJson(bindings0Json); - VERIFY_ARE_EQUAL(4u, keymap->_keyShortcuts.size()); + VERIFY_ARE_EQUAL(5u, keymap->_keyShortcuts.size()); + { + KeyChord kc{ true, false, false, static_cast('C') }; + auto actionAndArgs = ::TestUtils::GetActionAndArgs(*keymap, kc); + VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action()); + const auto& realArgs = actionAndArgs.Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_ARE_EQUAL(SplitState::Automatic, realArgs.SplitStyle()); + } { KeyChord kc{ true, false, false, static_cast('D') }; auto actionAndArgs = ::TestUtils::GetActionAndArgs(*keymap, kc); diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index bb5003f1757..e84b5a33363 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -8,7 +8,6 @@ #include "../TerminalApp/TabRowControl.h" #include "../TerminalApp/ShortcutActionDispatch.h" #include "../TerminalApp/TerminalTab.h" -#include "../TerminalApp/CommandPalette.h" #include "../CppWinrtTailored.h" using namespace Microsoft::Console; @@ -83,7 +82,6 @@ namespace TerminalAppLocalTests TEST_METHOD(CloseZoomedPane); TEST_METHOD(NextMRUTab); - TEST_METHOD(VerifyCommandPaletteTabSwitcherOrder); TEST_CLASS_SETUP(ClassSetup) { @@ -850,65 +848,4 @@ namespace TerminalAppLocalTests VERIFY_ARE_EQUAL(3u, focusedIndex, L"Verify the fourth tab is the focused one"); }); } - - void TabTests::VerifyCommandPaletteTabSwitcherOrder() - { - // This is a test for GH#8188 - we want to make sure that the order of tabs - // is preserved in the CommandPalette's TabSwitcher - - auto page = _commonSetup(); - - Log::Comment(L"Create 3 additional tabs"); - RunOnUIThread([&page]() { - NewTerminalArgs newTerminalArgs{ 1 }; - page->_OpenNewTab(newTerminalArgs); - page->_OpenNewTab(newTerminalArgs); - page->_OpenNewTab(newTerminalArgs); - }); - VERIFY_ARE_EQUAL(4u, page->_mruTabActions.Size()); - - Log::Comment(L"give alphabetical names to all switch tab actions"); - RunOnUIThread([&page]() { - page->_tabs.GetAt(0).SwitchToTabCommand().Name(L"a"); - page->_tabs.GetAt(1).SwitchToTabCommand().Name(L"b"); - page->_tabs.GetAt(2).SwitchToTabCommand().Name(L"c"); - page->_tabs.GetAt(3).SwitchToTabCommand().Name(L"d"); - }); - - Log::Comment(L"Change the tab switch order to MRU switching"); - TestOnUIThread([&page]() { - page->_settings.GlobalSettings().TabSwitcherMode(TabSwitcherMode::MostRecentlyUsed); - }); - - Log::Comment(L"Select the tabs from 0 to 3"); - RunOnUIThread([&page]() { - page->_UpdatedSelectedTab(0); - page->_UpdatedSelectedTab(1); - page->_UpdatedSelectedTab(2); - page->_UpdatedSelectedTab(3); - }); - - VERIFY_ARE_EQUAL(4u, page->_mruTabActions.Size()); - VERIFY_ARE_EQUAL(L"d", page->_mruTabActions.GetAt(0).Name()); - VERIFY_ARE_EQUAL(L"c", page->_mruTabActions.GetAt(1).Name()); - VERIFY_ARE_EQUAL(L"b", page->_mruTabActions.GetAt(2).Name()); - VERIFY_ARE_EQUAL(L"a", page->_mruTabActions.GetAt(3).Name()); - - Log::Comment(L"Switch to the next MRU tab, which is the third tab"); - RunOnUIThread([&page]() { - page->_SelectNextTab(true); - }); - - const auto palette = winrt::get_self(page->CommandPalette()); - - VERIFY_ARE_EQUAL(1u, palette->_switcherStartIdx, L"Verify the index is 1 as we went right"); - VERIFY_ARE_EQUAL(implementation::CommandPaletteMode::TabSwitchMode, palette->_currentMode, L"Verify we are in the tab switcher mode"); - - Log::Comment(L"Verify command palette preserves MRU order of tabs"); - VERIFY_ARE_EQUAL(4u, palette->_filteredActions.Size()); - VERIFY_ARE_EQUAL(L"d", palette->_filteredActions.GetAt(0).Command().Name()); - VERIFY_ARE_EQUAL(L"c", palette->_filteredActions.GetAt(1).Command().Name()); - VERIFY_ARE_EQUAL(L"b", palette->_filteredActions.GetAt(2).Command().Name()); - VERIFY_ARE_EQUAL(L"a", palette->_filteredActions.GetAt(3).Command().Name()); - } } diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 685a98838f0..e365ba565b1 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -118,87 +118,48 @@ namespace winrt::TerminalApp::implementation } // Method Description: - // - Scroll the command palette to the specified index + // - Scrolls the focus up or down the list of commands. // Arguments: - // - index within a list view of commands + // - pageDown: if true, we're attempting to move to the last visible item in the + // list. Otherwise, we're attempting to move to the first visible item. // Return Value: // - - void CommandPalette::_scrollToIndex(uint32_t index) - { - auto numItems = _filteredActionsView().Items().Size(); - - if (numItems == 0) - { - // if the list is empty no need to scroll - return; - } - - auto clampedIndex = std::clamp(index, 0, numItems - 1); - _filteredActionsView().SelectedIndex(clampedIndex); - _filteredActionsView().ScrollIntoView(_filteredActionsView().SelectedItem()); - } - - // Method Description: - // - Computes the number of visible commands - // Arguments: - // - - // Return Value: - // - the approximate number of items visible in the list (in other words the size of the page) - uint32_t CommandPalette::_getNumVisibleItems() + void CommandPalette::ScrollDown(const bool pageDown) { const auto container = _filteredActionsView().ContainerFromIndex(0); const auto item = container.try_as(); const auto itemHeight = ::base::saturated_cast(item.ActualHeight()); const auto listHeight = ::base::saturated_cast(_filteredActionsView().ActualHeight()); - return listHeight / itemHeight; - } - - // Method Description: - // - Scrolls the focus one page up the list of commands. - // Arguments: - // - - // Return Value: - // - - void CommandPalette::ScrollPageUp() - { - auto selected = _filteredActionsView().SelectedIndex(); - auto numVisibleItems = _getNumVisibleItems(); - _scrollToIndex(selected - numVisibleItems); - } + const int numVisibleItems = listHeight / itemHeight; - // Method Description: - // - Scrolls the focus one page down the list of commands. - // Arguments: - // - - // Return Value: - // - - void CommandPalette::ScrollPageDown() - { auto selected = _filteredActionsView().SelectedIndex(); - auto numVisibleItems = _getNumVisibleItems(); - _scrollToIndex(selected + numVisibleItems); - } + const int numItems = ::base::saturated_cast(_filteredActionsView().Items().Size()); - // Method Description: - // - Moves the focus to the top item in the list of commands. - // Arguments: - // - - // Return Value: - // - - void CommandPalette::ScrollToTop() - { - _scrollToIndex(0); + const auto newIndex = ((numItems + selected + (pageDown ? numVisibleItems : -numVisibleItems)) % numItems); + _filteredActionsView().SelectedIndex(newIndex); + _filteredActionsView().ScrollIntoView(_filteredActionsView().SelectedItem()); } // Method Description: - // - Moves the focus to the bottom item in the list of commands. + // - Moves the focus either to the top item or to the end item in the list of commands. // Arguments: - // - + // - end: if true, we're attempting to move to the last item in the + // list. Otherwise, we're attempting to move to the first item. + // Depends on the pageUpDown argument. // Return Value: // - - void CommandPalette::ScrollToBottom() + void CommandPalette::GoEnd(const bool end) { - _scrollToIndex(_filteredActionsView().Items().Size() - 1); + const auto lastIndex = ::base::saturated_cast(_filteredActionsView().Items().Size() - 1); + if (end) + { + _filteredActionsView().SelectedIndex(lastIndex); + } + else + { + _filteredActionsView().SelectedIndex(0); + } + _filteredActionsView().ScrollIntoView(_filteredActionsView().SelectedItem()); } // Method Description: @@ -280,25 +241,25 @@ namespace winrt::TerminalApp::implementation else if (key == VirtualKey::PageUp) { // Action Mode: Move focus to the first visible item in the list. - ScrollPageUp(); + ScrollDown(false); e.Handled(true); } else if (key == VirtualKey::PageDown) { // Action Mode: Move focus to the last visible item in the list. - ScrollPageDown(); + ScrollDown(true); e.Handled(true); } else if (key == VirtualKey::Home) { // Action Mode: Move focus to the first item in the list. - ScrollToTop(); + GoEnd(false); e.Handled(true); } else if (key == VirtualKey::End) { // Action Mode: Move focus to the last item in the list. - ScrollToBottom(); + GoEnd(true); e.Handled(true); } else if (key == VirtualKey::Enter) @@ -857,7 +818,7 @@ namespace winrt::TerminalApp::implementation for (const auto& action : commandsToFilter) { // Update filter for all commands - // This will modify the highlighting but will also lead to re-computation of weight (and consequently sorting). + // This will modify the highlighting but will also lead to recomputation of weight (and consequently sorting). // Pay attention that it already updates the highlighting in the UI action.UpdateFilter(searchText); @@ -868,13 +829,8 @@ namespace winrt::TerminalApp::implementation } } - // We want to present the commands sorted, - // unless we are in the TabSwitcherMode and TabSearchMode, - // in which we want to preserve the original order (to be aligned with the tab view) - if (_currentMode != CommandPaletteMode::TabSearchMode && _currentMode != CommandPaletteMode::TabSwitchMode) - { - std::sort(actions.begin(), actions.end(), FilteredCommand::Compare); - } + // Add all the commands, but make sure they're sorted. + std::sort(actions.begin(), actions.end(), FilteredCommand::Compare); return actions; } diff --git a/src/cascadia/TerminalApp/CommandPalette.h b/src/cascadia/TerminalApp/CommandPalette.h index ae7b6dde0e6..b366de8840e 100644 --- a/src/cascadia/TerminalApp/CommandPalette.h +++ b/src/cascadia/TerminalApp/CommandPalette.h @@ -10,7 +10,7 @@ // fwdecl unittest classes namespace TerminalAppLocalTests { - class TabTests; + class FilteredCommandTests; }; namespace winrt::TerminalApp::implementation @@ -41,10 +41,9 @@ namespace winrt::TerminalApp::implementation void SelectNextItem(const bool moveDown); - void ScrollPageUp(); - void ScrollPageDown(); - void ScrollToTop(); - void ScrollToBottom(); + void ScrollDown(const bool pageDown); + + void GoEnd(const bool end); // Tab Switcher void EnableTabSwitcherMode(const bool searchMode, const uint32_t startIdx); @@ -119,11 +118,6 @@ namespace winrt::TerminalApp::implementation void _dispatchCommand(winrt::TerminalApp::FilteredCommand const& command); void _dispatchCommandline(); void _dismissPalette(); - - void _scrollToIndex(uint32_t index); - uint32_t _getNumVisibleItems(); - - friend class TerminalAppLocalTests::TabTests; }; } diff --git a/src/cascadia/TerminalApp/TabHeaderControl.cpp b/src/cascadia/TerminalApp/TabHeaderControl.cpp new file mode 100644 index 00000000000..090fff877df --- /dev/null +++ b/src/cascadia/TerminalApp/TabHeaderControl.cpp @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "pch.h" +#include "TabHeaderControl.h" + +#include "TabHeaderControl.g.cpp" + +using namespace winrt; +using namespace winrt::Microsoft::UI::Xaml; + +namespace winrt::TerminalApp::implementation +{ + TabHeaderControl::TabHeaderControl() + { + InitializeComponent(); + } + + void TabHeaderControl::UpdateHeaderText(winrt::hstring title) + { + HeaderTextBlock().Text(title); + } + + void TabHeaderControl::SetZoomIcon(Windows::UI::Xaml::Visibility state) + { + HeaderZoomIcon().Visibility(state); + } +} diff --git a/src/cascadia/TerminalApp/TabHeaderControl.h b/src/cascadia/TerminalApp/TabHeaderControl.h new file mode 100644 index 00000000000..39494010211 --- /dev/null +++ b/src/cascadia/TerminalApp/TabHeaderControl.h @@ -0,0 +1,25 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#pragma once + +#include "winrt/Microsoft.UI.Xaml.Controls.h" + +#include "TabHeaderControl.g.h" + +namespace winrt::TerminalApp::implementation +{ + struct TabHeaderControl : TabHeaderControlT + { + TabHeaderControl(); + void UpdateHeaderText(winrt::hstring title); + void SetZoomIcon(Windows::UI::Xaml::Visibility state); + }; +} + +namespace winrt::TerminalApp::factory_implementation +{ + struct TabHeaderControl : TabHeaderControlT + { + }; +} diff --git a/src/cascadia/TerminalApp/TabHeaderControl.idl b/src/cascadia/TerminalApp/TabHeaderControl.idl new file mode 100644 index 00000000000..a516feb2e40 --- /dev/null +++ b/src/cascadia/TerminalApp/TabHeaderControl.idl @@ -0,0 +1,12 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +namespace TerminalApp +{ + [default_interface] runtimeclass TabHeaderControl : Windows.UI.Xaml.Controls.UserControl + { + TabHeaderControl(); + void UpdateHeaderText(String title); + void SetZoomIcon(Windows.UI.Xaml.Visibility state); + } +} diff --git a/src/cascadia/TerminalApp/TabHeaderControl.xaml b/src/cascadia/TerminalApp/TabHeaderControl.xaml new file mode 100644 index 00000000000..ab7ce0c1b61 --- /dev/null +++ b/src/cascadia/TerminalApp/TabHeaderControl.xaml @@ -0,0 +1,29 @@ + + + + + + + + + diff --git a/src/cascadia/TerminalApp/TerminalAppLib.vcxproj b/src/cascadia/TerminalApp/TerminalAppLib.vcxproj index f653d7d4264..42815038182 100644 --- a/src/cascadia/TerminalApp/TerminalAppLib.vcxproj +++ b/src/cascadia/TerminalApp/TerminalAppLib.vcxproj @@ -54,6 +54,9 @@ Designer + + Designer + Designer @@ -89,10 +92,13 @@ TabRowControl.xaml + + TabHeaderControl.xaml + HighlightedTextControl.xaml - + ColorPickupFlyout.xaml @@ -154,10 +160,13 @@ TabRowControl.xaml + + TabHeaderControl.xaml + HighlightedTextControl.xaml - + ColorPickupFlyout.xaml @@ -230,11 +239,15 @@ TabRowControl.xaml Code + + TabHeaderControl.xaml + Code + HighlightedTextControl.xaml Code - + ColorPickupFlyout.xaml Code @@ -315,7 +328,6 @@ - @@ -325,7 +337,6 @@ - inherited value --> system set value */ \ @@ -97,19 +101,19 @@ public: \ { \ const auto val{ _get##name##Impl() }; \ return val ? *val : type{ __VA_ARGS__ }; \ - } \ + }; \ \ /* Overwrite the user set value */ \ void name(const type& value) \ { \ _##name = value; \ - } \ + }; \ \ /* Clear the user set value */ \ void Clear##name() \ { \ _##name = std::nullopt; \ - } \ + }; \ \ private: \ std::optional _##name{ std::nullopt }; \ @@ -133,7 +137,7 @@ private: \ \ /*no value was found*/ \ return std::nullopt; \ - } + }; // This macro is similar to the one above, but is reserved for optional settings // like Profile.Foreground (where null is interpreted @@ -144,51 +148,51 @@ public: \ /* Returns true if the user explicitly set the value, false otherwise*/ \ bool Has##name() const \ { \ - return _##name.has_value(); \ - } \ + return _##name.set; \ + }; \ \ /* Returns the resolved value for this setting */ \ /* fallback: user set value --> inherited value --> system set value */ \ winrt::Windows::Foundation::IReference name() const \ { \ const auto val{ _get##name##Impl() }; \ - if (val) \ + if (val.set) \ { \ - if (*val) \ + if (val.setting) \ { \ - return **val; \ + return *val.setting; \ } \ return nullptr; \ } \ return winrt::Windows::Foundation::IReference{ __VA_ARGS__ }; \ - } \ + }; \ \ /* Overwrite the user set value */ \ void name(const winrt::Windows::Foundation::IReference& value) \ { \ if (value) /*set value is different*/ \ { \ - _##name = std::optional{ value.Value() }; \ + _##name.setting = value.Value(); \ } \ else \ { \ - /* note we're setting the _inner_ value */ \ - _##name = std::optional{ std::nullopt }; \ + _##name.setting = std::nullopt; \ } \ - } \ + _##name.set = true; \ + }; \ \ /* Clear the user set value */ \ void Clear##name() \ { \ - _##name = std::nullopt; \ - } \ + _##name.set = false; \ + }; \ \ private: \ NullableSetting _##name{}; \ NullableSetting _get##name##Impl() const \ { \ /*return user set value*/ \ - if (_##name) \ + if (Has##name()) \ { \ return _##name; \ } \ @@ -197,12 +201,12 @@ private: \ /*iterate through parents to find a value*/ \ for (auto parent : _parents) \ { \ - if (auto val{ parent->_get##name##Impl() }) \ + auto val{ parent->_get##name##Impl() }; \ + if (val.set) \ { \ return val; \ } \ } \ - \ /*no value was found*/ \ - return std::nullopt; \ - } + return { std::nullopt, false }; \ + }; diff --git a/src/cascadia/TerminalSettingsModel/JsonUtils.h b/src/cascadia/TerminalSettingsModel/JsonUtils.h index 13b77637a5d..2ed3f1bde9e 100644 --- a/src/cascadia/TerminalSettingsModel/JsonUtils.h +++ b/src/cascadia/TerminalSettingsModel/JsonUtils.h @@ -60,33 +60,35 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils const std::string_view zeroCopyString{ begin, gsl::narrow_cast(end - begin) }; return zeroCopyString; } - } - template - struct OptionOracle - { - template // universal parameter - static constexpr bool HasValue(U&&) + template + struct DeduceOptional { - return true; - } - }; + using Type = typename std::decay::type; + static constexpr bool IsOptional = false; + }; - template - struct OptionOracle> - { - static constexpr std::optional EmptyV() { return std::nullopt; } - static constexpr bool HasValue(const std::optional& o) { return o.has_value(); } - static constexpr auto&& Value(const std::optional& o) { return *o; } - }; + template + struct DeduceOptional> + { + using Type = typename std::decay::type; + static constexpr bool IsOptional = true; + }; - template - struct OptionOracle<::winrt::Windows::Foundation::IReference> - { - static constexpr ::winrt::Windows::Foundation::IReference EmptyV() { return nullptr; } - static constexpr bool HasValue(const ::winrt::Windows::Foundation::IReference& o) { return static_cast(o); } - static constexpr auto&& Value(const ::winrt::Windows::Foundation::IReference& o) { return o.Value(); } - }; + template + struct DeduceOptional<::winrt::Windows::Foundation::IReference> + { + using Type = typename std::decay::type; + static constexpr bool IsOptional = true; + }; + + template<> + struct DeduceOptional<::winrt::hstring> + { + using Type = typename ::winrt::hstring; + static constexpr bool IsOptional = true; + }; + } class DeserializationError : public std::runtime_error { @@ -175,27 +177,13 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils // Leverage the wstring converter's validation winrt::hstring FromJson(const Json::Value& json) { - if (json.isNull()) - { - return winrt::hstring{}; - } return winrt::hstring{ til::u8u16(Detail::GetStringView(json)) }; } Json::Value ToJson(const winrt::hstring& val) { - if (val == winrt::hstring{}) - { - return Json::Value::nullSingleton(); - } return til::u16u8(val); } - - bool CanConvert(const Json::Value& json) - { - // hstring has a specific behavior for null, so it can convert it - return ConversionTrait::CanConvert(json) || json.isNull(); - } }; #endif @@ -431,56 +419,6 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils }; #endif - template::type>, typename TOpt = std::optional::type>> - struct OptionalConverter - { - using Oracle = OptionOracle; - TDelegatedConverter delegatedConverter{}; - - TOpt FromJson(const Json::Value& json) - { - if (!json && !delegatedConverter.CanConvert(json)) - { - // If the nested converter can't deal with null, emit an empty optional - // If it can, it probably has specific null behavior that it wants to use. - return Oracle::EmptyV(); - } - TOpt val{ delegatedConverter.FromJson(json) }; - return val; - } - - bool CanConvert(const Json::Value& json) - { - return json.isNull() || delegatedConverter.CanConvert(json); - } - - Json::Value ToJson(const TOpt& val) - { - if (!Oracle::HasValue(val)) - { - return Json::Value::nullSingleton(); - } - return delegatedConverter.ToJson(Oracle::Value(val)); - } - - std::string TypeDescription() const - { - return delegatedConverter.TypeDescription(); - } - }; - - template - struct ConversionTrait> : public OptionalConverter, std::optional> - { - }; - -#ifdef WINRT_Windows_Foundation_H - template - struct ConversionTrait<::winrt::Windows::Foundation::IReference> : public OptionalConverter, ::winrt::Windows::Foundation::IReference> - { - }; -#endif - template struct EnumMapper { @@ -648,15 +586,31 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template bool GetValue(const Json::Value& json, T& target, Converter&& conv) { - if (!conv.CanConvert(json)) + if constexpr (Detail::DeduceOptional::IsOptional) { - DeserializationError e{ json }; - e.expectedType = conv.TypeDescription(); - throw e; + // FOR OPTION TYPES + // - If the json object is set to `null`, then + // we'll instead set the target back to the empty optional. + if (json.isNull()) + { + target = T{}; // zero-construct an empty optional + return true; + } } - target = conv.FromJson(json); - return true; + if (json) + { + if (!conv.CanConvert(json)) + { + DeserializationError e{ json }; + e.expectedType = conv.TypeDescription(); + throw e; + } + + target = conv.FromJson(json); + return true; + } + return false; } // GetValue, forced return type, manual converter @@ -700,7 +654,7 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template bool GetValue(const Json::Value& json, T& target) { - return GetValue(json, target, ConversionTrait::type>{}); + return GetValue(json, target, ConversionTrait::Type>{}); } // GetValue, forced return type, with automatic converter @@ -708,7 +662,7 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils std::decay_t GetValue(const Json::Value& json) { std::decay_t local{}; - GetValue(json, local, ConversionTrait::type>{}); + GetValue(json, local, ConversionTrait::Type>{}); return local; // returns zero-initialized or value } @@ -716,14 +670,14 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template bool GetValueForKey(const Json::Value& json, std::string_view key, T& target) { - return GetValueForKey(json, key, target, ConversionTrait::type>{}); + return GetValueForKey(json, key, target, ConversionTrait::Type>{}); } // GetValueForKey, forced return type, with automatic converter template std::decay_t GetValueForKey(const Json::Value& json, std::string_view key) { - return GetValueForKey(json, key, ConversionTrait::type>{}); + return GetValueForKey(json, key, ConversionTrait::Type>{}); } // Get multiple values for keys (json, k, &v, k, &v, k, &v, ...). @@ -742,19 +696,15 @@ namespace Microsoft::Terminal::Settings::Model::JsonUtils template void SetValueForKey(Json::Value& json, std::string_view key, const T& target, Converter&& conv) { - // We don't want to write any empty optionals into JSON (right now). - if (OptionOracle::HasValue(target)) - { - // demand guarantees that it will return a value or throw an exception - *json.demand(&*key.cbegin(), (&*key.cbegin()) + key.size()) = conv.ToJson(target); - } + // demand guarantees that it will return a value or throw an exception + *json.demand(&*key.cbegin(), (&*key.cbegin()) + key.size()) = conv.ToJson(target); } // SetValueForKey, type-deduced, with automatic converter template void SetValueForKey(Json::Value& json, std::string_view key, const T& target) { - SetValueForKey(json, key, target, ConversionTrait::type>{}); + SetValueForKey(json, key, target, ConversionTrait::Type>{}); } }; diff --git a/src/cascadia/TerminalSettingsModel/Profile.cpp b/src/cascadia/TerminalSettingsModel/Profile.cpp index 7017f15e255..d7404b98f1b 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.cpp +++ b/src/cascadia/TerminalSettingsModel/Profile.cpp @@ -59,7 +59,7 @@ static constexpr std::string_view AntialiasingModeKey{ "antialiasingMode" }; static constexpr std::string_view TabColorKey{ "tabColor" }; static constexpr std::string_view BellStyleKey{ "bellStyle" }; -static constexpr std::wstring_view DesktopWallpaperEnum{ L"desktopWallpaper" }; +static const winrt::hstring DesktopWallpaperEnum{ L"DesktopWallpaper" }; Profile::Profile() { @@ -293,10 +293,10 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, HiddenKey, _Hidden); // Core Settings - JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground); - JsonUtils::GetValueForKey(json, BackgroundKey, _Background); - JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground); - JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor); + _Foreground.set = JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground.setting); + _Background.set = JsonUtils::GetValueForKey(json, BackgroundKey, _Background.setting); + _SelectionBackground.set = JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground.setting); + _CursorColor.set = JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor.setting); JsonUtils::GetValueForKey(json, ColorSchemeKey, _ColorSchemeName); // TODO:MSFT:20642297 - Use a sentinel value (-1) for "Infinite scrollback" @@ -320,11 +320,18 @@ void Profile::LayerJson(const Json::Value& json) // Padding was never specified as an integer, but it was a common working mistake. // Allow it to be permissive. - JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::OptionalConverter>{}); + JsonUtils::GetValueForKey(json, PaddingKey, _Padding, JsonUtils::PermissiveStringConverter{}); JsonUtils::GetValueForKey(json, ScrollbarStateKey, _ScrollState); - JsonUtils::GetValueForKey(json, StartingDirectoryKey, _StartingDirectory); + // StartingDirectory is "nullable". But we represent a null starting directory as the empty string + // When null is set in the JSON, we empty initialize startDir (empty string), and set StartingDirectory to that + // Without this, we're accidentally setting StartingDirectory to nullopt instead. + hstring startDir; + if (JsonUtils::GetValueForKey(json, StartingDirectoryKey, startDir)) + { + _StartingDirectory = startDir; + } JsonUtils::GetValueForKey(json, IconKey, _Icon); JsonUtils::GetValueForKey(json, BackgroundImageKey, _BackgroundImagePath); @@ -334,7 +341,7 @@ void Profile::LayerJson(const Json::Value& json) JsonUtils::GetValueForKey(json, RetroTerminalEffectKey, _RetroTerminalEffect); JsonUtils::GetValueForKey(json, AntialiasingModeKey, _AntialiasingMode); - JsonUtils::GetValueForKey(json, TabColorKey, _TabColor); + _TabColor.set = JsonUtils::GetValueForKey(json, TabColorKey, _TabColor.setting); JsonUtils::GetValueForKey(json, BellStyleKey, _BellStyle); } @@ -355,7 +362,7 @@ winrt::hstring Profile::ExpandedBackgroundImagePath() const } // checks if the user would like to copy their desktop wallpaper // if so, replaces the path with the desktop wallpaper's path - else if (path == DesktopWallpaperEnum) + else if (path == to_hstring(DesktopWallpaperEnum)) { WCHAR desktopWallpaper[MAX_PATH]; diff --git a/src/cascadia/ut_app/JsonUtilsTests.cpp b/src/cascadia/ut_app/JsonUtilsTests.cpp index 7dbaf9fd201..efaac0da068 100644 --- a/src/cascadia/ut_app/JsonUtilsTests.cpp +++ b/src/cascadia/ut_app/JsonUtilsTests.cpp @@ -101,38 +101,6 @@ JSON_FLAG_MAPPER(JsonTestFlags) }; }; -struct hstring_like -{ - std::string value; -}; -template<> -struct ConversionTrait -{ - hstring_like FromJson(const Json::Value& json) - { - return { ConversionTrait{}.FromJson(json) }; - } - - bool CanConvert(const Json::Value& json) - { - return json.isNull() || ConversionTrait{}.CanConvert(json); - } - - Json::Value ToJson(const hstring_like& val) - { - if (val.value == "") - { - return Json::Value::nullSingleton(); - } - return val.value; - } - - std::string TypeDescription() const - { - return "string"; - } -}; - namespace TerminalAppUnitTests { class JsonUtilsTests @@ -151,11 +119,6 @@ namespace TerminalAppUnitTests TEST_METHOD(FlagMapper); TEST_METHOD(NestedExceptionDuringKeyParse); - - TEST_METHOD(SetValueHStringLike); - TEST_METHOD(GetValueHStringLike); - - TEST_METHOD(DoubleOptional); }; template @@ -173,8 +136,9 @@ namespace TerminalAppUnitTests //// 1.a. Type Invalid - Exception //// VERIFY_THROWS_SPECIFIC(GetValue(object), DeserializationError, _ReturnTrueForException); - //// 1.b. JSON NULL - Exception //// - VERIFY_THROWS_SPECIFIC(GetValue(Json::Value::nullSingleton()), DeserializationError, _ReturnTrueForException); + //// 1.b. JSON NULL - Zero Value //// + std::string zeroValueString{}; + VERIFY_ARE_EQUAL(zeroValueString, GetValue(Json::Value::nullSingleton())); //// 1.c. Valid - Valid //// VERIFY_ARE_EQUAL(expected, GetValue(object)); @@ -203,8 +167,9 @@ namespace TerminalAppUnitTests VERIFY_THROWS_SPECIFIC(GetValue(object, outputRedHerring), DeserializationError, _ReturnTrueForException); VERIFY_ARE_EQUAL(5, outputRedHerring); // unchanged - //// 1.b. JSON NULL - Exception //// - VERIFY_THROWS_SPECIFIC(GetValue(Json::Value::nullSingleton(), output), DeserializationError, _ReturnTrueForException); + //// 1.b. JSON NULL - Unchanged //// + VERIFY_IS_FALSE(GetValue(Json::Value::nullSingleton(), output)); // FALSE = storage not modified! + VERIFY_ARE_EQUAL("sentinel", output); // unchanged //// 1.c. Valid //// VERIFY_IS_TRUE(GetValue(object, output)); @@ -243,14 +208,14 @@ namespace TerminalAppUnitTests //// 1.a. Type Invalid - Exception //// VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key), DeserializationError, _ReturnTrueForException); - //// 1.b. JSON NULL - Exception //// - VERIFY_THROWS_SPECIFIC(GetValueForKey(object, nullKey), DeserializationError, _ReturnTrueForException); + //// 1.b. JSON NULL - Zero Value //// + std::string zeroValueString{}; + VERIFY_ARE_EQUAL(zeroValueString, GetValueForKey(object, nullKey)); //// 1.c. Valid - Valid //// VERIFY_ARE_EQUAL(expected, GetValueForKey(object, key)); //// 1.d. Not Found - Zero Value //// - std::string zeroValueString{}; VERIFY_ARE_EQUAL(zeroValueString, GetValueForKey(object, invalidKey)); //// 2. Optional //// @@ -288,7 +253,8 @@ namespace TerminalAppUnitTests VERIFY_ARE_EQUAL(5, outputRedHerring); // unchanged //// 1.b. JSON NULL - Unchanged //// - VERIFY_THROWS_SPECIFIC(GetValueForKey(object, nullKey, output), DeserializationError, _ReturnTrueForException); + VERIFY_IS_FALSE(GetValueForKey(object, nullKey, output)); // FALSE = storage not modified! + VERIFY_ARE_EQUAL("sentinel", output); // unchanged //// 1.c. Valid //// VERIFY_IS_TRUE(GetValueForKey(object, key, output)); @@ -539,89 +505,4 @@ namespace TerminalAppUnitTests VERIFY_THROWS_SPECIFIC(GetValueForKey(object, key), DeserializationError, CheckKeyInException); } - void JsonUtilsTests::SetValueHStringLike() - { - // Terminal has a string type (hstring) where null/"" are the same, and - // we want to make sure that optionals of that type serialize "properly". - hstring_like first{ "" }; - hstring_like second{ "second" }; - std::optional third{ { "" } }; - std::optional fourth{ { "fourth" } }; - std::optional fifth{}; - - Json::Value object{ Json::objectValue }; - - SetValueForKey(object, "first", first); - SetValueForKey(object, "second", second); - SetValueForKey(object, "third", third); - SetValueForKey(object, "fourth", fourth); - SetValueForKey(object, "fifth", fifth); - - VERIFY_ARE_EQUAL(Json::Value::nullSingleton(), object["first"]); // real empty value serializes as null - VERIFY_ARE_EQUAL("second", object["second"].asString()); // serializes as a string - VERIFY_ARE_EQUAL(Json::Value::nullSingleton(), object["third"]); // optional populated with real empty value serializes as null - VERIFY_ARE_EQUAL("fourth", object["fourth"].asString()); // serializes as a string - VERIFY_IS_FALSE(object.isMember("fifth")); // does not serialize - } - - void JsonUtilsTests::GetValueHStringLike() - { - Json::Value object{ Json::objectValue }; - object["string"] = "string"; - object["null"] = Json::Value::nullSingleton(); - // object["nonexistent"] can't be set, clearly, to continue not existing - - hstring_like v; - VERIFY_IS_TRUE(GetValueForKey(object, "string", v)); - VERIFY_ARE_EQUAL("string", v.value); // deserializes as string - VERIFY_IS_TRUE(GetValueForKey(object, "null", v)); - VERIFY_ARE_EQUAL("", v.value); // deserializes as real value, but empty - VERIFY_IS_FALSE(GetValueForKey(object, "nonexistent", v)); // does not deserialize - - std::optional optionalV; - // deserializes as populated optional containing string - VERIFY_IS_TRUE(GetValueForKey(object, "string", optionalV)); - VERIFY_IS_TRUE(optionalV.has_value()); - VERIFY_ARE_EQUAL("string", optionalV->value); - - optionalV = std::nullopt; - // deserializes as populated optional containing real empty value - VERIFY_IS_TRUE(GetValueForKey(object, "null", optionalV)); - VERIFY_IS_TRUE(optionalV.has_value()); - VERIFY_ARE_EQUAL("", optionalV->value); - - optionalV = std::nullopt; - // does not deserialize; optional remains nullopt - VERIFY_IS_FALSE(GetValueForKey(object, "nonexistent", optionalV)); - VERIFY_ARE_EQUAL(std::nullopt, optionalV); - } - - void JsonUtilsTests::DoubleOptional() - { - const std::optional> first{ std::nullopt }; // no value - const std::optional> second{ std::optional{ std::nullopt } }; // outer has a value, inner is "no value" - const std::optional> third{ std::optional{ 3 } }; // outer has a value, inner is "no value" - - Json::Value object{ Json::objectValue }; - - SetValueForKey(object, "first", first); - SetValueForKey(object, "second", second); - SetValueForKey(object, "third", third); - - VERIFY_IS_FALSE(object.isMember("first")); - VERIFY_IS_TRUE(object.isMember("second")); - VERIFY_ARE_EQUAL(Json::Value::nullSingleton(), object["second"]); - VERIFY_ARE_EQUAL(Json::Value{ 3 }, object["third"]); - - std::optional> firstOut, secondOut, thirdOut; - VERIFY_IS_FALSE(GetValueForKey(object, "first", firstOut)); - VERIFY_IS_TRUE(GetValueForKey(object, "second", secondOut)); - VERIFY_IS_TRUE(static_cast(secondOut)); - VERIFY_ARE_EQUAL(std::nullopt, *secondOut); // should have come back out as null - - VERIFY_IS_TRUE(GetValueForKey(object, "third", thirdOut)); - VERIFY_IS_TRUE(static_cast(thirdOut)); - VERIFY_IS_TRUE(static_cast(*thirdOut)); - VERIFY_ARE_EQUAL(3, **thirdOut); - } }