Add wxUILocale class usable under macOS #2464

vadz · 2021-08-14T18:21:03Z

There are a lot of changes here, but the end result is relatively simple: we now have wxUILocale class which can be used just by calling its UseDefault() static function to indicate that the application wants to use the user locale for the UI. This function comes with a warning in the documentation about changing C locale in wxGTK -- and not changing it anywhere else.

Additionally, various classes using numbers/dates formatting have been modified to use wxUILocale instead of wxLocale, ensuring that they use correct formats under macOS.

If you don't have time to look at everything, please check 819ca01 (Add wxUILocale with minimal functionality, 2021-08-13) as this is the most important commit here.

cc @csomor, @vslavik

This will allow using this class in the library code too. No real changes yet, this is a pure refactoring. This commit is best viewed using git --color-moved option.

Make GetInfo() return information for numbers by default in the Unix version too instead of asserting, as this is more consistent with the MSW and Mac versions and also seems more useful.

This provides a more convenient way of testing wxLANGUAGE_DEFAULT than selecting it from the dialog shown on startup. This commit is best viewed ignoring whitespace-only changes.

Show that file dialogs, spin and calendar controls also respect the selected locale conventions.

This shows that C and UI (or OS) locales can be different.

This is slightly more verbose, but more clear and allows to separate the translations-related part from locale-changing one.

Creating the controls directly on wxFrame is not recommended and looks ugly under MSW due to the wrong background being used, so add an intermediate panel.

Show that we can use the translations to the user language even without changing the locale.

Restrict the choice to using the current system locale or not using it, as setting any other language doesn't, and can't, work under macOS.

It makes more sense to skip loading the translations too, if the user decided not to use the current locale. This commit is best viewed ignoring whitespace-only changes.

Getting this dialog all the time is annoying when testing, so add another command line option to set the locale unconditionally. This commit is best viewed ignoring whitespace-only changes.

No real changes, just make it possible to use these constants without including the entire wx/intl.h. Note that the existing headers including wx/intl.h need to keep including it, unfortunately, as not doing it any longer would break compilation of any code including them which relies on _() or wxGetTranslation() being defined after including e.g. wx/dc.h. This was, of course, never guaranteed, but in practice it worked for a very long time, so don't break it now. This commit is best viewed with --color-moved git option.

This is more convenient than having to open the "About" dialog to see the details of the locale being used. It also makes it unnecessary to store wxLocale reference in the frame class, simplifying the sample code.

csomor · 2021-08-15T09:40:11Z

thanks for tackling this, I have to add the files to the xcode files, but I'll do that after the merge, because this PR doesn't have the PCRE2 files in it either

csomor

I didn't have the time yet, to look at all the changes in detail.

I'd still prefer not having the locale impl hard wired to current in the formatters, but I understand, if this is just too much for the moment.

csomor · 2021-08-15T09:44:22Z

include/wx/private/uilocale.h

+    // Under MSW and Mac it is the same as CreateStdC(), but it's different
+    // under Unix.


I don't understand this. Under macOS I get the C locale when calling CreateStdC and the correct locale when calling CreateUserDefault ...

Sorry, this comment is outdated, it was true for the original version of the code, but I've changed it since then to remain compatible with the current behaviour.

csomor · 2021-08-15T09:46:47Z

build/files

@@ -139,6 +141,7 @@ BASE_COREFOUNDATION_SRC =
    src/osx/core/evtloop_cf.cpp
    src/osx/core/secretstore.cpp
    src/osx/core/strconv_cf.cpp
+    src/osx/core/uilocale.cpp


I'll add this to Xcode proj after merge

Great, TIA!

csomor · 2021-08-15T09:58:01Z

samples/internat/internat.cpp

+                            wxNumberFormatter::ToString(5.67, 2),
+                            5.67


How about using a number > 1000 that also shows the thousands separator difference ?

Not sure if it actually works correctly right now, there was #1781 which was supposed to improve/fix it, but it's probably out of date by now... But yes, we should still show this, thanks for the idea.

