Make macOS date picker control locale aware #23956

utelle · 2023-10-11T12:47:09Z

Add global variable for pointer to current NSLocale
Set global NSLocale pointer in method wxUILocaleImplCF::Use()
Add helper function to retrieve the global NSLocale pointer
Add function declaration of the helper function in private/uilocale.h
Call wxUILocaleImpl::Use() in method wxUILocale::GetCurrent(), so that the current wxUILocale instance is always valid, even if wxUILocale::UseDefault() was not explicitly called

- Add global variable for pointer to current NSLocale - Set global NSLocale pointer in method wxUILocaleImplCF::Use() - Add helper function to retrieve the global NSLocale pointer - Add function declaration of the helper function in private/uilocale.h - Call wxUILocaleImpl::Use() in method wxUILocale::GetCurrent(), so that the current wxUILocale instance is always valid, even if wxUILocale::UseDefault() was not explicitly called

vadz

Thanks for your work on this, but I think it could be improved a bit.

Please let me know if I'm missing anything here.

vadz · 2023-10-13T00:19:09Z

src/common/uilocale.cpp

@@ -520,7 +520,14 @@ const wxUILocale& wxUILocale::GetCurrent()
    // We initialize it on demand.
    if ( !ms_current.m_impl )
    {
-        ms_current = wxUILocale(wxUILocaleImpl::CreateStdC());
+        // Instantiate standard C locale (should never fail) ...


I'm not 100% sure that calling Use() on C locale is a good idea. At least under Unix it's not a NOP.

Why not set g_nsloc in Mac-specific CreateStdC() instead?

I'm not 100% sure that calling Use() on C locale is a good idea. At least under Unix it's not a NOP.

Yes, wxUILocale::Use() is not a NOP under Unix. It sets the C runtime locale via setlocale. But does that matter in our case? IMHO no, because we would call wxUILocale::Use() only, if wxUILocale::Use() was not called. In that case we can assume that the current C runtime locale is the "C" locale anyway. Setting it in wxUILocale::Use() wouldn't cause any harm, would it?

Why not set g_nsloc in Mac-specific CreateStdC() instead?

Unfortunately, this is not possible. CreateStdC() can be called from various places, not only from Use() or GetCurrent(), but also from the wxUILocale() constructor. Only in Use() you can be sure that the locale is the current locale.

You're probably right, but the trouble is that it's not immediately obvious and for me it's a bad sign when you need to carefully think about something instead of it just being immediately clear.

And it just seems suspicious to call Use() from an accessor. In fact I am still not sure to understand why do we need to remember this locale as the default one here, i.e. why does querying the default locale should change anything?

You're probably right, but the trouble is that it's not immediately obvious and for me it's a bad sign when you need to carefully think about something instead of it just being immediately clear.

I fully agree. And you asked for a more detailed explanation in the general comments. However, I hope I could make my point in my answer there.

And it just seems suspicious to call Use() from an accessor. In fact I am still not sure to understand why do we need to remember this locale as the default one here, i.e. why does querying the default locale should change anything?

Well, actually it is the same as with any method that instantiates a static global object when called for the first time: it checks whether the global object already exists and is initialized; if yes, a reference to the global object is returned; if no, the global object will allocated and initialized, before a reference to it will be returned.

Here we have the situation that on the first call to GetCurrent() the global object ms_current is not initialized, because UseDefault() was not called. One option would have been to call UseDefault() from GetCurrent(), because it was not done before by the application. However, that would force the application to use the default user locale, but it is unlikely that that is the intention of the application, when it did not call UseDefault(). More likely is that the application wants to stay with the standard C locale. Therefore we would need a method UseStdC() to make clear that we want to use the standard C locale. But we don't have it. Therefore GetCurrent() calls CreateStdC() ... and misses to call Use() for it (which would be part of UseStdC(). That is adding the call to Use() actually fixes a bug.

include/wx/private/uilocale.h

src/osx/core/uilocale.mm

- Rename global pointer to current NSLocale - Declare function to retrieve the pointer in private OSX header file

vadz

Thanks for the updates and sorry for being so cautious, but I'm still not sure if calling Use() from GetCurrent() is a good idea. Could you please re-explain why should we do it from there?

vadz · 2023-10-13T21:46:28Z

include/wx/osx/private/uilocale.h

+// Name:        wx/osx/private/uilocale.h
+// Purpose:     Helper for making pointer to current NSLocale available
+//              for locale-dependent controls under macOS.
+// Author:      Vadim Zeitlin


Not really :-)

I just copied the comments from another file, adjusting the purpose but not paying attention to the other items.

Will you change it or should I, when merging?

I changed the author name.

vadz · 2023-10-13T21:47:05Z

include/wx/osx/private/uilocale.h

+
+#if wxUSE_INTL
+
+#ifdef __WXOSX_COCOA__


This is not needed here, is it?

No, probably not. At least if that header is not included from some non-Cocoa build. I don't know whether the latter can happen or not.

I'd still remove it.

vadz · 2023-10-13T21:51:56Z

src/common/uilocale.cpp

@@ -520,7 +520,14 @@ const wxUILocale& wxUILocale::GetCurrent()
    // We initialize it on demand.
    if ( !ms_current.m_impl )
    {
-        ms_current = wxUILocale(wxUILocaleImpl::CreateStdC());
+        // Instantiate standard C locale (should never fail) ...


You're probably right, but the trouble is that it's not immediately obvious and for me it's a bad sign when you need to carefully think about something instead of it just being immediately clear.

And it just seems suspicious to call Use() from an accessor. In fact I am still not sure to understand why do we need to remember this locale as the default one here, i.e. why does querying the default locale should change anything?

utelle · 2023-10-14T07:11:51Z

Thanks for the updates and sorry for being so cautious, but I'm still not sure if calling Use() from GetCurrent() is a good idea. Could you please re-explain why should we do it from there?

Sure.

We have to differentiate between creating and using a wxUILocale. You can create many, but you only can use one at a time. Even if it may not be a common use case to create many wxUILocale instances, it is perfectly allowed. To make use of a wxUILocale we have 2 methods: wxUILocale::UseDefault() and wxUILocale::UseLocaleName(). An application should typically call one of these methods exactly once.

Both use methods create a wxUILocaleImpl instance first, call then its wxUILocaleImpl::Use() method, and finally assign it to the global wxUILocale instance ms_current. This wxUILocaleImpl::Use() method is called from nowhere else, and that is the reason, why this method is the only place where we can be sure that this wxUILocaleImpl will be the current used locale. (And we need this fact to assign the current NSLocale* to the global variable in the MacOS implementation.)

Now, let us look at the implementation of wxUILocale::GetCurrent(). First of all this method is expected to always return a reference to a valid wxUILocale. Usually this method will simply return a reference to the global instance ms_current; and this will work, if one of the use methods was called.

However, we have to handle one special case, namely if the application did not call one of the use methods. In that case the global instance ms_current is not valid, but we need a valid instance.

The only assumption we can make is that the application uses (:bangbang:) the standard C locale. So, what should we do to reflect that? Yes, we should call another use method, namely wxUILocale::UseStdC(). Oops ... such a method does not exist... ok ... let's do it manually (as it is necessary to do it only here, and therefore probably not worth to add a method wxUILocale::UseStdC()): create a standard C locale and use it.

The current implementation of wxUILocale::GetCurrent() makes the first step (creating a standard C locale) by calling wxUILocaleImpl::CreateStdC(), but misses to actually use it. But remember: wxUILocale::GetCurrent() expects that the underlying locale is used (usually by calling wxUILocale::UseDefault()). Therefore I think it is a mistake to not call wxUILocaleImpl::Use().

To cut a long story short: calling wxUILocaleImpl::Use() for the standard C locale from wxUILocale::GetCurrent() is actually fixing a bug. The alternative would be to create a method wxUILocale::UseStdC() and call that from wxUILocale::GetCurrent() to make absolutely clear that a call to a use method was not done before.

Yes, calling wxUILocaleImpl::Use() is not a NOOP under Linux, but this is true for all use methods. They all call setlocale under the hood, but in this special case we just set the standard C locale again. And that should really do no harm at all.

vadz · 2023-10-14T20:57:24Z

Thanks for the explanations but I have to say that I very much disagree with the basic assumption underlying them, namely that calling GetCurrent() somehow implies that the application wants to use "C" locale because this is not the case at all, using this accessor doesn't mean that and I don't know why do you think it does? It's just a request to get information about the current locale whatever it is. And I still think that calling setlocale() from GetCurrent() is completely wrong as it modifies the state of the program which is not something that an accessor is supposed to do.

In fact, now I'm quite sure that doing this is wrong because you've allowed me to understand what exactly is wrong with it: calling GetCurrent() is absolutely not equivalent to what UseStdC() would do (if it existed).

vadz · 2023-10-14T21:04:56Z

Oh, and sorry for not putting this in the previous reply, but for the same reason GetCurrent() must not change the global wxOSX NSLocale* pointer neither. In fact, the fix is really trivial: the new code in wxUILocale::GetCurrent() should just be removed, i.e. this PR shouldn't change this function at all.

utelle · 2023-10-14T22:27:35Z

Thanks for the explanations but I have to say that I very much disagree with the basic assumption underlying them, namely that calling GetCurrent() somehow implies that the application wants to use "C" locale

Well, obviouly I was not able to make myself clear, unfortunately. The wxWidgets documentation states

"Localized applications should call wxUILocale::UseDefault() on startup to explicitly indicate that they opt-in using the current UI locale, ..."

That is, for a localized application that calls wxUILocale::UseDefault() the method GetCurrent() will simply return a reference to the current locale.

It is only the special case, if wxUILocale::UseDefault() was not called before GetCurrent() gets called, we have to deal with. And that's what GetCurrent() did already, namely calling CreateStdC() and assigning it to the global instance ms_current.

IMHO it is a fair assumption in this situation that the current C locale is the standard C locale. Actually, that's why GetCurrent() assigns the standard C locale to ms_current, thus making it valid. And it did so already, before I opened the PR.

Actually, nobody knows which locale the application wants to use. It's up to the developer to call the right methods. For a localized application UseDefault() should be called; for a non-localized application most likely it is not called.

If UseDefault() is not called, the C runtime locale is typically the standard C locale. So, IMHO it is the right thing to use it as the UI locale (as is the case since wxUILocale was introduced.

The only change I introduced in my PR is that additionally Use() is called for the standard C locale (if it had to be created). If you don't do it, you will get a locale mix of standard C and user default under MacOS - as can be seen in the internat sample: the date field in the grid will be displayed according to the standard C locale, but if you start to edit the field it will be formatted according to the user default locale.

Under MacOS and under Windows the method Use() does absolutely nothing when called from the implementation of the standard C locale, except for setting the global NSLocale* in the MacOS build in my PR.

Under Linux the net effect is also zero, because each application starts with the standard C locale as the C runtime

because this is not the case at all, using this accessor doesn't mean that and I don't know why do you think it does? It's just a request to get information about the current locale whatever it is.

Sorry, we are talking about the special case that a wxUILocale was not created before GetCurrent() was called. And you have to handle that case in GetCurrent(). Otherwise the code calling GetCurrent() would have to deal with an invalid wxUILocale.

And I still think that calling setlocale() from GetCurrent() is completely wrong as it modifies the state of the program which is not something that an accessor is supposed to do.

As explained above this is simply not true. If the application did not instantiate wxUILocale, what else could GetCurrent() do instead of creating and using the standard C locale? Call Exit() and stop the application? Certainly not.

In fact, now I'm quite sure that doing this is wrong because you've allowed me to understand what exactly is wrong with it: calling GetCurrent() is absolutely not equivalent to what UseStdC() would do (if it existed).

Again: we are talking about a special case that GetCurrent() must handle. And not calling Use() on the created standard C locale object leads to an inconsistent user experience.

utelle · 2023-10-14T22:37:04Z

Oh, and sorry for not putting this in the previous reply, but for the same reason GetCurrent() must not change the global wxOSX NSLocale* pointer neither. In fact, the fix is really trivial: the new code in wxUILocale::GetCurrent() should just be removed, i.e. this PR shouldn't change this function at all.

GetCurrent() will normally change nothing at all, but will simply return the reference to ms_current. It is only the special case that ms_current was not set and is therefore invalid, that a standard C locale will created and assigned to ms_current. And only in this special case method Use() is called. Not calling Use() is an error, because you get an inconsistent user experience if the global NSLocale pointer is not set (as explained in my previous reply). And - also as explained - the net effect on the application state is zero.

vadz · 2023-10-14T22:47:09Z

Just to make it clear: I'm only speaking about the case in which UseDefault() wasn't called. It's perfectly fine to call GetCurrent() in this case and it's also fine to change ms_current then because it has no observable effects. It's however not fine at all to call setlocale() because this does change the program state.

Sorry, but I still don't see at all why do you think that GetCurrent() need to do what you write. It simply returns the current locale, nothing else. If no locale has been set, it returns "C" locale, which is the only logical thing to do (at least we agree here). But it shouldn't do anything else.

I guess (but I could be wrong here, so please feel free to ignore this paragraph in this case...) that you're looking at it from macOS-centric point of view, and macOS is weird because it doesn't use "C" locale by default. This weirdness should, however, be dealt with in wxOSX itself and doesn't change anything for the other platforms and shouldn't result in calling setlocale() under Linux.

Calling Use() on MacOS is necessary to reduce locale-related inconsistencies.

utelle · 2023-10-14T23:15:02Z

Just to make it clear: I'm only speaking about the case in which UseDefault() wasn't called.

Me too.

It's perfectly fine to call GetCurrent() in this case and it's also fine to change ms_current then because it has no observable effects.

Agreed.

It's however not fine at all to call setlocale() because this does change the program state.

My point is that the current C runtime locale in this situation is "C". And therefore calling setlocale to set it to "C" should not have any effect. But I'm not an expert for setlocale. So, maybe I'm wrong, and even if the locale is the same, setlocale should not be called.

Sorry, but I still don't see at all why do you think that GetCurrent() need to do what you write. It simply returns the current locale, nothing else. If no locale has been set, it returns "C" locale, which is the only logical thing to do (at least we agree here). But it shouldn't do anything else.

Regarding Windows and Linux I agree. But under MacOS calling Use() is required so that the locale-dependent code in wxWidgets uses the right NSLocale. We can't set the new global pointer from CreateStdC unfortunately.

I guess (but I could be wrong here, so please feel free to ignore this paragraph in this case...) that you're looking at it from macOS-centric point of view,

Correct.

and macOS is weird because it doesn't use "C" locale by default. This weirdness should, however, be dealt with in wxOSX itself and doesn't change anything for the other platforms and shouldn't result in calling setlocale() under Linux.

Ok. I hope you can accept the compromise I just committed. Namely calling Use() only on MacOS.

vadz · 2023-10-14T23:20:41Z

It's however not fine at all to call setlocale() because this does change the program state.

My point is that the current C runtime locale in this situation is "C". And therefore calling setlocale to set it to "C" should not have any effect. But I'm not an expert for setlocale. So, maybe I'm wrong, and even if the locale is the same, setlocale should not be called.

It shouldn't because the program might call setlocale() itself (possibly with some other locale). And possibly overriding the global locale without being asked for is definitely rude (pointedly not looking at GTK).

Ok. I hope you can accept the compromise I just committed. Namely calling Use() only on MacOS.

I could live with this, even though I'd prefer not to have any unnecessary platform checks in the common code, but I still don't understand why are you so convinced that it is necessary. If the program doesn't call wxUILocale::UseDefault(), it basically doesn't care about the locale. And it's not clear at all to me that showing the dialog in US English (which is this change accomplishes, AFAIU?) is better than showing it in the system default language (which is how I believe it behaves by default?).

So if it were up to me, I would still just delete this code. And if you really want to use C locale in the dialog by default, it looks like you could easily do it by just handling the case of gs_currentNSLocale == nullptr as meaning this, couldn't you?

utelle · 2023-10-14T23:55:49Z

Ok. I hope you can accept the compromise I just committed. Namely calling Use() only on MacOS.

I could live with this, even though I'd prefer not to have any unnecessary platform checks in the common code,

IMHO it is not unnecessary.

but I still don't understand why are you so convinced that it is necessary. If the program doesn't call wxUILocale::UseDefault(), it basically doesn't care about the locale.

My understanding is that we want consistent behaviour as far as it is possible. Since the Standard C locale is assumed, if no locale was explicitly set, it should be used throughout. That is, not mixing C locale and default user locale where possible.

And it's not clear at all to me that showing the dialog in US English (which is this change accomplishes, AFAIU?) is better than showing it in the system default language (which is how I believe it behaves by default?).

I speak of number and date formatting. I would find it irritating, if a date is formatted according to US English for display, but according to German for editing.

So if it were up to me, I would still just delete this code. And if you really want to use C locale in the dialog by default, it looks like you could easily do it by just handling the case of gs_currentNSLocale == nullptr as meaning this, couldn't you?

Yes, in principle this could be done, but I find it inconvenient, because you need then to instantiate each time a NSLocale for C, instead of simply reusing the NSLocale of the current wxUILocale.

If no locale was explicitly set (UseDefault not called), then the global pointer gs_currentNSLocale will be null. In this case assign the standard C locale to gs_currentNSLocale .

utelle · 2023-10-15T08:31:15Z

@vadz I hope you will be satisfied by the latest changes. I removed the MacOS-specific code from wxUILocale::GetCurrent(). The helper function wxGetCurrentNSLocale() now returns a standard C locale if Use() was not called for any locale.

vadz

Thanks for the update! IMO it's indeed much better now, as there is less code and it's perfectly clear what's going on, so I'd be glad to merge it, just please let me know what do you think of the small issues below.

Thanks again!

vadz · 2023-10-15T13:41:13Z

include/wx/osx/private/uilocale.h

+// Name:        wx/osx/private/uilocale.h
+// Purpose:     Helper for making pointer to current NSLocale available
+//              for locale-dependent controls under macOS.
+// Author:      Vadim Zeitlin


Will you change it or should I, when merging?

vadz · 2023-10-15T13:41:35Z

include/wx/osx/private/uilocale.h

+
+#if wxUSE_INTL
+
+#ifdef __WXOSX_COCOA__


I'd still remove it.

vadz · 2023-10-15T13:43:47Z

src/osx/core/uilocale.mm

+
+NSLocale* wxGetCurrentNSLocale()
+{
+    static NSLocale* stdCLocale([[NSLocale localeWithLocaleIdentifier:@"en_US_POSIX"] retain]);


This should be inside if, why even instantiate it if it's never going to be needed?

It's also going to leak currently, not sure if it's really important, but using wxCFRef<NSLocale> for it would avoid it.

I moved the static locale into the if condition. And I used wxCFRef. However, I was not sure whether I have to keep the retain keyword or not.

This avoids to allocate a standard C locale if not needed.

utelle mentioned this pull request Oct 11, 2023

Expose wxUILocale::UseLocaleName or provide an equivalent public function #23862

Open

vadz reviewed Oct 13, 2023

View reviewed changes

Apply changes discussed in PR wxWidgets#23956

22ef45b

- Rename global pointer to current NSLocale - Declare function to retrieve the pointer in private OSX header file

vadz reviewed Oct 13, 2023

View reviewed changes

Call Use() in GetCurrent() only on MacOS

4c1cb36

Calling Use() on MacOS is necessary to reduce locale-related inconsistencies.

utelle added 2 commits October 15, 2023 10:20

Assign standard C locale to global pointer

e3e288d

If no locale was explicitly set (UseDefault not called), then the global pointer gs_currentNSLocale will be null. In this case assign the standard C locale to gs_currentNSLocale .

Remove OSX specific code from GetCurrent

73c12d9

vadz reviewed Oct 15, 2023

View reviewed changes

utelle added 2 commits October 15, 2023 18:57

Remove Cocoa compilation guard

d95d8f5

Move static standard C locale into if condition

810f0b8

This avoids to allocate a standard C locale if not needed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make macOS date picker control locale aware #23956

Make macOS date picker control locale aware #23956

utelle commented Oct 11, 2023

vadz left a comment

vadz Oct 13, 2023

utelle Oct 13, 2023

vadz Oct 13, 2023

utelle Oct 14, 2023

vadz left a comment

vadz Oct 13, 2023

utelle Oct 14, 2023

vadz Oct 15, 2023

utelle Oct 15, 2023

vadz Oct 13, 2023

utelle Oct 14, 2023

vadz Oct 15, 2023

utelle Oct 15, 2023

vadz Oct 13, 2023

utelle commented Oct 14, 2023

vadz commented Oct 14, 2023

vadz commented Oct 14, 2023

utelle commented Oct 14, 2023

utelle commented Oct 14, 2023

vadz commented Oct 14, 2023

utelle commented Oct 14, 2023

vadz commented Oct 14, 2023

utelle commented Oct 14, 2023

utelle commented Oct 15, 2023

vadz left a comment

vadz Oct 15, 2023

vadz Oct 15, 2023

vadz Oct 15, 2023

utelle Oct 15, 2023


		#if wxUSE_INTL

		#ifdef __WXOSX_COCOA__


		#if wxUSE_INTL

		#ifdef __WXOSX_COCOA__

Make macOS date picker control locale aware #23956

Are you sure you want to change the base?

Make macOS date picker control locale aware #23956

Conversation

utelle commented Oct 11, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utelle commented Oct 14, 2023

vadz commented Oct 14, 2023

vadz commented Oct 14, 2023

utelle commented Oct 14, 2023

utelle commented Oct 14, 2023

vadz commented Oct 14, 2023

utelle commented Oct 14, 2023

vadz commented Oct 14, 2023

utelle commented Oct 14, 2023

utelle commented Oct 15, 2023

vadz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment