Skip to content

Fix language match on Linux (Unix?)#104

Merged
lhecker merged 1 commit into
microsoft:mainfrom
AOSC-Tracking:fix-language-match
May 21, 2025
Merged

Fix language match on Linux (Unix?)#104
lhecker merged 1 commit into
microsoft:mainfrom
AOSC-Tracking:fix-language-match

Conversation

@eatradish

@eatradish eatradish commented May 20, 2025

Copy link
Copy Markdown
Contributor

In Linux, it is possible for the LANGUAGE variable to exist but have a empty value,

Before:

图片

After:

图片

@eatradish
eatradish force-pushed the fix-language-match branch from 2e04511 to 0e843c9 Compare May 20, 2025 06:11
@Kijewski

Kijewski commented May 20, 2025

Copy link
Copy Markdown

I don't think this is correct. E.g. LANGUAGE=de_DE.utf8:en_EN would be turned into ["de", "DE.utf8", "en", "EN"]. I would probably replace the whole function with:

pub fn preferred_languages(arena: &Arena) -> Vec<ArenaString<'_>, &Arena> {
    let mut locales = Vec::new_in(arena);

    for key in ["LANGUAGE", "LC_ALL", "LANG"] {
        if let Ok(langs) = std::env::var(key) {
            let langs = langs
                // split colon separated entries
                .split(':')
                // strip territory ("EN" in "en_EN")
                .map(|s| s.split('_').next().unwrap_or(s))
                // strip codeset ("utf8" in "C.utf8")
                .map(|s| s.split('.').next().unwrap_or(s))
                // filter out empty entries
                .filter(|s| !s.is_empty())
                // intern strings in arena
                .map(|s| ArenaString::from_str(arena, s));
            locales.extend(langs);
            if !locales.is_empty() {
                break;
            }
        }
    }

    locales
}

@eatradish
eatradish force-pushed the fix-language-match branch from 0e843c9 to e35b620 Compare May 20, 2025 12:14
@eatradish

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@eatradish

Copy link
Copy Markdown
Contributor Author

cc @Kijewski e35b620 You're right, I've reissued a version of the patch as per your statement

@eatradish
eatradish force-pushed the fix-language-match branch from e35b620 to 9f323ce Compare May 20, 2025 12:21
@Kijewski

Copy link
Copy Markdown

@eatradish, oops, I just noticed that the territory must not be stripped:

"pt-br" => LangId::pt_br,
"ru" => LangId::ru,
"zh-hant" => LangId::zh_hant,
"zh" => LangId::zh_hans,

Could you replace the stripping of everything after _ with replacing _ with -, please?

@eatradish

eatradish commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

@eatradish, oops, I just noticed that the territory must not be stripped:

"pt-br" => LangId::pt_br,
"ru" => LangId::ru,
"zh-hant" => LangId::zh_hant,
"zh" => LangId::zh_hans,

Could you replace the stripping of everything after _ with replacing _ with -, please?

I agree, but I think it's equally important to add the value you get from the - split to the result as well?

@Kijewski

Copy link
Copy Markdown

Sorry, I did not express myself properly (by using the word "replace" twice):

  • don't split at '-'
  • instead make the underscore '_' a dash '-'

@eatradish

eatradish commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

@eatradish, oops, I just noticed that the territory must not be stripped:

"pt-br" => LangId::pt_br,
"ru" => LangId::ru,
"zh-hant" => LangId::zh_hant,
"zh" => LangId::zh_hans,

Could you replace the stripping of everything after _ with replacing _ with -, please?

Sorry, I did not express myself properly (by using the word "replace" twice):

* don't split at '-'

* instead make the underscore '_' a dash '-'

I changed it to this, but for the final result I still have to split the '-' to successfully match 'zh':

pub fn preferred_languages(arena: &Arena) -> Vec<ArenaString<'_>, &Arena> {
    let mut locales = Vec::new_in(arena);

    for key in ["LANGUAGE", "LC_ALL", "LANG"] {
        if let Ok(langs) = std::env::var(key) {
            let langs = langs.replace('_', "-").to_ascii_lowercase();

            let langs = langs
                // split colon separated entries
                .split(':')
                // strip codeset ("utf8" in "C.utf8")
                .map(|s| s.split('.').next().unwrap_or(s))
                // filter out empty entries
                .filter(|s| !s.is_empty())
                // intern instrings in arena
                .map(|s| ArenaString::from_str(arena, s));
            locales.extend(langs);
            if !locales.is_empty() {
                break;
            }
        }
    }

    dbg!(&locales);

    locales
}
[src/sys/unix.rs:548:5] &locales = [
    "zh-cn",
]
"zh" => LangId::zh_hans, 

@eatradish

Copy link
Copy Markdown
Contributor Author

Maybe it should be changed to this?

pub fn preferred_languages(arena: &Arena) -> Vec<ArenaString<'_>, &Arena> {
    let mut locales = Vec::new_in(arena);

    for key in ["LANGUAGE", "LC_ALL", "LANG"] {
        if let Ok(langs) = std::env::var(key) {
            let langs_str = langs.replace('_', "-").to_ascii_lowercase();

            let langs = langs_str
                // split colon separated entries
                .split(':')
                // strip codeset ("utf8" in "C.utf8")
                .map(|s| s.split('.').next().unwrap_or(s))
                // filter out empty entries
                .filter(|s| !s.is_empty())
                // intern instrings in arena
                .map(|s| ArenaString::from_str(arena, s));

            locales.extend(langs);

            let Some((lang, _)) = langs_str.split_once('-') else {
                continue;
            };

            locales.push(ArenaString::from_str(arena, lang));

            if !locales.is_empty() {
                break;
            }
        }
    }

    dbg!(&locales);

    locales
}

Comment thread src/sys/unix.rs Outdated
@eatradish
eatradish force-pushed the fix-language-match branch 2 times, most recently from 7dc8f4b to 81e7253 Compare May 20, 2025 15:26
In Linux, it is possible for the `LANGUAGE` variable to exist but have a empty value
@eatradish
eatradish force-pushed the fix-language-match branch from 81e7253 to 33f12a6 Compare May 20, 2025 15:41
@lhecker

lhecker commented May 20, 2025

Copy link
Copy Markdown
Member

FYI we squash all our PRs (to keep our history clean). You can absolutely feel free to just keep pushing commits into this PR. 🙂

@eatradish

Copy link
Copy Markdown
Contributor Author

FYI we squash all our PRs (to keep our history clean). You can absolutely feel free to just keep pushing commits into this PR. 🙂

hmm... Mostly it seems like all the problems I want fixed should be fixed in #85

@lhecker lhecker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this! I'll try to merge in the other PR as soon as I can.

@lhecker
lhecker merged commit b4553d4 into microsoft:main May 21, 2025
@lhecker

lhecker commented May 21, 2025

Copy link
Copy Markdown
Member

...I now realize why you replaced _ with -. I apologize for that. 🥴
I'll fix this for #85.

Kyza pushed a commit to Kyza/edit that referenced this pull request May 22, 2025
In Linux, it is possible for the `LANGUAGE`
variable to exist but have an empty value.
@eatradish
eatradish deleted the fix-language-match branch May 22, 2025 05:14
diabloproject pushed a commit to diabloproject/edit that referenced this pull request May 29, 2025
In Linux, it is possible for the `LANGUAGE`
variable to exist but have an empty value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants