Skip to content
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

[Functional-Command Palette] - Case sensitive commands in Russian localization #9941

Closed
whatsupbros opened this issue Apr 24, 2021 · 5 comments
Labels
Area-CmdPal Command Palette issues and features Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@whatsupbros
Copy link

User Experience:
Users of Russian Windows localization can be confused, because commands are case sensitive. So, "Новая вкладка" ("New tab") command cannot be found with "новая" ("new") input string in lowercase, but can be found with "Новая" ("New"), which preserves the case.

Test Environment:
App: Windows Terminal 1.7.1033.0
Feature: Command Palette
OS: Microsoft Windows [Version 10.0.19042.928]

Repro Steps:

  1. Open Windows terminal app
  2. Open Command Palette using shortcut (ctrl+shift+p)
  3. Search for New tab command in the search field in Russian, use "новая" search string for this and see the empty result:
    1
  4. Now use "Новая" instead, see the different result
    2

Expected:
Commands in Command Pallete to be case insensitive.
In fact, commands should be findable in English also, even is Russian localization of the OS.

So, both "new" or "новая" search strings should find "Новая вкладка" command (which is Russian translation for "New tab").

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 24, 2021
@Don-Vito
Copy link
Contributor

@Zza - thanks for reporting.

It looks that we added locale aware case-sensitivity for commands sorting, but not for command filtering. Inside FilteredCommand::_computeHighlightedName we have:

const auto lowerCaseSearchChar = std::towlower(searchChar);
...
auto isCurrentCharMatched = std::towlower(commandName[currentOffset]) == lowerCaseSearchChar;

In the code above std::towlower uses default locale that for some reason is not Russian locale.

@Don-Vito
Copy link
Contributor

Forgot to mention, that the trivial fix is to use the same approach we have for sorting (lstrcmpi) to compare chars.
(There are other approaches, but I think this one is both trivial and robust).

@zadjii-msft - please assign this to me (hope to fix it later on).

@ghost ghost added the In-PR This issue has a related PR label Apr 25, 2021
@Don-Vito
Copy link
Contributor

OK.. it was kinda "one-liner", so I did it, though didn't test it in Russian.

@zadjii-msft zadjii-msft added Area-CmdPal Command Palette issues and features Product-Terminal The new Windows Terminal. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 29, 2021
@ghost ghost closed this as completed in cb55cec May 4, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels May 4, 2021
DHowett pushed a commit that referenced this issue May 14, 2021
## PR Checklist
* [x] Closes #9941
* [x] CLA signed.

## Detailed Description of the Pull Request / Additional comments
The bug is due to us using std::tolower, while the default locale is not user's locale.
The fix here is to use the same approach as upon sorting: lstrcmpi.
While there are additional methods to do locale aware comparison,
here we convert chars to string and call lstrcmpi.
While this approach seems somewhat inefficient it ensures consistency
(with the order of locales that lstrcmi tries to apply internally).

(cherry picked from commit cb55cec)
DHowett pushed a commit that referenced this issue May 14, 2021
## PR Checklist
* [x] Closes #9941
* [x] CLA signed.

## Detailed Description of the Pull Request / Additional comments
The bug is due to us using std::tolower, while the default locale is not user's locale.
The fix here is to use the same approach as upon sorting: lstrcmpi.
While there are additional methods to do locale aware comparison,
here we convert chars to string and call lstrcmpi.
While this approach seems somewhat inefficient it ensures consistency
(with the order of locales that lstrcmi tries to apply internally).

(cherry picked from commit cb55cec)
@ghost
Copy link

ghost commented May 25, 2021

🎉This issue was addressed in #9943, which has now been successfully released as Windows Terminal v1.8.1444.0.:tada:

Handy links:

@ghost
Copy link

ghost commented May 25, 2021

🎉This issue was addressed in #9943, which has now been successfully released as Windows Terminal Preview v1.9.1445.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CmdPal Command Palette issues and features Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

No branches or pull requests

3 participants