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

_compareCommandNames should use locale-aware string comparisons #6953

Closed
jtippet opened this issue Jul 16, 2020 · 9 comments
Closed

_compareCommandNames should use locale-aware string comparisons #6953

jtippet opened this issue Jul 16, 2020 · 9 comments
Labels
Area-Localization Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.

Comments

@jtippet
Copy link
Contributor

jtippet commented Jul 16, 2020

_compareCommandNames operates on strings that are presented to the end-user, so it should sort like a human, not by codepoint. I'm not sure whether this project prefers CompareStringEx or lstrcmp or some CRT routine.

@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 Jul 16, 2020
@DHowett
Copy link
Member

DHowett commented Jul 16, 2020

If there's some STL wstring locale-sensitive compare, I'd index towards that one. Thanks for pointing this out!

@DHowett DHowett added Area-Localization Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jul 17, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 17, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 17, 2020
@DHowett DHowett added this to the Terminal v1.3 milestone Jul 17, 2020
@Don-Vito
Copy link
Contributor

@DHowett - is something like this STL enough?

static bool _compareCommandNames(const Command& lhs, const Command& rhs)
{
        const auto& f = std::use_facet<std::collate<wchar_t>>(std::locale());
        const auto lhsName = lhs.Name();
        const auto rhsName = rhs.Name();
        return f.compare(lhsName.data(), lhsName.data() + lhsName.size(), rhsName.data(), rhsName.data() + rhsName.size()) < 0;            
}

I am just confused what locale should we use.. just written commands in 3 languages and now wondering what should happen 😄

@PankajBhojwani
Copy link
Contributor

PankajBhojwani commented Oct 16, 2020

Based on what I see here, @Don-Vito 's way seems the way to go, would you mind making a PR with the change?

@Don-Vito
Copy link
Contributor

@PankajBhojwani - I can absolutely create a PR, but I am not sure what locale should be used. Is std::locale() good enough?

@PankajBhojwani
Copy link
Contributor

I think std::locale("") obtains the native locale (source), so that might be what we want actually. Will look into it more and let you know!

@PankajBhojwani
Copy link
Contributor

@Don-Vito at this point I want to say it probably is a fine implementation for now, and to go ahead and make the PR for it if you'd like! We can handle potential issues in post

@Don-Vito
Copy link
Contributor

@PankajBhojwani - great! I am creating one.

@ghost ghost added the In-PR This issue has a related PR label Oct 22, 2020
@Don-Vito
Copy link
Contributor

@PankajBhojwani - made it work by explicitly invoking winapi's GetUserDefaultLocaleName. Still need to cleanup the code but please take a look.

@ghost ghost removed the In-PR This issue has a related PR label Nov 18, 2020
@zadjii-msft
Copy link
Member

After some additional thought, we're going to try and solve this with #7039 instead. Thanks for all the investigation everyone!

@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Nov 19, 2020
@zadjii-msft zadjii-msft removed this from the Terminal v1.6 milestone Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Localization Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

6 participants