From bf3c6e7029de817380b62ac56b45494dd5ffcc56 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 30 Sep 2021 12:53:08 -0500 Subject: [PATCH 01/59] spel is hard --- src/cascadia/WindowsTerminal/AppHost.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/WindowsTerminal/AppHost.cpp b/src/cascadia/WindowsTerminal/AppHost.cpp index 4111ef66e..298b9b489 100644 --- a/src/cascadia/WindowsTerminal/AppHost.cpp +++ b/src/cascadia/WindowsTerminal/AppHost.cpp @@ -839,7 +839,7 @@ winrt::Windows::Foundation::IAsyncAction AppHost::_SaveWindowLayouts() LOG_CAUGHT_EXCEPTION(); TraceLoggingWrite(g_hWindowsTerminalProvider, "AppHost_SaveWindowLayouts_Failed", - TraceLoggingDescription("An error occured when collecting or writing window state"), + TraceLoggingDescription("An error occurred when collecting or writing window state"), TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); } From 6657d2c3e543006aadd16758d5df83c9566b0748 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Thu, 30 Sep 2021 15:15:20 -0700 Subject: [PATCH 02/59] [deadlock fix] Remove lock for SIUP::GetSelectionRange() (#11386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary of the Pull Request The deadlock was caused by `ScreenInfoUiaProviderBase::GetSelection()` calling `TermControlUiaProvider::GetSelectionRange` (both of which attempted to lock the console). This PR removes the lock and initialization check from `TermControlUiaProvider`. It is no longer necessary because the only one that calls it is `SIUPB::GetSelection()`. Additionally, this adds some code that was useful in debugging this race condition. That should help us figure out any locking issues that may come up in the future. ## References #11312 Closes #11385 ## Validation Steps Performed ✅ Repro steps don't cause hang --- src/cascadia/TerminalCore/Terminal.cpp | 12 ++++++++++++ src/cascadia/TerminalCore/Terminal.hpp | 3 +++ src/cascadia/TerminalCore/terminalrenderdata.cpp | 3 +++ src/types/TermControlUiaProvider.cpp | 6 ------ 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index aba4d68de..1ecf6158c 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -849,7 +849,13 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept // will release this lock when it's destructed. [[nodiscard]] std::unique_lock Terminal::LockForReading() { +#ifdef NDEBUG return std::unique_lock{ _readWriteLock }; +#else + auto lock = std::unique_lock{ _readWriteLock }; + _lastLocker = GetCurrentThreadId(); + return lock; +#endif } // Method Description: @@ -859,7 +865,13 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept // will release this lock when it's destructed. [[nodiscard]] std::unique_lock Terminal::LockForWriting() { +#ifdef NDEBUG return std::unique_lock{ _readWriteLock }; +#else + auto lock = std::unique_lock{ _readWriteLock }; + _lastLocker = GetCurrentThreadId(); + return lock; +#endif } Viewport Terminal::_GetMutableViewport() const noexcept diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp index 5eb4db473..0e105b1e1 100644 --- a/src/cascadia/TerminalCore/Terminal.hpp +++ b/src/cascadia/TerminalCore/Terminal.hpp @@ -269,6 +269,9 @@ private: // But we can abuse the fact that the surrounding members rarely change and are huge // (std::function is like 64 bytes) to create some natural padding without wasting space. til::ticket_lock _readWriteLock; +#ifndef NDEBUG + DWORD _lastLocker; +#endif std::function _pfnScrollPositionChanged; std::function _pfnBackgroundColorChanged; diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp index 1ab2b5af8..fe1fee627 100644 --- a/src/cascadia/TerminalCore/terminalrenderdata.cpp +++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp @@ -233,6 +233,9 @@ catch (...) void Terminal::LockConsole() noexcept { _readWriteLock.lock(); +#ifndef NDEBUG + _lastLocker = GetCurrentThreadId(); +#endif } // Method Description: diff --git a/src/types/TermControlUiaProvider.cpp b/src/types/TermControlUiaProvider.cpp index 3ced07561..edcee5a15 100644 --- a/src/types/TermControlUiaProvider.cpp +++ b/src/types/TermControlUiaProvider.cpp @@ -114,12 +114,6 @@ HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr); *ppUtr = nullptr; - _pData->LockConsole(); - auto Unlock = wil::scope_exit([&]() noexcept { - _pData->UnlockConsole(); - }); - RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized() || !_pData->IsSelectionActive()); - const auto start = _pData->GetSelectionAnchor(); // we need to make end exclusive From 85f067403deea04aa48a8bca87057ccbac1fc372 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Fri, 1 Oct 2021 13:33:22 -0500 Subject: [PATCH 03/59] Replace the UWP file export with the shell32 one (#11365) Just like in #9760, we can't actually use the UWP file picker API, because it will absolutely not work at all when the Terminal is running elevated. That would prevent the picker from appearing at all. So instead, we'll just use the shell32 one manually. This also gets rid of the confirmation dialog, since the team felt we didn't really need that. We could maybe replace it with a Toast (#8592), but _meh_ * [x] closes #11356 * [x] closes #11358 * This is a lot like #9760 * introduced in #11062 * megathread: #9700 --- src/cascadia/TerminalApp/TabManagement.cpp | 55 ++++++++++++------- .../TerminalSettingsEditor/Appearances.cpp | 3 +- .../TerminalSettingsEditor/Profiles.cpp | 3 +- src/cascadia/TerminalSettingsEditor/Utils.cpp | 24 -------- src/cascadia/TerminalSettingsEditor/Utils.h | 36 ------------ .../CascadiaSettings.cpp | 11 ++++ .../TerminalSettingsModel/CascadiaSettings.h | 1 + .../CascadiaSettings.idl | 2 + src/cascadia/WinRTUtils/Utils.cpp | 33 +++++++++++ src/cascadia/WinRTUtils/WinRTUtils.vcxproj | 1 + src/cascadia/WinRTUtils/inc/Utils.h | 48 ++++++++++++++++ src/cascadia/WinRTUtils/pch.h | 5 +- 12 files changed, 139 insertions(+), 83 deletions(-) create mode 100644 src/cascadia/WinRTUtils/Utils.cpp diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp index a339b2bfe..b99af3236 100644 --- a/src/cascadia/TerminalApp/TabManagement.cpp +++ b/src/cascadia/TerminalApp/TabManagement.cpp @@ -19,6 +19,9 @@ #include "ColorHelper.h" #include "DebugTapConnection.h" #include "SettingsTab.h" +#include "..\TerminalSettingsModel\FileUtils.h" + +#include using namespace winrt; using namespace winrt::Windows::Foundation::Collections; @@ -410,33 +413,45 @@ namespace winrt::TerminalApp::implementation // - tab: tab to export winrt::fire_and_forget TerminalPage::_ExportTab(const TerminalTab& tab) { + // This will be used to set up the file picker "filter", to select .txt + // files by default. + static constexpr COMDLG_FILTERSPEC supportedFileTypes[] = { + { L"Text Files (*.txt)", L"*.txt" }, + { L"All Files (*.*)", L"*.*" } + }; + // An arbitrary GUID to associate with all instances of this + // dialog, so they all re-open in the same path as they were + // open before: + static constexpr winrt::guid clientGuidExportFile{ 0xF6AF20BB, 0x0800, 0x48E6, { 0xB0, 0x17, 0xA1, 0x4C, 0xD8, 0x73, 0xDD, 0x58 } }; + try { if (const auto control{ tab.GetActiveTerminalControl() }) { - const FileSavePicker savePicker; - savePicker.as()->Initialize(*_hostingHwnd); - savePicker.SuggestedStartLocation(PickerLocationId::Downloads); - const auto fileChoices = single_threaded_vector({ L".txt" }); - savePicker.FileTypeChoices().Insert(RS_(L"PlainText"), fileChoices); - savePicker.SuggestedFileName(control.Title()); + // GH#11356 - we can't use the UWP apis for writing the file, + // because they don't work elevated (shocker) So just use the + // shell32 file picker manually. + auto path = co_await SaveFilePicker(*_hostingHwnd, [control](auto&& dialog) { + THROW_IF_FAILED(dialog->SetClientGuid(clientGuidExportFile)); + try + { + // Default to the Downloads folder + auto folderShellItem{ winrt::capture(&SHGetKnownFolderItem, FOLDERID_Downloads, KF_FLAG_DEFAULT, nullptr) }; + dialog->SetDefaultFolder(folderShellItem.get()); + } + CATCH_LOG(); // non-fatal + THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedFileTypes), supportedFileTypes)); + THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed + THROW_IF_FAILED(dialog->SetDefaultExtension(L"txt")); - const StorageFile file = co_await savePicker.PickSaveFileAsync(); - if (file != nullptr) + // Default to using the tab title as the file name + THROW_IF_FAILED(dialog->SetFileName((control.Title() + L".txt").c_str())); + }); + + if (!path.empty()) { const auto buffer = control.ReadEntireBuffer(); - CachedFileManager::DeferUpdates(file); - co_await FileIO::WriteTextAsync(file, buffer); - const auto status = co_await CachedFileManager::CompleteUpdatesAsync(file); - switch (status) - { - case FileUpdateStatus::Complete: - case FileUpdateStatus::CompleteAndRenamed: - _ShowControlNoticeDialog(RS_(L"NoticeInfo"), RS_(L"ExportSuccess")); - break; - default: - _ShowControlNoticeDialog(RS_(L"NoticeError"), RS_(L"ExportFailure")); - } + CascadiaSettings::ExportFile(path, buffer); } } } diff --git a/src/cascadia/TerminalSettingsEditor/Appearances.cpp b/src/cascadia/TerminalSettingsEditor/Appearances.cpp index db7f1c16b..0eb24bb05 100644 --- a/src/cascadia/TerminalSettingsEditor/Appearances.cpp +++ b/src/cascadia/TerminalSettingsEditor/Appearances.cpp @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT license. #include "pch.h" @@ -7,6 +7,7 @@ #include "EnumEntry.h" #include +#include "..\WinRTUtils\inc\Utils.h" using namespace winrt::Windows::UI::Text; using namespace winrt::Windows::UI::Xaml; diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.cpp b/src/cascadia/TerminalSettingsEditor/Profiles.cpp index 94b49c2fa..8e43f9b10 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.cpp +++ b/src/cascadia/TerminalSettingsEditor/Profiles.cpp @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT license. #include "pch.h" @@ -8,6 +8,7 @@ #include "EnumEntry.h" #include +#include "..\WinRTUtils\inc\Utils.h" using namespace winrt::Windows::UI::Text; using namespace winrt::Windows::UI::Xaml; diff --git a/src/cascadia/TerminalSettingsEditor/Utils.cpp b/src/cascadia/TerminalSettingsEditor/Utils.cpp index 9fbca2258..d91953185 100644 --- a/src/cascadia/TerminalSettingsEditor/Utils.cpp +++ b/src/cascadia/TerminalSettingsEditor/Utils.cpp @@ -15,30 +15,6 @@ using namespace winrt::Windows::UI::Xaml; UTILS_DEFINE_LIBRARY_RESOURCE_SCOPE(L"Microsoft.Terminal.Settings.Editor/Resources"); -// Function Description: -// - Helper that opens a file picker pre-seeded with image file types. -winrt::Windows::Foundation::IAsyncOperation OpenImagePicker(HWND parentHwnd) -{ - static constexpr COMDLG_FILTERSPEC supportedImageFileTypes[] = { - { L"All Supported Bitmap Types (*.jpg, *.jpeg, *.png, *.bmp, *.gif, *.tiff, *.ico)", L"*.jpg;*.jpeg;*.png;*.bmp;*.gif;*.tiff;*.ico" }, - { L"All Files (*.*)", L"*.*" } - }; - - static constexpr winrt::guid clientGuidImagePicker{ 0x55675F54, 0x74A1, 0x4552, { 0xA3, 0x9D, 0x94, 0xAE, 0x85, 0xD8, 0xF2, 0x7A } }; - return OpenFilePicker(parentHwnd, [](auto&& dialog) { - THROW_IF_FAILED(dialog->SetClientGuid(clientGuidImagePicker)); - try - { - auto pictureFolderShellItem{ winrt::capture(&SHGetKnownFolderItem, FOLDERID_PicturesLibrary, KF_FLAG_DEFAULT, nullptr) }; - dialog->SetDefaultFolder(pictureFolderShellItem.get()); - } - CATCH_LOG(); // non-fatal - THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedImageFileTypes), supportedImageFileTypes)); - THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed - THROW_IF_FAILED(dialog->SetDefaultExtension(L"jpg;jpeg;png;bmp;gif;tiff;ico")); - }); -} - namespace winrt::Microsoft::Terminal::Settings { hstring GetSelectedItemTag(winrt::Windows::Foundation::IInspectable const& comboBoxAsInspectable) diff --git a/src/cascadia/TerminalSettingsEditor/Utils.h b/src/cascadia/TerminalSettingsEditor/Utils.h index d0eddf025..ec73f41c7 100644 --- a/src/cascadia/TerminalSettingsEditor/Utils.h +++ b/src/cascadia/TerminalSettingsEditor/Utils.h @@ -97,42 +97,6 @@ public: \ private: \ static winrt::Windows::UI::Xaml::DependencyProperty _##name##Property; -// Function Description: -// - This function presents a File Open "common dialog" and returns its selected file asynchronously. -// Parameters: -// - customize: A lambda that receives an IFileDialog* to customize. -// Return value: -// (async) path to the selected item. -template -winrt::Windows::Foundation::IAsyncOperation OpenFilePicker(HWND parentHwnd, TLambda&& customize) -{ - auto fileDialog{ winrt::create_instance(CLSID_FileOpenDialog) }; - DWORD flags{}; - THROW_IF_FAILED(fileDialog->GetOptions(&flags)); - THROW_IF_FAILED(fileDialog->SetOptions(flags | FOS_FORCEFILESYSTEM | FOS_NOCHANGEDIR | FOS_DONTADDTORECENT)); // filesystem objects only; no recent places - customize(fileDialog.get()); - - auto hr{ fileDialog->Show(parentHwnd) }; - if (!SUCCEEDED(hr)) - { - if (hr == HRESULT_FROM_WIN32(ERROR_CANCELLED)) - { - co_return winrt::hstring{}; - } - THROW_HR(hr); - } - - winrt::com_ptr result; - THROW_IF_FAILED(fileDialog->GetResult(result.put())); - - wil::unique_cotaskmem_string filePath; - THROW_IF_FAILED(result->GetDisplayName(SIGDN_FILESYSPATH, &filePath)); - - co_return winrt::hstring{ filePath.get() }; -} - -winrt::Windows::Foundation::IAsyncOperation OpenImagePicker(HWND parentHwnd); - namespace winrt::Microsoft::Terminal::Settings { winrt::hstring GetSelectedItemTag(winrt::Windows::Foundation::IInspectable const& comboBoxAsInspectable); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index d49ff4968..9ee9fc6b1 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -5,6 +5,8 @@ #include "CascadiaSettings.h" #include "CascadiaSettings.g.cpp" +#include "FileUtils.h" + #include #include @@ -880,3 +882,12 @@ void CascadiaSettings::CurrentDefaultTerminal(const Model::DefaultTerminal& term { _currentDefaultTerminal = terminal; } + +void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content) +{ + try + { + winrt::Microsoft::Terminal::Settings::Model::WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content)); + } + CATCH_LOG(); +} diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 6ee9d4f37..39ae689e4 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -89,6 +89,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::hstring DefaultSettingsPath(); static winrt::hstring ApplicationDisplayName(); static winrt::hstring ApplicationVersion(); + static void ExportFile(winrt::hstring path, winrt::hstring content); CascadiaSettings() noexcept = default; CascadiaSettings(const winrt::hstring& userJSON, const winrt::hstring& inboxJSON); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl index 092401ebc..e879474b2 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.idl @@ -18,6 +18,8 @@ namespace Microsoft.Terminal.Settings.Model static String ApplicationDisplayName { get; }; static String ApplicationVersion { get; }; + + static void ExportFile(String path, String content); CascadiaSettings(String userJSON, String inboxJSON); diff --git a/src/cascadia/WinRTUtils/Utils.cpp b/src/cascadia/WinRTUtils/Utils.cpp new file mode 100644 index 000000000..4c15fd332 --- /dev/null +++ b/src/cascadia/WinRTUtils/Utils.cpp @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#pragma once + +#include "pch.h" +#include "Utils.h" + +// Function Description: +// - Helper that opens a file picker pre-seeded with image file types. +winrt::Windows::Foundation::IAsyncOperation OpenImagePicker(HWND parentHwnd) +{ + static constexpr COMDLG_FILTERSPEC supportedImageFileTypes[] = { + { L"All Supported Bitmap Types (*.jpg, *.jpeg, *.png, *.bmp, *.gif, *.tiff, *.ico)", L"*.jpg;*.jpeg;*.png;*.bmp;*.gif;*.tiff;*.ico" }, + { L"All Files (*.*)", L"*.*" } + }; + + static constexpr winrt::guid clientGuidImagePicker{ 0x55675F54, 0x74A1, 0x4552, { 0xA3, 0x9D, 0x94, 0xAE, 0x85, 0xD8, 0xF2, 0x7A } }; + return OpenFilePicker(parentHwnd, [](auto&& dialog) { + THROW_IF_FAILED(dialog->SetClientGuid(clientGuidImagePicker)); + try + { + auto pictureFolderShellItem{ winrt::capture(&SHGetKnownFolderItem, FOLDERID_PicturesLibrary, KF_FLAG_DEFAULT, nullptr) }; + dialog->SetDefaultFolder(pictureFolderShellItem.get()); + } + CATCH_LOG(); // non-fatal + +#pragma warning(suppress : 26485) // so we can pass in the supportedImageFileTypes without the analyzer complaining + THROW_IF_FAILED(dialog->SetFileTypes(ARRAYSIZE(supportedImageFileTypes), supportedImageFileTypes)); + THROW_IF_FAILED(dialog->SetFileTypeIndex(1)); // the array is 1-indexed + THROW_IF_FAILED(dialog->SetDefaultExtension(L"jpg;jpeg;png;bmp;gif;tiff;ico")); + }); +} diff --git a/src/cascadia/WinRTUtils/WinRTUtils.vcxproj b/src/cascadia/WinRTUtils/WinRTUtils.vcxproj index 2bd60e143..0c6d74a8a 100644 --- a/src/cascadia/WinRTUtils/WinRTUtils.vcxproj +++ b/src/cascadia/WinRTUtils/WinRTUtils.vcxproj @@ -28,6 +28,7 @@ + diff --git a/src/cascadia/WinRTUtils/inc/Utils.h b/src/cascadia/WinRTUtils/inc/Utils.h index 07b9b4fe8..6c275b1e2 100644 --- a/src/cascadia/WinRTUtils/inc/Utils.h +++ b/src/cascadia/WinRTUtils/inc/Utils.h @@ -19,3 +19,51 @@ inline winrt::Windows::Foundation::Rect ScaleRect(winrt::Windows::Foundation::Re rect.Height = base::ClampMul(rect.Height, scaleLocal); return rect; } + +// Function Description: +// - This function presents a File Open "common dialog" and returns its selected file asynchronously. +// Parameters: +// - customize: A lambda that receives an IFileDialog* to customize. +// Return value: +// (async) path to the selected item. +template +winrt::Windows::Foundation::IAsyncOperation FilePicker(HWND parentHwnd, bool saveDialog, TLambda&& customize) +{ + auto fileDialog{ saveDialog ? winrt::create_instance(CLSID_FileSaveDialog) : + winrt::create_instance(CLSID_FileOpenDialog) }; + DWORD flags{}; + THROW_IF_FAILED(fileDialog->GetOptions(&flags)); + THROW_IF_FAILED(fileDialog->SetOptions(flags | FOS_FORCEFILESYSTEM | FOS_NOCHANGEDIR | FOS_DONTADDTORECENT)); // filesystem objects only; no recent places + customize(fileDialog.get()); + + const auto hr{ fileDialog->Show(parentHwnd) }; + if (!SUCCEEDED(hr)) + { + if (hr == HRESULT_FROM_WIN32(ERROR_CANCELLED)) + { + co_return winrt::hstring{}; + } + THROW_HR(hr); + } + + winrt::com_ptr result; + THROW_IF_FAILED(fileDialog->GetResult(result.put())); + + wil::unique_cotaskmem_string filePath; + THROW_IF_FAILED(result->GetDisplayName(SIGDN_FILESYSPATH, &filePath)); + + co_return winrt::hstring{ filePath.get() }; +} + +template +winrt::Windows::Foundation::IAsyncOperation OpenFilePicker(HWND parentHwnd, TLambda&& customize) +{ + return FilePicker(parentHwnd, false, customize); +} +template +winrt::Windows::Foundation::IAsyncOperation SaveFilePicker(HWND parentHwnd, TLambda&& customize) +{ + return FilePicker(parentHwnd, true, customize); +} + +winrt::Windows::Foundation::IAsyncOperation OpenImagePicker(HWND parentHwnd); diff --git a/src/cascadia/WinRTUtils/pch.h b/src/cascadia/WinRTUtils/pch.h index c793bb636..eab7083a0 100644 --- a/src/cascadia/WinRTUtils/pch.h +++ b/src/cascadia/WinRTUtils/pch.h @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT license. // // pch.h @@ -31,3 +31,6 @@ #include #include #include + +#include +#include From e5293b78143bdbb7bff4f83fadd5cf5c080608e9 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Sat, 2 Oct 2021 00:54:23 +0200 Subject: [PATCH 04/59] Fix all fragments not loading when one is badly formed (#11346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit introduces a number of poor abstractions to split `SettingsLoader::_parse` into `_parse` for content in the format of the user's settings.json and `_parseFragment` which is specialized for fragment files. The latter suppresses exceptions and supports the "updates" key for profiles. ## PR Checklist * [x] Closes #11330 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Wrote the following to `%LOCALAPPDATA%\Microsoft\Windows Terminal\Fragments\test\test.json`: ```json { "profiles": [ { "name": "bad", "unfocusedAppearance": "" }, { "name": "good" }, { "updates": "{574e775e-4f2a-5b96-ac1e-a2962a402336}", "background": "#333" } ] } ``` * Ensured that "bad" is ignored ✔️ * Ensured that "good" shows up and works ✔️ * Ensured that the pwsh profile has a gray background ✔️ --- .../TerminalSettingsModel/CascadiaSettings.h | 15 +- .../CascadiaSettingsSerialization.cpp | 165 +++++++++++------- 2 files changed, 120 insertions(+), 60 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 39ae689e4..d25e7373c 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -44,6 +44,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::com_ptr baseLayerProfile; std::vector> profiles; std::unordered_map> profilesByGuid; + + void clear(); }; struct SettingsLoader @@ -63,12 +65,23 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool duplicateProfile = false; private: + struct JsonSettings + { + Json::Value root; + const Json::Value& colorSchemes; + const Json::Value& profileDefaults; + const Json::Value& profilesList; + }; + static std::pair _lineAndColumnFromPosition(const std::string_view& string, const size_t position); static void _rethrowSerializationExceptionWithLocationInfo(const JsonUtils::DeserializationError& e, const std::string_view& settingsString); static Json::Value _parseJSON(const std::string_view& content); static const Json::Value& _getJSONValue(const Json::Value& json, const std::string_view& key) noexcept; gsl::span> _getNonUserOriginProfiles() const; - void _parse(const OriginTag origin, const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings, bool updatesKeyAllowed = false); + void _parse(const OriginTag origin, const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings); + void _parseFragment(const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings); + static JsonSettings _parseJson(const std::string_view& content); + static winrt::com_ptr _parseProfile(const OriginTag origin, const winrt::hstring& source, const Json::Value& profileJson); void _appendProfile(winrt::com_ptr&& profile, ParsedSettings& settings); static void _addParentProfile(const winrt::com_ptr& profile, ParsedSettings& settings); void _executeGenerator(const IDynamicProfileGenerator& generator); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 8720ad8c3..f667b9ede 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -80,6 +80,14 @@ static std::filesystem::path buildPath(const std::wstring_view& lhs, const std:: return { std::move(buffer) }; } +void ParsedSettings::clear() +{ + globals = {}; + baseLayerProfile = {}; + profiles.clear(); + profilesByGuid.clear(); +} + // This is a convenience method used by the CascadiaSettings constructor. // It runs some basic settings layering without relying on external programs or files. // This makes it suitable for most unit tests. @@ -199,7 +207,7 @@ void SettingsLoader::FindFragmentsAndMergeIntoUserSettings() try { const auto content = ReadUTF8File(fragmentExt.path()); - _parse(OriginTag::Fragment, source, content, fragmentSettings, true); + _parseFragment(source, content, fragmentSettings); for (const auto& fragmentProfile : fragmentSettings.profiles) { @@ -430,92 +438,131 @@ gsl::span> SettingsLoader::_getNonUserOriginProfil } // Parses the given JSON string ("content") and fills a ParsedSettings instance with it. -void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings, bool updatesKeyAllowed) +// This function is to be used for user settings files. +void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings) { - const auto json = content.empty() ? Json::Value{ Json::ValueType::objectValue } : _parseJSON(content); - const auto& profilesObject = _getJSONValue(json, ProfilesKey); - const auto& defaultsObject = _getJSONValue(profilesObject, DefaultSettingsKey); - const auto& profilesArray = profilesObject.isArray() ? profilesObject : _getJSONValue(profilesObject, ProfilesListKey); + const auto json = _parseJson(content); + + settings.clear(); - // globals { - settings.globals = GlobalAppSettings::FromJson(json); + settings.globals = GlobalAppSettings::FromJson(json.root); - if (const auto& schemes = _getJSONValue(json, SchemesKey)) + for (const auto& schemeJson : json.colorSchemes) { - for (const auto& schemeJson : schemes) + if (const auto scheme = ColorScheme::FromJson(schemeJson)) { - if (schemeJson.isObject()) - { - if (const auto scheme = ColorScheme::FromJson(schemeJson)) - { - settings.globals->AddColorScheme(*scheme); - } - } + settings.globals->AddColorScheme(*scheme); } } } - // profiles.defaults { - settings.baseLayerProfile = Profile::FromJson(defaultsObject); + settings.baseLayerProfile = Profile::FromJson(json.profileDefaults); // Remove the `guid` member from the default settings. // That will hyper-explode, so just don't let them do that. settings.baseLayerProfile->ClearGuid(); settings.baseLayerProfile->Origin(OriginTag::ProfilesDefaults); } - // profiles.list { - const auto size = profilesArray.size(); - - // NOTE: This function is supposed to *replace* the contents of ParsedSettings. Don't break this promise. - // SettingsLoader::FindFragmentsAndMergeIntoUserSettings relies on this. - settings.profiles.clear(); + const auto size = json.profilesList.size(); settings.profiles.reserve(size); - - settings.profilesByGuid.clear(); settings.profilesByGuid.reserve(size); - for (const auto& profileJson : profilesArray) + for (const auto& profileJson : json.profilesList) { - auto profile = Profile::FromJson(profileJson); - profile->Origin(origin); - - // The Guid() generation below depends on the value of Source(). - // --> Provide one if we got one. - if (!source.empty()) + auto profile = _parseProfile(origin, source, profileJson); + // GH#9962: Discard Guid-less, Name-less profiles. + if (profile->HasGuid()) { - profile->Source(source); + _appendProfile(std::move(profile), settings); } - - // The Guid() getter generates one from Name() and Source() if none exists otherwise. - // We want to ensure that every profile has a GUID no matter what, not just to - // cache the value, but also to make them consistently identifiable later on. - if (!profile->HasGuid()) - { - if (profile->HasName()) - { - profile->Guid(profile->Guid()); - } - else if (!updatesKeyAllowed || profile->Updates() == winrt::guid{}) - { - // We introduced a bug (GH#9962, fixed in GH#9964) that would result in one or - // more nameless, guid-less profiles being emitted into the user's settings file. - // Those profiles would show up in the list as "Default" later. - // - // Fragments however can contain an alternative "updates" key, which works similar to the "guid". - // If updatesKeyAllowed is true (see FindFragmentsAndMergeIntoUserSettings) we permit - // such Guid-less, Name-less profiles as long as they have a valid Updates field. - continue; - } - } - - _appendProfile(std::move(profile), settings); } } } +// Just like _parse, but is to be used for fragment files, which don't support anything but color +// schemes and profiles. Additionally this function supports profiles which specify an "updates" key. +void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings) +{ + const auto json = _parseJson(content); + + settings.clear(); + + { + settings.globals = winrt::make_self(); + + for (const auto& schemeJson : json.colorSchemes) + { + try + { + if (const auto scheme = ColorScheme::FromJson(schemeJson)) + { + settings.globals->AddColorScheme(*scheme); + } + } + CATCH_LOG() + } + } + + { + const auto size = json.profilesList.size(); + settings.profiles.reserve(size); + settings.profilesByGuid.reserve(size); + + for (const auto& profileJson : json.profilesList) + { + try + { + auto profile = _parseProfile(OriginTag::Fragment, source, profileJson); + // GH#9962: Discard Guid-less, Name-less profiles, but... + // allow ones with an Updates field, as those are special for fragments. + if (profile->HasGuid() || profile->Updates() != winrt::guid{}) + { + _appendProfile(std::move(profile), settings); + } + } + CATCH_LOG() + } + } +} + +SettingsLoader::JsonSettings SettingsLoader::_parseJson(const std::string_view& content) +{ + auto root = content.empty() ? Json::Value{ Json::ValueType::objectValue } : _parseJSON(content); + const auto& colorSchemes = _getJSONValue(root, SchemesKey); + const auto& profilesObject = _getJSONValue(root, ProfilesKey); + const auto& profileDefaults = _getJSONValue(profilesObject, DefaultSettingsKey); + const auto& profilesList = profilesObject.isArray() ? profilesObject : _getJSONValue(profilesObject, ProfilesListKey); + return JsonSettings{ std::move(root), colorSchemes, profileDefaults, profilesList }; +} + +// Just a common helper function between _parse and _parseFragment. +// Parses a profile and ensures it has a Guid if possible. +winrt::com_ptr SettingsLoader::_parseProfile(const OriginTag origin, const winrt::hstring& source, const Json::Value& profileJson) +{ + auto profile = Profile::FromJson(profileJson); + profile->Origin(origin); + + // The Guid() generation below depends on the value of Source(). + // --> Provide one if we got one. + if (!source.empty()) + { + profile->Source(source); + } + + // If none exists. the Guid() getter generates one from Name() and optionally Source(). + // We want to ensure that every profile has a GUID no matter what, not just to + // cache the value, but also to make them consistently identifiable later on. + if (!profile->HasGuid() && profile->HasName()) + { + profile->Guid(profile->Guid()); + } + + return profile; +} + // Adds a profile to the ParsedSettings instance. Takes ownership of the profile. // It ensures no duplicate GUIDs are added to the ParsedSettings instance. void SettingsLoader::_appendProfile(winrt::com_ptr&& profile, ParsedSettings& settings) From 99b1190734af72c612f55ae60b22987ae0632b1b Mon Sep 17 00:00:00 2001 From: NotWearingPants Date: Mon, 4 Oct 2021 15:38:33 +0300 Subject: [PATCH 05/59] Fix type of id in focusPane action in setttings schema (#11395) ## Summary of the Pull Request The type of the `"id"` argument of the `focusPane` action under `"actions"` in the `settings.json` schema was incorrectly set to a string. It's actually expecting a non-negative number, and defaults to 0. So I fixed the schema. ## PR Checklist * [x] Closes #11393 * [x] CLA signed * [ ] Tests added/passed * [ ] Documentation updated * [x] 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. ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed I've validated that a string makes Windows Terminal complain it's a string and not a number, and that a number works as expected, and that the default is indeed zero. --- doc/cascadia/profiles.schema.json | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index ba3387d84..4f55d576b 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -1184,8 +1184,9 @@ "pattern": "focusPane" }, "id": { - "type": "string", - "default": "", + "type": "integer", + "minimum": 0, + "default": 0, "description": "The ID of the pane to focus" } } @@ -1229,7 +1230,7 @@ "description": "When provided, summon the window whose name or ID matches the given name value. If no such window exists, then create a new window with that name." }, "dropdownDuration": { - "type": "number", + "type": "integer", "minimum": 0, "default": 0, "description": "When provided with a positive number, \"slide\" the window in from the top of the screen using an animation that lasts dropdownDuration milliseconds." From b2fd65c601431541d6a5e147ff8b33a4b446a28a Mon Sep 17 00:00:00 2001 From: Schuyler Rosefield Date: Mon, 4 Oct 2021 08:39:29 -0400 Subject: [PATCH 06/59] Try to persist the Pane's working directory if we know what it is. (#11374) ## Summary of the Pull Request Try to save the working directory if we know what it is (just copied what was done in duplicating a pane). I overlooked this in my original implementation that always used the settings StartingDirectory. ## References #9800 ## 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. * [ ] 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 Tried setting the working directory using the OSC 9;9 escape and confirmed that the directory saves correctly. --- src/cascadia/TerminalApp/Pane.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 82bc861b4..e04f558f6 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -128,7 +128,15 @@ NewTerminalArgs Pane::GetTerminalArgsForPane() const auto controlSettings = _control.Settings().as(); args.Profile(controlSettings.ProfileName()); - args.StartingDirectory(controlSettings.StartingDirectory()); + // If we know the user's working directory use it instead of the profile. + if (const auto dir = _control.WorkingDirectory(); !dir.empty()) + { + args.StartingDirectory(dir); + } + else + { + args.StartingDirectory(controlSettings.StartingDirectory()); + } args.TabTitle(controlSettings.StartingTitle()); args.Commandline(controlSettings.Commandline()); args.SuppressApplicationTitle(controlSettings.SuppressApplicationTitle()); From 856081229f644f3282a3af0f69539da389fd1038 Mon Sep 17 00:00:00 2001 From: NotWearingPants Date: Mon, 4 Oct 2021 15:40:15 +0300 Subject: [PATCH 07/59] [IslandWindow.cpp] Avoid double-fetching dropdownDuration (#11383) ## Summary of the Pull Request The code saved `args.DropdownDuration()` to a local and then called the function again, instead of using the local. Changed to use the local. ## PR Checklist * [ ] Closes #xxx * [x] CLA signed * [ ] Tests added/passed * [ ] Documentation updated * [ ] 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. ## Detailed Description of the Pull Request / Additional comments I think this getter simply accesses a member on `args`, it doesn't parse the settings or anything, so compiler optimizes it, but seemed to make more sense to use the local. --- src/cascadia/WindowsTerminal/IslandWindow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/WindowsTerminal/IslandWindow.cpp b/src/cascadia/WindowsTerminal/IslandWindow.cpp index 8fc7e78d3..16ac019af 100644 --- a/src/cascadia/WindowsTerminal/IslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/IslandWindow.cpp @@ -1191,7 +1191,7 @@ void IslandWindow::_summonWindowRoutineBody(Remoting::SummonWindowBehavior args) { uint32_t actualDropdownDuration = args.DropdownDuration(); // If the user requested an animation, let's check if animations are enabled in the OS. - if (args.DropdownDuration() > 0) + if (actualDropdownDuration > 0) { BOOL animationsEnabled = TRUE; SystemParametersInfoW(SPI_GETCLIENTAREAANIMATION, 0, &animationsEnabled, 0); From 703e349fd3609b4382f1d9a1d80ffc44355e9350 Mon Sep 17 00:00:00 2001 From: Ian O'Neill Date: Mon, 4 Oct 2021 13:42:46 +0100 Subject: [PATCH 08/59] Ensure "Reset to inherited value" button works for opacity (#11391) ## Summary of the Pull Request Fixes the "Reset to inherited value" button for the opacity slider and removes the unwanted padding between the header and the control. ## PR Checklist * [x] Closes #11352 * [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. * [ ] 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 ## Validation Steps Performed Manually tested --- src/cascadia/TerminalSettingsEditor/Profiles.h | 4 ++-- src/cascadia/TerminalSettingsEditor/Profiles.xaml | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.h b/src/cascadia/TerminalSettingsEditor/Profiles.h index 54d7d4108..74c31e49a 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.h +++ b/src/cascadia/TerminalSettingsEditor/Profiles.h @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT license. #pragma once @@ -22,7 +22,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void SetAcrylicOpacityPercentageValue(double value) { - _profile.DefaultAppearance().Opacity(winrt::Microsoft::Terminal::Settings::Editor::Converters::PercentageValueToPercentage(value)); + Opacity(winrt::Microsoft::Terminal::Settings::Editor::Converters::PercentageValueToPercentage(value)); }; void SetPadding(double value) diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.xaml b/src/cascadia/TerminalSettingsEditor/Profiles.xaml index 0208a8065..945001efe 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.xaml +++ b/src/cascadia/TerminalSettingsEditor/Profiles.xaml @@ -249,6 +249,7 @@ From e5180fe880f6a833d6ee1e1e7553e462721f9c62 Mon Sep 17 00:00:00 2001 From: NotWearingPants Date: Mon, 4 Oct 2021 16:15:50 +0300 Subject: [PATCH 09/59] Fix globalSummon.dropdownDuration not saving correctly (#11401) ## Summary of the Pull Request In `settings.json` there's an `actions` array to configure keybindings. The action `globalSummon` has an argument called `dropdownDuration`. The settings editor deleted this argument from the settings because of a typo in `ActionArgs.h`. ## PR Checklist * [x] Closes #11400 * [x] CLA signed * [ ] Tests added/passed * [ ] Documentation updated * [ ] 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. ## Detailed Description of the Pull Request / Additional comments There was a `JsonUtils::GetValueForKey` instead of a `JsonUtils::SetValueForKey`. This is what happens when such code is not autogenerated. ## Validation Steps Performed None --- src/cascadia/TerminalSettingsModel/ActionArgs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h index 74315c2ab..1c95209e4 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h @@ -1683,7 +1683,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation JsonUtils::SetValueForKey(json, NameKey, args->_Name); JsonUtils::SetValueForKey(json, DesktopKey, args->_Desktop); JsonUtils::SetValueForKey(json, MonitorKey, args->_Monitor); - JsonUtils::GetValueForKey(json, DropdownDurationKey, args->_DropdownDuration); + JsonUtils::SetValueForKey(json, DropdownDurationKey, args->_DropdownDuration); JsonUtils::SetValueForKey(json, ToggleVisibilityKey, args->_ToggleVisibility); return json; } From 3f1befb06eab0f717b5db4abda5c79c4b9294d0b Mon Sep 17 00:00:00 2001 From: Yoshiko <91673781+yosuemask2@users.noreply.github.com> Date: Mon, 4 Oct 2021 23:29:56 +0900 Subject: [PATCH 10/59] Fix Touch Keyboard invocation issue (#11389) This fixes an issue that Touch Keyboard is not invoked when user taps on the PowerShell. Before this change, it was returning small rectangle on the right of the cursor. Touch Keyboard should be invoked by tapping anywhere inside the console. ## 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 ITfContextOwner::GetScreenExt is used to define rectangle that can invoke Touch Keyboard. https://docs.microsoft.com/en-us/windows/win32/api/msctf/nf-msctf-itfcontextowner-getscreenext ## Validation Steps Performed * [x] Touch keyboard was invoked by tapping inside the Console while Hardware Keyboard was not attached. * [x] Selecting text worked as expected without invoking touch keyboard. * [x] Long tapping the console invoked Touch Keyboard. I would like to confirm if this is the expected behavior. --- src/inc/contsf.h | 3 ++- src/interactivity/win32/WindowIme.cpp | 12 ++++++++++++ src/interactivity/win32/windowime.hpp | 1 + src/interactivity/win32/windowproc.cpp | 2 +- src/tsf/ConsoleTSF.h | 15 ++++++++++++--- src/tsf/contsf.cpp | 4 ++-- 6 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/inc/contsf.h b/src/inc/contsf.h index d7fbacdb4..07594e104 100644 --- a/src/inc/contsf.h +++ b/src/inc/contsf.h @@ -29,8 +29,9 @@ extern "C" { #endif typedef RECT (*GetSuggestionWindowPos)(); +typedef RECT (*GetTextBoxAreaPos)(); -BOOL ActivateTextServices(HWND hwndConsole, GetSuggestionWindowPos pfnPosition); +BOOL ActivateTextServices(HWND hwndConsole, GetSuggestionWindowPos pfnPosition, GetTextBoxAreaPos pfnTextArea); void DeactivateTextServices(); BOOL NotifyTextServices(UINT uMsg, WPARAM wParam, LPARAM lParam, LRESULT* lplResult); diff --git a/src/interactivity/win32/WindowIme.cpp b/src/interactivity/win32/WindowIme.cpp index 37e722f1f..547f098c0 100644 --- a/src/interactivity/win32/WindowIme.cpp +++ b/src/interactivity/win32/WindowIme.cpp @@ -54,3 +54,15 @@ RECT GetImeSuggestionWindowPos() return rcSuggestion; } + +// Routine Description: +// - This method gives a rectangle to where text box is currently rendered +// such that the touch keyboard can pop up when the rectangle is tapped. +// Arguments: +// - +// Return Value: +// - Rectangle specifying current text box area. +RECT GetTextBoxArea() +{ + return Microsoft::Console::Interactivity::ServiceLocator::LocateConsoleWindow()->GetWindowRect(); +} diff --git a/src/interactivity/win32/windowime.hpp b/src/interactivity/win32/windowime.hpp index dc44c7f2d..dc7e6d241 100644 --- a/src/interactivity/win32/windowime.hpp +++ b/src/interactivity/win32/windowime.hpp @@ -6,3 +6,4 @@ #pragma hdrstop RECT GetImeSuggestionWindowPos(); +RECT GetTextBoxArea(); diff --git a/src/interactivity/win32/windowproc.cpp b/src/interactivity/win32/windowproc.cpp index ad88dc7fa..1557dbe9a 100644 --- a/src/interactivity/win32/windowproc.cpp +++ b/src/interactivity/win32/windowproc.cpp @@ -263,7 +263,7 @@ using namespace Microsoft::Console::Types; HandleFocusEvent(TRUE); // ActivateTextServices does nothing if already active so this is OK to be called every focus. - ActivateTextServices(ServiceLocator::LocateConsoleWindow()->GetWindowHandle(), GetImeSuggestionWindowPos); + ActivateTextServices(ServiceLocator::LocateConsoleWindow()->GetWindowHandle(), GetImeSuggestionWindowPos, GetTextBoxArea); // set the text area to have focus for accessibility consumers if (_pUiaProvider) diff --git a/src/tsf/ConsoleTSF.h b/src/tsf/ConsoleTSF.h index 833106078..a38a4f621 100644 --- a/src/tsf/ConsoleTSF.h +++ b/src/tsf/ConsoleTSF.h @@ -33,9 +33,11 @@ class CConsoleTSF final : { public: CConsoleTSF(HWND hwndConsole, - GetSuggestionWindowPos pfnPosition) : + GetSuggestionWindowPos pfnPosition, + GetTextBoxAreaPos pfnTextArea) : _hwndConsole(hwndConsole), _pfnPosition(pfnPosition), + _pfnTextArea(pfnTextArea), _cRef(1), _tid() { @@ -66,21 +68,27 @@ public: return S_OK; } + // This returns Rectangle of the text box of whole console. + // When a user taps inside the rectangle while hardware keyboard is not available, + // touch keyboard is invoked. STDMETHODIMP GetScreenExt(RECT* pRect) { if (pRect) { - *pRect = _pfnPosition(); + *pRect = _pfnTextArea(); } return S_OK; } + // This returns rectangle of current command line edit area. + // When a user types in East Asian language, candidate window is shown at this position. + // Emoji and more panel (Win+.) is shown at the position, too. STDMETHODIMP GetTextExt(LONG, LONG, RECT* pRect, BOOL* pbClipped) { if (pRect) { - GetScreenExt(pRect); + *pRect = _pfnPosition(); } if (pbClipped) @@ -198,6 +206,7 @@ private: // Console info. HWND _hwndConsole; GetSuggestionWindowPos _pfnPosition; + GetTextBoxAreaPos _pfnTextArea; // Miscellaneous flags BOOL _fModifyingDoc = FALSE; // Set TRUE, when calls ITfRange::SetText diff --git a/src/tsf/contsf.cpp b/src/tsf/contsf.cpp index e83a8080a..81a98c5b8 100644 --- a/src/tsf/contsf.cpp +++ b/src/tsf/contsf.cpp @@ -5,11 +5,11 @@ CConsoleTSF* g_pConsoleTSF = nullptr; -extern "C" BOOL ActivateTextServices(HWND hwndConsole, GetSuggestionWindowPos pfnPosition) +extern "C" BOOL ActivateTextServices(HWND hwndConsole, GetSuggestionWindowPos pfnPosition, GetTextBoxAreaPos pfnTextArea) { if (!g_pConsoleTSF && hwndConsole) { - g_pConsoleTSF = new (std::nothrow) CConsoleTSF(hwndConsole, pfnPosition); + g_pConsoleTSF = new (std::nothrow) CConsoleTSF(hwndConsole, pfnPosition, pfnTextArea); if (g_pConsoleTSF && SUCCEEDED(g_pConsoleTSF->Initialize())) { // Conhost calls this function only when the console window has focus. From 6295c8cc45379b2bc782f1cd64378c9b33441896 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 5 Oct 2021 10:28:24 -0500 Subject: [PATCH 11/59] Fix the tab color, part III (#11413) I've had a hard time with the tab colors this week. Turns out that setting the background to nullptr will make the tabviewitem invisible to hit tests. `Transparent`, on the other hand, is totally valid, and the expected default. Tabs as of this commit: ![tab-color-fix-3](https://user-images.githubusercontent.com/18356694/135915272-ff90b28b-f260-493e-bf0b-3450b4702dce.gif) ## PR Checklist * [x] Closes #11382 * [x] I work here This low-key reverts a bit of #11369, which fixed #11294, which regressed in #11240 --- src/cascadia/TerminalApp/TerminalTab.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index aaf8768cd..eb223e0ab 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -1461,15 +1461,10 @@ namespace winrt::TerminalApp::implementation // when the TabViewItem is unselected. So we still need to set the other // properties ourselves. // - // GH#11294: DESPITE the fact that there's a Background() API that we - // could just call like: - // - // TabViewItem().Background(deselectedTabBrush); - // - // We actually can't, because it will make the part of the tab that - // doesn't contain the text totally transparent to hit tests. So we - // actually _do_ still need to set TabViewItemHeaderBackground manually. - TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackground"), deselectedTabBrush); + // In GH#11294 we thought we'd still need to set + // TabViewItemHeaderBackground manually, but GH#11382 discovered that + // Background() was actually okay after all. + TabViewItem().Background(deselectedTabBrush); TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundSelected"), selectedTabBrush); TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundPointerOver"), hoverTabBrush); TabViewItem().Resources().Insert(winrt::box_value(L"TabViewItemHeaderBackgroundPressed"), selectedTabBrush); @@ -1536,6 +1531,11 @@ namespace winrt::TerminalApp::implementation } } + // GH#11382 DON'T set the background to null. If you do that, then the + // tab won't be hit testable at all. Transparent, however, is a totally + // valid hit test target. That makes sense. + TabViewItem().Background(WUX::Media::SolidColorBrush{ Windows::UI::Colors::Transparent() }); + _RefreshVisualState(); _colorCleared(); } From 0b552e1ae8802ec0b384550408e9ebfd7af3029b Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 5 Oct 2021 20:21:03 +0200 Subject: [PATCH 12/59] Fix failing TestHostApp unit tests (#11394) This commit fixes various failing TestHostApp unit tests. Most of these broke as part of 168d28b (#11184). ## PR Checklist * [x] Closes #11339 * [x] I work here * [x] Tests added/passed --- .../DeserializationTests.cpp | 2 +- .../LocalTests_SettingsModel/ProfileTests.cpp | 15 +- .../SerializationTests.cpp | 54 +++---- .../CommandlineTest.cpp | 10 +- .../LocalTests_TerminalApp/SettingsTests.cpp | 152 +++++++++++++----- 5 files changed, 161 insertions(+), 72 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index 7f6638954..ea603c942 100644 --- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp @@ -1092,7 +1092,7 @@ namespace SettingsModelLocalTests }, { "name": "profile1", - "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "guid": "{6239a42c-2222-49a3-80bd-e8fdd045185c}", "source": "Terminal.App.UnitTest.1", "historySize": 2222 }, diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp index 54c405592..ee42b89a4 100644 --- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp @@ -300,6 +300,17 @@ namespace SettingsModelLocalTests // the GUID generated for a dynamic profile (with a source) is different // than that of a profile without a source. + static constexpr std::string_view inboxSettings{ R"({ + "profiles": [ + { + "name" : "profile0", + "source": "Terminal.App.UnitTest.0" + }, + { + "name" : "profile1" + } + ] + })" }; static constexpr std::string_view userSettings{ R"({ "profiles": [ { @@ -312,9 +323,9 @@ namespace SettingsModelLocalTests ] })" }; - const auto settings = winrt::make_self(userSettings, DefaultJson); + const auto settings = winrt::make_self(userSettings, inboxSettings); - VERIFY_ARE_EQUAL(4u, settings->AllProfiles().Size()); + VERIFY_ARE_EQUAL(3u, settings->AllProfiles().Size()); VERIFY_ARE_EQUAL(L"profile0", settings->AllProfiles().GetAt(0).Name()); VERIFY_IS_TRUE(settings->AllProfiles().GetAt(0).HasGuid()); diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp index e538967f9..10214b861 100644 --- a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp @@ -53,7 +53,7 @@ namespace SettingsModelLocalTests // Return Value: // - the JsonObject representing this instance template - void RoundtripTest(const std::string& jsonString) + void RoundtripTest(const std::string_view& jsonString) { const auto json{ VerifyParseSucceeded(jsonString) }; const auto settings{ T::FromJson(json) }; @@ -69,7 +69,7 @@ namespace SettingsModelLocalTests void SerializationTests::GlobalSettings() { - const std::string globalsString{ R"( + static constexpr std::string_view globalsString{ R"( { "defaultProfile": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}", @@ -105,7 +105,7 @@ namespace SettingsModelLocalTests "actions": [] })" }; - const std::string smallGlobalsString{ R"( + static constexpr std::string_view smallGlobalsString{ R"( { "defaultProfile": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}", "actions": [] @@ -117,7 +117,7 @@ namespace SettingsModelLocalTests void SerializationTests::Profile() { - const std::string profileString{ R"( + static constexpr std::string_view profileString{ R"( { "name": "Windows PowerShell", "guid": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}", @@ -152,7 +152,7 @@ namespace SettingsModelLocalTests "selectionBackground": "#CCAABB", "useAcrylic": false, - "acrylicOpacity": 0.5, + "opacity": 50, "backgroundImage": "made_you_look.jpeg", "backgroundImageStretchMode": "uniformToFill", @@ -167,7 +167,7 @@ namespace SettingsModelLocalTests "experimental.retroTerminalEffect": false })" }; - const std::string smallProfileString{ R"( + static constexpr std::string_view smallProfileString{ R"( { "name": "Custom Profile" })" }; @@ -175,7 +175,7 @@ namespace SettingsModelLocalTests // Setting "tabColor" to null tests two things: // - null should count as an explicit user-set value, not falling back to the parent's value // - null should be acceptable even though we're working with colors - const std::string weirdProfileString{ R"( + static constexpr std::string_view weirdProfileString{ R"( { "guid" : "{8b039d4d-77ca-5a83-88e1-dfc8e895a127}", "name": "Weird Profile", @@ -192,7 +192,7 @@ namespace SettingsModelLocalTests void SerializationTests::ColorScheme() { - const std::string schemeString{ R"({ + static constexpr std::string_view schemeString{ R"({ "name": "Campbell", "cursorColor": "#FFFFFF", @@ -225,56 +225,56 @@ namespace SettingsModelLocalTests void SerializationTests::Actions() { // simple command - const std::string actionsString1{ R"([ + static constexpr std::string_view actionsString1{ R"([ { "command": "paste" } ])" }; // complex command - const std::string actionsString2A{ R"([ + static constexpr std::string_view actionsString2A{ R"([ { "command": { "action": "setTabColor" } } ])" }; - const std::string actionsString2B{ R"([ + static constexpr std::string_view actionsString2B{ R"([ { "command": { "action": "setTabColor", "color": "#112233" } } ])" }; - const std::string actionsString2C{ R"([ + static constexpr std::string_view actionsString2C{ R"([ { "command": { "action": "copy" } }, { "command": { "action": "copy", "singleLine": true, "copyFormatting": "html" } } ])" }; // simple command with key chords - const std::string actionsString3{ R"([ + static constexpr std::string_view actionsString3{ R"([ { "command": "toggleAlwaysOnTop", "keys": "ctrl+a" }, { "command": "toggleAlwaysOnTop", "keys": "ctrl+b" } ])" }; // complex command with key chords - const std::string actionsString4A{ R"([ + static constexpr std::string_view actionsString4A{ R"([ { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+c" }, { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+d" } ])" }; - const std::string actionsString4B{ R"([ + static constexpr std::string_view actionsString4B{ R"([ { "command": { "action": "findMatch", "direction": "next" }, "keys": "ctrl+shift+s" }, { "command": { "action": "findMatch", "direction": "prev" }, "keys": "ctrl+shift+r" } ])" }; // command with name and icon and multiple key chords - const std::string actionsString5{ R"([ + static constexpr std::string_view actionsString5{ R"([ { "icon": "image.png", "name": "Scroll To Top Name", "command": "scrollToTop", "keys": "ctrl+e" }, { "command": "scrollToTop", "keys": "ctrl+f" } ])" }; // complex command with new terminal args - const std::string actionsString6{ R"([ + static constexpr std::string_view actionsString6{ R"([ { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+g" }, ])" }; // complex command with meaningful null arg - const std::string actionsString7{ R"([ + static constexpr std::string_view actionsString7{ R"([ { "command": { "action": "renameWindow", "name": null }, "keys": "ctrl+h" } ])" }; // nested command - const std::string actionsString8{ R"([ + static constexpr std::string_view actionsString8{ R"([ { "name": "Change font size...", "commands": [ @@ -286,7 +286,7 @@ namespace SettingsModelLocalTests ])" }; // iterable command - const std::string actionsString9A{ R"([ + static constexpr std::string_view actionsString9A{ R"([ { "name": "New tab", "commands": [ @@ -299,7 +299,7 @@ namespace SettingsModelLocalTests ] } ])" }; - const std::string actionsString9B{ R"([ + static constexpr std::string_view actionsString9B{ R"([ { "commands": [ @@ -315,7 +315,7 @@ namespace SettingsModelLocalTests "name": "Send Input ..." } ])" }; - const std::string actionsString9C{ R""([ + static constexpr std::string_view actionsString9C{ R""([ { "commands": [ @@ -338,7 +338,7 @@ namespace SettingsModelLocalTests "name": "Send Input (Evil) ..." } ])"" }; - const std::string actionsString9D{ R""([ + static constexpr std::string_view actionsString9D{ R""([ { "command": { @@ -352,7 +352,7 @@ namespace SettingsModelLocalTests ])"" }; // unbound command - const std::string actionsString10{ R"([ + static constexpr std::string_view actionsString10{ R"([ { "command": "unbound", "keys": "ctrl+c" } ])" }; @@ -395,7 +395,7 @@ namespace SettingsModelLocalTests void SerializationTests::CascadiaSettings() { - const std::string settingsString{ R"({ + static constexpr std::string_view settingsString{ R"({ "$help" : "https://aka.ms/terminal-documentation", "$schema" : "https://aka.ms/terminal-profiles-schema", "defaultProfile": "{61c54bbd-1111-5271-96e7-009a87ff44bf}", @@ -465,7 +465,7 @@ namespace SettingsModelLocalTests void SerializationTests::LegacyFontSettings() { - const std::string profileString{ R"( + static constexpr std::string_view profileString{ R"( { "name": "Profile with legacy font settings", @@ -474,7 +474,7 @@ namespace SettingsModelLocalTests "fontWeight": "normal" })" }; - const std::string expectedOutput{ R"( + static constexpr std::string_view expectedOutput{ R"( { "name": "Profile with legacy font settings", diff --git a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp index a9975b661..0d63043de 100644 --- a/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp +++ b/src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp @@ -1031,9 +1031,11 @@ namespace TerminalAppLocalTests // The first action is going to always be a new-tab action VERIFY_ARE_EQUAL(ShortcutAction::NewTab, appArgs._startupActions.at(0).Action()); - auto actionAndArgs = appArgs._startupActions.at(1); + const auto actionAndArgs = appArgs._startupActions.at(1); VERIFY_ARE_EQUAL(ShortcutAction::NextTab, actionAndArgs.Action()); - VERIFY_IS_NULL(actionAndArgs.Args()); + VERIFY_IS_NOT_NULL(actionAndArgs.Args()); + const auto myArgs = actionAndArgs.Args().as(); + VERIFY_ARE_EQUAL(TabSwitcherMode::Disabled, myArgs.SwitcherMode().Value()); } { AppCommandlineArgs appArgs{}; @@ -1047,7 +1049,9 @@ namespace TerminalAppLocalTests auto actionAndArgs = appArgs._startupActions.at(1); VERIFY_ARE_EQUAL(ShortcutAction::PrevTab, actionAndArgs.Action()); - VERIFY_IS_NULL(actionAndArgs.Args()); + VERIFY_IS_NOT_NULL(actionAndArgs.Args()); + const auto myArgs = actionAndArgs.Args().as(); + VERIFY_ARE_EQUAL(TabSwitcherMode::Disabled, myArgs.SwitcherMode().Value()); } { AppCommandlineArgs appArgs{}; diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index fd7a8e446..f2c6fac9d 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -16,6 +16,31 @@ using namespace winrt::Microsoft::Terminal::Control; namespace TerminalAppLocalTests { + static constexpr std::wstring_view inboxSettings{ LR"({ + "schemes": [{ + "name": "Campbell", + "foreground": "#CCCCCC", + "background": "#0C0C0C", + "cursorColor": "#FFFFFF", + "black": "#0C0C0C", + "red": "#C50F1F", + "green": "#13A10E", + "yellow": "#C19C00", + "blue": "#0037DA", + "purple": "#881798", + "cyan": "#3A96DD", + "white": "#CCCCCC", + "brightBlack": "#767676", + "brightRed": "#E74856", + "brightGreen": "#16C60C", + "brightYellow": "#F9F1A5", + "brightBlue": "#3B78FF", + "brightPurple": "#B4009E", + "brightCyan": "#61D6D6", + "brightWhite": "#F2F2F2" + }] + })" }; + // TODO:microsoft/terminal#3838: // Unfortunately, these tests _WILL NOT_ work in our CI. We're waiting for // an updated TAEF that will let us install framework packages when the test @@ -107,11 +132,10 @@ namespace TerminalAppLocalTests "iterateOn": "profiles", "command": { "action": "splitPane", "profile": "${profile.name}" } }, - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); @@ -231,11 +255,10 @@ namespace TerminalAppLocalTests "iterateOn": "profiles", "command": { "action": "splitPane", "profile": "${profile.name}" } }, - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); @@ -357,11 +380,10 @@ namespace TerminalAppLocalTests "iterateOn": "profiles", "command": { "action": "splitPane", "profile": "${profile.name}" } }, - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); @@ -495,11 +517,10 @@ namespace TerminalAppLocalTests } ] }, - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size()); @@ -590,11 +611,10 @@ namespace TerminalAppLocalTests }, ] }, - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size()); @@ -714,11 +734,10 @@ namespace TerminalAppLocalTests { "command": { "action": "splitPane", "profile": "${profile.name}", "split": "down" } } ] } - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size()); @@ -851,11 +870,10 @@ namespace TerminalAppLocalTests } ] } - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size()); @@ -954,11 +972,10 @@ namespace TerminalAppLocalTests } ] } - ], - "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + ] })" }; - CascadiaSettings settings{ settingsJson, {} }; + CascadiaSettings settings{ settingsJson, inboxSettings }; VERIFY_ARE_EQUAL(0u, settings.Warnings().Size()); VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size()); @@ -1085,9 +1102,72 @@ namespace TerminalAppLocalTests } ], "schemes": [ - { "name": "scheme_0" }, - { "name": "scheme_1" }, - { "name": "scheme_2" }, + { + "name": "Campbell", + "foreground": "#CCCCCC", + "background": "#0C0C0C", + "cursorColor": "#FFFFFF", + "black": "#0C0C0C", + "red": "#C50F1F", + "green": "#13A10E", + "yellow": "#C19C00", + "blue": "#0037DA", + "purple": "#881798", + "cyan": "#3A96DD", + "white": "#CCCCCC", + "brightBlack": "#767676", + "brightRed": "#E74856", + "brightGreen": "#16C60C", + "brightYellow": "#F9F1A5", + "brightBlue": "#3B78FF", + "brightPurple": "#B4009E", + "brightCyan": "#61D6D6", + "brightWhite": "#F2F2F2" + }, + { + "name": "Campbell PowerShell", + "foreground": "#CCCCCC", + "background": "#012456", + "cursorColor": "#FFFFFF", + "black": "#0C0C0C", + "red": "#C50F1F", + "green": "#13A10E", + "yellow": "#C19C00", + "blue": "#0037DA", + "purple": "#881798", + "cyan": "#3A96DD", + "white": "#CCCCCC", + "brightBlack": "#767676", + "brightRed": "#E74856", + "brightGreen": "#16C60C", + "brightYellow": "#F9F1A5", + "brightBlue": "#3B78FF", + "brightPurple": "#B4009E", + "brightCyan": "#61D6D6", + "brightWhite": "#F2F2F2" + }, + { + "name": "Vintage", + "foreground": "#C0C0C0", + "background": "#000000", + "cursorColor": "#FFFFFF", + "black": "#000000", + "red": "#800000", + "green": "#008000", + "yellow": "#808000", + "blue": "#000080", + "purple": "#800080", + "cyan": "#008080", + "white": "#C0C0C0", + "brightBlack": "#808080", + "brightRed": "#FF0000", + "brightGreen": "#00FF00", + "brightYellow": "#FFFF00", + "brightBlue": "#0000FF", + "brightPurple": "#FF00FF", + "brightCyan": "#00FFFF", + "brightWhite": "#FFFFFF" + } ], "actions": [ { @@ -1100,10 +1180,6 @@ namespace TerminalAppLocalTests CascadiaSettings settings{ settingsJson, {} }; - // Since at least one profile does not reference a color scheme, - // we add a warning saying "the color scheme is unknown" - VERIFY_ARE_EQUAL(1u, settings.Warnings().Size()); - VERIFY_ARE_EQUAL(3u, settings.ActiveProfiles().Size()); auto nameMap{ settings.ActionMap().NameMap() }; @@ -1130,8 +1206,6 @@ namespace TerminalAppLocalTests auto expandedCommands = winrt::TerminalApp::implementation::TerminalPage::_ExpandCommands(nameMap, settings.ActiveProfiles().GetView(), settings.GlobalSettings().ColorSchemes()); _logCommandNames(expandedCommands.GetView()); - // This is the same warning as above - VERIFY_ARE_EQUAL(1u, settings.Warnings().Size()); VERIFY_ARE_EQUAL(3u, expandedCommands.Size()); // Yes, this test is testing splitPane with profiles named after each @@ -1139,7 +1213,7 @@ namespace TerminalAppLocalTests // just easy tests to write. { - auto command = expandedCommands.Lookup(L"iterable command scheme_0"); + auto command = expandedCommands.Lookup(L"iterable command Campbell"); VERIFY_IS_NOT_NULL(command); auto actionAndArgs = command.ActionAndArgs(); VERIFY_IS_NOT_NULL(actionAndArgs); @@ -1153,11 +1227,11 @@ namespace TerminalAppLocalTests VERIFY_IS_TRUE(realArgs.TerminalArgs().StartingDirectory().empty()); VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty()); VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty()); - VERIFY_ARE_EQUAL(L"scheme_0", realArgs.TerminalArgs().Profile()); + VERIFY_ARE_EQUAL(L"Campbell", realArgs.TerminalArgs().Profile()); } { - auto command = expandedCommands.Lookup(L"iterable command scheme_1"); + auto command = expandedCommands.Lookup(L"iterable command Campbell PowerShell"); VERIFY_IS_NOT_NULL(command); auto actionAndArgs = command.ActionAndArgs(); VERIFY_IS_NOT_NULL(actionAndArgs); @@ -1171,11 +1245,11 @@ namespace TerminalAppLocalTests VERIFY_IS_TRUE(realArgs.TerminalArgs().StartingDirectory().empty()); VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty()); VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty()); - VERIFY_ARE_EQUAL(L"scheme_1", realArgs.TerminalArgs().Profile()); + VERIFY_ARE_EQUAL(L"Campbell PowerShell", realArgs.TerminalArgs().Profile()); } { - auto command = expandedCommands.Lookup(L"iterable command scheme_2"); + auto command = expandedCommands.Lookup(L"iterable command Vintage"); VERIFY_IS_NOT_NULL(command); auto actionAndArgs = command.ActionAndArgs(); VERIFY_IS_NOT_NULL(actionAndArgs); @@ -1189,7 +1263,7 @@ namespace TerminalAppLocalTests VERIFY_IS_TRUE(realArgs.TerminalArgs().StartingDirectory().empty()); VERIFY_IS_TRUE(realArgs.TerminalArgs().TabTitle().empty()); VERIFY_IS_FALSE(realArgs.TerminalArgs().Profile().empty()); - VERIFY_ARE_EQUAL(L"scheme_2", realArgs.TerminalArgs().Profile()); + VERIFY_ARE_EQUAL(L"Vintage", realArgs.TerminalArgs().Profile()); } } From f7b5b5caf8bb108b450092b610171b57c701a6e0 Mon Sep 17 00:00:00 2001 From: Leon Liang Date: Wed, 6 Oct 2021 04:32:14 -0700 Subject: [PATCH 13/59] Enable DefApp hooks for stable (#11423) Uncommenting parts of stable's AppXManifest to allow defapp to work with it. --- src/cascadia/CascadiaPackage/Package.appxmanifest | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cascadia/CascadiaPackage/Package.appxmanifest b/src/cascadia/CascadiaPackage/Package.appxmanifest index db9cfe825..b73a6453b 100644 --- a/src/cascadia/CascadiaPackage/Package.appxmanifest +++ b/src/cascadia/CascadiaPackage/Package.appxmanifest @@ -74,7 +74,7 @@ Enabled="false" DisplayName="ms-resource:AppName" /> - + - + From 14d068f73b870ed43b0aaabab871d1bf1f5a5b2b Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 6 Oct 2021 04:33:05 -0700 Subject: [PATCH 14/59] Fix crash and empty action in SUI Actions Page (#11427) ## Summary of the Pull Request Fixes two issues related to SUI's Actions page: 1. Crash when adding an action and setting key chord to one that is already taken - **Cause**: the new key binding that was introduced with the "Add new" button appears in `_KeyBindingList` that we're iterating over. This has no `CurrentKeys()`, resulting in a null pointer exception. - **Fix**: null-check it 2. There's an action that appears as being nameless in the dropdown - **Cause**: The culprit seems to be `MultipleActions`. We would register it, but it wouldn't have a name, so it would appear as a nameless option. - **Fix**: if it has no name, don't register it. This is also future-proof in that any new nameless actions won't be automatically added. Closes #10981 Part of #11353 --- src/cascadia/TerminalSettingsEditor/Actions.cpp | 4 ++-- src/cascadia/TerminalSettingsModel/ActionMap.cpp | 13 ++++++------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalSettingsEditor/Actions.cpp b/src/cascadia/TerminalSettingsEditor/Actions.cpp index 4f65c8cba..8e3a06a07 100644 --- a/src/cascadia/TerminalSettingsEditor/Actions.cpp +++ b/src/cascadia/TerminalSettingsEditor/Actions.cpp @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT license. #include "pch.h" @@ -369,7 +369,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation { const auto kbdVM{ get_self(_KeyBindingList.GetAt(i)) }; const auto& otherKeys{ kbdVM->CurrentKeys() }; - if (keys.Modifiers() == otherKeys.Modifiers() && keys.Vkey() == otherKeys.Vkey()) + if (otherKeys && keys.Modifiers() == otherKeys.Modifiers() && keys.Vkey() == otherKeys.Vkey()) { return i; } diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 6472245aa..1bd4aa390 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -94,15 +94,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static void RegisterShortcutAction(ShortcutAction shortcutAction, std::unordered_map& list, std::unordered_set& visited) { const auto actionAndArgs{ make_self(shortcutAction) }; - if (actionAndArgs->Action() != ShortcutAction::Invalid) + /*We have a valid action.*/ + /*Check if the action was already added.*/ + if (visited.find(Hash(*actionAndArgs)) == visited.end()) { - /*We have a valid action.*/ - /*Check if the action was already added.*/ - if (visited.find(Hash(*actionAndArgs)) == visited.end()) + /*This is an action that wasn't added!*/ + /*Let's add it if it has a name.*/ + if (const auto name{ actionAndArgs->GenerateName() }; !name.empty()) { - /*This is an action that wasn't added!*/ - /*Let's add it.*/ - const auto name{ actionAndArgs->GenerateName() }; list.insert({ name, *actionAndArgs }); } } From 43ce9fda09dee47046febe28b8888581ab91a604 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 6 Oct 2021 04:34:53 -0700 Subject: [PATCH 15/59] Refresh frame margins when moving between monitors (#11412) ## Summary of the Pull Request Refresh the DPI and frame margins when we move the window between different DPI monitors. ## PR Checklist Closes #11367 --- src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp b/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp index b4f0683fa..60dd263a2 100644 --- a/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp +++ b/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp @@ -310,6 +310,12 @@ void NonClientIslandWindow::OnSize(const UINT width, const UINT height) { _UpdateIslandPosition(width, height); } + + // GH#11367: We need to do this, + // otherwise the titlebar may still be partially visible + // when we move between different DPI monitors. + RefreshCurrentDPI(); + _UpdateFrameMargins(); } // Method Description: From 925b05a3b71bc7584f2637900a204ed4f72a70ec Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 6 Oct 2021 11:55:55 -0500 Subject: [PATCH 16/59] Center-align the shield with the other tab row icons (#11441) I started from here: https://github.com/microsoft/microsoft-ui-xaml/blob/9052972906c8a0a1b6cb5d5c61b27d6d27cd7f11/dev/CommonStyles/Button_themeresources_v1.xaml#L121 but adding a padding of 3 was still off by one pixel, so it's 4 now. _enhance.png_ ![image](https://user-images.githubusercontent.com/18356694/136219225-3fcffd48-79b4-4efc-a4c3-4b59f9878962.png) * [x] closes #11421 --- src/cascadia/TerminalApp/TabRowControl.xaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TabRowControl.xaml b/src/cascadia/TerminalApp/TabRowControl.xaml index b3b0f58c3..0cce26ef3 100644 --- a/src/cascadia/TerminalApp/TabRowControl.xaml +++ b/src/cascadia/TerminalApp/TabRowControl.xaml @@ -23,7 +23,7 @@ Date: Wed, 6 Oct 2021 18:58:09 +0200 Subject: [PATCH 17/59] Fix default terminal setting dropdown (#11430) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WinUI/XAML requires the `SelectedItem` to be member of the list of `ItemsSource`. `CascadiaSettings::DefaultTerminals()` is such an `ItemsSource` and is called every time the launch settings page is visited. It calls `DefaultTerminal::Available()` which in turn calls `Refresh()`. While the `SelectedItem` was cached in `CascadiaSettings`, the value of `DefaultTerminals()` wasn't. Thus every time the page was visited, it refreshed the `ItemsSource` list without invalidating the current `SelectedItem`. This commit prevents such accidental mishaps from occurring in the future, by moving the responsibility of caching solely to the `CascadiaSettings` class. ## PR Checklist * [x] Closes #11424 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Navigating between SUI pages maintains the current dropdown selection ✔️ * Saving the settings saves the correct terminal at `HKCU:\Console\%%Startup` ✔️ --- .github/actions/spelling/expect/expect.txt | 5 +- .../CascadiaSettings.cpp | 37 ++++++----- .../TerminalSettingsModel/CascadiaSettings.h | 4 +- .../CascadiaSettingsSerialization.cpp | 3 +- .../TerminalSettingsModel/DefaultTerminal.cpp | 65 +++++++------------ .../TerminalSettingsModel/DefaultTerminal.h | 14 +--- .../TerminalSettingsModel/DefaultTerminal.idl | 3 - 7 files changed, 56 insertions(+), 75 deletions(-) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index c12609802..01dfe9d5e 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -189,13 +189,12 @@ cacafire callee capslock CARETBLINKINGENABLED +carlos CARRIAGERETURN cascadia cassert castsi catid -carlos -zamora cazamor CBash cbegin @@ -2724,6 +2723,7 @@ wixproj wline wlinestream wmain +wmemory WMSZ wnd WNDALLOC @@ -2850,6 +2850,7 @@ YSize YSubstantial YVIRTUALSCREEN YWalk +zamora ZCmd ZCtrl zsh diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 9ee9fc6b1..4a9b274bf 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -5,6 +5,7 @@ #include "CascadiaSettings.h" #include "CascadiaSettings.g.cpp" +#include "DefaultTerminal.h" #include "FileUtils.h" #include @@ -844,31 +845,21 @@ bool CascadiaSettings::IsDefaultTerminalAvailable() noexcept // - // Return Value: // - an iterable collection of all available terminals that could be the default. -IObservableVector CascadiaSettings::DefaultTerminals() const noexcept +IObservableVector CascadiaSettings::DefaultTerminals() noexcept { - const auto available = DefaultTerminal::Available(); - std::vector terminals{ available.Size(), nullptr }; - available.GetMany(0, terminals); - return winrt::single_threaded_observable_vector(std::move(terminals)); + _refreshDefaultTerminals(); + return _defaultTerminals; } // Method Description: // - Returns the currently selected default terminal application. -// - DANGER! This will be null unless you've called -// CascadiaSettings::RefreshDefaultTerminals. At the time of this comment (May - -// 2021), only the Launch page in the settings UI calls that method, so this -// value is unset unless you've navigated to that page. // Arguments: // - // Return Value: // - the selected default terminal application Settings::Model::DefaultTerminal CascadiaSettings::CurrentDefaultTerminal() noexcept { - if (!_currentDefaultTerminal) - { - _currentDefaultTerminal = DefaultTerminal::Current(); - } + _refreshDefaultTerminals(); return _currentDefaultTerminal; } @@ -883,11 +874,27 @@ void CascadiaSettings::CurrentDefaultTerminal(const Model::DefaultTerminal& term _currentDefaultTerminal = terminal; } +// This function is implicitly called by DefaultTerminals/CurrentDefaultTerminal(). +// It reloads the selection of available, installed terminals and caches them. +// WinUI requires us that the `SelectedItem` of a collection is member of the list given to `ItemsSource`. +// It's thus important that _currentDefaultTerminal is a member of _defaultTerminals. +// Right now this is implicitly the case thanks to DefaultTerminal::Available(), +// but in the future it might be worthwhile to change the code to use list indices instead. +void CascadiaSettings::_refreshDefaultTerminals() +{ + if (!_defaultTerminals) + { + auto [defaultTerminals, defaultTerminal] = DefaultTerminal::Available(); + _defaultTerminals = winrt::single_threaded_observable_vector(std::move(defaultTerminals)); + _currentDefaultTerminal = std::move(defaultTerminal); + } +} + void CascadiaSettings::ExportFile(winrt::hstring path, winrt::hstring content) { try { - winrt::Microsoft::Terminal::Settings::Model::WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content)); + WriteUTF8FileAtomic({ path.c_str() }, til::u16u8(content)); } CATCH_LOG(); } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index d25e7373c..176600c75 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -134,7 +134,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // defterm static bool IsDefaultTerminalAvailable() noexcept; - winrt::Windows::Foundation::Collections::IObservableVector DefaultTerminals() const noexcept; + winrt::Windows::Foundation::Collections::IObservableVector DefaultTerminals() noexcept; Model::DefaultTerminal CurrentDefaultTerminal() noexcept; void CurrentDefaultTerminal(const Model::DefaultTerminal& terminal); @@ -142,6 +142,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static const std::filesystem::path& _settingsPath(); winrt::com_ptr _createNewProfile(const std::wstring_view& name) const; + void _refreshDefaultTerminals(); void _resolveDefaultProfile() const; @@ -164,6 +165,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::hstring _deserializationErrorMessage; // defterm + winrt::Windows::Foundation::Collections::IObservableVector _defaultTerminals{ nullptr }; Model::DefaultTerminal _currentDefaultTerminal{ nullptr }; }; } diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index f667b9ede..6b92b99b7 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -24,6 +24,7 @@ #include "userDefaults.h" #include "ApplicationState.h" +#include "DefaultTerminal.h" #include "FileUtils.h" using namespace winrt::Microsoft::Terminal::Settings; @@ -36,8 +37,6 @@ static constexpr std::string_view ProfilesKey{ "profiles" }; static constexpr std::string_view DefaultSettingsKey{ "defaults" }; static constexpr std::string_view ProfilesListKey{ "list" }; static constexpr std::string_view SchemesKey{ "schemes" }; -static constexpr std::string_view NameKey{ "name" }; -static constexpr std::string_view GuidKey{ "guid" }; static constexpr std::wstring_view jsonExtension{ L".json" }; static constexpr std::wstring_view FragmentsSubDirectory{ L"\\Fragments" }; diff --git a/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp b/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp index 5f5079d2f..9a614baf3 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp +++ b/src/cascadia/TerminalSettingsModel/DefaultTerminal.cpp @@ -10,85 +10,70 @@ using namespace winrt::Microsoft::Terminal::Settings; using namespace winrt::Microsoft::Terminal::Settings::Model::implementation; -winrt::Windows::Foundation::Collections::IVector DefaultTerminal::_available = winrt::single_threaded_vector(); -Model::DefaultTerminal DefaultTerminal::_current = nullptr; - -DefaultTerminal::DefaultTerminal(DelegationConfig::DelegationPackage pkg) : - _pkg(pkg) +DefaultTerminal::DefaultTerminal(DelegationConfig::DelegationPackage&& pkg) : + _pkg{ pkg } { } winrt::hstring DefaultTerminal::Name() const { - static const std::wstring def{ RS_(L"InboxWindowsConsoleName") }; - return _pkg.terminal.name.empty() ? winrt::hstring{ def } : winrt::hstring{ _pkg.terminal.name }; + return _pkg.terminal.name.empty() ? winrt::hstring{ RS_(L"InboxWindowsConsoleName") } : winrt::hstring{ _pkg.terminal.name }; } winrt::hstring DefaultTerminal::Version() const { // If there's no version information... return empty string instead. - if (DelegationConfig::PkgVersion{} == _pkg.terminal.version) + const auto& version = _pkg.terminal.version; + if (DelegationConfig::PkgVersion{} == version) { return winrt::hstring{}; } - const auto name = fmt::format(L"{}.{}.{}.{}", _pkg.terminal.version.major, _pkg.terminal.version.minor, _pkg.terminal.version.build, _pkg.terminal.version.revision); - return winrt::hstring{ name }; + fmt::wmemory_buffer buffer; + fmt::format_to(buffer, L"{}.{}.{}.{}", version.major, version.minor, version.build, version.revision); + return winrt::hstring{ buffer.data(), gsl::narrow_cast(buffer.size()) }; } winrt::hstring DefaultTerminal::Author() const { - static const std::wstring def{ RS_(L"InboxWindowsConsoleAuthor") }; - return _pkg.terminal.author.empty() ? winrt::hstring{ def } : winrt::hstring{ _pkg.terminal.author }; + return _pkg.terminal.author.empty() ? winrt::hstring{ RS_(L"InboxWindowsConsoleAuthor") } : winrt::hstring{ _pkg.terminal.author }; } winrt::hstring DefaultTerminal::Icon() const { - static const std::wstring_view def{ L"\uE756" }; - return _pkg.terminal.logo.empty() ? winrt::hstring{ def } : winrt::hstring{ _pkg.terminal.logo }; + return _pkg.terminal.logo.empty() ? winrt::hstring{ L"\uE756" } : winrt::hstring{ _pkg.terminal.logo }; } -void DefaultTerminal::Refresh() +std::pair, Model::DefaultTerminal> DefaultTerminal::Available() { + // The potential of returning nullptr for defaultTerminal feels weird, but XAML can + // handle that appropriately and will select nothing as current in the dropdown. + std::vector defaultTerminals; + Model::DefaultTerminal defaultTerminal{ nullptr }; + std::vector allPackages; DelegationConfig::DelegationPackage currentPackage; - LOG_IF_FAILED(DelegationConfig::s_GetAvailablePackages(allPackages, currentPackage)); - _available.Clear(); - for (const auto& pkg : allPackages) + for (auto& pkg : allPackages) { - auto p = winrt::make(pkg); + // Be a bit careful here: We're moving pkg into the constructor. + // Afterwards it'll be invalid, so we need to cache isCurrent. + const auto isCurrent = pkg == currentPackage; + auto p = winrt::make(std::move(pkg)); - _available.Append(p); - - if (pkg == currentPackage) + if (isCurrent) { - _current = p; + defaultTerminal = p; } - } -} -winrt::Windows::Foundation::Collections::IVectorView DefaultTerminal::Available() -{ - Refresh(); - return _available.GetView(); -} - -Model::DefaultTerminal DefaultTerminal::Current() -{ - if (!_current) - { - Refresh(); + defaultTerminals.emplace_back(std::move(p)); } - // The potential of returning nullptr feels weird, but XAML can handle that appropriately - // and will select nothing as current in the dropdown. - return _current; + return { std::move(defaultTerminals), std::move(defaultTerminal) }; } void DefaultTerminal::Current(const Model::DefaultTerminal& term) { THROW_IF_FAILED(DelegationConfig::s_SetDefaultByPackage(winrt::get_self(term)->_pkg, true)); - _current = term; } diff --git a/src/cascadia/TerminalSettingsModel/DefaultTerminal.h b/src/cascadia/TerminalSettingsModel/DefaultTerminal.h index ddce7d576..19f3ae14c 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultTerminal.h +++ b/src/cascadia/TerminalSettingsModel/DefaultTerminal.h @@ -19,7 +19,6 @@ Author(s): #pragma once #include "DefaultTerminal.g.h" -#include "../inc/cppwinrt_utils.h" #include "../../propslib/DelegationConfig.hpp" @@ -27,26 +26,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { struct DefaultTerminal : public DefaultTerminalT { - DefaultTerminal(DelegationConfig::DelegationPackage pkg); + explicit DefaultTerminal(DelegationConfig::DelegationPackage&& pkg); hstring Name() const; hstring Author() const; hstring Version() const; hstring Icon() const; - static void Refresh(); - static Windows::Foundation::Collections::IVectorView Available(); - static Model::DefaultTerminal Current(); + static std::pair, Model::DefaultTerminal> Available(); static void Current(const Model::DefaultTerminal& term); private: DelegationConfig::DelegationPackage _pkg; - static Windows::Foundation::Collections::IVector _available; - static Model::DefaultTerminal _current; }; } - -namespace winrt::Microsoft::Terminal::Settings::Model::factory_implementation -{ - BASIC_FACTORY(DefaultTerminal); -} diff --git a/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl b/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl index 9b3cc662d..94ceb48dd 100644 --- a/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl +++ b/src/cascadia/TerminalSettingsModel/DefaultTerminal.idl @@ -9,8 +9,5 @@ namespace Microsoft.Terminal.Settings.Model String Author { get; }; String Version { get; }; String Icon { get; }; - - static DefaultTerminal Current; - static Windows.Foundation.Collections.IVectorView Available { get; }; } } From c727762602b8bd12e4a3a769053204d7e92b81c5 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 6 Oct 2021 19:02:53 +0200 Subject: [PATCH 18/59] Fix null pointer exceptions for default constructed CascadiaSettings instances (#11428) `CascadiaSettings` is default constructed when human readable error messages are returned. Even in such cases we need to ensure that all fields are properly initialized, as a caller might decide to call a `GlobalSettings` getter. Thus a crash occurred whenever a user was hot-reloading their settings file with invalid JSON as other code then tried to compare the `GlobalSettings()`. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Start Windows Terminal and ensure the settings load fine * Add `"commandline": 123` to any of the generated profiles in settings.json * The application doesn't crash and shows a warning message --- src/cascadia/TerminalSettingsModel/CascadiaSettings.h | 10 +++++----- .../CascadiaSettingsSerialization.cpp | 9 ++++++++- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 176600c75..65856b5d5 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -154,13 +154,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool _hasInvalidColorScheme(const Model::Command& command) const; // user settings - winrt::com_ptr _globals; - winrt::com_ptr _baseLayerProfile; - winrt::Windows::Foundation::Collections::IObservableVector _allProfiles; - winrt::Windows::Foundation::Collections::IObservableVector _activeProfiles; + winrt::com_ptr _globals = winrt::make_self(); + winrt::com_ptr _baseLayerProfile = winrt::make_self(); + winrt::Windows::Foundation::Collections::IObservableVector _allProfiles = winrt::single_threaded_observable_vector(); + winrt::Windows::Foundation::Collections::IObservableVector _activeProfiles = winrt::single_threaded_observable_vector(); // load errors - winrt::Windows::Foundation::Collections::IVector _warnings; + winrt::Windows::Foundation::Collections::IVector _warnings = winrt::single_threaded_vector(); winrt::Windows::Foundation::IReference _loadError; winrt::hstring _deserializationErrorMessage; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 6b92b99b7..224fe2851 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -742,7 +742,14 @@ CascadiaSettings::CascadiaSettings(const std::string_view& userJSON, const std:: { } -CascadiaSettings::CascadiaSettings(SettingsLoader&& loader) +CascadiaSettings::CascadiaSettings(SettingsLoader&& loader) : + // The CascadiaSettings class declaration initializes these fields by default, + // but we're going to set these fields in our constructor later on anyways. + _globals{}, + _baseLayerProfile{}, + _allProfiles{}, + _activeProfiles{}, + _warnings{} { std::vector allProfiles; std::vector activeProfiles; From bd8bfa13bb6ab6883536b0672abcf254aa666875 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 6 Oct 2021 16:11:09 -0500 Subject: [PATCH 19/59] Fix opening the debug tap (#11445) It's possible that we're about to be started, _before_ our paired connection is started. Both will get Start()'ed when their owning TermControl is finally laid out. However, if we're started first, then we'll immediately start printing to the other control as well, which might not have initialized yet. If we do that, we'll explode. Instead, wait here until the other connection is started too, before actually starting the connection to the client app. This will ensure both controls are initialized before the client app is. Fixes #11282 Tested: Opened about 100 debug taps. They all worked. :shipit: --- .../TerminalApp/DebugTapConnection.cpp | 19 ++++++++++++++++++- src/cascadia/TerminalApp/DebugTapConnection.h | 3 +++ src/cascadia/TerminalControl/ControlCore.cpp | 2 +- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalApp/DebugTapConnection.cpp b/src/cascadia/TerminalApp/DebugTapConnection.cpp index 54f327b3e..0d84d8f05 100644 --- a/src/cascadia/TerminalApp/DebugTapConnection.cpp +++ b/src/cascadia/TerminalApp/DebugTapConnection.cpp @@ -21,8 +21,22 @@ namespace winrt::Microsoft::TerminalApp::implementation } void Initialize(const Windows::Foundation::Collections::ValueSet& /*settings*/) {} ~DebugInputTapConnection() = default; - void Start() + winrt::fire_and_forget Start() { + // GH#11282: It's possible that we're about to be started, _before_ + // our paired connection is started. Both will get Start()'ed when + // their owning TermControl is finally laid out. However, if we're + // started first, then we'll immediately start printing to the other + // control as well, which might not have initialized yet. If we do + // that, we'll explode. + // + // Instead, wait here until the other connection is started too, + // before actually starting the connection to the client app. This + // will ensure both controls are initialized before the client app + // is. + co_await winrt::resume_background(); + _pairedTap->_start.wait(); + _wrappedConnection.Start(); } void WriteInput(hstring const& data) @@ -59,6 +73,9 @@ namespace winrt::Microsoft::TerminalApp::implementation void DebugTapConnection::Start() { // presume the wrapped connection is started. + + // This is explained in the comment for GH#11282 above. + _start.count_down(); } void DebugTapConnection::WriteInput(hstring const& data) diff --git a/src/cascadia/TerminalApp/DebugTapConnection.h b/src/cascadia/TerminalApp/DebugTapConnection.h index c5156afc4..56d509fbf 100644 --- a/src/cascadia/TerminalApp/DebugTapConnection.h +++ b/src/cascadia/TerminalApp/DebugTapConnection.h @@ -5,6 +5,7 @@ #include #include "../../inc/cppwinrt_utils.h" +#include namespace winrt::Microsoft::TerminalApp::implementation { @@ -36,6 +37,8 @@ namespace winrt::Microsoft::TerminalApp::implementation winrt::weak_ref _wrappedConnection; winrt::weak_ref _inputSide; + til::latch _start{ 1 }; + friend class DebugInputTapConnection; }; } diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index efcce89c8..9c3af6bc3 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -1025,7 +1025,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation TerminalConnection::ConnectionState ControlCore::ConnectionState() const { - return _connection.State(); + return _connection ? _connection.State() : TerminalConnection::ConnectionState::Closed; } hstring ControlCore::Title() From 694c6b263ff4a85a4f595cc3bd88d426bf822694 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 7 Oct 2021 06:39:20 -0500 Subject: [PATCH 20/59] When enabling opacity on win10, automatically enable acrylic (#11372) In #11180 we made `opacity` independent from `useAcrylic`. We also changed the mouse wheel behavior to only change opacity, and not mess with acrylic. However, on Windows 10, vintage opacity doesn't work at all. So there, we still need to manually enable acrylic when the user requests opacity. * [x] Closes #11285 SUI changes in action: ![auto-acrylic-win10](https://user-images.githubusercontent.com/18356694/136281935-db9a10f4-e0ad-4422-950b-0a01dc3e12c0.gif) --- src/cascadia/TerminalControl/ControlCore.cpp | 35 +++++++++++++++++++ src/cascadia/TerminalControl/ControlCore.h | 2 ++ .../TerminalSettingsEditor/Profiles.cpp | 16 +++++++++ .../TerminalSettingsEditor/Profiles.h | 14 ++++++++ .../UnitTests_Control/ControlCoreTests.cpp | 7 ++-- .../ControlInteractivityTests.cpp | 6 ++-- 6 files changed, 76 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 9c3af6bc3..c287cdbd7 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -442,6 +442,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation _settings.Opacity(newOpacity); + // GH#11285 - If the user is on Windows 10, and they changed the + // transparency of the control s.t. it should be partially opaque, then + // opt them in to acrylic. It's the only way to have transparency on + // Windows 10. + // We'll also turn the acrylic back off when they're fully opaque, which + // is what the Terminal did prior to 1.12. + if (!IsVintageOpacityAvailable()) + { + _settings.UseAcrylic(newOpacity < 1.0); + } + auto eventArgs = winrt::make_self(newOpacity); _TransparencyChangedHandlers(*this, *eventArgs); } @@ -570,6 +581,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation _settings = settings; + // GH#11285 - If the user is on Windows 10, and they wanted opacity, but + // didn't explicitly request acrylic, then opt them in to acrylic. + // On Windows 11+, this isn't needed, because we can have vintage opacity. + if (!IsVintageOpacityAvailable() && _settings.Opacity() < 1.0 && !_settings.UseAcrylic()) + { + _settings.UseAcrylic(true); + } + // Initialize our font information. const auto fontFace = _settings.FontFace(); const short fontHeight = ::base::saturated_cast(_settings.FontSize()); @@ -1545,4 +1564,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation return hstring(ss.str()); } + + // Helper to check if we're on Windows 11 or not. This is used to check if + // we need to use acrylic to achieve transparency, because vintage opacity + // doesn't work in islands on win10. + // Remove when we can remove the rest of GH#11285 + bool ControlCore::IsVintageOpacityAvailable() noexcept + { + OSVERSIONINFOEXW osver{}; + osver.dwOSVersionInfoSize = sizeof(osver); + osver.dwBuildNumber = 22000; + + DWORDLONG dwlConditionMask = 0; + VER_SET_CONDITION(dwlConditionMask, VER_BUILDNUMBER, VER_GREATER_EQUAL); + + return VerifyVersionInfoW(&osver, VER_BUILDNUMBER, dwlConditionMask) != FALSE; + } } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index a04032ef7..9b96f6c7e 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -149,6 +149,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation hstring ReadEntireBuffer() const; + static bool IsVintageOpacityAvailable() noexcept; + // -------------------------------- WinRT Events --------------------------------- // clang-format off WINRT_CALLBACK(FontSizeChanged, Control::FontSizeChangedEventArgs); diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.cpp b/src/cascadia/TerminalSettingsEditor/Profiles.cpp index 8e43f9b10..6c507ecf4 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.cpp +++ b/src/cascadia/TerminalSettingsEditor/Profiles.cpp @@ -47,6 +47,22 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation // NOTE: this is similar to what is done with BackgroundImagePath above _NotifyChanges(L"UseParentProcessDirectory", L"UseCustomStartingDirectory"); } + else if (viewModelProperty == L"UseAcrylic") + { + // GH#11372: If we're on Windows 10, and someone turns off + // acrylic, we're going to disable opacity for them. Opacity + // doesn't work without acrylic on Windows 10. + // + // BODGY: CascadiaSettings's function IsDefaultTerminalAvailable + // is basically a "are we on Windows 11" check, because defterm + // only works on Win11. So we'll use that. + // + // Remove when we can remove the rest of GH#11285 + if (!UseAcrylic() && !CascadiaSettings::IsDefaultTerminalAvailable()) + { + Opacity(1.0); + } + } }); // Do the same for the starting directory diff --git a/src/cascadia/TerminalSettingsEditor/Profiles.h b/src/cascadia/TerminalSettingsEditor/Profiles.h index 74c31e49a..161f84e39 100644 --- a/src/cascadia/TerminalSettingsEditor/Profiles.h +++ b/src/cascadia/TerminalSettingsEditor/Profiles.h @@ -23,6 +23,20 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation void SetAcrylicOpacityPercentageValue(double value) { Opacity(winrt::Microsoft::Terminal::Settings::Editor::Converters::PercentageValueToPercentage(value)); + + // GH#11372: If we're on Windows 10, and someone wants opacity, then + // we'll turn acrylic on for them. Opacity doesn't work without + // acrylic on Windows 10. + // + // BODGY: CascadiaSettings's function IsDefaultTerminalAvailable + // is basically a "are we on Windows 11" check, because defterm + // only works on Win11. So we'll use that. + // + // Remove when we can remove the rest of GH#11285 + if (value < 100.0 && winrt::Microsoft::Terminal::Settings::Model::CascadiaSettings::IsDefaultTerminalAvailable()) + { + UseAcrylic(true); + } }; void SetPadding(double value) diff --git a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp index 9e4a19e7b..4c090b27e 100644 --- a/src/cascadia/UnitTests_Control/ControlCoreTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlCoreTests.cpp @@ -139,8 +139,11 @@ namespace ControlUnitTests // GH#603: Adjusting opacity shouldn't change whether or not we // requested acrylic. - VERIFY_IS_TRUE(settings->UseAcrylic()); - VERIFY_IS_TRUE(core->_settings.UseAcrylic()); + + auto expectedUseAcrylic = winrt::Microsoft::Terminal::Control::implementation::ControlCore::IsVintageOpacityAvailable() ? true : + (expectedOpacity < 1.0 ? true : false); + VERIFY_ARE_EQUAL(expectedUseAcrylic, settings->UseAcrylic()); + VERIFY_ARE_EQUAL(expectedUseAcrylic, core->_settings.UseAcrylic()); }; core->TransparencyChanged(opacityCallback); diff --git a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp index db1e07435..51a2215e9 100644 --- a/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp +++ b/src/cascadia/UnitTests_Control/ControlInteractivityTests.cpp @@ -119,8 +119,10 @@ namespace ControlUnitTests VERIFY_ARE_EQUAL(expectedOpacity, settings->Opacity()); VERIFY_ARE_EQUAL(expectedOpacity, core->_settings.Opacity()); - VERIFY_ARE_EQUAL(useAcrylic, settings->UseAcrylic()); - VERIFY_ARE_EQUAL(useAcrylic, core->_settings.UseAcrylic()); + auto expectedUseAcrylic = winrt::Microsoft::Terminal::Control::implementation::ControlCore::IsVintageOpacityAvailable() ? useAcrylic : + (expectedOpacity < 1.0 ? true : false); + VERIFY_ARE_EQUAL(expectedUseAcrylic, settings->UseAcrylic()); + VERIFY_ARE_EQUAL(expectedUseAcrylic, core->_settings.UseAcrylic()); }; core->TransparencyChanged(opacityCallback); From 84e7ec4f96d1b83a177c5549b8f1e944f0fed20e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 7 Oct 2021 18:30:34 +0200 Subject: [PATCH 21/59] Fix layering issues with CascadiaSettings::_createNewProfile (#11447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CascadiaSettings::_createNewProfile` failed to call `_FinalizeInheritance`. This commits fixes the issue and adds a stern warning for future me. ## PR Checklist * [x] Closes #11392 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Open settings UI * Modify font size in base layer * _Don't_ save * Duplicate any profile with default font size * Ensure the duplicated profile shows the modified base layer font size ✔️ --- .../LocalTests_SettingsModel/ProfileTests.cpp | 26 +++++++++++++------ .../CascadiaSettings.cpp | 8 ++++++ .../CascadiaSettingsSerialization.cpp | 1 + 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp index ee42b89a4..2c5138f6d 100644 --- a/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/ProfileTests.cpp @@ -272,20 +272,30 @@ namespace SettingsModelLocalTests void ProfileTests::DuplicateProfileTest() { static constexpr std::string_view userProfiles{ R"({ - "profiles": [ - { - "name": "profile0", - "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", - "backgroundImage": "file:///some/path", - "hidden": false, - } - ] + "profiles": { + "defaults": { + "font": { + "size": 123 + } + }, + "list": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "backgroundImage": "file:///some/path", + "hidden": false, + } + ] + } })" }; const auto settings = winrt::make_self(userProfiles); const auto profile = settings->AllProfiles().GetAt(0); const auto duplicatedProfile = settings->DuplicateProfile(profile); + // GH#11392: Ensure duplicated profiles properly inherit the base layer, even for nested objects. + VERIFY_ARE_EQUAL(123, duplicatedProfile.FontInfo().FontSize()); + duplicatedProfile.Guid(profile.Guid()); duplicatedProfile.Name(profile.Name()); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 4a9b274bf..ac7008416 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -18,6 +18,13 @@ using namespace winrt::Microsoft::Terminal::Control; using namespace winrt::Windows::Foundation::Collections; using namespace Microsoft::Console; +// Creating a child of a profile requires us to copy certain +// required attributes. This method handles those attributes. +// +// NOTE however that it doesn't call _FinalizeInheritance() for you! Don't forget that! +// +// At the time of writing only one caller needs to call _FinalizeInheritance(), +// which is why this unsafety wasn't further abstracted away. winrt::com_ptr Model::implementation::CreateChild(const winrt::com_ptr& parent) { auto profile = winrt::make_self(); @@ -371,6 +378,7 @@ winrt::com_ptr CascadiaSettings::_createNewProfile(const std::wstring_v LOG_IF_FAILED(CoCreateGuid(&guid)); auto profile = CreateChild(_baseLayerProfile); + profile->_FinalizeInheritance(); profile->Guid(guid); profile->Name(winrt::hstring{ name }); return profile; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 224fe2851..c0e925c56 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -596,6 +596,7 @@ void SettingsLoader::_addParentProfile(const winrt::com_ptr Date: Thu, 7 Oct 2021 12:18:11 -0500 Subject: [PATCH 22/59] Make sure all the commandlines are fully qualified (#11437) This was originally in #11308. Thought we should check it in for 1.12 even though that won't merge this release. Should slightly mitigate the number of users that see this warning. --- src/cascadia/TerminalSettingsModel/Profile.h | 2 +- src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp | 6 +++++- src/cascadia/TerminalSettingsModel/userDefaults.json | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Profile.h b/src/cascadia/TerminalSettingsModel/Profile.h index 9e54b0e7e..f69471332 100644 --- a/src/cascadia/TerminalSettingsModel/Profile.h +++ b/src/cascadia/TerminalSettingsModel/Profile.h @@ -125,7 +125,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation INHERITABLE_SETTING(Model::Profile, hstring, Padding, DEFAULT_PADDING); - INHERITABLE_SETTING(Model::Profile, hstring, Commandline, L"cmd.exe"); + INHERITABLE_SETTING(Model::Profile, hstring, Commandline, L"%SystemRoot%\\System32\\cmd.exe"); INHERITABLE_SETTING(Model::Profile, hstring, StartingDirectory); INHERITABLE_SETTING(Model::Profile, Microsoft::Terminal::Control::TextAntialiasingMode, AntialiasingMode, Microsoft::Terminal::Control::TextAntialiasingMode::Grayscale); diff --git a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp index b0cfadfbd..8265b338a 100644 --- a/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp +++ b/src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp @@ -37,7 +37,11 @@ std::wstring_view WslDistroGenerator::GetNamespace() const noexcept static winrt::com_ptr makeProfile(const std::wstring& distName) { const auto WSLDistro{ CreateDynamicProfile(distName) }; - WSLDistro->Commandline(winrt::hstring{ L"wsl.exe -d " + distName }); + // GH#11096 - make sure the WSL path starts explicitly with + // C:\Windows\System32. Don't want someone path hijacking wsl.exe. + std::wstring command{}; + THROW_IF_FAILED(wil::GetSystemDirectoryW(command)); + WSLDistro->Commandline(winrt::hstring{ command + L"\\wsl.exe -d " + distName }); WSLDistro->DefaultAppearance().ColorSchemeName(L"Campbell"); WSLDistro->StartingDirectory(winrt::hstring{ DEFAULT_STARTING_DIRECTORY }); WSLDistro->Icon(L"ms-appx:///ProfileIcons/{9acb9455-ca41-5af7-950f-6bca1bc9722f}.png"); diff --git a/src/cascadia/TerminalSettingsModel/userDefaults.json b/src/cascadia/TerminalSettingsModel/userDefaults.json index 28ed3beef..59fa13a6d 100644 --- a/src/cascadia/TerminalSettingsModel/userDefaults.json +++ b/src/cascadia/TerminalSettingsModel/userDefaults.json @@ -10,13 +10,13 @@ { "guid": "{61c54bbd-c2c6-5271-96e7-009a87ff44bf}", "name": "Windows PowerShell", - "commandline": "powershell.exe", + "commandline": "%SystemRoot%\\System32\\WindowsPowerShell\\v1.0\\powershell.exe", "hidden": false }, { // "name" is filled in by CascadiaSettings as a localized string. "guid": "{0caa0dad-35be-5f56-a8ff-afceeeaa6101}", - "commandline": "cmd.exe", + "commandline": "%SystemRoot%\\System32\\cmd.exe", "hidden": false } ] From 2b1468eaa2ad3f5310c7a8b29bf4e1e52bb1a188 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 7 Oct 2021 19:44:03 +0200 Subject: [PATCH 23/59] Add a information popup about default terminals (#11397) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a simple information popup about default terminals, guiding first-time Windows 11 users into changing the default terminal. ## Validation Steps Performed * Info bar pops up on Windows 11 ✔️ * Info bar can be dismissed persistently ✔️ --- .../Resources/en-US/Resources.resw | 3 + src/cascadia/TerminalApp/TerminalPage.cpp | 58 +++++++++++++++++-- src/cascadia/TerminalApp/TerminalPage.h | 2 + src/cascadia/TerminalApp/TerminalPage.xaml | 13 +++++ .../ApplicationState.idl | 3 +- .../CascadiaSettings.cpp | 11 ++++ .../TerminalSettingsModel/CascadiaSettings.h | 1 + .../CascadiaSettings.idl | 1 + .../TerminalSettingsModel/DefaultTerminal.cpp | 10 ++++ .../TerminalSettingsModel/DefaultTerminal.h | 1 + .../TerminalSettingsSerializationHelpers.h | 3 +- 11 files changed, 99 insertions(+), 7 deletions(-) diff --git a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw index 55e51abbb..c9f84a239 100644 --- a/src/cascadia/TerminalApp/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalApp/Resources/en-US/Resources.resw @@ -718,6 +718,9 @@ Termination behavior can be configured in advanced profile settings. + + Windows Terminal can be set as the default terminal application in your settings. + Don't show again diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 25140362d..8501f117d 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -287,6 +287,8 @@ namespace winrt::TerminalApp::implementation _defaultPointerCursor = CoreWindow::GetForCurrentThread().PointerCursor(); } CATCH_LOG(); + + ShowSetAsDefaultInfoBar(); } // Method Description; @@ -2893,6 +2895,29 @@ namespace winrt::TerminalApp::implementation } } + // Method Description: + // - Displays a info popup guiding the user into setting their default terminal. + void TerminalPage::ShowSetAsDefaultInfoBar() const + { + if (!CascadiaSettings::IsDefaultTerminalAvailable() || _IsMessageDismissed(InfoBarMessage::SetAsDefault)) + { + return; + } + + // If the user has already configured any terminal for hand-off we + // shouldn't inform them again about the possibility to do so. + if (CascadiaSettings::IsDefaultTerminalSet()) + { + _DismissMessage(InfoBarMessage::SetAsDefault); + return; + } + + if (const auto infoBar = FindName(L"SetAsDefaultInfoBar").try_as()) + { + infoBar.IsOpen(true); + } + } + // Function Description: // - Helper function to get the OS-localized name for the "Touch Keyboard // and Handwriting Panel Service". If we can't open up the service for any @@ -3313,6 +3338,22 @@ namespace winrt::TerminalApp::implementation } } + // Method Description: + // - Persists the user's choice not to show the information bar warning about "Windows Terminal can be set as your default terminal application" + // Then hides this information buffer. + // Arguments: + // - + // Return Value: + // - + void TerminalPage::_SetAsDefaultDismissHandler(const IInspectable& /*sender*/, const IInspectable& /*args*/) const + { + _DismissMessage(InfoBarMessage::SetAsDefault); + if (const auto infoBar = FindName(L"SetAsDefaultInfoBar").try_as()) + { + infoBar.IsOpen(false); + } + } + // Method Description: // - Checks whether information bar message was dismissed earlier (in the application state) // Arguments: @@ -3342,13 +3383,20 @@ namespace winrt::TerminalApp::implementation // - void TerminalPage::_DismissMessage(const InfoBarMessage& message) { - auto dismissedMessages = ApplicationState::SharedInstance().DismissedMessages(); - if (!dismissedMessages) + const auto applicationState = ApplicationState::SharedInstance(); + std::vector messages; + + if (const auto values = applicationState.DismissedMessages()) { - dismissedMessages = winrt::single_threaded_vector(); + messages.resize(values.Size()); + values.GetMany(0, messages); } - dismissedMessages.Append(message); - ApplicationState::SharedInstance().DismissedMessages(dismissedMessages); + if (std::none_of(messages.begin(), messages.end(), [&](const auto& m) { return m == message; })) + { + messages.emplace_back(message); + } + + applicationState.DismissedMessages(std::move(messages)); } } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index 26332453a..6fdc88d15 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -95,6 +95,7 @@ namespace winrt::TerminalApp::implementation winrt::TerminalApp::TaskbarState TaskbarState() const; void ShowKeyboardServiceWarning() const; + void ShowSetAsDefaultInfoBar() const; winrt::hstring KeyboardServiceDisabledText(); winrt::fire_and_forget IdentifyWindow(); @@ -403,6 +404,7 @@ namespace winrt::TerminalApp::implementation winrt::fire_and_forget _ConnectionStateChangedHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args) const; void _CloseOnExitInfoDismissHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args) const; void _KeyboardServiceWarningInfoDismissHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args) const; + void _SetAsDefaultDismissHandler(const winrt::Windows::Foundation::IInspectable& sender, const winrt::Windows::Foundation::IInspectable& args) const; static bool _IsMessageDismissed(const winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage& message); static void _DismissMessage(const winrt::Microsoft::Terminal::Settings::Model::InfoBarMessage& message); diff --git a/src/cascadia/TerminalApp/TerminalPage.xaml b/src/cascadia/TerminalApp/TerminalPage.xaml index b6a4676dd..f5e5b7df4 100644 --- a/src/cascadia/TerminalApp/TerminalPage.xaml +++ b/src/cascadia/TerminalApp/TerminalPage.xaml @@ -141,6 +141,19 @@ Click="_CloseOnExitInfoDismissHandler" /> + + + +