Fix weekday/month name localization in generic wxCalendarCtrl on macOS (PR #23551)

Lauri Nurmi

unread,

May 16, 2023, 8:52:17 AM5/16/23

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

wxDateTime::GetWeekDayName()/GetMonthName() relied on strftime() to acquire localized strings for weekday/month names, but since the C locale is no longer set by wxUILocale on macOS, and/or setting the locale the old way may fail, the names were returned in English.

Instead we can emulate the necessary functionality of strftime() by implementing it through NSDateFormatter and get properly localized names.

Fixes #23191

This is one possible way to fix the situation, one that doesn't involve touching wxUILocale at all.

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

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

Commit Summary

df15fdb Fix weekday/month name localization in generic wxCalendarCtrl on macOS

File Changes

(3 files)

Patch Links:

—
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,

May 17, 2023, 9:16:48 AM5/17/23

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

@lanurmi pushed 1 commit.

5332bc1 Fix weekday/month name localization in generic wxCalendarCtrl on macOS

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

VZ

unread,

May 17, 2023, 10:13:34 AM5/17/23

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

Thanks, we do need to fix this problem but this doesn't look like a very nice solution to me. I'd much prefer to extend wxUILocale with the required functionality, i.e. add something like this

enum wxLocaleCalendarForm
{
    wxLOCALE_CALENDAR_FULL,
    wxLOCALE_CALENDAR_SHORT,
//  wxLOCALE_CALENDAR_MEDIUM ?
};

class wxUILocale
{
...
    wxString GetMonthName(wxDateTime::Month month, wxLocaleCalendarForm form);
    wxString GetWeekDayName(wxDateTime::WeekDay wday, wxLocaleCalendarForm form);
};

It looks like this should be relatively straightforward to implement, but more elegant and useful.

What do you think?

—
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,

May 18, 2023, 5:15:42 AM5/18/23

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

> Thanks, we do need to fix this problem but this doesn't look like a very nice solution to me. I'd much prefer to extend `wxUILocale` with the required functionality, i.e. add something like this
>

> ```c++

> class wxUILocale
> {
> ...
> wxString GetMonthName(wxDateTime::Month month, wxLocaleCalendarForm form);
> wxString GetWeekDayName(wxDateTime::WeekDay wday, wxLocaleCalendarForm form);
> };
> ```
>
> It looks like this should be relatively straightforward to implement, but more elegant and useful.
>
> What do you think?

That would indeed elegantly solve the problem for the calendar control. But there is another regression waiting for a solution: `wxDateTime::Format()` (and `wxStrftime()`) used to be the way to get localized timestamps in the desired format – but they also return English now on macOS

VZ

unread,

May 18, 2023, 2:32:21 PM5/18/23

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

What do you think?

That would indeed elegantly solve the problem for the calendar control. But there is another regression waiting for a solution: wxDateTime::Format() (and wxStrftime()) used to be the way to get localized timestamps in the desired format – but they also return English now on macOS.

We need to decide how are these functions expected to behave. wxStrftime() should really give the same results as strftime(), so I don't think it should be changed unless you explicitly change the C locale -- which is something you still can do. For wxDateTime, it's less obvious. Generally speaking we try to make C++ classes compatible with C functions, but OTOH I can see that using wxDateTime::Format() could be more useful if it respected the UI locale.

If we keep using C locale in this function, we would need to introduce some new API to allow formatting dates in locale-specific way:

Either some wxNumberFormatter-like new wxDateFormatter class.
Or add wxUILocale::FormatDate().

That issue could be solved independently, but my idea was that the ultimate solution would be to reimplement wxStrftime using NSDateFormatter, which would fix the calendar control too in the process, without any changes to its code. I am not 100% sure yet if such a reimplementation with complete functionality is possible, but I decided to submit what I have ready now using that approach.

Sorry, I really don't think it's correct to change wxStrftime() to behave differently from the corresponding standard C function.

—
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,

May 18, 2023, 5:34:03 PM5/18/23

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

> I'd much prefer to extend `wxUILocale` with the required functionality, i.e. add something like this
>
> ```c++

> enum wxLocaleCalendarForm
> {
> wxLOCALE_CALENDAR_FULL,
> wxLOCALE_CALENDAR_SHORT,
> // wxLOCALE_CALENDAR_MEDIUM ?
> };
>

> class wxUILocale
> {
> ...
> wxString GetMonthName(wxDateTime::Month month, wxLocaleCalendarForm form);
> wxString GetWeekDayName(wxDateTime::WeekDay wday, wxLocaleCalendarForm form);
> };
> ```
>
> It looks like this should be relatively straightforward to implement, but more elegant and useful.

IMHO this would be the right approach to solve this in a generic way.

I'm willing to implement those methods. However, I don't want to interfer with @lanurmi 's work on the issue.

One question, though: What is `wxLOCALE_CALENDAR_MEDIUM` supposed to mean? Windows for example supports full and abbreviated month and day names, but nothing in between.

> > That would indeed elegantly solve the problem for the calendar control. But there is another regression waiting for a solution: `wxDateTime::Format()` (and `wxStrftime()`) used to be the way to get localized timestamps in the desired format – but they also return English now on macOS.
>
> We need to decide how are these functions expected to behave. `wxStrftime()` should really give the same results as `strftime()`, so I don't think it should be changed unless you explicitly change the C locale -- which is something you still can do.

Since `wxStrftime()` is a wrapper around a CRT function, most likely it should behave like the CRT function. That is, it should depend on the C locale actually set.

> For `wxDateTime`, it's less obvious. Generally speaking we try to make C++ classes compatible with C functions, but OTOH I can see that using `wxDateTime::Format()` could be more useful if it respected the UI locale.

`wxDateTime` has methods `GetMonthName`, `GetEnglishMonthName`, `GetWeekDayName`, and `GetEnglishWeekDayName`. `GetMonthName` and `GetWeekDayName` use `wxStrftime()`, if it is available. If a locale is set via `wxLocale` the C locale is always changed as well. However, `wxUILocale` does **not** change the C locale. This results in unexpected behaviour, because for an i18n application `GetMonthName` and `GetWeekDayName` are expected to return localized names.

> If we keep using C locale in this function, we would need to introduce some new API to allow formatting dates in locale-specific way:
>

> 1. Either some `wxNumberFormatter`-like new `wxDateFormatter` class

VZ

unread,

May 18, 2023, 7:07:27 PM5/18/23

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

IMHO this would be the right approach to solve this in a generic way.

I'm willing to implement those methods. However, I don't want to interfer with @lanurmi 's work on the issue.

I think it would be useful to have these functions in any way, whatever else we decide to do, so unless @lanurmi has any serious objections (or is already working on implementing them), it would be great if you could do it, TIA!

One question, though: What is wxLOCALE_CALENDAR_MEDIUM supposed to mean? Windows for example supports full and abbreviated month and day names, but nothing in between.

Yes, but other platforms do support more than this, e.g. macOS NSDateFormatter has monthSymbols, shortMonthSymbols and veryShortMonthSymbols, see its docs. And Unicode standard also defines more than 2 forms (it also defines separate formatting and standalone forms, which it would probably be nice to support...).

And for week days even Windows has 3 forms: LOCALE_SDAYNAMEx, LOCALE_SABBREVDAYNAMEx and LOCALE_SSHORTESTDAYNAMEx.

Since wxStrftime() is a wrapper around a CRT function, most likely it should behave like the CRT function. That is, it should depend on the C locale actually set.

As I had already written, I agree with this.

For wxDateTime, it's less obvious. Generally speaking we try to make C++ classes compatible with C functions, but OTOH I can see that using wxDateTime::Format() could be more useful if it respected the UI locale.

wxDateTime has methods GetMonthName, GetEnglishMonthName, GetWeekDayName, and GetEnglishWeekDayName. GetMonthName and GetWeekDayName use wxStrftime(), if it is available. If a locale is set via wxLocale the C locale is always changed as well. However, wxUILocale does not change the C locale. This results in unexpected behaviour, because for an i18n application GetMonthName and GetWeekDayName are expected to return localized names.

So you would be for wxDateTime always using UI locale?

If we keep using C locale in this function, we would need to introduce some new API to allow formatting dates in locale-specific way:

1. Either some `wxNumberFormatter`-like new `wxDateFormatter` class.
2. Or add `wxUILocale::FormatDate()`.
Is this really necessary? IMHO it should be sufficient to implement methods wxUILocale::GetMonthName() and wxUILocale::GetWeekDayName() and use them in wxDateTime::GetMonthName resp wxDateTime::GetWeekDayName.

This would be necessary if wxDateTime::Format() (and all the other locale-dependent functions, including ParseXXX()) kept using C locale. If we decide to use the UI locale in Format(), then we don't need any other new API. But I'm still not sure if this is the right choice...

—
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,

May 18, 2023, 7:57:37 PM5/18/23

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

That would indeed elegantly solve the problem for the calendar control. But there is another regression waiting for a solution: wxDateTime::Format() (and wxStrftime()) used to be the way to get localized timestamps in the desired format – but they also return English now on macOS.

We need to decide how are these functions expected to behave. wxStrftime() should really give the same results as strftime()

Fair enough, the reimplemented function could be called wxAlmostStrftimeButNotQuite() and be used in appropriate places, leaving the actual wxStrftime() as it is. wxStrftime() was not my primary focus to begin with.

Generally speaking we try to make C++ classes compatible with C functions, but OTOH I can see that using wxDateTime::Format() could be more useful if it respected the UI locale.

For the case of (wx)strftime there obviously is a 1:1 corresponding C function to be compatible with, but what is the point of reference in C that wxDateTime::Format() aims to be compatible with? Sure, its documentation says it "does the same as ANSI C strftime", but is that to be interpreted literally? Besides it already does more, e.g. supports non-standard format specifiers.

To me it would seem perfectly logical that wxDateTime::Format() in 3.2 aims to be compatible with wxDateTime::Format() of wx 3.0 and earlier, yet it now sadly fails to be compatible.

If one has existing code that creates a wxLocale object the traditional way, with wx 3.0.x and most of 3.1.x one would get localized timestamps with Format(). But with wx 3.2 on macOS, that is no longer the case, the timestamps will be in English. Behavior of existing code has dramatically changed, and knowing how much you value backwards compatibility, I am surprised you do not appear to be worried about this.

In fact, I am not sure if there is currently any way to format localized timestamps with wx 3.2 on macOS, without first directly calling setlocale() with appropriate arguments.

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

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

VZ

unread,

May 18, 2023, 8:08:38 PM5/18/23

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

For the case of (wx)strftime there obviously is a 1:1 corresponding C function to be compatible with, but what is the point of reference in C that wxDateTime::Format() aims to be compatible with? Sure, its documentation says it "does the same as ANSI C strftime", but is that to be interpreted literally?

I'm not sure. I think it could be argued that it should, as this is how it always worked so far, and it does say "the same" and not "like". There is also the question of compatibility with wxString::{To,From}Double() functions: those are supposed to work like the standard C functions and use the current C locale too and we added wxNumberFormatter which uses UI locale. It could be argued that wxDateTime is similar.

To me it would seem perfectly logical that wxDateTime::Format() in 3.2 aims to be compatible with wxDateTime::Format() of wx 3.0 and earlier, yet it now sadly fails to be compatible.

If one has existing code that creates a wxLocale object the traditional way, with wx 3.0.x and most of 3.1.x one would get localized timestamps with Format(). But with wx 3.2 on macOS, that is no longer the case, the timestamps will be in English. Behavior of existing code has dramatically changed, and knowing how much you value backwards compatibility, I am surprised you do not appear to be worried about this.

Sorry, this is because I didn't realize this was an incompatible change. I thought that using wxLocale still worked and it's only using wxUILocale which didn't. Of course, we do recommend using wxUILocale in the new code, so it's still a problem and must be fixed, but I didn't view it as a compatibility break. Could you please confirm that the dates are not localized any longer even when using wxLocale? I really don't see why would this happen from looking at the code, but I could be missing something.

In fact, I am not sure if there is currently any way to format localized timestamps with wx 3.2 on macOS, without first directly calling setlocale() with appropriate arguments.

I thought using wxLocale was this way...

—
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,

May 19, 2023, 6:51:04 AM5/19/23

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

If one has existing code that creates a wxLocale object the traditional way, with wx 3.0.x and most of 3.1.x one would get localized timestamps with Format(). But with wx 3.2 on macOS, that is no longer the case

Sorry, this is because I didn't realize this was an incompatible change. I thought that using wxLocale still worked and it's only using wxUILocale which didn't. Of course, we do recommend using wxUILocale in the new code, so it's still a problem and must be fixed, but I didn't view it as a compatibility break. Could you please confirm that the dates are not localized any longer even when using wxLocale? I really don't see why would this happen from looking at the code, but I could be missing something.

Okay, after further investigation localized formatting along with wxLocale is not as completely broken as I first thought, but it still is broken in a very common use case.

In short, when wxLocale is initialized to use the system default language, numerical parts of a date are formatted as localized, but day/month names are not. I opened #23557 and tried to describe the details there.

—
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,

May 19, 2023, 7:09:22 AM5/19/23

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

{
...
    wxString GetMonthName(wxDateTime::Month month, wxLocaleCalendarForm form);
    wxString GetWeekDayName(wxDateTime::WeekDay wday, wxLocaleCalendarForm form);
};

I'm willing to implement those methods. However, I don't want to interfer with @lanurmi 's work on the issue.

I am not currently planning to implement those methods, so feel free to do it. The approach I planned is mostly independent of wxUILocale, so both could co-exist, in theory.

For wxDateTime, it's less obvious. Generally speaking we try to make C++ classes compatible with C functions, but OTOH I can see that using wxDateTime::Format() could be more useful if it respected the UI locale.

wxDateTime has methods GetMonthName, GetEnglishMonthName, GetWeekDayName, and GetEnglishWeekDayName. GetMonthName and GetWeekDayName use wxStrftime(), if it is available. If a locale is set via wxLocale the C locale is always changed as well. However, wxUILocale does not change the C locale. This results in unexpected behaviour, because for an i18n application GetMonthName and GetWeekDayName are expected to return localized names.

I agree it is unexpected. From the library user's perspective, I suppose most have inserted a few lines of code to make localization and translations work, and do not care that much how exactly the localization is achieved under the hood. And then suddenly the way to use the library changes, for no apparent reason.

—
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,

May 19, 2023, 8:09:42 AM5/19/23

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

For the case of (wx)strftime there obviously is a 1:1 corresponding C function to be compatible with, but what is the point of reference in C that wxDateTime::Format() aims to be compatible with? Sure, its documentation says it "does the same as ANSI C strftime", but is that to be interpreted literally?

I'm not sure. I think it could be argued that it should, as this is how it always worked so far, and it does say "the same" and not "like". There is also the question of compatibility with wxString::{To,From}Double() functions: those are supposed to work like the standard C functions and use the current C locale too and we added wxNumberFormatter which uses UI locale. It could be argued that wxDateTime is similar.

Actually, my expectation as a user of wxWidgets never was that wxString::{To,From}Double() work like the standard C functions. My expectation was and is that these methods respect the locale setting of my application. Since wxLocale sets the C locale (via setlocale), too, that worked flawlessly in the past. However, if I don't use wxLocale, but wxUILocale it doesn't work anymore.

IMHO, wxString::{To,From}Double() should use wxUILocale. If I want to use the C locale, I would resort to wxString::{To,From}CDouble().

What definitely should be avoided is that one has to use both wxLocale and wxUILocale to get the expected results.

To me it would seem perfectly logical that wxDateTime::Format() in 3.2 aims to be compatible with wxDateTime::Format() of wx 3.0 and earlier, yet it now sadly fails to be compatible.
If one has existing code that creates a wxLocale object the traditional way, with wx 3.0.x and most of 3.1.x one would get localized timestamps with Format(). But with wx 3.2 on macOS, that is no longer the case, the timestamps will be in English. Behavior of existing code has dramatically changed, and knowing how much you value backwards compatibility, I am surprised you do not appear to be worried about this.

Sorry, this is because I didn't realize this was an incompatible change. I thought that using wxLocale still worked and it's only using wxUILocale which didn't.

I really feel unsure what works as expected and what not.

Using wxLocale should definitely work as expected. It instantiates a corresponding wxUILocale under the hood and sets the C locale with wxSetlocale. The latter is not done, if only wxUILocale is used. And that leads to problems, because several methods still assume that the C locale was set properly.

Of course, we do recommend using wxUILocale in the new code, so it's still a problem and must be fixed, but I didn't view it as a compatibility break.

Locale dependent methods should use wxUILocale methods to determine the correct way to fulfill their task.

Could you please confirm that the dates are not localized any longer even when using wxLocale? I really don't see why would this happen from looking at the code, but I could be missing something.

I will take a look.

In fact, I am not sure if there is currently any way to format localized timestamps with wx 3.2 on macOS, without first directly calling setlocale() with appropriate arguments.

I thought using wxLocale was this way...

As said above a user of wxWidgets should not be forced to use both wxLocale and wxUILocale to get the expected results. And this should be true on all supported platforms.

—
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,

May 19, 2023, 8:14:17 AM5/19/23

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

I am not currently planning to implement those methods, so feel free to do it. The approach I planned is mostly independent of wxUILocale, so both could co-exist, in theory.

Ok, I will work on implementing those methods in the near future.

If a locale is set via wxLocale the C locale is always changed as well. However, wxUILocale does not change the C locale. This results in unexpected behaviour, because for an i18n application GetMonthName and GetWeekDayName are expected to return localized names.

I agree it is unexpected. From the library user's perspective, I suppose most have inserted a few lines of code to make localization and translations work, and do not care that much how exactly the localization is achieved under the hood. And then suddenly the way to use the library changes, for no apparent reason.

Exactly. Localization should simply work. How it is done under the hood should not be a concern for a library user.

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

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

VZ

unread,

May 19, 2023, 10:22:53 AM5/19/23

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

Actually, my expectation as a user of wxWidgets never was that wxString::{To,From}Double() work like the standard C functions. My expectation was and is that these methods respect the locale setting of my application. Since wxLocale sets the C locale (via setlocale), too, that worked flawlessly in the past. However, if I don't use wxLocale, but wxUILocale it doesn't work anymore.

I guess this makes sense now that wxString is only supposed to be used as a string class in the GUI code. It definitely wasn't the original intention when this class was designed as a general purpose string class back at the time when std::wstring was not universally available.

IMHO, wxString::{To,From}Double() should use wxUILocale. If I want to use the C locale, I would resort to wxString::{To,From}CDouble().

Sorry, there is a confusion between two different meanings of "C" here. When I say "the C locale", I mean "the locale set by setlocale()", not "the "C" locale", which is the same as "POSIX" locale and is basically the same as "US English". The {To,From}CDouble() functions use "C" in the latter sense.

What definitely should be avoided is that one has to use both wxLocale and wxUILocale to get the expected results.

Sorry, this is because I didn't realize this was an incompatible change. I thought that using wxLocale still worked and it's only using wxUILocale which didn't.

I really feel unsure what works as expected and what not.

This was clarified in #23557 (Thanks Lauri!).

As said above a user of wxWidgets should not be forced to use both wxLocale and wxUILocale to get the expected results. And this should be true on all supported platforms.

What bothers me is that "UI" in wxUILocale is there because it indicates that it's the locale used for the UI, and dates and, especially, strings can both be used for non-UI things too and are, in fact, routinely used for this (e.g. I've definitely seen both used a lot in IO-related code). And so you need to also consider the converse problem: someone might decide to follow our advice to just add wxUILocale::UseDefault() to their application and then be unpleasantly surprised by their network code not working any longer because the dates/strings are formatted differently.

