Fix issue #22526 (PR #22538)

Ulrich Telle

unread,

Jun 16, 2022, 8:44:09 AM6/16/22

to wx-...@googlegroups.com, Subscribed

GetSystemLocale() creates internally a default locale instance that is then queried for its identifier. Under Unix this fails, because GetLocaleId() returns any empty wxLocaleIdent. With this change the method will determine the identifier correctly for the default locale.

You can view, comment on, or merge this pull request online at:

https://github.com/wxWidgets/wxWidgets/pull/22538

Commit Summary

1198900 Fix issue #22526

File Changes

(1 file)

M src/unix/uilocale.cpp (2)

Patch Links:

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 16, 2022, 9:35:17 AM6/16/22

to wx-...@googlegroups.com, Subscribed

Thanks, I had already done the same thing locally after your explanations in #22526, but this doesn't work as can be seen by simply adding

diff --git a/tests/intl/intltest.cpp b/tests/intl/intltest.cpp
index 50be7f0277..685ad5bb71 100644
--- a/tests/intl/intltest.cpp
+++ b/tests/intl/intltest.cpp
@@ -233,6 +233,8 @@ void IntlTestCase::IsAvailable()

 TEST_CASE("wxLocale::Default", "[locale]")
 {
+    CHECK( wxLocale::IsAvailable(wxLANGUAGE_DEFAULT) );
+
     wxLocale loc;

     REQUIRE( loc.Init(wxLANGUAGE_DEFAULT, wxLOCALE_DONT_LOAD_DEFAULT) );

to the test.

The problem is that looking up "C" in the language database is not going to work. This could be handled specially, of course, as is already done in Language(), but I'm not sure if it's the best thing to do...

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 16, 2022, 9:40:48 AM6/16/22

to wx-...@googlegroups.com, Subscribed

BTW, it's confusing that wxUILocaleImpl::GetPreferredUILanguages() returns "en-US" for unspecified locale and "en_US" if the default "C" locale is explicitly specified.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 16, 2022, 11:11:37 AM6/16/22

to wx-...@googlegroups.com, Subscribed

Thanks, I had already done the same thing locally after your explanations in #22526, but this doesn't work as can be seen by simply adding [...] to the test.

The problem is that looking up "C" in the language database is not going to work.

Well, I guess we have an even bigger problem here. In method wxUILocale::GetSystemLocale an instance of the system default locale should be created. However, under Unix this doesn't work, because we have no means to create it. The currently called method CreateUserDefault actually does nothing, and therefore method GetName which internally calls setlocale(LC_ALL, NULL); returns "C" instead of the "real" system default locale.

This could be handled specially, of course, as is already done in Language(), but I'm not sure if it's the best thing to do...

Since the system default locale could actually be "C", we will have to handle this case explicitly (as it was handled in prior wx versions).

I will try to find a working solution.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 16, 2022, 11:18:36 AM6/16/22

to wx-...@googlegroups.com, Subscribed

Thanks, I had already done the same thing locally after your explanations in #22526, but this doesn't work as can be seen by simply adding [...] to the test.
The problem is that looking up "C" in the language database is not going to work.

Well, I guess we have an even bigger problem here. In method wxUILocale::GetSystemLocale an instance of the system default locale should be created. However, under Unix this doesn't work, because we have no means to create it. The currently called method CreateUserDefault actually does nothing, and therefore method GetName which internally calls setlocale(LC_ALL, NULL); returns "C" instead of the "real" system default locale.

I didn't notice this because my real locale is "C" (I have LC_ALL=C.UTF-8), but this is indeed another problem. We need to either call setlocale(LC_ALL, "") and then restore the locale back or, probably better, examine LC_ALL, LC_MESSAGES and LANG ourselves as is already done in GetPreferredUILanguages().

I.e. while it would be wrong to conflate languages and locales, we do need to reuse the same code for both system language and system locale determination and we don't do this any more.

This could be handled specially, of course, as is already done in Language(), but I'm not sure if it's the best thing to do...

Since the system default locale could actually be "C", we will have to handle this case explicitly (as it was handled in prior wx versions).

I will try to find a working solution.

TIA!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kumazuma

unread,

Jun 16, 2022, 12:45:34 PM6/16/22

to wx-...@googlegroups.com, Subscribed

wxUILocale returns wrong codeset in Internat sample.

I expected UTF-8, but it returned 'ANSI_X3.4-1968' that korean never use.
I think it is needed some fix...

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 16, 2022, 5:50:39 PM6/16/22

to wx-...@googlegroups.com, Subscribed

wxUILocale returns wrong codeset in Internat sample.

I expected UTF-8, but it returned 'ANSI_X3.4-1968' that korean never use. I think it is needed some fix...

Your screenshot shows codeset ko_KR.UTF-8. So, where from do you get ANSI_X3.4-1968?

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kumazuma

unread,

Jun 16, 2022, 6:13:49 PM6/16/22

to wx-...@googlegroups.com, Subscribed

In internat sample, 'wxUILocale::GetCurrent().GetLocalizedName(wxLOCALE_NAME_LOCALE, wxLOCALE_FORM_NATIVE)' GetLocalizedName is decode the codeset, it was getten in wxUILocaleimlpUnix? initializer, the codeset was not UTF-8, so string was broken

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 17, 2022, 2:58:00 AM6/17/22

to wx-...@googlegroups.com, Subscribed

In internat sample, 'wxUILocale::GetCurrent().GetLocalizedName(wxLOCALE_NAME_LOCALE, wxLOCALE_FORM_NATIVE)' GetLocalizedName is decode the codeset, it was getten in wxUILocaleimlpUnix? initializer, the codeset was not UTF-8, so string was broken

Ah, I see. On initializing a locale instance the codeset is retrieved to avoid retrieving it every time a localized string is asked for. Maybe it can't be done in this way. I'll look into the issue.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kumazuma

unread,

Jun 17, 2022, 7:31:34 AM6/17/22

to wx-...@googlegroups.com, Subscribed

#0  (anonymous namespace)::wxUILocaleImplUnix::GetLangInfo(nl_item) const (this=0x555555662a30, item=14) at /home/user/wxWidgets/src/unix/uilocale.cpp:373
#1  0x00007ffff72ea028 in (anonymous namespace)::wxUILocaleImplUnix::wxUILocaleImplUnix(wxLocaleIdent, locale_t) (this=0x555555662a30, locId=..., loc=0x0) at /home/user/wxWidgets/src/unix/uilocale.cpp:295
#2  0x00007ffff72eaede in wxUILocaleImpl::CreateUserDefault() () at /home/user/wxWidgets/src/unix/uilocale.cpp:539
#3  0x00007ffff72a7817 in wxUILocale::UseDefault() () at /home/user/wxWidgets/src/common/uilocale.cpp:459
#4  0x0000555555576fa2 in MyApp::OnInit() (this=0x5555555fd510) at /home/user/wxWidgets/samples/internat/internat.cpp:231
#5  0x000055555557ffd1 in wxAppConsoleBase::CallOnInit() (this=0x5555555fd510) at /home/user/wxWidgets/include/wx/app.h:93
#6  0x00007ffff7223ae6 in wxEntry(int&, wchar_t**) (argc=@0x7ffff73aabe4: 1, argv=0x5555555fd2f0) at /home/user/wxWidgets/src/common/init.cpp:487
#7  0x00007ffff7223c02 in wxEntry(int&, char**) (argc=@0x7fffffffe05c: 1, argv=0x7fffffffe158) at /home/user/wxWidgets/src/common/init.cpp:515
#8  0x000055555557675b in main(int, char**) (argc=1, argv=0x7fffffffe158) at /home/user/wxWidgets/samples/internat/internat.cpp:150
#9  0x00007ffff6b16083 in __libc_start_main (main=0x555555576735 <main(int, char**)>, argc=1, argv=0x7fffffffe158, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe148) at ../csu/libc-start.c:308
#10 0x000055555557664e in _start ()

'nl_langinfo(item)' return ANSI_X3.4-1968 instead of UTF-8

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 17, 2022, 8:54:31 AM6/17/22

to wx-...@googlegroups.com, Subscribed

'nl_langinfo(item)' return ANSI_X3.4-1968 instead of UTF-8

Thanks for the additional info (including the locale settings).

The problem in the current code is that in case of the default system locale the codeset is queried before the locale is actually set (that happens later in method Use()). Since the locale member variable is NULL and the locale was not yet set, the default C locale will be queried (definitely a bug - thanks for reporting). That would explain the codeset value.

Most likely it will be sufficient to repeat querying the codeset in method Use() after the locale was set.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 17, 2022, 10:22:50 AM6/17/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

bfeac2c Determine codeset properly

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 17, 2022, 10:26:35 AM6/17/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

9861b09 Remove trailing whitespace

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 17, 2022, 10:48:58 AM6/17/22

to wx-...@googlegroups.com, Subscribed

@Kumazuma The commit bfeac2c should fix the issue reported by you. The codeset is now properly determined for the system default locale.

The constructor still calls GetLangInfo(CODESET), because this is the right thing to do in case of a specific locale that was already instantiated (since the internal method Use() is not called in that case).

However, this is not yet the complete fix for the main issue.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 17, 2022, 6:10:09 PM6/17/22

to wx-...@googlegroups.com, Subscribed

@vadz IMHO the problem boils down to the implementation of wxUILocaleImplUnix::GetName():

wxString wxUILocaleImplUnix::GetName() const
{
    wxString name = m_locId.GetName();
    if (name.empty())
    {
        char* rv = setlocale(LC_ALL, NULL);
        if (rv)
            name = wxString::FromUTF8(rv);
    }
    return name;
}

The locale name is determined by calling setlocale(LC_ALL, NULL) in case of the system default locale. However, this only works if the locale was actually set. In our use case the locale is not set.

At least the following 2 implementation variants come to mind to solve the problem:

Variant 1 using setlocale (needs to save and restore the current locale):

wxString wxUILocaleImplUnix::GetName() const
{
    wxString name = m_locId.GetName();
    if (name.empty())
    {
        // Save current locale
        char* locCurrent = setlocale(LC_ALL, NULL);
        char* locSaved = locCurrent ? strdup(locCurrent) : NULL;
        // Set system default locale and retrieve its name
        char* rv = setlocale(LC_ALL, "");
        if (rv)
            name = wxString::FromUTF8(rv);
        // Restore current locale
        if ( locSaved )
        {
            setlocale(LC_ALL, locSaved);
            free(locSaved);
        }
    }
    return name;
}

Variant 2 using newlocale / freelocale (does not tamper with the current locale):

wxString wxUILocaleImplUnix::GetName() const
{
    wxString name = m_locId.GetName();
    if (name.empty())
    {
#ifdef HAVE_LOCALE_T
        locale_t loc = newlocale(LC_ALL_MASK, "", NULL);
        if ( loc )
        {
            const char *s = nl_langinfo_l(_NL_LOCALE_NAME(LC_ALL));
            if (rv)
                name = wxString::FromUTF8(rv);
            freelocale(loc);
        }
#else
        // Not sure what to do here, if HAVE_LOCALE_T is false
        // Maybe variant 1?
#endif
    }
    return name;
}

Which variant would you prefer? Or do you have alternative ideas?

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 9:57:00 AM6/18/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

e70053f Change method wxUILocaleImplUnix::GetName

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 10:03:42 AM6/18/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

b1aa647 Handle C/POSIX locale in FindLanguageInfo methods

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 10:09:02 AM6/18/22

to wx-...@googlegroups.com, Subscribed

To avoid determining the name of the default user locale in method GetName in each invocation, I moved the respective code to the wxUILocaleImplUnix ctor (using variant 1) and stored the result in a member variable that is then referenced in method GetName.

BTW, it's confusing that wxUILocaleImpl::GetPreferredUILanguages() returns "en-US" for unspecified locale and "en_US" if the default "C" locale is explicitly specified.

Now always "en_US" is returned.

Additionally, the locale tags "C" and "POSIX" are now handled as equivalent to "en_US" in the FindLanguageInfo methods of wxUILocale.

Hopefully these modifications now solve the issue.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 18, 2022, 10:18:31 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle When I wrote

We need to either call setlocale(LC_ALL, "") and then restore the locale back or, probably better, examine LC_ALL, LC_MESSAGES and LANG ourselves as is already done in GetPreferredUILanguages().

my first alternative was exactly your "Variant 1", while the second one was "Variant 3" and cosnsited in parsing the (first non-empty) environment variable ourselves to extract the name from it, using the code we already have.

The advantage of this "Variant 3" is that we don't affect the current locale which might be undesirable, as it also affects the other threads (at least according to POSIX, I'm not sure if it's always the case in practice). The disadvantage, of course, is that we might misparse the value, especially if its format gets extended with some extra fields we don't know about in the future, while libc should always get it right, by definition (i.e. what libc does is the right thing to do).

Your "Variant 2" is almost perfect, but there is still the problem of the systems that don't have locale_t support. I think that by now it's pretty ubiquitous, but as long as we still want to support the systems without it (older BSDs don't have it, I think), we need to fall back on something in that case. I guess ideal would be to fall back on "Variant 3", as this should be pretty safe for the code only used on old systems, whose behaviour is not going to change (and so no extra fields are going to appear). But in a pinch falling back to "Variant 1" is better than nothing.

Thanks again for looking at this!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 18, 2022, 10:30:37 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@vadz commented on this pull request.

Thanks, I think we can merge this already and then improve it futher by using extended POSIX locale support to avoid changing the process locale if possible.

Please let me know what do you think and thanks again!

In src/common/uilocale.cpp:

> @@ -752,9 +752,14 @@ const wxLanguageInfo* wxUILocale::FindLanguageInfo(const wxString& locale)

     // to the entry description in the language database.

     // The locale string may have the form "language[_region][.codeset]".

     // We ignore the "codeset" part here.

+    wxString myLocale = locale;

This is a pretty minor issue, but I'd rather rename locale function argument to something like localeIn and then call its local copy locale in order to avoid gratuitous diffs below that just rename the variable.

In src/common/uilocale.cpp:

> @@ -752,9 +752,14 @@ const wxLanguageInfo* wxUILocale::FindLanguageInfo(const wxString& locale)

     // to the entry description in the language database.

     // The locale string may have the form "language[_region][.codeset]".

     // We ignore the "codeset" part here.

+    wxString myLocale = locale;

+    if (myLocale.IsSameAs("C", false) || myLocale.IsSameAs("POSIX", false))

Another minor comment, but I'd add a helper function to use it here and in the other place where we perform the same test to allow writing just

⬇️ Suggested change

-    if (myLocale.IsSameAs("C", false) || myLocale.IsSameAs("POSIX", false))

+    if (IsDefaultCLocale(myLocale))

In src/unix/uilocale.cpp:

> @@ -87,6 +87,7 @@ class wxUILocaleImplUnix : public wxUILocaleImpl

 

     wxLocaleIdent m_locId;

     wxString m_codeset;

+    wxString m_nameDefault;

Why isn't this just called m_name? There doesn't seem to be anything particular "default" about it, it's used for any locale, not just the default one.

In src/unix/uilocale.cpp:

> +

+#ifdef HAVE_LANGINFO_H

+    // In case of the system default locale the codeset can not be

+    // determined properly in the constructor, because the default

+    // locale is actually not set yet.

+    // Therefore we have to do it here (again).

+    m_codeset = GetLangInfo(CODESET);

+#endif

Sorry, this seems confusing to me: either we don't need it until Use() is called and then we shouldn't set it in ctor at all and only set it here or, if we do need it even before Use(), we should set it correctly in the ctor for the default locale too, by switching to it first (as we already do now).

But having wrong value of m_codeset only for the default locale and only until Use() is called just doesn't seem right.

P.S. See also wxLocale::GetSystemEncodingName().

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 11:23:21 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@vadz

my first alternative was exactly your "Variant 1", while the second one was "Variant 3" and consisted in parsing the (first non-empty) environment variable ourselves to extract the name from it, using the code we already have.

The advantage of this "Variant 3" is that we don't affect the current locale which might be undesirable, as it also affects the other threads (at least according to POSIX, I'm not sure if it's always the case in practice).

Actually, I'm not very happy with variant 1 for exact the reasons you name. Tampering with the locale should be avoided. To reduce chances of conflicts and side effects, I moved the code to the ctor, so that at least it gets executed only once.

The disadvantage, of course, is that we might misparse the value, especially if its format gets extended with some extra fields we don't know about in the future, while libc should always get it right, by definition (i.e. what libc does is the right thing to do).

Using setlocale(LC_ALL, "") we can be sure to get the locale that libc associates with the default user locale. Parsing the environment variables ourselves might not deliver the same result. Besides that we can't reuse the related code in GetPreferredUILanguages easily, because here we must not look at environment variable LANGUAGE AFAIK.

Your "Variant 2" is almost perfect, but there is still the problem of the systems that don't have locale_t support. I think that by now it's pretty ubiquitous, but as long as we still want to support the systems without it (older BSDs don't have it, I think), we need to fall back on something in that case. I guess ideal would be to fall back on "Variant 3", as this should be pretty safe for the code only used on old systems, whose behaviour is not going to change (and so no extra fields are going to appear). But in a pinch falling back to "Variant 1" is better than nothing.

IMHO we shouldn't make too much fuss about variant 1. A typical application will check for the default system locale in the initialization phase and will make its decisions based on the result. I find it very unlikely that an application will call that method every now and then while it is running.

Nevertheless, I can use my variant 2 if possible (that is, if locale_t support is available) and fall back to variant 1 otherwise.

I think we can merge this already and then improve it further by using extended POSIX locale support to avoid changing the process locale if possible.

Please let me know what do you think and thanks again!

Please wait with merging. I will adjust the code to use variant 2 and to fall back to variant 1 if locale_t support is missing. I'll try to do that today. I intend to address your other comments as well, where necessary.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 11:24:43 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle commented on this pull request.

In src/common/uilocale.cpp:

> @@ -752,9 +752,14 @@ const wxLanguageInfo* wxUILocale::FindLanguageInfo(const wxString& locale)
     // to the entry description in the language database.
     // The locale string may have the form "language[_region][.codeset]".
     // We ignore the "codeset" part here.
+    wxString myLocale = locale;

Ok, I'll do that.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Kumazuma

unread,

Jun 18, 2022, 11:29:19 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle the bug is fixed after commit bfeac2c

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 11:30:27 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle commented on this pull request.

In src/unix/uilocale.cpp:

> @@ -87,6 +87,7 @@ class wxUILocaleImplUnix : public wxUILocaleImpl
 
     wxLocaleIdent m_locId;
     wxString m_codeset;
+    wxString m_nameDefault;

Well, the member variable is only set and used for the default user locale. Otherwise it is left blank, and m_locId.GetName() will be used to retrieve the name of the locale. I can use m_name, of course, but m_nameDefaultLocale may be even better.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 18, 2022, 11:34:53 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@vadz commented on this pull request.

In src/unix/uilocale.cpp:

> @@ -87,6 +87,7 @@ class wxUILocaleImplUnix : public wxUILocaleImpl
 
     wxLocaleIdent m_locId;
     wxString m_codeset;
+    wxString m_nameDefault;

But the ctor does

m_nameDefault = m_locId.GetName();

unconditionally currently. It's true that later we only use it if m_locId.GetName().empty() but I don't really understand why don't we just always use it -- as long as we already have it anyhow.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 11:58:14 AM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle commented on this pull request.

In src/unix/uilocale.cpp:

> +
+#ifdef HAVE_LANGINFO_H
+    // In case of the system default locale the codeset can not be
+    // determined properly in the constructor, because the default
+    // locale is actually not set yet.
+    // Therefore we have to do it here (again).
+    m_codeset = GetLangInfo(CODESET);
+#endif

Sorry, this seems confusing to me: either we don't need it until Use() is called and then we shouldn't set it in ctor at all and only set it here or, if we do need it even before Use(), we should set it correctly in the ctor for the default locale too, by switching to it first (as we already do now).

Unfortunately, the situation is rather complex. Method GetLangInfo that is used to determine the codeset depends on locale_t support, it uses the current locale if locale_t support is not available. And the current locale will not be the right one in the ctor.

But having wrong value of m_codeset only for the default locale and only until Use() is called just doesn't seem right.

Hm, it's all about avoiding to tamper with the locale. If we want to determine the correct value in the ctor we need to set the locale temporarily and to reset to the previous locale, although we know that Use() will be called for the default locale anyway shortly afterwards.

Theoretically, the codeset and the name could be determined in method Use(). That would avoid most problems. BUT we have the special use case of method GetSystemLocale where we need to retrieve the locale name without calling Use().

P.S. See also wxLocale::GetSystemEncodingName().

Yes, that code follows variant 1.

I will look into the code again and will try to apply variant 2 - and to fall back to variant 1, if locale_t support is missing - for both items, locale name and codeset.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 12:03:25 PM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle commented on this pull request.

In src/unix/uilocale.cpp:

> @@ -87,6 +87,7 @@ class wxUILocaleImplUnix : public wxUILocaleImpl
 
     wxLocaleIdent m_locId;
     wxString m_codeset;
+    wxString m_nameDefault;

Yes, most likely we can change method GetName to always simply return m_name.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 18, 2022, 1:14:03 PM6/18/22

to wx-...@googlegroups.com, Subscribed

@vadz commented on this pull request.

In src/unix/uilocale.cpp:

> +
+#ifdef HAVE_LANGINFO_H
+    // In case of the system default locale the codeset can not be
+    // determined properly in the constructor, because the default
+    // locale is actually not set yet.
+    // Therefore we have to do it here (again).
+    m_codeset = GetLangInfo(CODESET);
+#endif

Ideal would be to have a (internal) class TempDefaultLocaleSetter that could be used to set the given LC_XXX to "" in its ctor and back to the original value in its dtor. Then we'd provide 2 implementations of it: one if extended locale support is available, and the other, using setlocale(), if it isn't.

Thanks again!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 4:39:35 PM6/18/22

to wx-...@googlegroups.com, Subscribed

@utelle commented on this pull request.

In src/unix/uilocale.cpp:

> +
+#ifdef HAVE_LANGINFO_H
+    // In case of the system default locale the codeset can not be
+    // determined properly in the constructor, because the default
+    // locale is actually not set yet.
+    // Therefore we have to do it here (again).
+    m_codeset = GetLangInfo(CODESET);
+#endif

Ideal would be to have a (internal) class TempDefaultLocaleSetter that could be used to set the given LC_XXX to "" in its ctor and back to the original value in its dtor. Then we'd provide 2 implementations of it: one if extended locale support is available, and the other, using setlocale(), if it isn't.

I saw your message just now, after already having modified my local code without using such an internal class. Before I decide to implement such a class (not sure whether it's really worth the effort), we have to further discuss the whole task,

We have 4 use cases:

wxUILocale::UseDefault()
Name and codeset can be determined in the ctor, either via locale_t support or via setlocale.
wxUILocale::UseLocaleName
For wxLocale compatibility. If locale_t support is available, a matching locale (with UTF-8 codeset, if possible) will be created and determining name and codeset can be done in the ctor. However, without locale_t support no locale is created and we would have to duplicate the code from the Use() method, where various charset variants (for UTF-8) are tested. And that's why I would prefer to determine the codeset in the Use() method, after the locale has been set.
wxUILocale::GetSystemLocale()
Can be handled in the same way as wxUILocale::UseDefault().
wxUILocale::wxUILocale(const wxLocaleIdent& localeId)
The current locale will not be changed. If locale_t support is available this case is similar as wxUILocale::UseLocaleName. However, if locale_t support is not available, calls to the GetInfo method will use the current locale. And that will result in wrong results, unless the wxUILocale instance references the same locale as the current locale.

All will work as long as locale_t support is available. But if it's not, then use cases 2 and 4 are problematic.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 4:52:11 PM6/18/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

5b4208d Determine locale name and codeset in ctor

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 5:02:04 PM6/18/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

adc7b31 Introduce helper function IsDefaultCLocale

—
View it on GitHub or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 18, 2022, 5:13:12 PM6/18/22

to wx-...@googlegroups.com, Subscribed

The values for locale name and codeset are now determined in the wxUILocaleImplUnix ctor. Additionally, I applied most other discussed modifications. Hopefully, I didn't introduce new bugs.

The implementation should now work properly, if locale_t support is available.

If locale_t support is not available, retrieving information via GetInfo for explicitly specified wxUILocale instances will most likely deliver surprising results, because the information will be based on the current active locale.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 18, 2022, 9:15:55 PM6/18/22

to wx-...@googlegroups.com, Push

@vadz pushed 7 commits.

aedd4ca Add a test for wxLocale::IsAvailable(wxLANGUAGE_DEFAULT)
2051098 Initialize wxUILocaleImplUnix name and codeset on demand
1dc1e7c Improve inexact comment describing wxUILocaleImplUnix::m_locale
37c7e2e Create helper TempDefautLocaleSetter class
f1402c2 Simplify wxUILocaleImplUnix::InitLocaleNameAndCodeset()
21945f9 Check that _NL_LOCALE_NAME is available before using it
e0319a9 Always use a valid locale_t in wxUILocaleImplUnix

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 18, 2022, 9:17:59 PM6/18/22

to wx-...@googlegroups.com, Subscribed

I've pushed a few changes that seem to simplify things significantly, hopefully without breaking anything -- please let me know what do you think. The commits here are supposed to be atomic and reviewing them one by one should be relatively simple.

I believe we could avoid using TempDefautLocaleSetter sometimes, i.e. after Use() had been called for the locale, but I didn't have to implement this optimization. In practice, all Linux systems do have extended locale support, so it shouldn't change anything for them, but we might still want to do this if any other common Unix systems still don't have it.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 19, 2022, 2:46:13 AM6/19/22

to wx-...@googlegroups.com, Subscribed

@utelle commented on this pull request.

I fear that using the TempDefaultLocaleSetter in method InitLocaleNameAndCodeset will not work as intended in all use cases.

We have to look at the 4 use cases, if locale_t support is not available:

Use default user locale
This case is handled correctly.
Use C locale
This will not work as expected, unless the C locale is the default user locale.
Use locale based on locale identifier
This will not work as expected, unless the locale identifier is that of the default user locale.
Instantiate locale for a specific locale identifier (without calling Use())
This will not work as expected, unless the specific locale identifier is that of the default user locale (which is highly unlikely).

Conclusion: As long as we have locale_t support all is fine. But if we don't have locale_t support, the TempDefaultLocaleSetter in its current form will not work in most use cases. Actually, it will be necessary to make TempDefaultLocaleSetter configurable. Using the default user locale ("") as the default is fine, but we need to be able to temporarily switch to "C" locale (use case 2) or the specific locale (use cases 3 and 4).

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 19, 2022, 3:07:52 AM6/19/22

to wx-...@googlegroups.com, Subscribed

I've pushed a few changes that seem to simplify things significantly, hopefully without breaking anything -- please let me know what do you think. The commits here are supposed to be atomic and reviewing them one by one should be relatively simple.

I just reviewed the code (without actually testing it). In principle, I'm fine with the changes. The idea to determine name and codeset on demand is definitely a good approach. Always instantiating a locale_t (if support for it is available) is also a good idea.

However, if _NL_LOCALE_NAME is not defined (Do you happen to know when that is the case?), m_locId.GetName() will return an empty string for the default user locale, resulting in calling InitLocaleNameAndCodeset again and again.

I believe we could avoid using TempDefautLocaleSetter sometimes, i.e. after Use() had been called for the locale, but I didn't have to implement this optimization. In practice, all Linux systems do have extended locale support, so it shouldn't change anything for them, but we might still want to do this if any other common Unix systems still don't have it.

As I explained in my previous post TempDefautLocaleSetter does not do what we want it to do in most use cases. In use cases 1, 2, and 3 we don't need it, because the correct locale was set by method Use(). In use case 4 we need to explicitly switch to the specified locale temporarily.

Unfortunately, a wxUILocale object does not know which use case it handles. Therefore temporarily switching the locale for all use cases will be a dumb, but working solution. For this, it remains to make TempDefautLocaleSetter configurable. Optimizations can be applied later, if there is really a demand.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 19, 2022, 6:45:15 AM6/19/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

e0e34fc Make TempDefaultLocaleSetter configurable

—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 19, 2022, 7:06:00 AM6/19/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

443c2f4 Configure TempDefautLocaleSetter

—
View it on GitHub or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 19, 2022, 2:00:40 PM6/19/22

to wx-...@googlegroups.com, Push

@utelle pushed 1 commit.

959b300 Fix CI build error Ubuntu 18.04 wxGTK 3 with clang

—
View it on GitHub or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 19, 2022, 4:25:50 PM6/19/22

to wx-...@googlegroups.com, Push

@vadz pushed 4 commits.

db588c5 Rename TempDefautLocaleSetter to TempLocaleSetter
fe88494 Remove unnecessary TempLocaleSetter::m_localeId
3a94ba6 Refactor: extract existing code into GetLocaleFromEnvironment()
a7c09c0 Get default locale name from environment as a fallback

—
View it on GitHub or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 19, 2022, 4:26:02 PM6/19/22

to wx-...@googlegroups.com, Subscribed

Thanks for review and the fixes! I've done a few more changes and I think we should merge this now if it looks ok to you.

However, if _NL_LOCALE_NAME is not defined (Do you happen to know when that is the case?),

I am almost sure that it's only available under Linux with glibc, i.e. not under *BSD or Solaris (does it still exist?). These systems have their own APIs for querying the locale name, see e.g. the code here but I don't really have time to test it now, so I've just added some hack to get the name from the environment which should hopefully be better than nothing...

m_locId.GetName() will return an empty string for the default user locale, resulting in calling InitLocaleNameAndCodeset again and again.

The changes mentioned above should fix this, at least.

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 19, 2022, 4:49:18 PM6/19/22

to wx-...@googlegroups.com, Subscribed

Thanks for review and the fixes! I've done a few more changes and I think we should merge this now if it looks ok to you.

Thanks! From my side all changes look good. In the meantime I tested the code under OpenSuSE Linux - all seems to work as expected.

However, if _NL_LOCALE_NAME is not defined (Do you happen to know when that is the case?),

I am almost sure that it's only available under Linux with glibc, i.e. not under *BSD or Solaris (does it still exist?).

Yes, Solaris still exists (as Oracle Solaris). However, I haven't used Solaris for ages.

These systems have their own APIs for querying the locale name, see e.g. the code here but I don't really have time to test it now, so I've just added some hack to get the name from the environment which should hopefully be better than nothing...

For now this is most likely good enough.

m_locId.GetName() will return an empty string for the default user locale, resulting in calling InitLocaleNameAndCodeset again and again.

The changes mentioned above should fix this, at least.

Great. Thanks!

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 19, 2022, 6:44:13 PM6/19/22

to wx-...@googlegroups.com, Subscribed

Merged #22538 into master.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Lauri Nurmi

unread,

Jun 20, 2022, 4:18:46 AM6/20/22

to wx-...@googlegroups.com, Subscribed

However, if _NL_LOCALE_NAME is not defined (Do you happen to know when that is the case?),

I am almost sure that it's only available under Linux with glibc, i.e. not under *BSD or Solaris (does it still exist?).

Yes, Solaris still exists (as Oracle Solaris). However, I haven't used Solaris for ages.

Solaris indeed exists, also as the free OpenIndiana.

That being said, the current master unfortunately fails to compile on Solaris because of wxUILocaleImplUnix::GetLocalizedName().

The reason is that all the _NL_* constants the function refers to are unknown symbols. The quick (but not necessarily the universally correct) fix is to enclose the function implementation with #if defined(HAVE_LANGINFO_H) && defined(HAVE_LOCALE_T) instead of just HAVE_LANGINFO_H.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 20, 2022, 8:20:02 AM6/20/22

to wx-...@googlegroups.com, Subscribed

@lanurmi

Solaris indeed exists, also as the free OpenIndiana.

That being said, the current master unfortunately fails to compile on Solaris because of wxUILocaleImplUnix::GetLocalizedName().

Just out of curiosity I set up a VM with OpenIndiana ... and I can confirm that compiling wx master fails.

The reason is that all the _NL_* constants the function refers to are unknown symbols. The quick (but not necessarily the universally correct) fix is to enclose the function implementation with #if defined(HAVE_LANGINFO_H) && defined(HAVE_LOCALE_T) instead of just HAVE_LANGINFO_H.

I would prefer to check explicitly whether the _NL_* constants are defined of not. Currently only method wxUILocaleImplUnix::GetLocalizedName() would be affected.

For me the big question is whether Solaris/OpenIndiana has other means to determine this locale information. Where could I find relevant documentation?

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 20, 2022, 10:33:01 AM6/20/22

to wx-...@googlegroups.com, Subscribed

Unfortunately the _NL_* symbols are not preprocessor symbols, but enum values. So, it is not possible to check via the preprocessor whether the values are defined or not.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 20, 2022, 1:55:36 PM6/20/22

to wx-...@googlegroups.com, Subscribed

Unfortunately the _NL_* symbols are not preprocessor symbols, but enum values. So, it is not possible to check via the preprocessor whether the values are defined or not.

We need configure (and CMake) tests for them... It's probably indicative of how many non-Linux Unix users we have that we haven't heard about the problem until now, but it would still be nice to avoid having it in 3.2.0.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 20, 2022, 6:37:10 PM6/20/22

to wx-...@googlegroups.com, Subscribed

We need configure (and CMake) tests for them...

That's something I'd like to leave to you (or someone else experienced in tweaking configure and CMake).

I will look into options to provide a fallback solution via our language database.

It's probably indicative of how many non-Linux Unix users we have that we haven't heard about the problem until now, but it would still be nice to avoid having it in 3.2.0.

Solaris/OpenIndiana seem to be pretty much alive with quite recent versions available. However, maybe these OSes are used primarily on servers, where GUI libraries like wx don't play such an important role.

I don't know whether it is possible to detect non-Linux Unix variants without having to check some preprocessor symbols for every single variant. Maybe checking for the symbol __linux__ would be sufficient to exclude "exotic" Unix systems. If so, that would be simpler to implement than explicitly checking for the availabilty of some enum values.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

VZ

unread,

Jun 20, 2022, 6:54:04 PM6/20/22

to wx-...@googlegroups.com, Subscribed

In principle, it's possible to use glibc under the other systems too, but I agree that it's probably rather unlikely, so maybe testing for __LINUX__ would indeed be good enough right now. It would be definitely better than just breaking the build there...

FWIW Solaris has its own API for getting the locale name called getlocalename_l(), but it's probably not worth supporting it.

—
Reply to this email directly, view it on GitHub, or unsubscribe.

You are receiving this because you are subscribed to this thread.

Ulrich Telle

unread,

Jun 21, 2022, 5:15:48 AM6/21/22

to wx-...@googlegroups.com, Subscribed

In principle, it's possible to use glibc under the other systems too, but I agree that it's probably rather unlikely, so maybe testing for __LINUX__ would indeed be good enough right now. It would be definitely better than just breaking the build there...

I just prepared PR #22558 to implement this approach.

FWIW Solaris has its own API for getting the locale name called getlocalename_l(), but it's probably not worth supporting it.

Unless there is a high demand to natively support this functionality on Solaris, I don't think it's worth the effort, because there are several other non-Linux-like systems out there not providing getlocalename_l(). Using setlocale for such systems should be good enough for now.