-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
6953: use native locale when comparing command names #8017
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
858d722
6953: use native locale when comparing command names
Don-Vito db52ee7
6953: explicitly read default user local name
Don-Vito 1ad0477
6953: extract locale loading into a function
Don-Vito 0887e8e
6953: increase the locale buffer by 1 to accommodate locale character…
Don-Vito 61c3827
6953: add try catch to be on the safe side if for some reason the run…
Don-Vito 8951cbb
Merge remote-tracking branch 'upstream/main' into fet/6953-user-locale
Don-Vito File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,8 @@ namespace winrt::TerminalApp::implementation | |
_allCommands = winrt::single_threaded_vector<Command>(); | ||
_tabActions = winrt::single_threaded_vector<Command>(); | ||
|
||
_userLocale = _loadUserLocale(); | ||
|
||
_switchToMode(CommandPaletteMode::ActionMode); | ||
|
||
if (CommandPaletteShadow()) | ||
|
@@ -97,6 +99,33 @@ 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() | ||
{ | ||
try | ||
{ | ||
// 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<unsigned int>(localeNameSize) }; | ||
return std::locale::locale(til::u16u8(loadedLocale)); | ||
} | ||
CATCH_LOG(); | ||
|
||
// if something went wrong e.g., the required locale cannot be created, use the default one | ||
return std::locale(); | ||
} | ||
|
||
// 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. | ||
|
@@ -778,11 +807,14 @@ 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) | ||
{ | ||
std::wstring_view leftName{ lhs.Name() }; | ||
std::wstring_view rightName{ rhs.Name() }; | ||
return leftName.compare(rightName) < 0; | ||
// Pay attention that obtaining the facet involves a critical section. | ||
// Irrelevant concern for now as no contention is expected. | ||
const auto& userLocaleFacet = std::use_facet<std::collate<wchar_t>>(_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; | ||
} | ||
|
||
// This is a helper struct to aid in sorting Commands by a given weighting. | ||
|
@@ -791,6 +823,7 @@ namespace winrt::TerminalApp::implementation | |
Command command; | ||
int weight; | ||
int inOrderCounter; | ||
std::function<bool(const Command&, const Command&)> pfnCompareCommandNames; | ||
|
||
bool operator<(const WeightedCommand& other) const | ||
{ | ||
|
@@ -805,7 +838,7 @@ namespace winrt::TerminalApp::implementation | |
} | ||
else | ||
{ | ||
return !_compareCommandNames(command, other.command); | ||
return !pfnCompareCommandNames(command, other.command); | ||
} | ||
} | ||
return weight < other.weight; | ||
|
@@ -828,6 +861,7 @@ namespace winrt::TerminalApp::implementation | |
const bool addAll = searchText.empty(); | ||
|
||
auto commandsToFilter = _commandsToFilter(); | ||
auto pfnCompareCommandNames = std::bind(&CommandPalette::_compareCommandNames, this, std::placeholders::_1, std::placeholders::_2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DHowett, didn't we have a better option than |
||
|
||
// 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 | ||
|
@@ -856,7 +890,7 @@ namespace winrt::TerminalApp::implementation | |
} | ||
std::sort(sortedCommands.begin(), | ||
sortedCommands.end(), | ||
_compareCommandNames); | ||
pfnCompareCommandNames); | ||
|
||
for (auto action : sortedCommands) | ||
{ | ||
|
@@ -884,6 +918,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) | ||
{ | ||
|
@@ -894,6 +929,7 @@ namespace winrt::TerminalApp::implementation | |
wc.command = action; | ||
wc.weight = weight; | ||
wc.inOrderCounter = counter++; | ||
wc.pfnCompareCommandNames = pfnCompareCommandNames; | ||
|
||
heap.push(wc); | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShawnSteele, is this an appropriate way of getting the locale for
std::locale
and using it to compare correctly below?