So while I agree with changing wxDateTime to use UI locale in master, IMO changing wxString::{To,From}Double() to use UI locale instead of the C-level locale in 3.2 is out of the question, this risks silently breaking too much of the existing code. But I'm still not sure what to do with the two "intermediate" cases: wxDateTime in 3.2 and wxString in master.

—
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,

May 19, 2023, 3:16:03 PM5/19/23

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

Actually, my expectation as a user of wxWidgets never was that wxString::{To,From}Double() work like the standard C functions. My expectation was and is that these methods respect the locale setting of my application.

The real question is what was meant by "setting the locale" - in earlier versions of wxWidgets and in current versions (after wxUILocale was introduced). Respecting the current locale was and is documented behaviour (for example for FromDouble()):

"Notice that the string returned by this function uses the decimal separator appropriate for the current locale"

In earlier versions there was no destinction between the current wxLocale and the current C locale, because wxLocale used setlocale internally. With the arrival of wxUILocale this has changed. The C locale is no longer set.

How is a wxWidgets user supposed to know what is meant by the "current locale" of an application. That is, the documentation of wxString::{To,From}Double() has to be updated to explicitly mention that the formatting is done according to the current C locale, not the current locale as set by wxUILocale, although on some platforms they will still be equal.

I guess this makes sense now that wxString is only supposed to be used as a string class in the GUI code. It definitely wasn't the original intention when this class was designed as a general purpose string class back at the time when std::wstring was not universally available.

It might not have been the original intention, but due to the fact that wxLocale always changed the C locale, too, the current behaviour based on wxUILocale can be confusing.

IMHO, wxString::{To,From}Double() should use wxUILocale. If I want to use the C locale, I would resort to wxString::{To,From}CDouble().

Sorry, there is a confusion between two different meanings of "C" here. When I say "the C locale", I mean "the locale set by setlocale()", not "the "C" locale", which is the same as "POSIX" locale and is basically the same as "US English". The {To,From}CDouble() functions use "C" in the latter sense.

I don't think I mixed up anything. I'm fully aware of the difference of the C/POSIX locale and the locale set by setlocale. AFAIK any C/C++ application starts with a C/POSIX locale. wxLocale modifies the locale at C level, when it calls setlocale, while wxUILocale does not change the locale at C level, because it doesn't call setlocale.

In earlier versions of wxWidgets wxString::{To,From}Double() used the C locale that was set by wxLocale. And that is expected. If I want to make sure that numbers are formatted according to C/POSIX locale, I use the methods wxString::{To,From}CDouble().

Now, it is recommended to use wxUILocale, but as a consequence the C locale is no longer modified, and wxString::{To,From}Double() do not use the locale separator. Most likely this comes unexpected to most wxWidgets users.

What bothers me is that "UI" in wxUILocale is there because it indicates that it's the locale used for the UI, and dates and, especially, strings can both be used for non-UI things too and are, in fact, routinely used for this (e.g. I've definitely seen both used a lot in IO-related code).

Yes, you are absolutely right. However, in IO related code one is usually interested in a portable, locale-independent representation. That's why we have wxString::{To,From}CDouble() and wxDateTime::FormatISODate() and so on.

Usually applications not using wxWidgets set the locale via setlocale or other means, because they need to present information to the user formatted according to the user's UI locale.

And so you need to also consider the converse problem: someone might decide to follow our advice to just add wxUILocale::UseDefault() to their application and then be unpleasantly surprised by their network code not working any longer because the dates/strings are formatted differently.

Well, it is bad programming practice to rely on methods and functions that depend on the current C locale. Many libraries out there simply ignore that applications may change the C-level locale to something els than C/POSIX, and cease to work properly, when the C-level locale is not C/POSIX.

So while I agree with changing wxDateTime to use UI locale in master, IMO changing wxString::{To,From}Double() to use UI locale instead of the C-level locale in 3.2 is out of the question, this risks silently breaking too much of the existing code. But I'm still not sure what to do with the two "intermediate" cases: wxDateTime in 3.2 and wxString in master.

IMO it is inconsistent to use the UI locale for wxDateTime but not for wxString. Formatting dates with wxDateTime according to the C locale makes perfectly sense, too. Things can get even worse, if a user chooses to set a C locale that differs from the UI locale.

Actually, formatting could be done in 3 different ways:

based on C/POSIX locale (for a portable representation)
based on C level locale
based on UI locale

That is, to be on the safe side it would require to implement 3 different methods, one for each purpose.

—
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,

May 20, 2023, 7:30:05 AM5/20/23

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

I started to implement methods GetMonthName() and GetWeekDayName() in wxUILocale.

However, I have a few questions.

Do we need a separate enum wxLocaleCalendarForm?

enum wxLocaleCalendarForm
{
    wxLOCALE_CALENDAR_FULL,
     wxLOCALE_CALENDAR_SHORT,
//  wxLOCALE_CALENDAR_MEDIUM ?
};

We already have enum wxDateTime::NameFlags:

enum NameFlags
{
    Name_Full = 0x01,    // return full name
    Name_Abbr = 0x02     // return abbreviated name
};

[...] but other platforms do support more than this, e.g. macOS NSDateFormatter has monthSymbols, shortMonthSymbols and veryShortMonthSymbols, see its docs.

If you really want additional name variations (like shortest abbreviation), this enum could be extended:

enum NameFlags
{
    Name_Full = 0x01,      // return full name
    Name_Abbr = 0x02       // return abbreviated name
    Name_Shortest = 0x03   // return shortest name
};

macOS supports very short abbreviations for month and weekday names, Windows supports only very short month names, Linux doesn't support very short names at all.

And Unicode standard also defines more than 2 forms (it also defines separate formatting and standalone forms, which it would probably be nice to support...).

Well, I think this is beyond the scope of wxWidgets. If a user really needs such special formatting a library like ICU should be used.

Regarding the Linux/Unix implementation of GetMonthName() and GetWeekDayName() I have another question:

What should we do, if langinfo.h is not available? We could return an empty string, but that is really bad. Or we could return the English names using wxDateTime::GetEnglishMonthName() resp wxDateTime::GetEnglishWeekDayName(). Or do you have any better idea?

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

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

VZ

unread,

May 20, 2023, 9:29:33 AM5/20/23

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

The real question is what was meant by "setting the locale"

For me this rather unambiguously means "C-level locale", but I realize that there might be different interpretations of this.

How is a wxWidgets user supposed to know what is meant by the "current locale" of an application. That is, the documentation of wxString::{To,From}Double() has to be updated to explicitly mention that the formatting is done according to the current C locale, not the current locale as set by wxUILocale, although on some platforms they will still be equal.

Yes, we need to update the documentation in any case. But we need to actually agree on how things should work first.

IMHO, wxString::{To,From}Double() should use wxUILocale. If I want to use the C locale, I would resort to wxString::{To,From}CDouble().

Sorry, there is a confusion between two different meanings of "C" here.

[...]

I don't think I mixed up anything.

Sorry, I didn't mean to imply you did, I just wanted to clarify that "C" in the name of these functions meant that they used "C-as-POSIX locale" and not "C-level locale".

So while I agree with changing wxDateTime to use UI locale in master, IMO changing wxString::{To,From}Double() to use UI locale instead of the C-level locale in 3.2 is out of the question, this risks silently breaking too much of the existing code. But I'm still not sure what to do with the two "intermediate" cases: wxDateTime in 3.2 and wxString in master.

IMO it is inconsistent to use the UI locale for wxDateTime but not for wxString.

Yes, it is. Which is exactly why I was originally uncomfortable (and still somewhat is) with changing wxDateTime to use UI locale.

Formatting dates with wxDateTime according to the C locale makes perfectly sense, too. Things can get even worse, if a user chooses to set a C locale that differs from the UI locale.

Actually, formatting could be done in 3 different ways:

1. based on C/POSIX locale (for a portable representation)

2. based on C level locale

3. based on UI locale

That is, to be on the safe side it would require to implement 3 different methods, one for each purpose.

