From 858d7229437bd08df524625979c42abf96527875 Mon Sep 17 00:00:00 2001 From: khvitaly Date: Fri, 23 Oct 2020 01:33:39 +0300 Subject: [PATCH 1/5] 6953: use native locale when comparing command names --- src/cascadia/TerminalApp/CommandPalette.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index d0129706746..6b73d209c6d 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -747,9 +747,12 @@ namespace winrt::TerminalApp::implementation // This is a helper to aid in sorting commands by their `Name`s, alphabetically. static bool _compareCommandNames(const Command& lhs, const Command& rhs) { - std::wstring_view leftName{ lhs.Name() }; - std::wstring_view rightName{ rhs.Name() }; - return leftName.compare(rightName) < 0; + // Pay attention that obtaining the facet and the locale involves a critical section. + // Irrelevant concern for now as no contention is expected. + const auto& userLocaleFacet = std::use_facet>(std::locale("")); + const auto lhsName = lhs.Name(); + const auto rhsName = rhs.Name(); + return userLocaleFacet.compare(lhsName.data(), lhsName.data() + lhsName.size(), rhsName.data(), rhsName.data() + rhsName.size()) < 0; } // This is a helper struct to aid in sorting Commands by a given weighting. From db52ee7f4f7677f85310fdf0f4c9b831d07a658e Mon Sep 17 00:00:00 2001 From: khvitaly Date: Fri, 23 Oct 2020 03:11:34 +0300 Subject: [PATCH 2/5] 6953: explicitly read default user local name --- src/cascadia/TerminalApp/CommandPalette.cpp | 26 +++++++++++++++++---- src/cascadia/TerminalApp/CommandPalette.h | 3 +++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 6b73d209c6d..1480cc169f8 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -30,6 +30,19 @@ namespace winrt::TerminalApp::implementation _allCommands = winrt::single_threaded_vector(); _allTabActions = winrt::single_threaded_vector(); + wchar_t localeName[LOCALE_NAME_MAX_LENGTH] = { 0 }; + auto localeNameSize = GetUserDefaultLocaleName(localeName, ARRAYSIZE(localeName)); + if (localeNameSize == 0) + { + LOG_LAST_ERROR(); + _userLocaleName = std::locale().name(); + } + else + { + std::wstring loadedLocale(localeName); + _userLocaleName = til::u16u8(loadedLocale); + } + _switchToMode(CommandPaletteMode::ActionMode); if (CommandPaletteShadow()) @@ -745,11 +758,12 @@ namespace winrt::TerminalApp::implementation } // This is a helper to aid in sorting commands by their `Name`s, alphabetically. - static bool _compareCommandNames(const Command& lhs, const Command& rhs) + bool CommandPalette::_compareCommandNames(const Command& lhs, const Command& rhs) { // Pay attention that obtaining the facet and the locale involves a critical section. // Irrelevant concern for now as no contention is expected. - const auto& userLocaleFacet = std::use_facet>(std::locale("")); + const auto userLocale = std::locale::locale(_userLocaleName); + const auto& userLocaleFacet = std::use_facet>(userLocale); const auto lhsName = lhs.Name(); const auto rhsName = rhs.Name(); return userLocaleFacet.compare(lhsName.data(), lhsName.data() + lhsName.size(), rhsName.data(), rhsName.data() + rhsName.size()) < 0; @@ -761,6 +775,7 @@ namespace winrt::TerminalApp::implementation Command command; int weight; int inOrderCounter; + std::function nameComparator; bool operator<(const WeightedCommand& other) const { @@ -775,7 +790,7 @@ namespace winrt::TerminalApp::implementation } else { - return !_compareCommandNames(command, other.command); + return !nameComparator(command, other.command); } } return weight < other.weight; @@ -798,6 +813,7 @@ namespace winrt::TerminalApp::implementation const bool addAll = searchText.empty(); auto commandsToFilter = _commandsToFilter(); + auto pfnCompareCommands = std::bind(&CommandPalette::_compareCommandNames, this, std::placeholders::_1, std::placeholders::_2); // If there's no filter text, then just add all the commands in order to the list. // - TODO GH#6647:Possibly add the MRU commands first in order, followed @@ -826,7 +842,7 @@ namespace winrt::TerminalApp::implementation } std::sort(sortedCommands.begin(), sortedCommands.end(), - _compareCommandNames); + pfnCompareCommands); for (auto action : sortedCommands) { @@ -854,6 +870,7 @@ namespace winrt::TerminalApp::implementation // TODO GH#7205: Find a better way to ensure that WCs of the same // weight and name stay in the order in which they were pushed onto // the PQ. + uint32_t counter = 0; for (auto action : commandsToFilter) { @@ -864,6 +881,7 @@ namespace winrt::TerminalApp::implementation wc.command = action; wc.weight = weight; wc.inOrderCounter = counter++; + wc.nameComparator = pfnCompareCommands; heap.push(wc); } diff --git a/src/cascadia/TerminalApp/CommandPalette.h b/src/cascadia/TerminalApp/CommandPalette.h index 1dad812de26..2348fd7df47 100644 --- a/src/cascadia/TerminalApp/CommandPalette.h +++ b/src/cascadia/TerminalApp/CommandPalette.h @@ -59,6 +59,7 @@ namespace winrt::TerminalApp::implementation winrt::TerminalApp::ShortcutActionDispatch _dispatch; Windows::Foundation::Collections::IVector _commandsToFilter(); + std::string _userLocaleName; bool _lastFilterTextWasEmpty{ true }; @@ -106,6 +107,8 @@ namespace winrt::TerminalApp::implementation void _dispatchCommand(const Microsoft::Terminal::Settings::Model::Command& command); void _dispatchCommandline(); void _dismissPalette(); + + bool _compareCommandNames(const Microsoft::Terminal::Settings::Model::Command& lhs, const Microsoft::Terminal::Settings::Model::Command& rhs); }; } From 1ad0477237e30ce031e5469c9017448021fc478e Mon Sep 17 00:00:00 2001 From: khvitaly Date: Fri, 23 Oct 2020 10:56:54 +0300 Subject: [PATCH 3/5] 6953: extract locale loading into a function --- src/cascadia/TerminalApp/CommandPalette.cpp | 48 ++++++++++++--------- src/cascadia/TerminalApp/CommandPalette.h | 4 +- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 1480cc169f8..636233c7be4 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -30,18 +30,7 @@ namespace winrt::TerminalApp::implementation _allCommands = winrt::single_threaded_vector(); _allTabActions = winrt::single_threaded_vector(); - wchar_t localeName[LOCALE_NAME_MAX_LENGTH] = { 0 }; - auto localeNameSize = GetUserDefaultLocaleName(localeName, ARRAYSIZE(localeName)); - if (localeNameSize == 0) - { - LOG_LAST_ERROR(); - _userLocaleName = std::locale().name(); - } - else - { - std::wstring loadedLocale(localeName); - _userLocaleName = til::u16u8(loadedLocale); - } + _userLocale = _loadUserLocale(); _switchToMode(CommandPaletteMode::ActionMode); @@ -110,6 +99,26 @@ namespace winrt::TerminalApp::implementation _filteredActionsView().SelectionChanged({ this, &CommandPalette::_selectedCommandChanged }); } + // Method Description: + // - Attempts to load user locale, to further use it when sorting the actions in the filtered view + // Return Value: + // - The user level locale, upon failure returns a default locale + std::locale CommandPalette::_loadUserLocale() + { + // Loading user locale name explicitly using GetUserDefaultLocaleName, + // as std:locale("") crashes on some setups + wchar_t userLocaleName[LOCALE_NAME_MAX_LENGTH] = { 0 }; + auto localeNameSize = GetUserDefaultLocaleName(userLocaleName, ARRAYSIZE(userLocaleName)); + if (localeNameSize == 0) + { + LOG_LAST_ERROR(); + return std::locale(); + } + + std::wstring loadedLocale(userLocaleName); + return std::locale::locale(til::u16u8(loadedLocale)); + } + // Method Description: // - Moves the focus up or down the list of commands. If we're at the top, // we'll loop around to the bottom, and vice-versa. @@ -760,10 +769,9 @@ namespace winrt::TerminalApp::implementation // This is a helper to aid in sorting commands by their `Name`s, alphabetically. bool CommandPalette::_compareCommandNames(const Command& lhs, const Command& rhs) { - // Pay attention that obtaining the facet and the locale involves a critical section. + // Pay attention that obtaining the facet involves a critical section. // Irrelevant concern for now as no contention is expected. - const auto userLocale = std::locale::locale(_userLocaleName); - const auto& userLocaleFacet = std::use_facet>(userLocale); + const auto& userLocaleFacet = std::use_facet>(_userLocale); const auto lhsName = lhs.Name(); const auto rhsName = rhs.Name(); return userLocaleFacet.compare(lhsName.data(), lhsName.data() + lhsName.size(), rhsName.data(), rhsName.data() + rhsName.size()) < 0; @@ -775,7 +783,7 @@ namespace winrt::TerminalApp::implementation Command command; int weight; int inOrderCounter; - std::function nameComparator; + std::function pfnCompareCommandNames; bool operator<(const WeightedCommand& other) const { @@ -790,7 +798,7 @@ namespace winrt::TerminalApp::implementation } else { - return !nameComparator(command, other.command); + return !pfnCompareCommandNames(command, other.command); } } return weight < other.weight; @@ -813,7 +821,7 @@ namespace winrt::TerminalApp::implementation const bool addAll = searchText.empty(); auto commandsToFilter = _commandsToFilter(); - auto pfnCompareCommands = std::bind(&CommandPalette::_compareCommandNames, this, std::placeholders::_1, std::placeholders::_2); + auto pfnCompareCommandNames = std::bind(&CommandPalette::_compareCommandNames, this, std::placeholders::_1, std::placeholders::_2); // If there's no filter text, then just add all the commands in order to the list. // - TODO GH#6647:Possibly add the MRU commands first in order, followed @@ -842,7 +850,7 @@ namespace winrt::TerminalApp::implementation } std::sort(sortedCommands.begin(), sortedCommands.end(), - pfnCompareCommands); + pfnCompareCommandNames); for (auto action : sortedCommands) { @@ -881,7 +889,7 @@ namespace winrt::TerminalApp::implementation wc.command = action; wc.weight = weight; wc.inOrderCounter = counter++; - wc.nameComparator = pfnCompareCommands; + wc.pfnCompareCommandNames = pfnCompareCommandNames; heap.push(wc); } diff --git a/src/cascadia/TerminalApp/CommandPalette.h b/src/cascadia/TerminalApp/CommandPalette.h index 2348fd7df47..fb5400d5ec3 100644 --- a/src/cascadia/TerminalApp/CommandPalette.h +++ b/src/cascadia/TerminalApp/CommandPalette.h @@ -57,9 +57,11 @@ namespace winrt::TerminalApp::implementation Windows::Foundation::Collections::IVector _nestedActionStack{ nullptr }; winrt::TerminalApp::ShortcutActionDispatch _dispatch; + std::locale _userLocale; + + static std::locale _loadUserLocale(); Windows::Foundation::Collections::IVector _commandsToFilter(); - std::string _userLocaleName; bool _lastFilterTextWasEmpty{ true }; From 0887e8e52b1769da8639c32e2c020a0d395f44c1 Mon Sep 17 00:00:00 2001 From: khvitaly Date: Fri, 23 Oct 2020 21:31:48 +0300 Subject: [PATCH 4/5] 6953: increase the locale buffer by 1 to accommodate locale character + use wstring ctor receiving the size --- src/cascadia/TerminalApp/CommandPalette.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 636233c7be4..531a0a689c3 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -106,8 +106,8 @@ namespace winrt::TerminalApp::implementation std::locale CommandPalette::_loadUserLocale() { // Loading user locale name explicitly using GetUserDefaultLocaleName, - // as std:locale("") crashes on some setups - wchar_t userLocaleName[LOCALE_NAME_MAX_LENGTH] = { 0 }; + // as std:locale("") crashes on some setups. + wchar_t userLocaleName[LOCALE_NAME_MAX_LENGTH + 1]; auto localeNameSize = GetUserDefaultLocaleName(userLocaleName, ARRAYSIZE(userLocaleName)); if (localeNameSize == 0) { @@ -115,7 +115,7 @@ namespace winrt::TerminalApp::implementation return std::locale(); } - std::wstring loadedLocale(userLocaleName); + std::wstring loadedLocale{ userLocaleName, gsl::narrow(localeNameSize) }; return std::locale::locale(til::u16u8(loadedLocale)); } From 61c38272690da0f5ceb701b56e2d69efbf0e6cee Mon Sep 17 00:00:00 2001 From: khvitaly Date: Fri, 23 Oct 2020 21:53:19 +0300 Subject: [PATCH 5/5] 6953: add try catch to be on the safe side if for some reason the runtimee will fail to load the custom locale --- src/cascadia/TerminalApp/CommandPalette.cpp | 25 +++++++++++++-------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 531a0a689c3..ecb17cca8f3 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -105,18 +105,25 @@ namespace winrt::TerminalApp::implementation // - The user level locale, upon failure returns a default locale std::locale CommandPalette::_loadUserLocale() { - // Loading user locale name explicitly using GetUserDefaultLocaleName, - // as std:locale("") crashes on some setups. - wchar_t userLocaleName[LOCALE_NAME_MAX_LENGTH + 1]; - auto localeNameSize = GetUserDefaultLocaleName(userLocaleName, ARRAYSIZE(userLocaleName)); - if (localeNameSize == 0) + try { - LOG_LAST_ERROR(); - return std::locale(); + // Loading user locale name explicitly using GetUserDefaultLocaleName, + // as std:locale("") crashes on some setups. + wchar_t userLocaleName[LOCALE_NAME_MAX_LENGTH + 1]; + auto localeNameSize = GetUserDefaultLocaleName(userLocaleName, ARRAYSIZE(userLocaleName)); + if (localeNameSize == 0) + { + LOG_LAST_ERROR(); + return std::locale(); + } + + std::wstring loadedLocale{ userLocaleName, gsl::narrow(localeNameSize) }; + return std::locale::locale(til::u16u8(loadedLocale)); } + CATCH_LOG(); - std::wstring loadedLocale{ userLocaleName, gsl::narrow(localeNameSize) }; - return std::locale::locale(til::u16u8(loadedLocale)); + // if something went wrong e.g., the required locale cannot be created, use the default one + return std::locale(); } // Method Description: