Fix issue #26274 (PR #26354)
Ulrich Telle
On Windows the default locale instantiated by wxUILocale::UseDefault() referenced the locale configured for regional formatting in the Windows settings. This could lead to unintended behaviour (like using the wrong layout direction), if the locale for the Windows display language differs from the locale for regional formatting.
This has been fixed by referencing the locale corresponding to the display language resp the regional formatting depending on the locale property in question.
Additionally, the German translation of the internat sample was updated.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/26354
Commit Summary
File Changes
(6 files)
- M include/wx/uilocale.h (6)
- M interface/wx/uilocale.h (7)
- M samples/internat/de/internat.mo (0)
- M samples/internat/de/internat.po (156)
- M src/common/uilocale.cpp (2)
- M src/msw/uilocale.cpp (102)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/26354.patch
- https://github.com/wxWidgets/wxWidgets/pull/26354.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- dfdb14f Fix trailing white space
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#26354)AFAICT the failing checks have nothing to do with my changes.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26354)Yes, we really need to stop using Ubuntu 18.04 for the CI jobs, its repositories are unusably flaky... I've restarted the failed CI jobs, let's hope they fare better this time.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz approved this pull request.
Thanks! I didn't test this yet but it looks like it should work fine, let's merge it (maybe after minor tweaks if you'd like to address the comments below) to see what we're missing...
>
static wxUILocale ms_current;
wxUILocaleImpl* m_impl;
+ bool m_isDefault;
Minor, but I'd prefer to do it like this:
⬇️ Suggested change- bool m_isDefault; + bool m_isDefault = false;
and remove the initialization above.
> @@ -854,6 +855,7 @@ bool wxUILocale::UseLocaleName(const wxString& localeName)
impl->Use();
ms_current = wxUILocale(impl);
+ ms_current.m_isDefault = false;
This is unnecessary?
> @@ -173,14 +171,23 @@ WXDLLIMPEXP_BASE wxString wxGetMSWDateTimeFormat(wxLocaleInfo index)
return wxString{};
}
+ // Determine the name of the locale to be used for querying the date/time format
+ // Use a nullptr in case of the system default locale
OK, this works because LOCALE_NAME_USER_DEFAULT is defined as NULL, but it would still be better/more clear to use LOCALE_NAME_USER_DEFAULT as name value.
> + wxString DoGetInfoDisplay(LCTYPE lctype) const
+ {
+ wchar_t buf[256];
+ if ( !::GetLocaleInfoEx(m_nameDisplay, lctype, buf, WXSIZEOF(buf)) )
+ {
+ wxLogLastError(wxT("GetLocaleInfoEx"));
+ return wxString();
+ }
+
+ return buf;
I'm not sure if it's really better to duplicate DoGetInfo() (which is pretty short, of course, but still) than to add an extra parameter for it. I.e. maybe it would be better to use DoGetInfo(m_nameDisplay, lctype) instead of DoGetInfoDisplay(lctype) etc?
> @@ -820,7 +853,8 @@ class wxUILocaleImplName : public wxUILocaleImpl
return wxLocaleNumberFormatting(groupSeparator, grouping, decimalSeparator, fractionalDigits);
}
- const wchar_t* const m_name;
+ const wchar_t* const m_nameRegion; // Name of locale for regional formatting
+ const wchar_t* m_nameDisplay; // Name of locale for display language
I think we need to explain in the comment here that it is usually equal to m_nameRegion and only different from it when using default locale.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- ab34290 Apply changes requested in review
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
>
static wxUILocale ms_current;
wxUILocaleImpl* m_impl;
+ bool m_isDefault;
Done.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> @@ -854,6 +855,7 @@ bool wxUILocale::UseLocaleName(const wxString& localeName)
impl->Use();
ms_current = wxUILocale(impl);
+ ms_current.m_isDefault = false;
Although wxUILocale::UseDefault() and wxUILocale:UseLocaleName() are meant to be called only once, it is not forbidden or prevented. And one older application of mine - still using wxLocale - allows to change the UI language (resp locale) on the fly. In such a case it is necessary to reset m_isDefault. So, yes, I think it is 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
@utelle commented on this pull request.
> @@ -173,14 +171,23 @@ WXDLLIMPEXP_BASE wxString wxGetMSWDateTimeFormat(wxLocaleInfo index)
return wxString{};
}
+ // Determine the name of the locale to be used for querying the date/time format
+ // Use a nullptr in case of the system default locale
Done.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> + wxString DoGetInfoDisplay(LCTYPE lctype) const
+ {
+ wchar_t buf[256];
+ if ( !::GetLocaleInfoEx(m_nameDisplay, lctype, buf, WXSIZEOF(buf)) )
+ {
+ wxLogLastError(wxT("GetLocaleInfoEx"));
+ return wxString();
+ }
+
+ return buf;
I applied the proposed change.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 9d7a9b8 Add comment for locale name member variables
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> @@ -820,7 +853,8 @@ class wxUILocaleImplName : public wxUILocaleImpl
return wxLocaleNumberFormatting(groupSeparator, grouping, decimalSeparator, fractionalDigits);
}
- const wchar_t* const m_name;
+ const wchar_t* const m_nameRegion; // Name of locale for regional formatting
+ const wchar_t* m_nameDisplay; // Name of locale for display language
Done.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- ce15b58 Fix trailing whitespace
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#26354)Great, thanks, I'll merge and push this soon.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.