In my original proposal I wanted to leave wxDateTime methods themselves for doing (1) and (2) and add a separate wxDateFormatter for doing (3), but both you and Lauri were against it and in favour of using wxDateTime to do (3) instead -- which leaves us without any way to do (1) or (2).

I'm still not sure at all what is the right thing to do here...

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

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

VZ

unread,

May 20, 2023, 9:49:06 AM5/20/23

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

I started to implement methods GetMonthName() and GetWeekDayName() in wxUILocale.

Thanks!

However, I have a few questions.

Do we need a separate enum wxLocaleCalendarForm?

...

We already have enum wxDateTime::NameFlags:

enum NameFlags
{
    Name_Full = 0x01,    // return full name
    Name_Abbr = 0x02     // return abbreviated name
};

No, you're right, I just forgot about this enum existence, we should reuse it here.

If you really want additional name variations (like shortest abbreviation), this enum could be extended:

enum NameFlags
{
    Name_Full = 0x01,      // return full name
    Name_Abbr = 0x02       // return abbreviated name
    Name_Shortest = 0x03   // return shortest name
};

We could indeed extend it like this. It would also be fine not do it right now and add it later if anybody really needs it.

And Unicode standard also defines more than 2 forms (it also defines separate formatting and standalone forms, which it would probably be nice to support...).

Well, I think this is beyond the scope of wxWidgets. If a user really needs such special formatting a library like ICU should be used.

The difference is actually pretty important for the languages in which it's present (such as all Slavic languages, at least), i.e. it's much more important than Name_Shortest. In Ukrainian, for example, the name of the current month that should be shown in the calendar control is "травень", but if you write todays date, it would be "20 травня", and it would be really wrong to use either form in the other context (a bit like mixing up Akkusativ and Dativ in German, which is, as you know, very wrong even if very easy for foreigners to do -- as I can speak from experience :-).

So if we could support this, it would be nice. We'd have to extend NameFlags with Name_Format = 0 and Name_Standalone = 0x80 or something like this, which would be just ignored at wxDateTime level.

Regarding the Linux/Unix implementation of GetMonthName() and GetWeekDayName() I have another question:

What should we do, if langinfo.h is not available? We could return an empty string, but that is really bad. Or we could return the English names using wxDateTime::GetEnglishMonthName() resp wxDateTime::GetEnglishWeekDayName(). Or do you have any better idea?

No, sorry, no better ideas. And either of those would work, but we just need to document clearly whether the various functions return empty string or the English name if they can't retrieve the localized one. I guess the latter would be a bit more useful in practice because I don't see what else could you do than fall back on the untranslated name in the application code anyhow, but maybe I'm missing something here.

—
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,

May 20, 2023, 10:36:52 AM5/20/23

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

We already have enum wxDateTime::NameFlags:
enum NameFlags
{
    Name_Full = 0x01,    // return full name
    Name_Abbr = 0x02     // return abbreviated name
};
No, you're right, I just forgot about this enum existence, we should reuse it here.

Ok. Expecting this answer I already did that. 😄

If you really want additional name variations (like shortest abbreviation), this enum could be extended:
enum NameFlags
{
    Name_Full = 0x01,      // return full name
    Name_Abbr = 0x02       // return abbreviated name
    Name_Shortest = 0x03   // return shortest name
};
We could indeed extend it like this. It would also be fine not do it right now and add it later if anybody really needs it.

I'm fine with extending it right away, although it is fully supported only under macOS.

And Unicode standard also defines more than 2 forms (it also defines separate formatting and standalone forms, which it would probably be nice to support...).

Well, I think this is beyond the scope of wxWidgets. If a user really needs such special formatting a library like ICU should be used.

The difference is actually pretty important for the languages in which it's present (such as all Slavic languages, at least), i.e. it's much more important than Name_Shortest. In Ukrainian, for example, the name of the current month that should be shown in the calendar control is "травень", but if you write todays date, it would be "20 травня", and it would be really wrong to use either form in the other context (a bit like mixing up Akkusativ and Dativ in German, which is, as you know, very wrong even if very easy for foreigners to do -- as I can speak from experience :-).

Ok, it wasn't clear to me what you had in mind. It certainly makes sense to have a way to get month and weekday names in the date formatting context and the standalone context. Again, not all systems provide this information - at least as far as I checked the documentation so far.

So if we could support this, it would be nice. We'd have to extend NameFlags with Name_Format = 0 and Name_Standalone = 0x80 or something like this, which would be just ignored at wxDateTime level.

Ok, I can introduce these additional flags.

What should we do, if langinfo.h is not available? We could return an empty string, but that is really bad. Or we could return the English names using wxDateTime::GetEnglishMonthName() resp wxDateTime::GetEnglishWeekDayName(). Or do you have any better idea?

No, sorry, no better ideas. And either of those would work, but we just need to document clearly whether the various functions return empty string or the English name if they can't retrieve the localized one. I guess the latter would be a bit more useful in practice because I don't see what else could you do than fall back on the untranslated name in the application code anyhow, but maybe I'm missing something here.

At the moment, I use the English names as a fallback. It's definitely better than empty strings.

—
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,

May 20, 2023, 10:42:54 AM5/20/23

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

Yes, we need to update the documentation in any case. But we need to actually agree on how things should work first.

I saw your posting on the mailing list. I will comment there later.

IMO it is inconsistent to use the UI locale for wxDateTime but not for wxString.

Yes, it is. Which is exactly why I was originally uncomfortable (and still somewhat is) with changing wxDateTime to use UI locale.

IMHO we should avoid such inconsistencies. It makes things worse, and harder to fix in later wxWidgets versions.

Formatting dates with wxDateTime according to the C locale makes perfectly sense, too. Things can get even worse, if a user chooses to set a C locale that differs from the UI locale.
Actually, formatting could be done in 3 different ways:
1. based on C/POSIX locale (for a portable representation)
2. based on C level locale
3. based on UI locale

