-
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
Conversation
@PankajBhojwani - Do not merge this 😄. This is still not ready - with the std::locale("") i am getting an access violation. Hopefully, tomorrow I will windbg it (need symbols and stuff). Everything works with std::locale(), which is not surprising, as this is a default. But it probably means that the problem is not with the code, but rather with the "", which is somehow not considered valid. I believe we can also resolve this by explicitly obtaining user locale via an API. |
Accidentally wrote this in the ticket rather than here - was able to workaround by explicitly calling GetUserDefaultLocaleName. I still need to cleanup the code, but hope you can take a look and let me know what you think. |
return std::locale(); | ||
} | ||
|
||
std::wstring loadedLocale(userLocaleName); |
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.
nit: std::wstring loadedLocale{ userLocaleName };
(we prefer using curly braces for variable initialization)
… + use wstring ctor receiving the size
…timee will fail to load the custom locale
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.
Looks good to me! Thank you for the contribution
@@ -795,6 +828,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 comment
The reason will be displayed to describe this comment to others. Learn more.
@DHowett, didn't we have a better option than std::bind
?
// 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)); |
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?
Abandoning this one. It is blocked for more than 3 weeks already and with all the changes requires a complete rework. I can try to work on this again, if it is still relevant. I hope other blocking PRs will have a better fate 😊 |
Hey @Don-Vito, sorry for the delays in getting back to you on this one. It must've been lost in all the other PR's 😆 Part of me thinks that the better solution to this problem is what was outlined over in #7039. Apparently VSCode displays both the localized command name, but also the English one (the un-localized one), and then the palette filters based off the English string. I don't know if there is an easy way at getting at the I also don't know if it only does that for Chinese, Japanese, Korean, but not German or other latin scripts - that might need to be investigated more. |
@zadjii-msft - I am actually tracking this #7039 for a while, and agree that it suggests a better solution (I guess not localizing the palette is even better, but I cannot say for everyone). I think yo need to close one of them as they seem quite mutually exclusive - no? |
They sure do, don't they! |
Summary of the Pull Request
Sort the names in the command palette based on the user level localization settings
References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Loading user local when palette is created (to perform caching and ensure consistent behavior). Using WINAPI's GetUserDefaultLocaleName explicitly to retrieve the user's default locale, as std::locale("") crashes.
Validation Steps Performed