Add some symbols for retrieving locale info (PR #25765)
Ulrich Telle
The following symbols were added:
- wxLOCALE_MEASURE_METRIC : check whether locale uses metric measurements
- wxLOCALE_CURRENCY_SYMBOL : the currency symbol (like "$")
- wxLOCALE_CURRENCY_CODE : the ISO currency code (like "USD" or "EUR")
- wxLOCALE_CURRENCY_DIGITS : number of currency digits
Documentation and test cases are included.
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/25765
Commit Summary
- 3f627be Add some symbols for retrieving locale info
- 4bc092c Adjust implementations and add documentation
File Changes
(7 files)
- M include/wx/localedefs.h (13)
- M interface/wx/intl.h (38)
- M src/common/intl.cpp (53)
- M src/msw/uilocale.cpp (20)
- M src/osx/core/uilocale.mm (26)
- M src/unix/uilocale.cpp (42)
- M tests/intl/intltest.cpp (27)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/25765.patch
- https://github.com/wxWidgets/wxWidgets/pull/25765.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 6471360 Remove trailing whitespace
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 5503532 Fix trailing whitespace (macOS)
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 485c845 Fix wrong variable name in macOS implementation
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
oneeyeman1
oneeyeman1 left a comment (wxWidgets/wxWidgets#25765)@utelle ,
Thx
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- ab98590 Add missing closing parenthesis
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz requested changes on this pull request.
Thanks, having access to this information definitely seems useful, but it doesn't seem to fit GetInfo() very well. We can make it fit there if you prefer not to add the new functions but in this case I'd like to at least define constants for the various values.
Thanks again!
> @@ -481,6 +481,26 @@ class wxUILocaleImplName : public wxUILocaleImpl
: LOCALE_SDECIMAL);
break;
+ case wxLOCALE_MEASURE_METRIC:
+ str = DoGetInfo(LOCALE_IMEASURE);
+ if (str.IsEmpty())
We prefer using standard-like functions:
⬇️ Suggested change- if (str.IsEmpty()) + if (str.empty())
> @@ -594,6 +594,48 @@ wxUILocaleImplUnix::GetInfo(wxLocaleInfo index, wxLocaleCategory cat) const
return GetLangInfo(RADIXCHAR);
+ case wxLOCALE_MEASURE_METRIC:
+ {
+ wxString measureStr = GetLangInfo(_NL_MEASUREMENT_MEASUREMENT);
+ if (measureStr.IsEmpty())
+ measureStr = wxString("\0");
This return value is not documented and it's not clear what does it correspond to.
> @@ -89,6 +89,19 @@ enum wxLocaleInfo
// the character used as decimal point (for wxLOCALE_CAT_NUMBER or MONEY)
wxLOCALE_DECIMAL_POINT,
+ // check whether locale uses the metric system for measurements
+ // returns "Yes", "No", or "Unknown"
I'd probably rather use "1", "0" and "" instead of these strings.
In any case, let's define wxLOCALE_TRUE, _FALSE and _UNKNOWN constants for them instead of using the literal strings.
Alternatively, perhaps it would be better to have a wxLocaleMeasurement enum and a separate function GetMeasurementSystem() returning it? This would be much cleaner IMO.
> + // the currency symbol (for example "$") + wxLOCALE_CURRENCY_SYMBOL, + + // the currency ISO code (for example "USD") + wxLOCALE_CURRENCY_CODE, + + // the number of currency digits + wxLOCALE_CURRENCY_DIGITS,
We could also define wxLocaleCurrencyInfo struct with int digits field which would avoid to have to return this information as a string.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)Thanks, having access to this information definitely seems useful, but it doesn't seem to fit
GetInfo()very well. We can make it fit there if you prefer not to add the new functions but in this case I'd like to at least define constants for the various values.
I don't have any preferences. Using GetInfo() was the simplest approach to test the functionality under Windows and Unix. Unfortunately, I don't have access to a macOS system right now, so that I had to add the implementation based on information from the Apple documentation.
If you prefer additional methods for wxUILocale, I'm open for proposals, how to name them. The same new methods would have to be added to wxLocale.
I will look into your other comments later today.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
There are 6 failing checks (while over 30 succeed - a better ratio than I had expected):
- [Cirrus CI / FreeBSD wxGTK3](https://github.com/wxWidgets/wxWidgets/pull/25765/checks?check_run_id=49764252622)
Under FreeBSD most `nl_langinfo` symbols are not available. As a workaround, we could use `localeconv` (which should work). The drawback is that `localeconv` only works for the global locale.
- [CMake builds / macOS 14 wxOSX Xcode](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773749/job/49764257927?pr=25765)
Just like the other failing macOS runs an older macOS version 14.7.6 is used. For some reason a `wxLocale` method is used as a fallback (and in that method not all symbols are handled).
- [continuous-integration/appveyor](https://ci.appveyor.com/project/wxWidgets/wxwidgets/builds/52697298)
The VS14 run fails, probably due to misinterpreting the €-sign. Maybe it's not such a good idea to encode it directly in the test source code. I'll try `"\u20AC"` instead.
- [Mac builds / wxMac ARM ASAN](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773760/job/49764252808?pr=25765)
- [Mac builds / wxMac Intel C++17](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773760/job/49764252806?pr=25765)
- [Mac builds / wxMac Universal C++14](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773760/job/49764252802?pr=25765)
For the above 3 macOS runs it looks like that the locales are not properly installed on the runner, even the `en_US` locale returns the general currency symbol `"\u00A4"` instead of `"$"`.
Before I change anything in the API, I'd like to get rid of those failing CI runs.
Now, regarding the API I'd like to stick with `GetInfo`. Actually, the same system functions are used under the hood. And implementing of handling the new `wxLOCALE` symbols requires no changes to the API of `wxUILocale` and `wxLocale`. However, we could add a convenience method for retrieving all the currency information in one call and one data structure. Not sure, whether that convenience method should be added to `wxLocale` at all
Ulrich Telle
@utelle pushed 1 commit.
- bfb184c Fix various issues
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 57d298e Replace NULL by nullptr
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 5f9bcc0 Fix macOS issues
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- e21c411 Add missing type casts
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- ae678e6 Correct retrieval of number of currency digits
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)@vadz: Before looking into the implementation details I'd like to make some remarks about the failing CI runs.
There are 6 failing checks (while over 30 succeed - a better ratio than I had expected):
* [Cirrus CI / FreeBSD wxGTK3](https://github.com/wxWidgets/wxWidgets/pull/25765/checks?check_run_id=49764252622) Under FreeBSD most `nl_langinfo` symbols are not available. As a workaround, we could use `localeconv` (which should work). The drawback is that `localeconv` only works for the global locale.
Yes, the symbols you use are not POSIX (see https://pubs.opengroup.org/onlinepubs/009695399/basedefs/langinfo.h.html for the list of the standard ones), so you have to either add specific checks for them to configure/CMake or use them only for glibc, which is known to have them (assuming nobody else does). Note that POSIX does have CRNCYSTR, so we probably should use it instead of CURRENCY_SYMBOL (which is a glibc extension).
* [CMake builds / macOS 14 wxOSX Xcode](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773749/job/49764257927?pr=25765) Just like the other failing macOS runs an older macOS version 14.7.6 is used. For some reason a `wxLocale` method is used as a fallback (and in that method not all symbols are handled).
It's ok to make the test conditional on the functionality being actually supported, I think.
* [continuous-integration/appveyor](https://ci.appveyor.com/project/wxWidgets/wxwidgets/builds/52697298) The VS14 run fails, probably due to misinterpreting the €-sign. Maybe it's not such a good idea to encode it directly in the test source code. I'll try `"\u20AC"` instead.
I think we use /utf-8 now, don't we? If so, this should work... But I don't have this version of MSVS to test with any longer.
* [Mac builds / wxMac ARM ASAN](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773760/job/49764252808?pr=25765) * [Mac builds / wxMac Intel C++17](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773760/job/49764252806?pr=25765) * [Mac builds / wxMac Universal C++14](https://github.com/wxWidgets/wxWidgets/actions/runs/17520773760/job/49764252802?pr=25765)
For the above 3 macOS runs it looks like that the locales are not properly installed on the runner, even the
en_USlocale returns the general currency symbol"\u00A4"instead of"$".
This character is probably used for "C" locale, so maybe en_US is not even supported there?
Before I change anything in the API, I'd like to get rid of those failing CI runs.
Sure.
Now, regarding the API I'd like to stick with
GetInfo. Actually, the same system functions are used under the hood.
I don't think this is a good argument. The user of the API doesn't really care about how it is implemented. And returning integers as strings is rather gross.
And implementing of handling the new
wxLOCALEsymbols requires no changes to the API ofwxUILocaleandwxLocale. However, we could add a convenience method for retrieving all the currency information in one call and one data structure.
Yes, this is what I proposed with wxLocaleCurrencyInfo wxUILocale::GetCurrencyInfo() in my first comment.
Not sure, whether that convenience method should be added to
wxLocaleat all.
I don't think it needs to be, it's fine to add new functionality only to wxUILocale IMO.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
>
Except for `CRNCYSTR` there is no POSIX equivalent for the other symbols added by this PR. However, using `localeconv()` allows to retrieve the values in question. And if we use `TempLocaleSetter` as in the `wxUILocaleImplUnix::GetLangInfo()` method in conjunction with `localeconv`, it should work for the general case in `wxUILocale`, too.
At the moment, the global static method `wxLocale::GetInfo()` (without `TempLocaleSetter`) is used as a fallback, if the symbols are not available. However, I intend to change that.
The reason that this CI run still fails is that I forgot to check for symbol availability in one place. All other Unix CI runs now already succeed
Ulrich Telle
@utelle pushed 1 commit.
- c848101 Add test with French locale, add missing check
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 65f0d2d Correct test of French locale
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)Or whether you prefer just 2 additional methods, one for the curreny values (symbol, code, number of fractional digits) and one for determining whether the measurement system is metric or not.
I think I'd prefer to have just the functions. They can share the implementation with GetInfo() internally, of course, if this is convenient (although I don't believe it really helps that much).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 932160e Use locale with regions for currency tests
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)Or whether you prefer just 2 additional methods, one for the curreny values (symbol, code, number of fractional digits) and one for determining whether the measurement system is metric or not.
I think I'd prefer to have just the functions. They can share the implementation with
GetInfo()internally, of course, if this is convenient (although I don't believe it really helps that much).
Ok, I will then start to change the implementation in that direction.
BTW, I found out what was causing the failing macOS tests: the tests instantiated locales for en, de, and fr. While Windows and Linux seem to associate a default region to the locale in that case, macOS does not. If asked for locale fr, you get a locale without an associated region. Without region, macOS does not know which currency to select and returns the general currency symbol (¤ == \00A4) and no currency code.
After I changed the tests to instantiate locales en-US, de-DE, and fr-FR, the tests succeeded.
The FreeBSD CI run still fails, but now on running the test. The results returned for currency look strange. At the moment I have no idea, what's going.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)In view of PR #1781, where locale grouping for formatting numbers is added by retrieving the locale information via GetInfo(), and in view that the decimal separator as well as the thousands separator is already implemented via GetInfo(), it seems reasonable to add retrieval of the currency symbol (wxLOCALE_CURRENCY_SYMBOL) and the currency code (wxLOCALE_CURRENCY_CODE) also via GetInfo().
For wxLOCALE_DECIMAL_POINT, wxLOCALE_THOUSANDS_SEP, wxLOCALE_GROUPING the category (wxLOCALE_CAT_NUMBER resp wxLOCALE_CAT_MONEY) should be taken into account. Currently this is only partially implemented in wxWidgets.
The only "problematic" value is the number of fractional digits, because it is from its nature not a string, but an integer number. In theory, this number could be different for ordinary values and monetary values, but in practice there is no known locale in which the values differ.
Under Windows and Linux the system functions deliver a one-character string with values between 0 and 9. However, if you prefer, we can add a separate method instead of using GetInfo(). The name could be GetNumberOfFractionalDigits or shorter GetFractionalDigits or something else.
For determining whether the locale uses the metric system or not, I would add a separate method in any case. Proposed name: bool UsesMetricSystem(). In theory, we could have a 3-state value (yes, no, unknown). But in view that only 3 countries in the world haven't standardized on the metric system as of today, it is a fair assumption that the metric system is in use, if it can't be determined (which should rarely happen).
There is one additional parameter I have not taken into account up to now for monetary values: there exist locale-dependent conventions whether the currency symbol is placed before or after the numerical value, and whether it is separated by a non-breakable space or not.
Last questions:
- Should there be convenience methods that retrieve the number formatting parameters
wxLOCALE_DECIMAL_POINT,wxLOCALE_THOUSANDS_SEP,wxLOCALE_GROUPING, and possibly fractional digits in one go? - If yes for the previous question: Should there be separate methods for ordinary numerical values and monetary values? The latter would then include the curreny symbol, code and position.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)In view of PR #1781, where locale grouping for formatting numbers is added by retrieving the locale information via
GetInfo(), and in view that the decimal separator as well as the thousands separator is already implemented viaGetInfo(), it seems reasonable to add retrieval of the currency symbol (wxLOCALE_CURRENCY_SYMBOL) and the currency code (wxLOCALE_CURRENCY_CODE) also viaGetInfo().
From the point of view that they are strings, I agree. But it doesn't fit GetInfo() that well because they imply using wxLOCALE_CAT_MONEY category. But, as I wrote in the other PR, we can live with this if you prefer to do it like this, after all we already have wxLOCALE_XXX_DATE_FMT that similarly can only be used with only wxLOCALE_CAT_DATE.
In fact, I think all I'm trying to say is that GetInfo() is a badly designed API.
The only "problematic" value is the number of fractional digits, because it is from its nature not a string, but an integer number. In theory, this number could be different for ordinary values and monetary values, but in practice there is no known locale in which the values differ.
This is probably the case for wxLOCALE_THOUSANDS_SEP and wxLOCALE_DECIMAL_POINT too.
Under Windows and Linux the system functions deliver a one-character string with values between
0and9. However, if you prefer, we can add a separate method instead of usingGetInfo(). The name could beGetNumberOfFractionalDigitsor shorterGetFractionalDigitsor something else.
GetFractionalDigits() looks fine to me. I could also live with this being in GetInfo(). But I'd prefer not to.
For determining whether the locale uses the metric system or not, I would add a separate method in any case.
Strongly agree.
Proposed name:
bool UsesMetricSystem(). In theory, we could have a 3-state value (yes,no,unknown). But in view that only 3 countries in the world haven't standardized on the metric system as of today, it is a fair assumption that the metric system is in use, if it can't be determined (which should rarely happen).
I don't like it, it would be weird if a program started using Imperial measurements just because this function returned false due to some error. Let it return a (class) enum as I proposed, then the application will be able to easily implement the fallback from wxMeasurementSystem::Unknown to Metric on its own.
There is one additional parameter I have not taken into account up to now for monetary values: there exist locale-dependent conventions whether the currency symbol is placed before or after the numerical value, and whether it is separated by a non-breakable space or not.
So another candidate for a field in the mooted wxCurrencyInfo class.
BTW, another commonly needed information is the number of currency subdivisions (e.g. 100 for Euro and cents but 1 for Yen). I don't know if it's provided by any of the platform APIs?
Last questions:
* Should there be convenience methods that retrieve the number formatting parameters `wxLOCALE_DECIMAL_POINT`, `wxLOCALE_THOUSANDS_SEP`, `wxLOCALE_GROUPING`, and possibly _fractional digits_ in one go?
Probably not, at least I don't see what do we gain by providing it.
Thanks again for working on this!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)[...] it seems reasonable to add retrieval of the currency symbol (
wxLOCALE_CURRENCY_SYMBOL) and the currency code (wxLOCALE_CURRENCY_CODE) also viaGetInfo().
From the point of view that they are strings, I agree. But it doesn't fit
GetInfo()that well because they imply usingwxLOCALE_CAT_MONEYcategory.
Currency symbol and code are in the category wxLOCALE_CAT_MONEY, of course, but you don't have to specify the category, when calling GetInfo.
However, it is no problem, to implement separate methods, like GetCurrencySymbol() and GetCurrencyCode().
In fact, I think all I'm trying to say is that
GetInfo()is a badly designed API.
Ok, no big deal to remove the wxLOCALE_ symbols for currency symbol and code from the PR. I'll use the method names mentioned above.
In theory, this number could be different for ordinary values and monetary values, but in practice there is no known locale in which the values differ.
This is probably the case for
wxLOCALE_THOUSANDS_SEPandwxLOCALE_DECIMAL_POINTtoo.
Yes, for most locales you are right. But I found at least one, for which glibc defines different values for wxLOCALE_THOUSANDS_SEP, namely de-AT.
GetFractionalDigits()looks fine to me. I could also live with this being inGetInfo(). But I'd prefer not to.
Ok, so I'll use that name.
Proposed name:
bool UsesMetricSystem(). In theory, we could have a 3-state value (yes,no,unknown). But in view that only 3 countries in the world haven't standardized on the metric system as of today, it is a fair assumption that the metric system is in use, if it can't be determined (which should rarely happen).I don't like it, it would be weird if a program started using Imperial measurements just because this function returned
falsedue to some error. Let it return a (class) enum as I proposed, then the application will be able to easily implement the fallback fromwxMeasurementSystem::UnknowntoMetricon its own.
Ok.
There is one additional parameter I have not taken into account up to now for monetary values: there exist locale-dependent conventions whether the currency symbol is placed before or after the numerical value, and whether it is separated by a non-breakable space or not.
So another candidate for a field in the mooted
wxCurrencyInfoclass.
Yes, I think this information should be provided. However, the user should be able to tell the number formatter whether to include the currency symbol or code or not. For example, for input fields it could be inconvenient to have the currency symbol within the field.
BTW, another commonly needed information is the number of currency subdivisions (e.g. 100 for Euro and cents but 1 for Yen). I don't know if it's provided by any of the platform APIs?
Actually, that can be deduced from the number of fractional digits for the currency. For Euro you have 2 fractional digits, for Yen 0.
- Should there be convenience methods that retrieve the number formatting parameters
wxLOCALE_DECIMAL_POINT,wxLOCALE_THOUSANDS_SEP,wxLOCALE_GROUPING, and possibly fractional digits in one go?
Probably not, at least I don't see what do we gain by providing it.
Well, for number formatting you need all this information anyway. So, it would be convenient to get all information with just one call, instead of having to call at least 3 methods.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 3343e8d Rework wxUILocale API (number/currency formatting)
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 6cbeaec Handle unused method, include different macOS header
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)@vadz: I have reworked the wxUILocale API. In the end I removed all new GetInfo symbols and created separate methods to access the currency information.
Additionally, there is now a method to retrieve the information regarding locale-depending formatting of ordinary numbers. This method retrieves all relevant data in one go - especially on macOS this is efficient, because it requires to instantiate a NSNumberFormatter only once.
2 GitHub CI runs currently fail:
- CMake builds / MSW/MSVC wxQt 5.15 - a GUI test fails - IMHO not related to my changes
- Cirrus CI / FreeBSD wxGTK 3 - all currency tests fail.
It's unclear why the tests fail. And it's difficult to find out, because I don't have access to a FreeBSD system. I started to set up a FreeBSD VM, but that is a major task I already spent several hours on.
Nevertheless, I'd like to ask you to review the PR again. If you agree with the API, I will add documentation for the new methods.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)Thanks a lot! I'll have a look a.s.a.p. but I'll be busy with non-programming stuff during the next few days so it might not happen before the next week.
For now I'd just like to say that Cirrus CI allows you to "rerun with a terminal" which gives you a web-based ssh client allowing to explore/debug in a running system, this is pretty useful and even better than tmate.io action on GitHub.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)P.S. And the other failure is #25779 i.e. indeed spurious.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)Thanks a lot! I'll have a look a.s.a.p. but I'll be busy with non-programming stuff during the next few days so it might not happen before the next week.
No problem. I'm quite busy myself.
For now I'd just like to say that Cirrus CI allows you to "rerun with a terminal" which gives you a web-based ssh client allowing to explore/debug in a running system, this is pretty useful and even better than tmate.io action on GitHub.
Is there any documentation available about this feature, and how to actually debug an application. I would need to see, what data exactly are retrieved from the system.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)Is there any documentation available about this feature, and how to actually debug an application. I would need to see, what data exactly are retrieved from the system.
If you have access to it (perhaps you don't?) just select "Re-Run with Terminal Access" from the button menu:
image.png (view on web)and you'll be dropped (once the build starts) into a ssh session on the build machine, from where you can do anything you want, e.g. debug, compile and run test code etc.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- b3b01a8 Use LC_ALL instead of LC_CTYPE
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)Actually, I managed to set up my FreeBSD VM to such a degree that I could use it for compiling and testing wxWidgets.
This allowed me to find out, why the GitHub CI run failed for FreeBSD. The reason was actually quite simple: the structure delivered by localeconv (which has to be used on FreeBSD, because nl_langinfo_l() resp nl_langinfo() is not available) was not properly filled. And that was, because the temporary locale (which has to be used, to make wxUILocale work with any locale) was set with type LC_CTYPE - I had copied the TempLocaleSetter from the initialization code without thinking about the implications of the type parameter. Changing LC_CTYPE to LC_ALL in the relevant methods fixed the problem.
Still 1 GitHub CI run failed - again not due to my changes:
Unix builds / Ubuntu 18.04 wxGTK 3 compatible 3.0 failed with the error message Error: retrieving gpg key timed out during setup.
Since this run worked before, IMHO it is a fair assumption that the PR is now operational.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)Locally, I adjusted the class wxNumberFormatter to take the grouping of digits into account for applying the grouping separator aka thousands separator. No change to the public API so far, because IMHO distinguishing between thousands separator and grouping separator makes no sense - in effect they are synonyms.
The question is whether to make adjustments to wxNumberFormatter part of this PR or not.
Regarding wxNumberFormatter I would like to add style flags to explicitly format currency values:
Style_Currency: currency value without currency symbolStyle_Currency_Local: currency value with local currency symbol (prefix or suffix according to local conventions)Style_Currency_International: currency value with international currency symbol aka ISO4217 currency code (always prefix)
Tests could be adopted more or less from PR #1781.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for all your work on this and sorry for the belated review!
Globally, the least clear thing to me (partially because it is not covered by the tests) is how do we handle digits grouping. AFAICS you use Win32 format for this string which is perhaps less confusing than POSIX one but still not great. I wonder why we can't represent it with a vector<int> containing the group sizes, with some special value (0? -1?) meaning "repeat last size". This is obviously less efficient than a simple string, but we don't care about efficiency that much and I think it would be much more clear.
Also, after looking at the code and the tests, I think that mayb you were right and we only need to have one public function, after all, returning all information at once instead of separate functions for separate parts of it. If you (still?) think this is better, please feel free to only export this function, I believe it would be simpler for you too — and there would be just one function to document, too.
Thanks again!
> @@ -122,6 +122,53 @@ enum wxLocaleForm
wxLOCALE_FORM_ENGLISH
};
+enum class wxMeasurementSystem
+{
+ UnknownSystem,
Minor, but I think wxMeasurementSystem::UnknownSystem is unnecessarily redundant, so I suggest just
- UnknownSystem, + Unknown,
> @@ -122,6 +122,53 @@ enum wxLocaleForm
wxLOCALE_FORM_ENGLISH
};
+enum class wxMeasurementSystem
+{
+ UnknownSystem,
+ Metric,
+ NonMetric
+};
+
+enum class wxCurrencySymbolPosition
It's not really obvious whether having 4 values in the same enum or having 2 values and a separate useSeparator flag is better. I'd be curious to know why did you decide to make this choice?
> + wxLocaleNumberFormatting() = default;
+ wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
+ const wxString& decimalSeparator, int fractionalDigits)
+ : GroupSeparator(groupSeparator), Grouping(grouping),
+ DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
+ {
+ }
Wouldn't it be better to just omit both ctors to make this struct an aggregate (even in C++20 and later)?
> + wxString GroupSeparator; + wxString Grouping; + wxString DecimalSeparator; + int FractionalDigits = 0;
The naming convention here is non-standard for wxWidgets and unless there is some really good reason we should use the usual camelCase instead:
- wxString GroupSeparator; - wxString Grouping; - wxString DecimalSeparator; - int FractionalDigits = 0; + wxString groupSeparator; + wxString grouping; + wxString decimalSeparator; + int fractionalDigits = 0;
> +enum class wxMeasurementSystem
+{
+ UnknownSystem,
+ Metric,
+ NonMetric
+};
+
+enum class wxCurrencySymbolPosition
+{
+ PrefixNoSep,
+ PrefixWithSep,
+ SuffixNoSep,
+ SuffixWithSep
+};
+
+struct WXDLLIMPEXP_BASE wxLocaleNumberFormatting
There are no functions to export from DLL in this struct, so:
⬇️ Suggested change-struct WXDLLIMPEXP_BASE wxLocaleNumberFormatting +struct wxLocaleNumberFormatting
> +struct WXDLLIMPEXP_BASE wxLocaleNumberFormatting
+{
+ wxLocaleNumberFormatting() = default;
+ wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
+ const wxString& decimalSeparator, int fractionalDigits)
+ : GroupSeparator(groupSeparator), Grouping(grouping),
+ DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
+ {
+ }
+ wxString GroupSeparator;
+ wxString Grouping;
+ wxString DecimalSeparator;
+ int FractionalDigits = 0;
+};
+
+struct WXDLLIMPEXP_BASE wxLocaleCurrencyInfo
All the comments for the struct above apply to this one too.
> @@ -60,6 +60,7 @@
#include "wx/osx/core/cfstring.h"
#include <CoreFoundation/CFLocale.h>
#include <CoreFoundation/CFDateFormatter.h>
+ #include <CoreFoundation/CFNumberFormatter.h>
Looks like this is not needed any more
⬇️ Suggested change- #include <CoreFoundation/CFNumberFormatter.h>
> + {
+ return DoGetInfo(LOCALE_SCURRENCY);
+ }
+
+ wxString GetCurrencyCode() const override
+ {
+ return wxString(DoGetInfo(LOCALE_SINTLSYMBOL)).Left(3);
+ }
+
+ wxCurrencySymbolPosition GetCurrencySymbolPosition() const override
+ {
+ static std::array<wxCurrencySymbolPosition, 4> symPos = {
+ wxCurrencySymbolPosition::PrefixNoSep, wxCurrencySymbolPosition::PrefixWithSep,
+ wxCurrencySymbolPosition::SuffixNoSep, wxCurrencySymbolPosition::SuffixWithSep };
+ wxString posStr = wxString(DoGetInfo(LOCALE_ICURRENCY));
+ wxUint32 posIdx = posStr.GetChar(0).GetValue() - wxUniChar('0').GetValue();
We must check that posStr is not empty before using GetChar(0) on it.
> @@ -27,10 +27,13 @@ #include "wx/osx/core/cfref.h" #include "wx/osx/core/cfstring.h" +#import <Foundation/Foundation.h> +#if 0
This shouldn't be in the final version.
> + return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
+ formatter.locale = m_nsloc;
+ formatter.numberStyle = NSNumberFormatterCurrencyStyle;
+
+ NSString* formatted = [formatter stringFromNumber:@123];
+ NSString* symbol = formatter.currencySymbol;
+
+ NSRange symbolRange = [formatted rangeOfString:symbol];
I think all this string manipulation stuff would be much more clear to an average wx programmer if it were written using wxString functions and not Cocoa ones... It's not worth rewriting it, of course, now that it has been already written like this.
> +
+struct WXDLLIMPEXP_BASE wxLocaleCurrencyInfo
+{
+ wxLocaleCurrencyInfo() = default;
+ wxLocaleCurrencyInfo(const wxString& symbol, const wxString& code,
+ const wxCurrencySymbolPosition currencySymbolPos,
+ const wxLocaleNumberFormatting currencyFormat)
+ : CurrencySymbol(symbol), CurrencyCode(code),
+ CurrencySymbolPos(currencySymbolPos),
+ CurrencyFormat(currencyFormat)
+ {
+ }
+ wxString CurrencySymbol; // the currency symbol (for example "$")
+ wxString CurrencyCode; // the currency ISO code (for example "USD")
+ wxCurrencySymbolPosition CurrencySymbolPos = wxCurrencySymbolPosition::PrefixNoSep;
+ wxLocaleNumberFormatting CurrencyFormat;
Not sure if it's better to aggregate it or inherit from it here. Writing currencyInfo.currencyFormat.decimalSeparator is rather heavy and currencyInfo.decimalSeparator would be clear enough, I think.
> @@ -304,6 +306,40 @@ class wxUILocaleImplStdC : public wxUILocaleImpl
return wxLayout_Default;
}
+ wxLocaleNumberFormatting GetNumberFormatting() const override
+ {
+ return wxLocaleNumberFormatting("", "", ".", 2);
Very minor, but I'd write this as
⬇️ Suggested change- return wxLocaleNumberFormatting("", "", ".", 2);
+ return wxLocaleNumberFormatting(wxString{}, wxString{}, ".", 2);
> @@ -304,6 +306,40 @@ class wxUILocaleImplStdC : public wxUILocaleImpl
return wxLayout_Default;
}
+ wxLocaleNumberFormatting GetNumberFormatting() const override
+ {
+ return wxLocaleNumberFormatting("", "", ".", 2);
+ }
+
+ wxString GetCurrencySymbol() const override
+ {
+ return wxString("$");
OTOH I'd omit wxString here, why bother.
> + wxString region = GetLocaleId().GetRegion(); + // In 2025 only in the United States, Liberia, and Myanmar + // the metric system is not the default measurement system. + bool usesMetric = !(region == "US" || region == "LR" || region == "MM"); + return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
I still would just return Unknown here. If you'd like, we could provide this code in a separate function called GuessMetricSystemFromRegion() or something like this. But I'd like to avoid providing wrong information to the program without it even knowing it that it could be wrong.
> + return wxCFStringRef::AsString(str);
+}
+
+wxString
+wxUILocaleImplCF::GetCurrencyCode() const
+{
+ NSString* str = [m_nsloc currencyCode];
+ return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
This has to be released somewhere. Typically this would be done by using wxCFRef for it.
> + unichar before = [formatted characterAtIndex:symbolRange.location - 1];
+ hasSpace = [[NSCharacterSet whitespaceAndNewlineCharacterSet] characterIsMember:before];
+ }
+ symbolPosition = (hasSpace)
+ ? wxCurrencySymbolPosition::SuffixWithSep
+ : wxCurrencySymbolPosition::SuffixNoSep;
+ }
+ }
+
+ return symbolPosition;
+}
+
+wxLocaleNumberFormatting
+wxUILocaleImplCF::DoGetNumberFormatting(wxLocaleCategory cat) const
+{
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
Also needs to be released.
> + {
+ wxString region = GetLocaleId().GetRegion();
+ // In 2025 only in the United States, Liberia, and Myanmar
+ // the metric system is not the default measurement system.
+ bool usesMetric = !(region == "US" || region == "LR" || region == "MM");
+ return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
+ }
Same comment as for wxMSW version.
> + if (val == 0 || val == CHAR_MAX)
+ {
+ lastCharIsZero = (val == CHAR_MAX);
+ break;
+ }
This seems rather unclear to me. I'd write it as
⬇️ Suggested change- if (val == 0 || val == CHAR_MAX)
- {
- lastCharIsZero = (val == CHAR_MAX);
- break;
- }
+ if (val == 0)
+ {
+ lastCharIsZero = false;
+ break;
+ }
+
+ if (val == CHAR_MAX)
+ break;
but it still begs the question of why do we set lastCharIsZero to false if last char is 0. Maybe this variable could be named better?
> + (currencyStr[0] == wxT('+') ||
+ currencyStr[0] == wxT('-') ||
+ currencyStr[0] == wxT('.')))
If we want to use any macros in the new code, we should use wxS and not wxT.
> +wxCurrencySymbolPosition
+wxUILocaleImplUnix::GetCurrencySymbolPosition() const
+{
+ static std::array<wxCurrencySymbolPosition, 4> symPos = {
+ wxCurrencySymbolPosition::PrefixNoSep, wxCurrencySymbolPosition::PrefixWithSep,
+ wxCurrencySymbolPosition::SuffixNoSep, wxCurrencySymbolPosition::SuffixWithSep };
+
+ wxUint32 posIdx = 0;
+#if defined(HAVE_LANGINFO_H) && defined(__GLIBC__)
+ const char* csPrecedes = GetLangInfo(P_CS_PRECEDES);
+ const char* sepBySpace = GetLangInfo(P_SEP_BY_SPACE);
+ posIdx += (csPrecedes[0] == 0) ? 2 : 0;
+ posIdx += (sepBySpace[0] == 0) ? 0 : 1;
+#else
+ wxString symbolPosition = DoGetInfoFromLocaleConv(LOCALE_CONV_CURRENCY_SYM_POS, wxLOCALE_CAT_DEFAULT);
+ posIdx = symbolPosition.GetChar(0).GetValue();
We must also check here that symbolPosition is not empty.
> + wxString region = GetLocaleId().GetRegion();
+ // In 2025 only in the United States, Liberia, and Myanmar
+ // the metric system is not the default measurement system.
+ if (!region.empty())
+ {
+ if (region == "US" || region == "LR" || region == "MM")
+ return wxMeasurementSystem::NonMetric;
+ else
+ return wxMeasurementSystem::Metric;
+ }
+ else
+ return wxMeasurementSystem::UnknownSystem;
Same comment as for the other ports.
> +{
+ // localeconv accesses only the global locale
+ // temporarily set this locale
+ TempLocaleSetter setThisLocale(LC_ALL, m_locId.GetName());
+
+ lconv* const lc = localeconv();
+ if (!lc)
+ return wxString();
+
+ switch (index)
+ {
+ case LOCALE_CONV_THOUSANDS_SEP:
+ {
+ wchar_t thousandsSep[16];
+ if (cat == wxLOCALE_CAT_MONEY)
+ mbstowcs(thousandsSep, lc->mon_thousands_sep, 16);
Why do we need to use mbstowcs() explicitly here instead of just relying on wxConvLibc used when constructing wxString from char*?
> @@ -417,11 +417,35 @@ TEST_CASE("wxUILocale::IsSupported", "[uilocale]")
TEST_CASE("wxUILocale::GetInfo", "[uilocale]")
{
- CHECK( wxUILocale::FromTag("en").GetInfo(wxLOCALE_DECIMAL_POINT) == "." );
+ const wxUILocale locEN(wxUILocale::FromTag("en-US"));
+ CHECK( locEN.GetInfo(wxLOCALE_DECIMAL_POINT) == "." );
+ CHECK( locEN.GetCurrencySymbol() == "$");
+ CHECK( locEN.GetCurrencyCode() == "USD");
+ CHECK( locEN.GetCurrencyInfo().CurrencyFormat.FractionalDigits == 2);
+ CHECK( locEN.GetCurrencySymbolPosition() == wxCurrencySymbolPosition::PrefixNoSep);
+ CHECK( locEN.UsesMetricSystem() == wxMeasurementSystem::NonMetric);
+
+ const wxUILocale locDE(wxUILocale::FromTag("de-DE"));
+ if (CheckSupported(locDE, "German"))
+ {
+ CHECK( locDE.GetInfo(wxLOCALE_DECIMAL_POINT) == ",");
+ CHECK( locDE.GetCurrencySymbol() == L"\u20AC");
I'm almost sure that using
⬇️ Suggested change- CHECK( locDE.GetCurrencySymbol() == L"\u20AC"); + CHECK( locDE.GetCurrencySymbol() == L"€");
should work everywhere nowadays.
I think this is out of date now?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)Thanks for all your work on this and sorry for the belated review!
No problem, I was busy with other things anyway.
The question is whether to make adjustments to
wxNumberFormatterpart of this PR or not.
I'd like to get an answer to the above question. However, please take also another look at the other items in my other post. Thanks.
Globally, the least clear thing to me (partially because it is not covered by the tests) is how do we handle digits grouping.
Currently, only the wxNumberFormatter class would use this information.
AFAICS you use Win32 format for this string which is perhaps less confusing than POSIX one but still not great.
Yes, in that point I followed the other old PR. However, it can be implemented differently, of course. In the first attempt, when method GetLocaleInfo() was used to retrieve the information, a string representation was the right thing. Now, with a dedicated structure for the related items, using a vector may be the better choice.
I wonder why we can't represent it with a
vector<int>containing the group sizes, with some special value (0? -1?) meaning "repeat last size". This is obviously less efficient than a simple string, but we don't care about efficiency that much and I think it would be much more clear.
Sure, in my changes to wxNumberFormatter I actually convert the string to a vector. So, it makes sense to do it immediately on retrieving the information from the operating system.
Also, after looking at the code and the tests, I think that mayb you were right and we only need to have one public function, after all, returning all information at once instead of separate functions for separate parts of it.
Retrieving the currency symbol and code on its own could be useful for the "end user".
If you (still?) think this is better, please feel free to only export this function, I believe it would be simpler for you too — and there would be just one function to document, too.
I have to look into the code again. Of course, I will keep only those functions that are useful on their own.
I will look at your comments to the code details later. Thanks for reviewing.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> @@ -122,6 +122,53 @@ enum wxLocaleForm
wxLOCALE_FORM_ENGLISH
};
+enum class wxMeasurementSystem
+{
+ UnknownSystem,
Minor, but I think
wxMeasurementSystem::UnknownSystemis unnecessarily redundant, so I suggest just
Yes, I'll change that back to just Unknown. The name conflicted with another symbol, before I added the class attribute to the enum.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> @@ -122,6 +122,53 @@ enum wxLocaleForm
wxLOCALE_FORM_ENGLISH
};
+enum class wxMeasurementSystem
+{
+ UnknownSystem,
+ Metric,
+ NonMetric
+};
+
+enum class wxCurrencySymbolPosition
It's not really obvious whether having 4 values in the same enum or having 2 values and a separate
useSeparatorflag is better. I'd be curious to know why did you decide to make this choice?
Well, actually that is how the information is returned from the operating system under Windows. Under Linux/Unix there are 2 different property values. And under macOS the information has to be extracted manually from a formatted string.
I decided to define 4 enum values to be able to return the information in a single value that can be returned as a function value easily.
Currently, I have a separate function returning this value. If we decide to return the information within the currency structure only, then we could split the information into 2 separate values.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> + wxLocaleNumberFormatting() = default;
+ wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
+ const wxString& decimalSeparator, int fractionalDigits)
+ : GroupSeparator(groupSeparator), Grouping(grouping),
+ DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
+ {
+ }
Wouldn't it be better to just omit both ctors to make this struct an aggregate (even in C++20 and later)?
Yes, if you prefer.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> + wxString GroupSeparator; + wxString Grouping; + wxString DecimalSeparator; + int FractionalDigits = 0;
The naming convention here is non-standard for wxWidgets and unless there is some really good reason we should use the usual
camelCaseinstead:
No, there is no special reason. Except that the same convention is used in wxLanguageInfo where all member variable names start with a capital letter. And that was already so in wxWidgets 2.x - long before I started to work on the locale classes.
However, I'll change it for the new structures to use camelCase. It can't be changed for wxLanguageInfo, because it would break backward compatibility.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> +enum class wxMeasurementSystem
+{
+ UnknownSystem,
+ Metric,
+ NonMetric
+};
+
+enum class wxCurrencySymbolPosition
+{
+ PrefixNoSep,
+ PrefixWithSep,
+ SuffixNoSep,
+ SuffixWithSep
+};
+
+struct WXDLLIMPEXP_BASE wxLocaleNumberFormatting
There are no functions to export from DLL in this struct, so:
Ok.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + {
+ return DoGetInfo(LOCALE_SCURRENCY);
+ }
+
+ wxString GetCurrencyCode() const override
+ {
+ return wxString(DoGetInfo(LOCALE_SINTLSYMBOL)).Left(3);
+ }
+
+ wxCurrencySymbolPosition GetCurrencySymbolPosition() const override
+ {
+ static std::array<wxCurrencySymbolPosition, 4> symPos = {
+ wxCurrencySymbolPosition::PrefixNoSep, wxCurrencySymbolPosition::PrefixWithSep,
+ wxCurrencySymbolPosition::SuffixNoSep, wxCurrencySymbolPosition::SuffixWithSep };
+ wxString posStr = wxString(DoGetInfo(LOCALE_ICURRENCY));
+ wxUint32 posIdx = posStr.GetChar(0).GetValue() - wxUniChar('0').GetValue();
We must check that
posStris not empty before usingGetChar(0)on it.
Yes, certainly better to do that.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> @@ -27,10 +27,13 @@ #include "wx/osx/core/cfref.h" #include "wx/osx/core/cfstring.h" +#import <Foundation/Foundation.h> +#if 0
This shouldn't be in the final version.
Right. Will be deleted.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
> + return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
+ formatter.locale = m_nsloc;
+ formatter.numberStyle = NSNumberFormatterCurrencyStyle;
+
+ NSString* formatted = [formatter stringFromNumber:@123];
+ NSString* symbol = formatter.currencySymbol;
+
+ NSRange symbolRange = [formatted rangeOfString:symbol];
I think all this string manipulation stuff would be much more clear to an average wx programmer if it were written using
wxStringfunctions and not Cocoa ones... It's not worth rewriting it, of course, now that it has been already written like this.
I found it simpler to use Cocoa functions instead of converting the strings to wxStrings, because they are only used locally.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> +
+struct WXDLLIMPEXP_BASE wxLocaleCurrencyInfo
+{
+ wxLocaleCurrencyInfo() = default;
+ wxLocaleCurrencyInfo(const wxString& symbol, const wxString& code,
+ const wxCurrencySymbolPosition currencySymbolPos,
+ const wxLocaleNumberFormatting currencyFormat)
+ : CurrencySymbol(symbol), CurrencyCode(code),
+ CurrencySymbolPos(currencySymbolPos),
+ CurrencyFormat(currencyFormat)
+ {
+ }
+ wxString CurrencySymbol; // the currency symbol (for example "$")
+ wxString CurrencyCode; // the currency ISO code (for example "USD")
+ wxCurrencySymbolPosition CurrencySymbolPos = wxCurrencySymbolPosition::PrefixNoSep;
+ wxLocaleNumberFormatting CurrencyFormat;
Not sure if it's better to aggregate it or inherit from it here. Writing
currencyInfo.currencyFormat.decimalSeparatoris rather heavy andcurrencyInfo.decimalSeparatorwould be clear enough, I think.
I prefer to inherit, because the structure wxLocaleNumberFormatting is used for ordinary number formatting and currency value formatting. Thus, it is easier to pass onward the substructure instead of having to fill them from individual member variables.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)The question is whether to make adjustments to
wxNumberFormatterpart of this PR or not.
I'd like to get an answer to the above question. However, please take also another look at the other items in my other post. Thanks.
As usual, I'd prefer to keep this PR as minimal as possible and add the extra functionality later, but if it's easier for you to combine them, we could also do it like this — I don't think it would add that much to the already existing changes.
As for the questions in the linked comment: I don't see why would we need the "International" variant, who would ever use it? If we need to customize the currency symbol, we could just allow passing wxString currency (which could be empty) as a separate parameter, perhaps?
Globally, the least clear thing to me (partially because it is not covered by the tests) is how do we handle digits grouping.
Currently, only the
wxNumberFormatterclass would use this information.
This is a good point, I'm not really sure if anybody else is ever going to use it. But it's conceivable that somebody could, e.g. to align different groups in a column of numbers, perhaps?
Retrieving the currency symbol and code on its own could be useful for the "end user".
Yes, but the overhead of getting the other stuff too is probably not that great.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + wxString region = GetLocaleId().GetRegion(); + // In 2025 only in the United States, Liberia, and Myanmar + // the metric system is not the default measurement system. + bool usesMetric = !(region == "US" || region == "LR" || region == "MM"); + return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
I still would just return
Unknownhere. If you'd like, we could provide this code in a separate function calledGuessMetricSystemFromRegion()or something like this. But I'd like to avoid providing wrong information to the program without it even knowing it that it could be wrong.
Only if the Windows API function GetLocaleInfoEx() fails, it would be necessary to deduce the metric system from the region. That should usually not happen at all. And if it fails, wxLogLastError will be called. So, the user will know that something went wrong.
Almost the whole world agreed on using the metric system, except for the 3 regions mentioned in the code. Therefore it is fair assumption to return Metric. macOS for example uses a boolean for this item, that is, you will never get Unknown on macOS.
Typically, an application would/should offer the user the choice of the measurement system, using the result of this function just as the default.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + wxString region = GetLocaleId().GetRegion(); + // In 2025 only in the United States, Liberia, and Myanmar + // the metric system is not the default measurement system. + bool usesMetric = !(region == "US" || region == "LR" || region == "MM"); + return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
wxLogLastError() is just a debugging helper, the program would never use it to know if anything went wrong.
I agree that returning Unknown would be rare, but it still seems more honest to do it than just trying to guess. And by using a separate function we would at least avoid having the same code in all 3 ports with all the usual maintenance issues due to it (what if Gibraltar becomes independent in 2027 and decides to use imperial system?).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + return wxCFStringRef::AsString(str);
+}
+
+wxString
+wxUILocaleImplCF::GetCurrencyCode() const
+{
+ NSString* str = [m_nsloc currencyCode];
+ return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
This has to be released somewhere. Typically this would be done by using
wxCFReffor it.
Would it be ok to specify autorelease?
NSNumberFormatter* formatter = [[[NSNumberFormatter alloc] init] autorelease];
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + return wxCFStringRef::AsString(str);
+}
+
+wxString
+wxUILocaleImplCF::GetCurrencyCode() const
+{
+ NSString* str = [m_nsloc currencyCode];
+ return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
I'm not sure, it would be in GUI application but this code can, in principle, be used from apps using only wxBase and it doesn't create an autorelease pool AFAIK.
Using smart pointer instead of raw one shouldn't be difficult...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + {
+ wxString region = GetLocaleId().GetRegion();
+ // In 2025 only in the United States, Liberia, and Myanmar
+ // the metric system is not the default measurement system.
+ bool usesMetric = !(region == "US" || region == "LR" || region == "MM");
+ return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
+ }
Same comment as for wxMSW version.
For recent macOS versions (10.0 and above) NSLocale will have usesMetricSystem. macOS 10.0 was released in 2001, IIRC. That is, it will be very unlikely that the fallback code will be executed. And as I argued for Windows already, it is a fair assumption to use Metric if the region is not one of the 3 mentioned in the code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + {
+ wxString region = GetLocaleId().GetRegion();
+ // In 2025 only in the United States, Liberia, and Myanmar
+ // the metric system is not the default measurement system.
+ bool usesMetric = !(region == "US" || region == "LR" || region == "MM");
+ return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
+ }
No, this is not quite right, this method is only available since 10.12 according to the docs. This is still pretty old but we still support 10.10 officially, so we still need it.
And as I also argued before, it would really be best not to have this identical code snippet in 3 places.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)As usual, I'd prefer to keep this PR as minimal as possible and add the extra functionality later, but if it's easier for you to combine them, we could also do it like this — I don't think it would add that much to the already existing changes.
There are only minor changes to wxNumberFormatter. And adding test cases for the grouping feature would be easier based on wxNumberFormatter.
As for the questions in the linked comment: I don't see why would we need the "International" variant, who would ever use it?
Well, I guess almost every banking application would use the international variant. At least my banking app uses EUR only, not €. Additionally, the local currency symbol may not be available in all fonts, but the ASCII characters for the latin alphabet usually are.
If we need to customize the currency symbol, we could just allow passing
wxString currency(which could be empty) as a separate parameter, perhaps?
IMHO it's not such a good idea to allow arbitrary strings for the currency symbol. The wxNumberFormatter will retrieve the information from the current locale. A maybe useful option would be to pass a different locale to wxNumberFormatter.
Currently, only the
wxNumberFormatterclass would use this information.This is a good point, I'm not really sure if anybody else is ever going to use it. But it's conceivable that somebody could, e.g. to align different groups in a column of numbers, perhaps?
Yes, that is a valid use case.
Retrieving the currency symbol and code on its own could be useful for the "end user".
Yes, but the overhead of getting the other stuff too is probably not that great.
Probably not. However, at the moment the methods are already implemented in the PR. But could be removed, of course, if that is preferred.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)As usual, I'd prefer to keep this PR as minimal as possible and add the extra functionality later, but if it's easier for you to combine them, we could also do it like this — I don't think it would add that much to the already existing changes.
OK, let's do this all here then.
As for the questions in the linked comment: I don't see why would we need the "International" variant, who would ever use it?
Well, I guess almost every banking application would use the international variant. At least my banking app uses
EURonly, not€. Additionally, the local currency symbol may not be available in all fonts, but the ASCII characters for the latin alphabet usually are.
Sorry, I had misunderstood you, apparently. I thought "local" would be $ or € or £ and "international" would be always be ¤. But now I think that you mean "symbol" by "local" and "ISO 4217 code" by "international", which seems to be confusing. I'd just call them "symbol" and "code", respectively.
If we need to customize the currency symbol, we could just allow passing
wxString currency(which could be empty) as a separate parameter, perhaps?IMHO it's not such a good idea to allow arbitrary strings for the currency symbol.
Why not? This would allow those who really want it to use ₿ for it...
The
wxNumberFormatterwill retrieve the information from the current locale. A maybe useful option would be to pass a different locale towxNumberFormatter.
... which would be more convenient (and probably more stable) than using Salvador locale for it.
Retrieving the currency symbol and code on its own could be useful for the "end user".
Yes, but the overhead of getting the other stuff too is probably not that great.
Probably not. However, at the moment the methods are already implemented in the PR. But could be removed, of course, if that is preferred.
I don't have any strong opinion, so please do it as you prefer. Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + return wxCFStringRef::AsString(str);
+}
+
+wxString
+wxUILocaleImplCF::GetCurrencyCode() const
+{
+ NSString* str = [m_nsloc currencyCode];
+ return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
I'm not experienced with macOS programming. However, if I include this header:
#import <objc/NSObject.h>
and then use
objc::strong_ptr<NSNumberFormatter> formatter([[NSNumberFormatter alloc] init]);
it should work, I guess.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + return wxCFStringRef::AsString(str);
+}
+
+wxString
+wxUILocaleImplCF::GetCurrencyCode() const
+{
+ NSString* str = [m_nsloc currencyCode];
+ return wxCFStringRef::AsString(str);
+}
+
+wxCurrencySymbolPosition
+wxUILocaleImplCF::GetCurrencySymbolPosition() const
+{
+ wxCurrencySymbolPosition symbolPosition = wxCurrencySymbolPosition::PrefixWithSep;
+
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
I don't have much experience with Cocoa/Obj-C either, which is why I prefer to use wxString and wxCFRef which are pretty simple and clear. As we don't use strong_ptr anywhere in the current wxOSX sources, I'd really prefer not to start using it but stick to wxCFRef.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)I don't think it would add that much to the already existing changes.
OK, let's do this all here then.
It's really not much, because I don't change the wxNumberFormatter API, except for the additional style flags.
However, we will have to discuss, if and how wxNumberFormatter should support using a specific locale instead of just the current locale. And this may lead to API changes.
"International" variant
Sorry, I had misunderstood you, apparently. I thought "local" would be
$or€or£
Correct.
and "international" would be always be
¤. But now I think that you mean "symbol" by "local" and "ISO 4217 code" by "international", which seems to be confusing. I'd just call them "symbol" and "code", respectively.
Well, "international" is the term used elsewhere. "local" = within the country, "international" = across country borders. The ISO 4217 code is usually preferred in international business transactions.
IMHO it's not such a good idea to allow arbitrary strings for the currency symbol.
Why not? This would allow those who really want it to use
₿for it...
If we really want to support locales other than the current and/or arbitrary formatting information, it would be better to have a real wxNumberFormatter class - and not just static methods, where we would have to extend the parameter lists of the static methods.
However, at the moment the methods are already implemented in the PR. But could be removed, of course, if that is preferred.
I don't have any strong opinion, so please do it as you prefer. Thanks!
At the moment I'm still undecided. For now I will apply the agreed modifications.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> @@ -417,11 +417,35 @@ TEST_CASE("wxUILocale::IsSupported", "[uilocale]")
TEST_CASE("wxUILocale::GetInfo", "[uilocale]")
{
- CHECK( wxUILocale::FromTag("en").GetInfo(wxLOCALE_DECIMAL_POINT) == "." );
+ const wxUILocale locEN(wxUILocale::FromTag("en-US"));
+ CHECK( locEN.GetInfo(wxLOCALE_DECIMAL_POINT) == "." );
+ CHECK( locEN.GetCurrencySymbol() == "$");
+ CHECK( locEN.GetCurrencyCode() == "USD");
+ CHECK( locEN.GetCurrencyInfo().CurrencyFormat.FractionalDigits == 2);
+ CHECK( locEN.GetCurrencySymbolPosition() == wxCurrencySymbolPosition::PrefixNoSep);
+ CHECK( locEN.UsesMetricSystem() == wxMeasurementSystem::NonMetric);
+
+ const wxUILocale locDE(wxUILocale::FromTag("de-DE"));
+ if (CheckSupported(locDE, "German"))
+ {
+ CHECK( locDE.GetInfo(wxLOCALE_DECIMAL_POINT) == ",");
+ CHECK( locDE.GetCurrencySymbol() == L"\u20AC");
I started with €, but IIRC I changed that, because one of the CI runs failed when € was used.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> +{
+ // localeconv accesses only the global locale
+ // temporarily set this locale
+ TempLocaleSetter setThisLocale(LC_ALL, m_locId.GetName());
+
+ lconv* const lc = localeconv();
+ if (!lc)
+ return wxString();
+
+ switch (index)
+ {
+ case LOCALE_CONV_THOUSANDS_SEP:
+ {
+ wchar_t thousandsSep[16];
+ if (cat == wxLOCALE_CAT_MONEY)
+ mbstowcs(thousandsSep, lc->mon_thousands_sep, 16);
I will check that as soon as I am working on the Linux/Unix version again.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + {
+ wxString region = GetLocaleId().GetRegion();
+ // In 2025 only in the United States, Liberia, and Myanmar
+ // the metric system is not the default measurement system.
+ bool usesMetric = !(region == "US" || region == "LR" || region == "MM");
+ return (usesMetric) ? wxMeasurementSystem::Metric : wxMeasurementSystem::NonMetric;
+ }
No, this is not quite right, this method is only available since 10.12 according to the docs. This is still pretty old but we still support 10.10 officially, so we still need it.
Yes, you are right.
And as I also argued before, it would really be best not to have this identical code snippet in 3 places.
I already added your proposed method for guessing the measurement system from the region. Unknown will be returned from UsesMetricSystem() if the information couldn't be retrieved reliably from the operating system.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)However, we will have to discuss, if and how
wxNumberFormattershould support using a specific locale instead of just the current locale. And this may lead to API changes.
Maybe it's not worth doing this. wxNumberFormatter was supposed to be a simple way to format numbers in a GUI program and I think we may leave it like this. At the very least, I'd prefer to leave any big changes to it out of this PR.
Well, "international" is the term used elsewhere. "local" = within the country, "international" = across country borders.
It's an interesting point of view, but I don't think it's very common. Surely country codes are used in plenty of financial contexts "locally" too. I really think "currency symbol" and "currency code" are much simpler and completely unambiguous.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)However, we will have to discuss, if and how
wxNumberFormattershould support using a specific locale instead of just the current locale. And this may lead to API changes.Maybe it's not worth doing this.
wxNumberFormatterwas supposed to be a simple way to format numbers in a GUI program and I think we may leave it like this. At the very least, I'd prefer to leave any big changes to it out of this PR.
IMHO the approach with these static wxNumberFormatter methods is highly inefficient, if a large number of values has to be formatted, because the formatting information has to be retrieved over and over again.
However, I agree that changes in that respect should be made in a separate PR.
Well, "international" is the term used elsewhere. "local" = within the country, "international" = across country borders.
It's an interesting point of view, but I don't think it's very common. Surely country codes are used in plenty of financial contexts "locally" too. I really think "currency symbol" and "currency code" are much simpler and completely unambiguous.
Well, names are just names. If it makes you happy, we call them "currency symbol" and "currency code".
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 44c87d3 Apply changes requested by review
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)@vadz:
Building wxWidgets now works in all CI jobs.
However, currently all Mac builds that do perform the tests fail for one specific test case. Namely the test for checking different number formatting for ordinary numeric values and currency values. According to the Unicode CLDR database for the locale de-AT there are different thousands separators. But unfortunately not all systems follow the CLDR specifications. Especially, GLIBC defines the values for the thousands separator almost the other way around than CLDR. Obviously, MacOS follows neither CLDR nor GLIBC.
The question is whether we should keep those tests at all. If we keep them, we need to check for MacOS. However, for me it is difficult to tell how this should be done.
Additionally, 2 AppVeyor runs fail during the tests for formatting according to Indian rules. Looking at the failing tests it seems that the Windows system on which the build takes place belong to those that return the wrong grouping information. A comment in the old PR #1781 reads:
// On some Windows systems the grouping is "3;2" which is incorrect // Skip this test in that case
I guess I have to check for that situation in the test case, too.
Nevertheless, I'd like to ask you to review the code of this PR once again.
P.S.: The documentation is not yet done. I will write it up as soon as the code is finalized.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks for the updates!
Just a few comments left, please let me know if you disagree with them.
Concerning the failing tests, perhaps we could check that the locale functions return the expected data for the locale before continuing with the rest of the tests?
> - wxLocaleCurrencyInfo(const wxString& symbol, const wxString& code,
- const wxCurrencySymbolPosition currencySymbolPos,
- const wxLocaleNumberFormatting currencyFormat)
- : CurrencySymbol(symbol), CurrencyCode(code),
- CurrencySymbolPos(currencySymbolPos),
- CurrencyFormat(currencyFormat)
- {
- }
- wxString CurrencySymbol; // the currency symbol (for example "$")
- wxString CurrencyCode; // the currency ISO code (for example "USD")
- wxCurrencySymbolPosition CurrencySymbolPos = wxCurrencySymbolPosition::PrefixNoSep;
- wxLocaleNumberFormatting CurrencyFormat;
+ wxString currencySymbol; // the currency symbol (for example "$")
+ wxString currencyCode; // the currency ISO code (for example "USD")
+ wxCurrencySymbolPosition currencySymbolPos = wxCurrencySymbolPosition::Prefix;
+ bool hasCurrencySeparator = false;
This is very minor but I think
⬇️ Suggested change- bool hasCurrencySeparator = false; + bool useCurrencySeparator = false;
would be a slightly better name.
More importantly, I wonder what "separator" itself is? Is it always a space? Could it be a non-separable space? Something else? Perhaps we should have wxString separator instead?
> {
- wxLocaleNumberFormatting() = default;
- wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
- const wxString& decimalSeparator, int fractionalDigits)
- : GroupSeparator(groupSeparator), Grouping(grouping),
- DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
- {
- }
- wxString GroupSeparator;
- wxString Grouping;
- wxString DecimalSeparator;
- int FractionalDigits = 0;
+ wxString decimalSeparator;
+ wxString groupSeparator;
+ std::vector<size_t> grouping;
I don't know if this is documented elsewhere, but it would be nice to have a comment explaining how this works, i.e. repeating the last element.
Also, I'd prefer to use int instead of size_t for the vector elements.
>
// Add the sign prefix to a string representing a number without
// the prefix. This is used by ToString().
static void AddSignPrefix(wxString& s, int style);
+ // Add currency symbol or code depending on style
+ static void AddCurrency(wxString& s, int style);
+
+ // Add currency symbol or code depending on style
This comment is wrong, but I am not sure how to correct it, considering that this function doesn't take style at all (maybe it should?).
In include/wx/private/uilocale.h:
> @@ -100,7 +100,7 @@ class wxUILocaleImpl : public wxRefCounter
virtual wxLocaleNumberFormatting GetNumberFormatting() const = 0;
virtual wxString GetCurrencySymbol() const = 0;
virtual wxString GetCurrencyCode() const = 0;
- virtual wxCurrencySymbolPosition GetCurrencySymbolPosition() const = 0;
+ virtual void GetCurrencySymbolPosition(wxCurrencySymbolPosition& position, bool& hasSeparator) const = 0;
You're going to hate, but I think using output parameters like this is really not optimal. What if we have
struct wxCurrencySymbolInfo { wxCurrencySymbolPosition pos; bool useSeparator = false; };
and returned such object from here instead?
In src/common/numformatter.cpp:
> - while ( pos > start + GROUP_LEN ) + size_t groupIndex = 0; + size_t groupLen = grouping[0]; + size_t nextGroupLen = groupLen; + + while (groupLen > 0 && pos > start)
I hope that this code is correct but I'd be lying if I said that I find it obviously correct at a glance. I wonder if using a second string, i.e. copying from s to sWithGrouping char by char inserting separators as necessary could be simpler and more clearly correct?
In src/common/numformatter.cpp:
> +
+void wxNumberFormatter::RemoveCurrency(wxString& s)
+{
+ // The currency symbol or code can be a prefix or a suffix of the
+ // given string. Therefore we remove all characters from the head
+ // and the tail of the string that do not belong to a valid number.
+
+ // Every valid number starts with one of the characters in startChars
+ const wxString startChars = "+-0123456789";
+
+ // Every valid number ends with one of the characters in finalChars
+ const wxString finalChars = "0123456789.";
+
+ // Find start and final position of the number
+ size_t start = s.find_first_of(startChars);
+ size_t final = s.find_last_of(finalChars);
Probably a bad idea to use a keyword, even if it's a contextual one, for the variable name, e.g. it's mishighlighted as a keyword here. Could you please rename it to finalPos (and then the other one to startPos for consistency)?
> + position = (posIdx % 2 == 0) + ? wxCurrencySymbolPosition::Prefix + : wxCurrencySymbolPosition::Suffix; + hasSeparator = (((posIdx / 2) % 2) == 1);
Again, this does look correct but I think doing a simple switch ( posIdx ) with 4 cases would have been much more clear.
> wxString posStr = wxString(DoGetInfo(LOCALE_ICURRENCY));
- wxUint32 posIdx = posStr.GetChar(0).GetValue() - wxUniChar('0').GetValue();
- return (posIdx < symPos.size()) ? symPos[posIdx] : wxCurrencySymbolPosition::PrefixWithSep;
+ wxUint32 posIdx = (!posStr.empty()) ? posStr.GetChar(0).GetValue() - wxUniChar('0').GetValue() : 1;
Thinking more about this, I'd add DoGetInfoAsNumber() that would call GetLocaleInfoEx() with LOCALE_RETURN_NUMBER flag rather than doing this.
> @@ -963,7 +940,8 @@ wxUILocaleImplUnix::UsesMetricSystem() const
return wxMeasurementSystem::Metric;
else if (!measureStr.empty() && measureStr[0].GetValue() == 2)
return wxMeasurementSystem::NonMetric;
-#endif
+#else
+ // Fallback for Unix systems without GLIBC
This should use the new GuessXXX() function too.
> + wxLocaleNumberFormatting numForm; + numForm.decimalSeparator = decimalSeparator; + numForm.groupSeparator = groupSeparator; + numForm.grouping = grouping; + numForm.fractionalDigits = fractionalDigits; + return numForm;
I'm not sure why did this have to be changed (here and elsewhere), does constructing it from the initializer list not work any more? The intention of making it an aggregate was to allow doing just return { .... }.
Or do you just think this is more clear?
> if (cat == wxLOCALE_CAT_MONEY) - mbstowcs(thousandsSep, lc->mon_thousands_sep, 16); + return wxString(lc->mon_thousands_sep, wxCSConv(GetCodeSet()));
I still don't understand why do we need to use anything else than (default) wxConvLibc here, we've just changed locale to use m_locId so AFAICS it should work?
If we really need to use a custom conversion, I'd construct it once before the switch instead of doing it in every case.
> CHECK( locEN.GetInfo(wxLOCALE_DECIMAL_POINT) == "." );
CHECK( locEN.GetCurrencySymbol() == "$");
CHECK( locEN.GetCurrencyCode() == "USD");
- CHECK( locEN.GetCurrencyInfo().CurrencyFormat.FractionalDigits == 2);
- CHECK( locEN.GetCurrencySymbolPosition() == wxCurrencySymbolPosition::PrefixNoSep);
+ CHECK( locEN.GetCurrencyInfo().currencyFormat.fractionalDigits == 2);
+ CHECK( (currencyPosition == wxCurrencySymbolPosition::Prefix && !currencyHasSeparator) );
This should be broken in 2 separate CHECKs because if this one fails we won't know which comparison was false.
Also below.
In tests/strings/numformatter.cpp:
> +
+ wxDECLARE_NO_COPY_CLASS(CurrencyFormatterTestCase);
+};
+
+// ----------------------------------------------------------------------------
+// tests themselves
+// ----------------------------------------------------------------------------
+
+TEST_CASE_METHOD(CurrencyFormatterTestCase, "CurrencyFormatterTestCase::NumericValuesToString", "[numformatter]")
+{
+ if (!CanRunTest())
+ return;
+
+#ifdef __GLIBC__
+ CHECK( wxString::FromUTF8("12.345.678") == wxNumberFormatter::ToString(12345678L, wxNumberFormatter::Style_WithThousandsSep));
+ CHECK( wxString::FromUTF8("12\u202F345\u202F678") == wxNumberFormatter::ToString(12345678L, wxNumberFormatter::Style_Currency|wxNumberFormatter::Style_WithThousandsSep));
This is really unreadable :-( Could we have at least something like
#define NNBSP "\xe2\x80\xaf"
and then use it as
wxString::FromUTF8("12" NNBSP "345" NNBSP "678");
?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
I will apply the changes soon, but most likely not before Monday.
> + unichar before = [formatted characterAtIndex:symbolRange.location - 1];
+ hasSpace = [[NSCharacterSet whitespaceAndNewlineCharacterSet] characterIsMember:before];
+ }
+ symbolPosition = (hasSpace)
+ ? wxCurrencySymbolPosition::SuffixWithSep
+ : wxCurrencySymbolPosition::SuffixNoSep;
+ }
+ }
+
+ return symbolPosition;
+}
+
+wxLocaleNumberFormatting
+wxUILocaleImplCF::DoGetNumberFormatting(wxLocaleCategory cat) const
+{
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
I added explicitly
[formatter release]
This approach is used at several other places in macOS code.
Yes. As said elsewhere I will update the documentation when the code is finalized.
> - wxLocaleCurrencyInfo(const wxString& symbol, const wxString& code,
- const wxCurrencySymbolPosition currencySymbolPos,
- const wxLocaleNumberFormatting currencyFormat)
- : CurrencySymbol(symbol), CurrencyCode(code),
- CurrencySymbolPos(currencySymbolPos),
- CurrencyFormat(currencyFormat)
- {
- }
- wxString CurrencySymbol; // the currency symbol (for example "$")
- wxString CurrencyCode; // the currency ISO code (for example "USD")
- wxCurrencySymbolPosition CurrencySymbolPos = wxCurrencySymbolPosition::PrefixNoSep;
- wxLocaleNumberFormatting CurrencyFormat;
+ wxString currencySymbol; // the currency symbol (for example "$")
+ wxString currencyCode; // the currency ISO code (for example "USD")
+ wxCurrencySymbolPosition currencySymbolPos = wxCurrencySymbolPosition::Prefix;
+ bool hasCurrencySeparator = false;
This is very minor but I think
useCurrencySeparatorwould be a slightly better name.
Ok.
More importantly, I wonder what "separator" itself is? Is it always a space? Could it be a non-separable space? Something else? Perhaps we should have
wxString separatorinstead?
Well, the currency separator is not explicitly defined in the locale data. The descriptions always refer to a space.
> {
- wxLocaleNumberFormatting() = default;
- wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
- const wxString& decimalSeparator, int fractionalDigits)
- : GroupSeparator(groupSeparator), Grouping(grouping),
- DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
- {
- }
- wxString GroupSeparator;
- wxString Grouping;
- wxString DecimalSeparator;
- int FractionalDigits = 0;
+ wxString decimalSeparator;
+ wxString groupSeparator;
+ std::vector<size_t> grouping;
I don't know if this is documented elsewhere, but it would be nice to have a comment explaining how this works, i.e. repeating the last element.
Ok, I will add a comment describing how the grouping is specified.
Also, I'd prefer to use
intinstead ofsize_tfor the vector elements.
Why? For example, all position parameters in wxString methods have the type size_t. So, it seemed only logical to use size_t here, too, to avoid conversions.
>
// Add the sign prefix to a string representing a number without
// the prefix. This is used by ToString().
static void AddSignPrefix(wxString& s, int style);
+ // Add currency symbol or code depending on style
+ static void AddCurrency(wxString& s, int style);
+
+ // Add currency symbol or code depending on style
You are right, the comment needs to be adjusted.
At the places where this function is called, the style parameter is not available. The methods FromString() work independent of style. Therefore RemoveCurrency() will simply remove any non-numeric prefix and suffix from the string given, so that the remaining string can be converted to int or double and so on.
In include/wx/private/uilocale.h:
> @@ -100,7 +100,7 @@ class wxUILocaleImpl : public wxRefCounter
virtual wxLocaleNumberFormatting GetNumberFormatting() const = 0;
virtual wxString GetCurrencySymbol() const = 0;
virtual wxString GetCurrencyCode() const = 0;
- virtual wxCurrencySymbolPosition GetCurrencySymbolPosition() const = 0;
+ virtual void GetCurrencySymbolPosition(wxCurrencySymbolPosition& position, bool& hasSeparator) const = 0;
You're going to hate,
A strong word, but I admit that I would have preferred my first approach using just an enum with values for the 4 combinations.
but I think using output parameters like this is really not optimal. What if we have
struct wxCurrencySymbolInfo { wxCurrencySymbolPosition pos; bool useSeparator = false; };and returned such object from here instead?
If you prefer that approach. However, the name wxCurrencySymbolInfo is somewhat misleading, because info is not about the symbol itself, but how and where it should be placed in the formatted currency string. IMHO wxCurrencySymbolPositionInfo suits better - although it is a rather long name.
In src/common/numformatter.cpp:
> - while ( pos > start + GROUP_LEN ) + size_t groupIndex = 0; + size_t groupLen = grouping[0]; + size_t nextGroupLen = groupLen; + + while (groupLen > 0 && pos > start)
I can add some more comments describing the steps taken, but doing it char by char is really inefficient and not necessarily more clear. Actually, the algorithm is straight forward. And not much different than before. Just the possible change of the group length had to be added.
In src/common/numformatter.cpp:
> +
+void wxNumberFormatter::RemoveCurrency(wxString& s)
+{
+ // The currency symbol or code can be a prefix or a suffix of the
+ // given string. Therefore we remove all characters from the head
+ // and the tail of the string that do not belong to a valid number.
+
+ // Every valid number starts with one of the characters in startChars
+ const wxString startChars = "+-0123456789";
+
+ // Every valid number ends with one of the characters in finalChars
+ const wxString finalChars = "0123456789.";
+
+ // Find start and final position of the number
+ size_t start = s.find_first_of(startChars);
+ size_t final = s.find_last_of(finalChars);
Sure. No big deal.
> + position = (posIdx % 2 == 0) + ? wxCurrencySymbolPosition::Prefix + : wxCurrencySymbolPosition::Suffix; + hasSeparator = (((posIdx / 2) % 2) == 1);
Using switch is overkill, but I can use the below code (instead of the bit magic):
position = (posIdx == 0 || posIdx == 2) ? wxCurrencySymbolPosition::Prefix : wxCurrencySymbolPosition::Suffix; hasSeparator = (posIdx == 2 || posIdx == 3);
> wxString posStr = wxString(DoGetInfo(LOCALE_ICURRENCY));
- wxUint32 posIdx = posStr.GetChar(0).GetValue() - wxUniChar('0').GetValue();
- return (posIdx < symPos.size()) ? symPos[posIdx] : wxCurrencySymbolPosition::PrefixWithSep;
+ wxUint32 posIdx = (!posStr.empty()) ? posStr.GetChar(0).GetValue() - wxUniChar('0').GetValue() : 1;
I think I stay with the string return value. GetLocaleInfoEx() can fail, in which case an empty string will be returned. from DoGetInfo(). If a number is returned it would be necessary to define some special value to be able to detect the failure. However, I can use wxString::ToInt() to convert the result to a number.
> @@ -963,7 +940,8 @@ wxUILocaleImplUnix::UsesMetricSystem() const
return wxMeasurementSystem::Metric;
else if (!measureStr.empty() && measureStr[0].GetValue() == 2)
return wxMeasurementSystem::NonMetric;
-#endif
+#else
+ // Fallback for Unix systems without GLIBC
Currently, the GuessXXX() function is a member function of wxUILocale. However, the wxUILocaleImplXXX classes have no means to call a member function of wxUILocale. However, I can change the method to a static method with a wxLocaleIdent parameter.
> + wxLocaleNumberFormatting numForm; + numForm.decimalSeparator = decimalSeparator; + numForm.groupSeparator = groupSeparator; + numForm.grouping = grouping; + numForm.fractionalDigits = fractionalDigits; + return numForm;
I'm not sure why did this have to be changed (here and elsewhere), does constructing it from the initializer list not work any more?
Right. Unfortunately, several CI runs failed to compile the version with the inizializer list. The error messages reported a missing constructor.
The intention of making it an aggregate was to allow doing just
return { .... }.
I know. And I started that way. But it didn't work for all compilers.
Or do you just think this is more clear?
No. The alternative would be to define a constructor.
> if (cat == wxLOCALE_CAT_MONEY) - mbstowcs(thousandsSep, lc->mon_thousands_sep, 16); + return wxString(lc->mon_thousands_sep, wxCSConv(GetCodeSet()));
I still don't understand why do we need to use anything else than (default)
wxConvLibchere, we've just changed locale to usem_locIdso AFAICS it should work?
No, not in the general case. It would work for the locale set by wxUILocale::UseDefault(). However, you can instantiate other instances of wxUILocale without changing the C locale. And then the charset is not necessarily the same.
If we really need to use a custom conversion, I'd construct it once before the
switchinstead of doing it in everycase.
Well, the conversion will be done exactly once per call of DoGetInfoFromLocaleConv. That is, it will be constructed only once per call.
> CHECK( locEN.GetInfo(wxLOCALE_DECIMAL_POINT) == "." );
CHECK( locEN.GetCurrencySymbol() == "$");
CHECK( locEN.GetCurrencyCode() == "USD");
- CHECK( locEN.GetCurrencyInfo().CurrencyFormat.FractionalDigits == 2);
- CHECK( locEN.GetCurrencySymbolPosition() == wxCurrencySymbolPosition::PrefixNoSep);
+ CHECK( locEN.GetCurrencyInfo().currencyFormat.fractionalDigits == 2);
+ CHECK( (currencyPosition == wxCurrencySymbolPosition::Prefix && !currencyHasSeparator) );
A single check was enough in my first implementation using 4 wxCurrencySymbolPosition values incorporating the use of a separator.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 29ac14e Apply changes from last review
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> {
- wxLocaleNumberFormatting() = default;
- wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
- const wxString& decimalSeparator, int fractionalDigits)
- : GroupSeparator(groupSeparator), Grouping(grouping),
- DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
- {
- }
- wxString GroupSeparator;
- wxString Grouping;
- wxString DecimalSeparator;
- int FractionalDigits = 0;
+ wxString decimalSeparator;
+ wxString groupSeparator;
+ std::vector<size_t> grouping;
size_t was a mistake, basically all C++ experts recognize it by now (and "now" means at least a decade already). wxString has to use it for compatibility with std::string, but here we don't need to do it.
I think there will be fewer casts needed, if any, not more if you switch to int here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + unichar before = [formatted characterAtIndex:symbolRange.location - 1];
+ hasSpace = [[NSCharacterSet whitespaceAndNewlineCharacterSet] characterIsMember:before];
+ }
+ symbolPosition = (hasSpace)
+ ? wxCurrencySymbolPosition::SuffixWithSep
+ : wxCurrencySymbolPosition::SuffixNoSep;
+ }
+ }
+
+ return symbolPosition;
+}
+
+wxLocaleNumberFormatting
+wxUILocaleImplCF::DoGetNumberFormatting(wxLocaleCategory cat) const
+{
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
Yes, it is, and it's not great because it will still leak if we ever add a premature return (maybe hidden inside wxCHECK macro) or an exception to this code. But let's leave it like this, I'll change it to use wxCFRef before merging if I have time/energy to do it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + wxLocaleNumberFormatting numForm; + numForm.decimalSeparator = decimalSeparator; + numForm.groupSeparator = groupSeparator; + numForm.grouping = grouping; + numForm.fractionalDigits = fractionalDigits; + return numForm;
Sorry for misleading you into removing the ctor :-( I guess if we can't use the initializer list, it should be restored.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
> if (cat == wxLOCALE_CAT_MONEY) - mbstowcs(thousandsSep, lc->mon_thousands_sep, 16); + return wxString(lc->mon_thousands_sep, wxCSConv(GetCodeSet()));
I still don't understand why do we need to use anything else than (default)
wxConvLibchere, we've just changed locale to usem_locIdso AFAICS it should work?No, not in the general case. It would work for the locale set by
wxUILocale::UseDefault(). However, you can instantiate other instances ofwxUILocalewithout changing the C locale. And then the charset is not necessarily the same.
Sorry to insist but there is TempLocaleSetter just above. Doesn't it set locale to exactly this locale?
If we really need to use a custom conversion, I'd construct it once before the
switchinstead of doing it in everycase.Well, the conversion will be done exactly once per call of
DoGetInfoFromLocaleConv. That is, it will be constructed only once per call.
Yes, but it's still more code than just constructing it once.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In include/wx/private/uilocale.h:
> @@ -100,7 +100,7 @@ class wxUILocaleImpl : public wxRefCounter
virtual wxLocaleNumberFormatting GetNumberFormatting() const = 0;
virtual wxString GetCurrencySymbol() const = 0;
virtual wxString GetCurrencyCode() const = 0;
- virtual wxCurrencySymbolPosition GetCurrencySymbolPosition() const = 0;
+ virtual void GetCurrencySymbolPosition(wxCurrencySymbolPosition& position, bool& hasSeparator) const = 0;
Sorry, I hadn't realized that splitting the enum in 2 would lead to using output parameters. If the choice is between using an enum with 4 values and the version with the output parameters, I think the enum with 4 values is better.
If you decide to add a struct containing both fields, I think calling it "position info" would be wrong as separator is not part of the position. Maybe just wxCurrencySymbolPlacement?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle commented on this pull request.
> + unichar before = [formatted characterAtIndex:symbolRange.location - 1];
+ hasSpace = [[NSCharacterSet whitespaceAndNewlineCharacterSet] characterIsMember:before];
+ }
+ symbolPosition = (hasSpace)
+ ? wxCurrencySymbolPosition::SuffixWithSep
+ : wxCurrencySymbolPosition::SuffixNoSep;
+ }
+ }
+
+ return symbolPosition;
+}
+
+wxLocaleNumberFormatting
+wxUILocaleImplCF::DoGetNumberFormatting(wxLocaleCategory cat) const
+{
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
I would change the code to using wxCFRef, if I knew what exactly I would have to do.
Would it be enough to write wxCFRef<NSNumberFormatter*> instead of NSNumberFormatter*? Or what else needs to be done?
> {
- wxLocaleNumberFormatting() = default;
- wxLocaleNumberFormatting(const wxString& groupSeparator, const wxString& grouping,
- const wxString& decimalSeparator, int fractionalDigits)
- : GroupSeparator(groupSeparator), Grouping(grouping),
- DecimalSeparator(decimalSeparator), FractionalDigits(fractionalDigits)
- {
- }
- wxString GroupSeparator;
- wxString Grouping;
- wxString DecimalSeparator;
- int FractionalDigits = 0;
+ wxString decimalSeparator;
+ wxString groupSeparator;
+ std::vector<size_t> grouping;
I'll change it to int.
In include/wx/private/uilocale.h:
> @@ -100,7 +100,7 @@ class wxUILocaleImpl : public wxRefCounter
virtual wxLocaleNumberFormatting GetNumberFormatting() const = 0;
virtual wxString GetCurrencySymbol() const = 0;
virtual wxString GetCurrencyCode() const = 0;
- virtual wxCurrencySymbolPosition GetCurrencySymbolPosition() const = 0;
+ virtual void GetCurrencySymbolPosition(wxCurrencySymbolPosition& position, bool& hasSeparator) const = 0;
Sorry, I hadn't realized that splitting the enum in 2 would lead to using output parameters. If the choice is between using an enum with 4 values and the version with the output parameters, I think the enum with 4 values is better.
I'll revert to using the enum with 4 values.
> + wxLocaleNumberFormatting numForm; + numForm.decimalSeparator = decimalSeparator; + numForm.groupSeparator = groupSeparator; + numForm.grouping = grouping; + numForm.fractionalDigits = fractionalDigits; + return numForm;
Sorry for misleading you into removing the ctor :-( I guess if we can't use the initializer list, it should be restored.
As said I tried to use initializer lists. It compiled locally under Windows and Linux, but some of the GitHub CI runs failed with compile time errors complaining about missing matching ctors.
I'll revert to using ctors.
> if (cat == wxLOCALE_CAT_MONEY) - mbstowcs(thousandsSep, lc->mon_thousands_sep, 16); + return wxString(lc->mon_thousands_sep, wxCSConv(GetCodeSet()));
You are right: in function DoGetInfoFromLocaleConv() TempLocaleSetter is used, because the whole function is a fallback method if GLIBC is not available or HAVE_LANGINFO_H is not defined.
I'll use wxConvLibc. It should work.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> + unichar before = [formatted characterAtIndex:symbolRange.location - 1];
+ hasSpace = [[NSCharacterSet whitespaceAndNewlineCharacterSet] characterIsMember:before];
+ }
+ symbolPosition = (hasSpace)
+ ? wxCurrencySymbolPosition::SuffixWithSep
+ : wxCurrencySymbolPosition::SuffixNoSep;
+ }
+ }
+
+ return symbolPosition;
+}
+
+wxLocaleNumberFormatting
+wxUILocaleImplCF::DoGetNumberFormatting(wxLocaleCategory cat) const
+{
+ NSNumberFormatter* formatter = [[NSNumberFormatter alloc] init];
Yes, this should be enough (and the explicit call to release must be removed).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
> if (cat == wxLOCALE_CAT_MONEY) - mbstowcs(thousandsSep, lc->mon_thousands_sep, 16); + return wxString(lc->mon_thousands_sep, wxCSConv(GetCodeSet()));
Note that wxConvLibc is implicit anyhow, so you don't need to use it explicitly (although you can do it, of course, if you find it more clear).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- d3610f0 Revert to ctors, and to 4-value symbol position
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
utelle left a comment (wxWidgets/wxWidgets#25765)@vadz
Obviously, using just wxCFRef<NSNumberFormatter*> formatter = [[NSNumberFormatter alloc] init]; doesn't work. All mac builds fail with compile time errors. The compiler complains about unaccessible members. So, would something like the following work?
wxCFRef<NSNumberFormatter*> cfRefFormatter = [[NSNumberFormatter alloc] init]; NSNumberFormatter* formatter = cfRefFormatter;
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Ulrich Telle
@utelle pushed 1 commit.
- 8f6ef05 Attempt to fix macOS code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)Sorry, just to check: are there any more changes planned 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
utelle left a comment (wxWidgets/wxWidgets#25765)Sorry, just to check: are there any more changes planned here?
Not really. IMHO the added wxUILocale methods allow to retrieve the locale-dependent information relevant for formatting ordinary numeric values and currency values. Additionally, wxNumberFormatter was enhanced to apply locale-dependent grouping rules and to format currency values.
Regarding wxNumberFormatter there are of course conceivable extensions, but from my point of view they would make sense only, if wxNumberFormatter is a class and not just a bunch of static methods. Certainly not anything to be added to this PR.
The only point to clarify is whether the documentation I added is enough.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#25765)Thanks for confirmation, I'll try to rereview and merge this a.s.a.p. then (hopefully this Saturday).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.