1Password / sys-locale

A small and lightweight Rust library to obtain the active locale on the system.
Apache License 2.0
75 stars 15 forks source link

Error detecting locale with an empty LC_ALL #28

Closed pasabanov closed 4 weeks ago

pasabanov commented 4 weeks ago

Current behavior

If LC_ALL is empty, the function sys_locale::get_locale() returns a locale from an empty string. This occurs because the _get function does not check whether LC_ALL is empty or not:

fn _get(env: &impl EnvAccess) -> Option<String> {
    let code = env
        .get(LC_ALL)
        .or_else(|| env.get(LC_CTYPE))
        .or_else(|| env.get(LANG))?;

    parse_locale_code(&code)
}

Expected behavior

I expect the get_locale function to continue evaluating the locale using other variables like LANG and LANGUAGE if LC_ALL is empty.

complexspaces commented 4 weeks ago

Hey there, thank you for the bug report. I've fixed the oversight in #29. Would you mind trying the latest commit on main in your application and confirm the issue is gone? If so, I can make a new patch release soon.

pasabanov commented 4 weeks ago

@complexspaces Thank you for the fix! I’ve tested the latest commit on the main branch and the issue is resolved.

Also, could you clarify why the LANGUAGE variable is not used as one of the fallback options?

pasabanov commented 3 weeks ago

Also, the LANGUAGE variable contains a list of locales in descending order of preference, so it would be reasonable to use it in the get_locales() function, which currently returns only one locale.

complexspaces commented 3 weeks ago

IIUC LANGUAGE seems to be something that gettext handles but not something glibc looks for when determining what the current locale is? I don't believe there's a strong reason to not support it, its just not something we seem to have run into yet.

complexspaces commented 3 weeks ago

Also, the LANGUAGE variable contains a list of locales in descending order of preference

Do you happen to know if any desktop environments support setting this variable for spawned apps? When I last looked KDE seemed to be the only desktop environment that supported multiple, ordered locales but they were just in a config file.

pasabanov commented 3 weeks ago

I’m not sure if any desktop environments set the LANGUAGE variable for spawned applications. However, I found some relevant sources that might help clarify the role of the LANGUAGE variable:

  1. Debian mailing list discussion where it is mentioned that LANGUAGE primarily affects the locale for message translation and takes precedence over other variables, even LC_ALL.
  2. Baeldung article on Linux locale environment variables, which also states that LANGUAGE has priority over other locale-related variables. But it might not be entirely accurate, as this should only be true in the context of message localization, which is what the article discusses.

Ideally, it would be useful to implement separate functions for determining the general locale and the message locale (and, ideally, for each specific LC_*** category). However, if we want to keep the locale determination process simple, one approach could be to give LANGUAGE priority over LC_ALL when determining the overall locale, because the locale is mainly used for message translation.

Additionally, if we aim to provide the user with a list of locales in descending order of preference (with the get_locales() function), LANGUAGE is well-suited for this purpose, as it literally stores such a list. If we want to go further, we could append the locale from LC_ALL, followed by those from LANG, to the list from LANGUAGE. We might consider excluding LC_CTYPE since it is more focused on specifying character encoding rather than language.

pasabanov commented 3 weeks ago

Also, as in electron/shell/common/language_util_linux.cc and in chromium/src/l10n/l10n_util.cc (links from this issue), I think we should use the LC_MESSAGES variable in locale evaluation: the precedence order: LANGUAGE, LC_ALL, LC_MESSAGES and LANG.