In my original proposal I wanted to leave wxDateTime methods themselves for doing (1) and (2) and add a separate wxDateFormatter for doing (3), but both you and Lauri were against it and in favour of using wxDateTime to do (3) instead -- which leaves us without any way to do (1) or (2).

I'm still not sure at all what is the right thing to do here...

I will explain my reasoning in more detail in a response to your posting on the mailing list.

—
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,

May 20, 2023, 12:36:48 PM5/20/23

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

The difference is actually pretty important for the languages in which it's present (such as all Slavic languages, at least), i.e. it's much more important than Name_Shortest. In Ukrainian, for example, the name of the current month that should be shown in the calendar control is "травень", but if you write todays date, it would be "20 травня", and it would be really wrong to use either form in the other context (a bit like mixing up Akkusativ and Dativ in German, which is, as you know, very wrong even if very easy for foreigners to do -- as I can speak from experience :-).

So if we could support this, it would be nice. We'd have to extend NameFlags with Name_Format = 0 and Name_Standalone = 0x80 or something like this, which would be just ignored at wxDateTime level.

Regarding NameFlags I have a further question.

Are these additional flag values meant to be combined with the other flags like shown in the table below (which shows the available support in macOS)?

	Date formatting	Standalone
Flag	`Name_Format`	`Name_Standalone`
`Name_Full`	monthSymbols	standaloneMonthSymbols
`Name_Abbr`	shortMonthSymbols	shortStandaloneMonthSymbols
`Name_Shortest`	veryShortMonthSymbols	veryShortStandaloneMonthSymbols

However, wouldn't that contradict the use of the enum type for the individual flag 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,

May 20, 2023, 4:27:52 PM5/20/23

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

Regarding NameFlags I have a further question.

Are these additional flag values meant to be combined with the other flags like shown in the table below (which shows the available support in macOS)?
Date formatting Standalone
Flag Name_Format Name_Standalone
Name_Full monthSymbols standaloneMonthSymbols
Name_Abbr shortMonthSymbols shortStandaloneMonthSymbols
Name_Shortest veryShortMonthSymbols veryShortStandaloneMonthSymbols

However, wouldn't that contradict the use of the enum type for the individual flag values?

You're again right, I was misled by the name of this enum, which includes the words Flags, but actually they're not flags at all, as they're mutually exclusive and can't be combined.

We could make them real flags and change the existing functions to take int instead of NameFlags, but it would be cleaner to add a different enum NameContext with the elements Context_Standalone and Context_Formatting. And then we could either add a new NameContext context = Context_Formatting parameter to all functions currently taking NameFlags or, if you prefer, add something like this:

// Describes the form of the month or week-day name.
class NameForm
{
public:
    // Ctor is non-explicit for compatibility.
    NameForm(NameFlags flags = Name_Full) : m_flags(flags) {}

    // Chainable methods allowing to set various fields.
    NameFlags& Abbr() { m_flags = Name_Abbr; return *this; }
    NameFlags& Full() { m_flags = Name_Full; return *this; }
    NameFlags& Formatting() { m_context = Context_Formatting; return *this; }
    NameFlags& Standalone() { m_context = Context_Standalone; return *this; }

private:
    NameFlags m_flags;
    NameContext m_context = Context_Formatting;
};

and change the functions taking NameFlags flags = Name_Full to take NameForm form = {} instead. I like this API because it's IMO more readable to write something like

wxString name = wxDateTime::GetMonthName(wxDateTime::May, wxDateTime::NameForm().Standalone().Shortest());

than

wxString name = wxDateTime::GetMonthName(wxDateTime::May, wxDateTime::Name_Shortest, wxDateTime::Context_Standalone);

and you don't have to remember the order.

—
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,

May 20, 2023, 6:30:06 PM5/20/23

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

You're again right, I was misled by the name of this enum, which includes the words Flags, but actually they're not flags at all, as they're mutually exclusive and can't be combined.

We could make them real flags and change the existing functions to take int instead of NameFlags, but it would be cleaner to add a different enum NameContext with the elements Context_Standalone and Context_Formatting. And then we could either add a new NameContext context = Context_Formatting parameter to all functions currently taking NameFlags or, if you prefer, add something like this: [...]

I like that approach. I will modify my code accordingly.

BTW, in principle, my implementation already works for wxMSW. wxGTK and wxOSX versions will follow shortly - hopefully.

—
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,

May 21, 2023, 12:08:49 PM5/21/23

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

[...] and change the functions taking NameFlags flags = Name_Full to take NameForm form = {} instead.

Well, maybe changing the signature of the existing methods is not such a good idea. It would break code that uses them. The current signatures are:

wxString wxDateTime::GetMonthName(wxDateTime::Month month, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);
wxString wxDateTime::GetEnglishMonthName(wxDateTime::Month month, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);

wxString wxDateTime::GetWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);
wxString wxDateTime::GetEnglishWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);

We could add methods that take a wxDateTime::NameForm parameter:

wxString wxDateTime::GetMonthName(wxDateTime::Month month, wxDateTime::NameForm form = {});
wxString wxDateTime::GetMonthName(wxDateTime::Month month, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);
wxString wxDateTime::GetEnglishMonthName(wxDateTime::Month month, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);

wxString wxDateTime::GetWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameForm form = {});
wxString wxDateTime::GetWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);
wxString wxDateTime::GetEnglishWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);

Unfortunately, this doesn't compile, because the signatures are ambiguous. So, we have to either change the name or make the form parameter a required one. What do you prefer?