vadz · 2021-08-15T13:00:11Z

I didn't have the time yet, to look at all the changes in detail.

I'd just like to make sure that we all agree with this approach, i.e.

Use wxUILocale for formatting everything shown in the UI.
Provide UseDefault() to opt-in using the user locale.
Keep the current wxLocale behaviour for compatibility.

I'd still prefer not having the locale impl hard wired to current in the formatters, but I understand, if this is just too much for the moment.

We can always add an optional wxUILocale argument to wxNumberFormatter functions later, it should be pretty simple to do. But this would only be useful once we allow creating wxUILocale from name and this is quite some work and I'd rather do it after we drop XP support after 3.2 because locale names are "only" supported in Vista+.

For now this is just the barest minimum to allow writing applications using the user locale on all platforms (without breaking the display under macOS 11).

csomor · 2021-08-15T14:49:18Z

I'd just like to make sure that we all agree with this approach, i.e.

Use wxUILocale for formatting everything shown in the UI.

+1

Provide UseDefault() to opt-in using the user locale.

before this change, IIRC it was not necessary on macOS, wxNumFormatter used the current locale from the start

Keep the current wxLocale behaviour for compatibility.

so over time we should try to get rid of all wxLocale methods we use internally ?

I'd still prefer not having the locale impl hard wired to current in the formatters, but I understand, if this is just too much for the moment.

We can always add an optional wxUILocale argument to wxNumberFormatter functions later, it should be pretty simple to do. But this would only be useful once we allow creating wxUILocale from name and this is quite some work and I'd rather do it after we drop XP support after 3.2 because locale names are "only" supported in Vista+.

I think we actually do have something already not bad, we have UseLanguage(const wxLanguageInfo& info) which in turn alls the CreateForLanguage. We could expose this for creating a wxUILocale from wxLanguageInfo and allow copying wxUILocale.

vadz · 2021-08-15T15:15:32Z

Provide UseDefault() to opt-in using the user locale.

before this change, IIRC it was not necessary on macOS, wxNumFormatter used the current locale from the start

However wxNumberFormatter wasn't used anywhere in the library, only wxString::{To,From}Double() were and they were affected by the current C locale, i.e. you had to use wxLocale to use proper format (which wasn't that proper neither for e.g. de_CH). But I guess we do break compatibility for people who used wxNumberFormatter directly... Not sure what to do about them though, we can't make it work differently from the rest of the UI any more now that the UI uses it, and we can't change C locale by default because this would break the existing code. I guess I'll just have to document this in the release notes.

Keep the current wxLocale behaviour for compatibility.

so over time we should try to get rid of all wxLocale methods we use internally ?

There shouldn't be any important places where wxLocale is used internally left. It's not deprecated because applications not targeting macOS can still use it and it does something that wxUILocale doesn't, and won't, namely the ability to use something other than the default user locale. But wx itself ideally shouldn't use it.

I think we actually do have something already not bad, we have UseLanguage(const wxLanguageInfo& info) which in turn alls the CreateForLanguage. We could expose this for creating a wxUILocale from wxLanguageInfo and allow copying wxUILocale.

I don't want to make this function public as we want to use names, and not wxLanguage int IDs.

vslavik · 2021-08-15T15:26:10Z

I'd just like to make sure that we all agree with this approach, i.e.

I do (not much of a surprise here), but would like to review the implementation and see if porting Poedit to wxUILocale triggers some unforeseen issues.

csomor · 2021-08-15T15:29:45Z

Provide UseDefault() to opt-in using the user locale.

before this change, IIRC it was not necessary on macOS, wxNumFormatter used the current locale from the start

However wxNumberFormatter wasn't used anywhere in the library, only wxString::{To,From}Double() were and they were affected by the current C locale, i.e. you had to use wxLocale to use proper format (which wasn't that proper neither for e.g. de_CH). But I guess we do break compatibility for people who used wxNumberFormatter directly... Not sure what to do about them though, we can't make it work differently from the rest of the UI any more now that the UI uses it, and we can't change C locale by default because this would break the existing code. I guess I'll just have to document this in the release notes.

Yes, I guess this should be sufficient

Keep the current wxLocale behaviour for compatibility.

so over time we should try to get rid of all wxLocale methods we use internally ?

There shouldn't be any important places where wxLocale is used internally left. It's not deprecated because applications not targeting macOS can still use it and it does something that wxUILocale doesn't, and won't, namely the ability to use something other than the default user locale. But wx itself ideally shouldn't use it.

ok, thanks

I think we actually do have something already not bad, we have UseLanguage(const wxLanguageInfo& info) which in turn alls the CreateForLanguage. We could expose this for creating a wxUILocale from wxLanguageInfo and allow copying wxUILocale.

I don't want to make this function public as we want to use names, and not wxLanguage int IDs.

and GetLcidFromRfc1766 as a replacement on XP is not enough ?

vadz · 2021-08-15T15:56:13Z

I do (not much of a surprise here), but would like to review the implementation and see if porting Poedit to wxUILocale triggers some unforeseen issues.

This would be definitely great, TIA!

and GetLcidFromRfc1766 as a replacement on XP is not enough ?

The issue is that all the functions using names don't exist under XP and so need to be dynamically loaded. This is definitely doable, but it's a lot of boilerplate and I'd rather avoid it.

discnl

Thanks! Agreed with the approach and intentions (such as creating with a different locale later). I tried with the improved internat sample on macOS 11.5 and it's correctly asking to use the system language (and has no more menu rendering issue, with this PR). Also followed at least the commit messages and some diffs. Particularly relevant parts of earlier suggested commit to check, which currently is cb9b46b.

include/wx/unix/private/uilocale.h

Use it in the sample to show what (little) it can do right now.

This code, added back in 420de41 (workaround for buggy setlocale() under AIX (without this wxLocale didn't work at all), 2005-09-18), is probably irrelevant anyhow because AIX 5.2 must not be used any more, but also seems to have lost its purpose at some time during the intervening years, as we don't use "retloc" as argument to setlocale() anyhow, so it is doubly safe to simply remove it.

Using setlocale() is not the right way to do it there, try checking if the locale is supported by Core Foundation instead of whether it's supported by the Unix layer which is neglected and not used under Mac.

No real changes, just allow wxUILocale code to use this struct without including the full wx/intl.h. This commit is best viewed with --color-moved git option.

vadz · 2021-08-20T20:07:00Z

@discnl I wanted to push a (rebased) fix for the issue in wx/unix/private/uilocale.h but realized this could be confusing to you if you're reviewing and/or testing this right now, so I won't do it for the moment, please let me know when you will have finished, so that I could push. TIA!

discnl · 2021-08-20T20:40:17Z

@discnl I wanted to push a (rebased) fix for the issue in wx/unix/private/uilocale.h but realized this could be confusing to you if you're reviewing and/or testing this right now, so I won't do it for the moment, please let me know when you will have finished, so that I could push. TIA!

Thanks, go ahead. I am using the state of the branch to look at Xcode builds (including regenerating them, @csomor). Which still should be fine after your coming Mac change I think (in build/files there are no files from private dirs to worry about).

Most of the code in this function was Unix-specific and didn't make sense for Mac, where we never have to apply the workarounds in it nor deal with languages specified without the territory etc, so separate Mac branch using CoreFouundation API from the rest of the Unix code to make things more understandable. No real changes.

This is tidier than using #ifdefs in the same common file and also ensures that initializing wxLocale affects wxUILocale too, which will be important for compatibility when the code elsewhere is modified to use wxUILocale::GetInfo() instead of wxLocale::GetInfo() in the upcoming commits. This commit is best viewed with --color-moved git option.

Always use the UI locale conventions for floating point numbers in wxNumberFormatter, making it different from wxString::{To,From}Double() functions that use C locale, which may, or not, correspond to the UI locale.

This ensures that we use the decimal separator corresponding to the UI locale of the application, rather than C locale, so that e.g. comma is used under macOS even if setlocale() is not used.

The existing ToString() is not flexible enough to be used in wxGrid, which supports specifying the width (and not just the precision) as well as using formats other than "%g" and "%f" which are the only ones supported by the existing function. Note that currently the implementation simply calls wxString::Format() and then adjusts the decimal separator, but it could, in principle, use wxUILocale methods for formatting the floating point numbers using native platform functions for doing this, e.g. CFNumberFormatter under macOS.

This ensures that the correct, i.e. matching the users default, decimal point separator is used even if setlocale() is not called. Also simplify the code a bit by removing checks for wxUSE_INTL already present in wxNumberFormatter itself.

Retrieve thr date format to use, even when wxUSE_INTL==0.

This is similar to the previous commit and ensures that the correct date format is used even if setlocale() is not called.

Show that the numbers and dates in wxGrid use the expected formats.

Use the appropriate format for the dates by using the UI, rather than C, locale, similarly to wxGrid.

Use a number > 1000 to show the difference between the locales.

vadz added 13 commits August 7, 2021 18:04

Extract wxLocaleSetter from the tests into a private header

a4940bf

This will allow using this class in the library code too. No real changes yet, this is a pure refactoring. This commit is best viewed using git --color-moved option.

Allow using wxLOCALE_CAT_DEFAULT for numeric or money locale info

33ae58e

Make GetInfo() return information for numbers by default in the Unix version too instead of asserting, as this is more consistent with the MSW and Mac versions and also seems more useful.

Interpret empty language as "default" in the internat sample

93190d6

This provides a more convenient way of testing wxLANGUAGE_DEFAULT than selecting it from the dialog shown on startup. This commit is best viewed ignoring whitespace-only changes.

Show more locale-dependent controls in the internat sample

1f74e7c

Show that file dialogs, spin and calendar controls also respect the selected locale conventions.

Show using wxNumberFormatter in the internat sample too

7dc6462

This shows that C and UI (or OS) locales can be different.

Use wxTranslations directly instead of passing by wxLocale

4db5de9

This is slightly more verbose, but more clear and allows to separate the translations-related part from locale-changing one.

Use a panel in internat sample to make it look better under MSW

e0cd2c6

Creating the controls directly on wxFrame is not recommended and looks ugly under MSW due to the wrong background being used, so add an intermediate panel.

Add an option to skip changing locale in the internat sample

4adb58b

Show that we can use the translations to the user language even without changing the locale.

Stop allowing to select the language in the internat sample

eb5bffd

Restrict the choice to using the current system locale or not using it, as setting any other language doesn't, and can't, work under macOS.

Don't load translations when setting the locale is disabled too

31677e5

It makes more sense to skip loading the translations too, if the user decided not to use the current locale. This commit is best viewed ignoring whitespace-only changes.

Add internat sample command line option to skip the locale dialog

afe04f7

Getting this dialog all the time is annoying when testing, so add another command line option to set the locale unconditionally. This commit is best viewed ignoring whitespace-only changes.

Display locale information directly in the main sample frame

b12961f

This is more convenient than having to open the "About" dialog to see the details of the locale being used. It also makes it unnecessary to store wxLocale reference in the frame class, simplifying the sample code.

vadz added this to the 3.1.6 milestone Aug 14, 2021

vadz force-pushed the ui-locale branch 2 times, most recently from 1fe0752 to 3116c2a Compare August 14, 2021 23:58

csomor approved these changes Aug 15, 2021

View reviewed changes

discnl approved these changes Aug 17, 2021

View reviewed changes

discnl reviewed Aug 20, 2021

View reviewed changes

include/wx/unix/private/uilocale.h Outdated Show resolved Hide resolved

vadz added 4 commits August 20, 2021 22:02

Add wxUILocale with minimal functionality

b9cbe67

Use it in the sample to show what (little) it can do right now.

Use Core Foundation for testing locale availability under Mac

59a4505

Using setlocale() is not the right way to do it there, try checking if the locale is supported by Core Foundation instead of whether it's supported by the Unix layer which is neglected and not used under Mac.

Move wxLanguageInfo declaration to wx/localedefs.h too

405bb3e

No real changes, just allow wxUILocale code to use this struct without including the full wx/intl.h. This commit is best viewed with --color-moved git option.

vadz force-pushed the ui-locale branch from 3116c2a to 1669d9b Compare August 20, 2021 20:52

vadz added 11 commits August 20, 2021 23:37

Use wxUILocale in wxNumberFormatter

b3cab73

Always use the UI locale conventions for floating point numbers in wxNumberFormatter, making it different from wxString::{To,From}Double() functions that use C locale, which may, or not, correspond to the UI locale.

Use wxUILocale in wxSpinCtrlDouble

85d3799

This ensures that we use the decimal separator corresponding to the UI locale of the application, rather than C locale, so that e.g. comma is used under macOS even if setlocale() is not used.

Add wxGetUIDateFormat() helper function

8fa853a

Retrieve thr date format to use, even when wxUSE_INTL==0.

Use wxUILocale for formatting dates in wxGrid

d08fc75

This is similar to the previous commit and ensures that the correct date format is used even if setlocale() is not called.

Add a wxGrid with numbers and dates to the internat sample

8373b14

Show that the numbers and dates in wxGrid use the expected formats.

Use wxUILocale for date formatting in wxDataViewDateRenderer

c746953

Use the appropriate format for the dates by using the UI, rather than C, locale, similarly to wxGrid.

Demonstrate the thousands separator in internat sample too

9205209

Use a number > 1000 to show the difference between the locales.

vadz force-pushed the ui-locale branch from 1669d9b to 9205209 Compare August 20, 2021 21:37

discnl mentioned this pull request Aug 22, 2021

Update xcode builds #2484

Merged

vadz merged commit e307945 into wxWidgets:master Aug 26, 2021

vadz deleted the ui-locale branch August 26, 2021 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wxUILocale class usable under macOS #2464

Add wxUILocale class usable under macOS #2464

vadz commented Aug 14, 2021

csomor commented Aug 15, 2021

csomor left a comment

csomor Aug 15, 2021

vadz Aug 15, 2021

csomor Aug 15, 2021

vadz Aug 15, 2021

csomor Aug 15, 2021

vadz Aug 15, 2021

vadz commented Aug 15, 2021

csomor commented Aug 15, 2021

vadz commented Aug 15, 2021

vslavik commented Aug 15, 2021

csomor commented Aug 15, 2021

vadz commented Aug 15, 2021

discnl left a comment

vadz commented Aug 20, 2021

discnl commented Aug 20, 2021

		// Under MSW and Mac it is the same as CreateStdC(), but it's different
		// under Unix.

Add wxUILocale class usable under macOS #2464

Add wxUILocale class usable under macOS #2464

Conversation

vadz commented Aug 14, 2021

csomor commented Aug 15, 2021

csomor left a comment

Choose a reason for hiding this comment

csomor Aug 15, 2021

Choose a reason for hiding this comment

vadz Aug 15, 2021

Choose a reason for hiding this comment

csomor Aug 15, 2021

Choose a reason for hiding this comment

vadz Aug 15, 2021

Choose a reason for hiding this comment

csomor Aug 15, 2021

Choose a reason for hiding this comment

vadz Aug 15, 2021

Choose a reason for hiding this comment

vadz commented Aug 15, 2021

csomor commented Aug 15, 2021

vadz commented Aug 15, 2021

vslavik commented Aug 15, 2021

csomor commented Aug 15, 2021

vadz commented Aug 15, 2021

discnl left a comment

Choose a reason for hiding this comment

vadz commented Aug 20, 2021

discnl commented Aug 20, 2021