Additionally, we have to decide whether versions with a form parameter should be introduced for methods retrieving the English names. Actually, it would not be necessary, because the names are equal for all contexts AFAIK.

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

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

VZ

unread,

May 21, 2023, 12:48:39 PM5/21/23

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

[...] and change the functions taking NameFlags flags = Name_Full to take NameForm form = {} instead.

Well, maybe changing the signature of the existing methods is not such a good idea. It would break code that uses them.

The idea was that it wouldn't break them because the existing code would work by implicitly converting Name_XXX to NameForm first (which is why it was important to make its ctor non-explicit). This is not 100%-compatible because it breaks the existing code already relying on implicit conversion of something else to wxDateTime::NameFlags, but I have trouble believing such code really exists.

So why do you think this change would break them? Could you please show some example of the code that it breaks?

We could add methods that take a wxDateTime::NameForm parameter:

Yes, but it would be nicer to avoid having all these overloads.

wxString wxDateTime::GetMonthName(wxDateTime::Month month, wxDateTime::NameForm form = {});
wxString wxDateTime::GetMonthName(wxDateTime::Month month, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);
wxString wxDateTime::GetEnglishMonthName(wxDateTime::Month month, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);

wxString wxDateTime::GetWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameForm form = {});
wxString wxDateTime::GetWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);
wxString wxDateTime::GetEnglishWeekDayName(wxDateTime::WeekDay weekday, wxDateTime::NameFlags flags = wxDateTime::NAME_Full);

Unfortunately, this doesn't compile, because the signatures are ambiguous. So, we have to either change the name or make the form parameter a required one. What do you prefer?

If we do it like this, the parameter should be required.

Additionally, we have to decide whether versions with a form parameter should be introduced for methods retrieving the English names. Actually, it would not be necessary, because the names are equal for all contexts AFAIK.

Yes, for English it's useless, but could be nice to have for consistency, especially if we replace NameFlags with NameForm, as originally proposed.

—
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,

May 21, 2023, 1:08:57 PM5/21/23

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

The idea was that it wouldn't break them because the existing code would work by implicitly converting Name_XXX to NameForm first (which is why it was important to make its ctor non-explicit). This is not 100%-compatible because it breaks the existing code already relying on implicit conversion of something else to wxDateTime::NameFlags, but I have trouble believing such code really exists.

Sorry for the noise. All compiles and works as expected. The problem was that I had 2 instances of Visual Studio open, one for wxWidgets itself and one for a sample program. Somehow Visual Studio did not clean up its cache properly. After deleting all intermediate VS files and starting from scratch, the compile succeeded

So why do you think this change would break them? Could you please show some example of the code that it breaks?

No, I can't. It works as you intended.

We could add methods that take a wxDateTime::NameForm parameter:

Yes, but it would be nicer to avoid having all these overloads.

So, no additional overloads needed.

Additionally, we have to decide whether versions with a form parameter should be introduced for methods retrieving the English names. Actually, it would not be necessary, because the names are equal for all contexts AFAIK.

Yes, for English it's useless, but could be nice to have for consistency, especially if we replace NameFlags with NameForm, as originally proposed.

Ok, I will change the signature accordingly.

—
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,

May 21, 2023, 5:50:33 PM5/21/23

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

Today I worked on the wxOSX implementation. I finished it a few minutes ago ... and it works! ✨ The calendar control of the internat sample now displays month and weekday names in the wxUILocale language.

So, the wxMSW and wxOSX implementations are operational. Next step will be the wxGTK implementation. If all goes well I hope to be able to provide a PR within the next week.

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

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

VZ

unread,

May 28, 2023, 5:01:40 PM5/28/23

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

Closed #23551.

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

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

VZ

unread,

May 28, 2023, 5:01:40 PM5/28/23

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

This is superseded by the updated version of #23556, which includes the changes from #23562 too now.

Please test it and let us know about any problems. TIA!

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

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

Captain Pumpkinhead

unread,

Feb 16, 2025, 1:48:51 PMFeb 16

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

Captain-Pumpkinhead left a comment (wxWidgets/wxWidgets#23551)

I'm having some problems with the Linux Server container image of KiCAD. Every time I try to create a new page in Schematic Editor, it gives me this error:

An assertion failed!

/home/buildozer/aports/community/wxwidgets/src/wxWidgets-3.2.5/src/common/datetimefmt.cpp(318): assert ""!formatp.empty()"" failed in Format(): NULL format in wxDateTime::Format

image.png (view on web)

I'm trying to find a way to fix this, and so far, this thread is the best lead I've gotten. Weirdly, it worked once. I've tried detaching all volumes and changing environmental the Docker Compose stuff to see if there was an interference there and tried older images. Nothing has worked so far. I'm thinking maybe the auto-update means that the older versions of KiCAD I'm trying are updating before I even get to them.

Here's the section of my Docker Compose script that's running KiCAD:

  kicad:
    image: lscr.io/linuxserver/kicad:latest
    devices:
      - /dev/dxg:/dev/kfd
      - /dev/dri:/dev/dri
    #security_opt:
      #- seccomp:unconfined #optional
    environment:
      - PUID=1000
      - PGID=1000
      - TZ=America/Denver
      #- DRINODE=AMD
    volumes:
      - kicad:/config
      - kasm-local:/config/.local
      - cad-common:/config/common
      - usr-share-freecad:/usr/share/freecad
      - usr-share-kicad:/usr/share/kicad
      - usr-share-orca:/usr/share/orca
    ports:
      - 3019:3000
      - 3020:3001
    restart: unless-stopped

I'm not sure if this is a KiCAD problem or a problem with the Linux Server image. If you guys think I should take this over there, let me know.