organicmaps/organicmaps-tmp
Archived
13
Fork 1
Code Issues 2.7k Pull requests 182 Projects Releases 65 Wiki Activity Actions

Set decimal separator depending on locale #4029

Merged
root merged 1 commit from decimal into master 2023-09-19 08:01:30 +00:00
Conversation 11 Commits 1 Files changed 15 +314 -111
gpesquero commented 2022-12-05 00:27:06 +00:00
Member
Copy link

Signed-off-by: Gonzalo Pesquero gpesquero@yahoo.es

Proposal for setting the decimal separator based on system locale for distances in some controls and text views of the app in Android, as suggested in issue 3992.

Checking the code, I've noticed that all the displayed distances come as strings from jni calls, so the proposed solution just replaces the decimal dot '.' with the system locale separator (commonly a comma ',') when applicable.

This is just a first approach for only a few controls and panels and it has been correctly tested in a Android simulator with different system languages.

The proposed approach may not be the best solution, so any comment or feedback is welcomed. This PR may be completed later to implement the remaining controls/views.

Signed-off-by: Gonzalo Pesquero <gpesquero@yahoo.es> Proposal for setting the decimal separator based on system locale for distances in some controls and text views of the app in Android, as suggested in [issue 3992](https://git.omaps.dev/organicmaps/organicmaps/issues/3992). Checking the code, I've noticed that all the displayed distances come as strings from jni calls, so the proposed solution just replaces the decimal dot '.' with the system locale separator (commonly a comma ',') when applicable. This is just a first approach for only a few controls and panels and it has been correctly tested in a Android simulator with different system languages. The proposed approach may not be the best solution, so any comment or feedback is welcomed. This PR may be completed later to implement the remaining controls/views.
❤️ 1
biodranik (Migrated from github.com) requested changes 2022-12-05 06:38:38 +00:00
biodranik (Migrated from github.com) left a comment

Thanks! This solution does not fix the core issue, and won't work on all platforms, including iOS.

A proper fix should be done in C++ code that generates these strings. It should be easy now as you already identified the source of these strings.

Thanks! This solution does not fix the core issue, and won't work on all platforms, including iOS. A proper fix should be done in C++ code that generates these strings. It should be easy now as you already identified the source of these strings.
gpesquero commented 2022-12-05 07:37:01 +00:00
Author
Member
Copy link

Thanks @biodranik for your fast response... Your comment makes a lot of sense, so I will have a look at the C++ code and check how we can implement it.

Thanks @biodranik for your fast response... Your comment makes a lot of sense, so I will have a look at the C++ code and check how we can implement it.
gpesquero commented 2022-12-06 22:45:53 +00:00
Author
Member
Copy link

Hello @biodranik... Checking the C++ code it looks like all the distance/speed strings used for routing and search are generated in the same place: measurement_util.cpp.

I've updated the commit with a new approach, changing the ToStringPrecision() function and getting the decimal separator char from the Locale struct, which I've also changed.

I've tested these changes with the Android emulator on different languages, and everything seems to work Ok, but I don't know if it would also work on ios.

Again, most probably this solution may not be optimal, so any other proposal is welcomed...

Hello @biodranik... Checking the C++ code it looks like all the distance/speed strings used for routing and search are generated in the same place: `measurement_util.cpp`. I've updated the commit with a new approach, changing the `ToStringPrecision()` function and getting the decimal separator char from the Locale struct, which I've also changed. I've tested these changes with the Android emulator on different languages, and everything seems to work Ok, but I don't know if it would also work on ios. Again, most probably this solution may not be optimal, so any other proposal is welcomed...
biodranik (Migrated from github.com) reviewed 2022-12-06 22:56:23 +00:00
biodranik (Migrated from github.com) left a comment

The bicycle was already invented, did you check std::ios_base::imbue?

The bicycle was already invented, did you check [std::ios_base::imbue](https://en.cppreference.com/w/cpp/io/basic_ios/imbue)?
gpesquero commented 2022-12-07 00:06:01 +00:00
Author
Member
Copy link

Yes, I checked imbue, but I could not get it working (most probably due to my low knowledge/experience with cpp)

The code below does compile, but it crashes....

string ToStringPrecision(double d, int pr)
{
  stringstream ss;
  ss.imbue(std::locale("es_ES.UTF8"));
  ss << setprecision(pr) << fixed << d;
  return ss.str();
}
Yes, I checked `imbue`, but I could not get it working (most probably due to my low knowledge/experience with cpp) The code below does compile, but it crashes.... ``` string ToStringPrecision(double d, int pr) { stringstream ss; ss.imbue(std::locale("es_ES.UTF8")); ss << setprecision(pr) << fixed << d; return ss.str(); } ```
biodranik commented 2022-12-07 10:17:36 +00:00 (Migrated from github.com)
Copy link

Yeah, sorry, I forgot that NDK does not support C/C++ locales.

Then we have several possible options:

  1. The one you implemented. It creates some overhead though.
  2. Store the bool flag for dot (or command) in our C++ locale wrapper (pass it from Android and iOS if it also doesn't support locales), make sure that it re-initializes when the device's locale changes, and use that flag internally in the C++ code for a proper choice.
  3. Pass raw values to Android and iOS code and format them there instead of C++.

Let's evaluate and compare pros and cons of all options. Need to test it on iOS too.

Yeah, sorry, I forgot that NDK does not support C/C++ locales. Then we have several possible options: 1. The one you implemented. It creates some overhead though. 2. Store the bool flag for dot (or command) in our C++ locale wrapper (pass it from Android and iOS if it also doesn't support locales), make sure that it re-initializes when the device's locale changes, and use that flag internally in the C++ code for a proper choice. 3. Pass raw values to Android and iOS code and format them there instead of C++. Let's evaluate and compare pros and cons of all options. Need to test it on iOS too.
gpesquero commented 2022-12-12 01:10:49 +00:00
Author
Member
Copy link

Hello again... I've managed to get an iOS dev environment working and I've confirmed that the ss.imbue(std::locale("es_ES.UTF-8")); line does work on iOS and it provides localized strings.

For the pros and cons analysis, I think that we shall also take into consideration the implementation of the conversion of the thousands separator.

From my point of view, these are the pros and cons if we want to keep the locale conversion in the CPP side (options 1 & 2):

Opt. 1&2 pros:

  • Conversion done in a single point (no need to make conversion calls all around the UI for both Android/iOS).
  • For iOS, it can be directly done in CPP using std locale calls. In CPP in iOS, thousands separator will also be directly done.

Opt. 1&2 cons:

  • Locale does not work for Android NDK. Need to make "manual" conversions (as done in option 1) and differently from iOS CPP code.
  • Thousand separator shall also be done "manually" for Android NDK.
  • Need to pass system locale to CPP code and store in locale wrapper class.
  • Need to implement a method to detect system locales changes and update info in wrapper class (Is this really necessary? A system locale change is rarely done, so if the user has to re-start the app in this situation, I don't think that I'd be a big issue...)

And these are the pros and cons for option 3 (perform conversion in Android/iOS side):

Opt. 3 pros:

  • No need to store system locale in CPP wrapper class.
  • System locale changes are automatically detected.

Opt. 3 cons:

  • Need to convert strings in many different places in the Android/iOS code.
  • Need to convert strings containing distances and speed received from CPP code in different formats (sometimes these strings contain only numbers, and sometimes they also include units as km, miles, km/h).

Please fell free to add or comment all these pros and cons...

Hello again... I've managed to get an iOS dev environment working and I've confirmed that the `ss.imbue(std::locale("es_ES.UTF-8"));` line does work on iOS and it provides localized strings. For the pros and cons analysis, I think that we shall also take into consideration the implementation of the conversion of the thousands separator. From my point of view, these are the pros and cons if we want to keep the locale conversion in the CPP side (options 1 & 2): **Opt. 1&2 pros:** - Conversion done in a single point (no need to make conversion calls all around the UI for both Android/iOS). - For iOS, it can be directly done in CPP using std locale calls. In CPP in iOS, thousands separator will also be directly done. **Opt. 1&2 cons:** - Locale does not work for Android NDK. Need to make "manual" conversions (as done in option 1) and differently from iOS CPP code. - Thousand separator shall also be done "manually" for Android NDK. - Need to pass system locale to CPP code and store in locale wrapper class. - Need to implement a method to detect system locales changes and update info in wrapper class (Is this really necessary? A system locale change is rarely done, so if the user has to re-start the app in this situation, I don't think that I'd be a big issue...) And these are the pros and cons for option 3 (perform conversion in Android/iOS side): **Opt. 3 pros:** - No need to store system locale in CPP wrapper class. - System locale changes are automatically detected. **Opt. 3 cons:** - Need to convert strings in many different places in the Android/iOS code. - Need to convert strings containing distances and speed received from CPP code in different formats (sometimes these strings contain only numbers, and sometimes they also include units as km, miles, km/h). Please fell free to add or comment all these pros and cons...
biodranik commented 2022-12-12 11:57:41 +00:00 (Migrated from github.com)
Copy link

Thanks for the analysis! If it works on iOS, we can save some time. By the way, did you test it on iOS 12 (the lowest we support)? It is possible to install the simulator and test it.

Another important point IMO is that proper support in C++ will allow it to work properly on all other systems (Linux, Windows, macOS). Otherwise, they must implement the same "conversion" in many places.

There is already some logic to get the current locale/language implemented in platform/preferred_languages.cpp that we can reuse/extend.

The ideal solution should have less code and should not have runtime overhead (e.g. a lot of calculations/allocations each second).

Thanks for the analysis! If it works on iOS, we can save some time. By the way, did you test it on iOS 12 (the lowest we support)? It is possible to install the simulator and test it. Another important point IMO is that proper support in C++ will allow it to work properly on all other systems (Linux, Windows, macOS). Otherwise, they must implement the same "conversion" in many places. There is already some logic to get the current locale/language implemented in platform/preferred_languages.cpp that we can reuse/extend. The ideal solution should have less code and should not have runtime overhead (e.g. a lot of calculations/allocations each second).
gpesquero commented 2022-12-12 20:25:39 +00:00
Author
Member
Copy link

I did the tests on a simulator running iOS 12.4.

I will check platform/preferred_languages.cpp and see what we can reuse/extend...

I did the tests on a simulator running iOS 12.4. I will check `platform/preferred_languages.cpp` and see what we can reuse/extend...
biodranik commented 2022-12-12 21:07:37 +00:00 (Migrated from github.com)
Copy link

Thanks! We can also live well without real-time updates of the changed locale if it helps to make an easier implementation. Users don't change locale often, and it's perfectly ok if after changing the locale OM should be restarted to properly start drawing commas/dots.

Thanks! We can also live well without real-time updates of the changed locale if it helps to make an easier implementation. Users don't change locale often, and it's perfectly ok if after changing the locale OM should be restarted to properly start drawing commas/dots.
gpesquero commented 2022-12-19 00:40:43 +00:00
Author
Member
Copy link

Hello @biodranik!! I've pushed a new commit with a draft implementation for the decimal and grouping (thousands) separator for both Android and iOS in the CPP code.

As we confirmed earlier, Android NDK does not support locales, so the separator conversion is done manually extending the Locale struct. The conversion is done in function FormatDistanceImpl and only where it's needed in order to optimize performance.

For iOS, I haven't been able to get imbue() to work properly with system locale, so I've decided to use the same solution as for Android, extending also the Locale struct.

I have tested this code using both Android & iOS simulators, and it seems to work OK with some languages and system country settings, but further testing might be needed for many other configurations.

The code also compiles for Linux and macOS desktop environments, though it looks like the FormatDistance functions are not used in the desktop versions of the app.

I haven't tried to compile/test it on Windows.

Hello @biodranik!! I've pushed a new commit with a draft implementation for the decimal and grouping (thousands) separator for both Android and iOS in the CPP code. As we confirmed earlier, Android NDK does not support locales, so the separator conversion is done manually extending the Locale struct. The conversion is done in function `FormatDistanceImpl` and only where it's needed in order to optimize performance. For iOS, I haven't been able to get `imbue()` to work properly with system locale, so I've decided to use the same solution as for Android, extending also the Locale struct. I have tested this code using both Android & iOS simulators, and it seems to work OK with some languages and system country settings, but further testing might be needed for many other configurations. The code also compiles for Linux and macOS desktop environments, though it looks like the `FormatDistance` functions are not used in the desktop versions of the app. I haven't tried to compile/test it on Windows.
biodranik (Migrated from github.com) reviewed 2023-01-19 21:30:05 +00:00
biodranik (Migrated from github.com) left a comment

Thank you for your efforts, the code is cleaner now IMO.

For iOS, I haven't been able to get imbue() to work properly with system locale, so I've decided to use the same solution as for Android, extending also the Locale struct.

What exactly did you try?

Did you try to change the locale globally in the main.mm ?

Thank you for your efforts, the code is cleaner now IMO. > For iOS, I haven't been able to get imbue() to work properly with system locale, so I've decided to use the same solution as for Android, extending also the Locale struct. What exactly did you try? Did you try to change the locale globally in the main.mm ?
android/src/app/organicmaps/util/Utils.java
biodranik (Migrated from github.com) commented 2023-01-19 20:43:14 +00:00

Why are you converting it to a string? That's a lot of overhead. Why jbyte can't be used instead?

Why are you converting it to a string? That's a lot of overhead. Why jbyte can't be used instead?
biodranik (Migrated from github.com) commented 2023-01-19 20:43:19 +00:00

ditto

ditto
platform/locale.hpp
@ -6,1 +6,4 @@
{
std::string const kNonBreakingSpace = "\u00A0";
std::string const kNarrowNonBreakingSpace = "\u202F";
biodranik (Migrated from github.com) commented 2023-01-19 20:44:04 +00:00 
  char m_decimalSeparator;
  char m_groupingSeparator;
```suggestion char m_decimalSeparator; char m_groupingSeparator; ```
platform/locale.mm
@ -4,11 +4,28 @@
namespace platform
{
biodranik (Migrated from github.com) commented 2023-01-19 20:39:10 +00:00

Can an empty grouping separator be perfectly valid in some cases?
If yes, can we support these cases without performance degradation? Maybe by using char(0)?

Can an empty grouping separator be perfectly valid in some cases? If yes, can we support these cases without performance degradation? Maybe by using char(0)?
biodranik (Migrated from github.com) commented 2023-01-19 20:45:34 +00:00

There is no need to create a temporary string, NSString can be used directly, right?

There is no need to create a temporary string, NSString can be used directly, right?
platform/locale_linux.cpp
@ -1,10 +1,41 @@
#include "platform/locale.hpp"
#include <locale>
biodranik (Migrated from github.com) commented 2023-01-19 20:47:17 +00:00

This should be a bit faster:

  return {{}, {}, {}, '.', ','};
This should be a bit faster: ```suggestion return {{}, {}, {}, '.', ','}; ```
biodranik (Migrated from github.com) commented 2023-01-19 20:48:27 +00:00

Can we properly add the Linux code? https://www.systutorials.com/docs/linux/man/7-locale/

Can we properly add the Linux code? https://www.systutorials.com/docs/linux/man/7-locale/
platform/measurement_utils.cpp
@ -28,0 +30,4 @@
return ToStringPrecisionLocale(loc, d, pr);
}
biodranik (Migrated from github.com) commented 2023-01-19 20:55:02 +00:00 
  // We assume that the app will be restarted if a user changes device's locale.
  static Locale const loc = GetCurrentLocale();
```suggestion // We assume that the app will be restarted if a user changes device's locale. static Locale const loc = GetCurrentLocale(); ```
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
biodranik (Migrated from github.com) commented 2023-01-19 20:51:57 +00:00 
    int const precision = round(highV * 10) / 10 >= 10.0 ? 0 : 1;
```suggestion int const precision = round(highV * 10) / 10 >= 10.0 ? 0 : 1; ```
biodranik (Migrated from github.com) commented 2023-01-19 20:59:32 +00:00

Can we assume here that the '.' will always be at out.size() - 2 for better performance?
How 3.0 is printed? As 3.0 or as 3?

Is it possible to add unit tests for this and other cases?

Can we assume here that the '.' will always be at `out.size() - 2` for better performance? How 3.0 is printed? As 3.0 or as 3? Is it possible to add unit tests for this and other cases?
biodranik (Migrated from github.com) commented 2023-01-19 20:59:47 +00:00

We don't use {} for one-liners.

We don't use {} for one-liners.
biodranik (Migrated from github.com) commented 2023-01-19 21:02:36 +00:00

Mac, Linux, and maybe Windows may properly format the string using the system's locale. Can we at least mention it here? If you see that it should not be corrected on Mac, for example, then an #ifdef may be needed.

Mac, Linux, and maybe Windows may properly format the string using the system's locale. Can we at least mention it here? If you see that it should not be corrected on Mac, for example, then an `#ifdef` may be needed.
biodranik (Migrated from github.com) commented 2023-01-19 21:03:09 +00:00

We also try to use . at the end of comment sentences. ;-)

We also try to use . at the end of comment sentences. ;-)
biodranik (Migrated from github.com) commented 2023-01-19 21:03:58 +00:00

ditto, the grouping may be already applied by mac and Linux.

ditto, the grouping may be already applied by mac and Linux.
biodranik (Migrated from github.com) commented 2023-01-19 21:20:24 +00:00 
      if (highV >= 1000.0 && out.size() >= 4)
        out.insert(out.size() - 3, 1, loc.m_grouping_separator);
    }

Is it possible to have two or more grouping separators?

```suggestion if (highV >= 1000.0 && out.size() >= 4) out.insert(out.size() - 3, 1, loc.m_grouping_separator); } ``` Is it possible to have two or more grouping separators?
biodranik (Migrated from github.com) commented 2023-01-19 21:26:18 +00:00 
    // Add units.
    return out + ' ' + high;
```suggestion // Add units. return out + ' ' + high; ```
gpesquero commented 2023-01-22 22:53:47 +00:00
Author
Member
Copy link

On IOS I've set the locale globally in main.mm with the call std::locale::global(std::locale(""));, and I've added also the line ss.imbue(std::locale("")); in the ToStringPrecision(double d, int pr) function, but the decimal and grouping separators do not change according to the system environment settings.

On IOS I've set the locale globally in `main.mm` with the call `std::locale::global(std::locale(""));`, and I've added also the line `ss.imbue(std::locale(""));` in the `ToStringPrecision(double d, int pr)` function, but the decimal and grouping separators do not change according to the system environment settings.
gpesquero reviewed 2023-01-23 21:00:22 +00:00
platform/locale.mm
@ -4,11 +4,28 @@
namespace platform
{
gpesquero commented 2023-01-23 21:00:22 +00:00
Author
Member

Yes, empty grouping separators are valid, so I will modify the code to support these situations using a char(0) as grouping separator.

Yes, empty grouping separators are valid, so I will modify the code to support these situations using a `char(0)` as grouping separator.
gpesquero reviewed 2023-01-23 21:02:48 +00:00
android/src/app/organicmaps/util/Utils.java
gpesquero commented 2023-01-23 21:02:48 +00:00
Author
Member

I converted both decimal and grouping separators to string so the jni code was the same as for the country and language code. I've already modified the code so instead of string, we use chars.

I converted both decimal and grouping separators to string so the jni code was the same as for the country and language code. I've already modified the code so instead of string, we use chars.
gpesquero reviewed 2023-01-23 21:03:47 +00:00
android/src/app/organicmaps/util/Utils.java
gpesquero commented 2023-01-23 21:03:47 +00:00
Author
Member

Same as before... already changed code to use char instead of strings...

Same as before... already changed code to use char instead of strings...
gpesquero reviewed 2023-01-23 21:04:10 +00:00
platform/locale.hpp
@ -6,1 +6,4 @@
{
std::string const kNonBreakingSpace = "\u00A0";
std::string const kNarrowNonBreakingSpace = "\u202F";
gpesquero commented 2023-01-23 21:04:10 +00:00
Author
Member

Corrected !!

Corrected !!
gpesquero reviewed 2023-01-23 21:07:02 +00:00
platform/locale_linux.cpp
@ -1,10 +1,41 @@
#include "platform/locale.hpp"
#include <locale>
gpesquero commented 2023-01-23 21:07:02 +00:00
Author
Member

Replaced "" with {}...

Regarding the Linux code, I don't exactly understand what functionality you want to implement here...

Replaced "" with {}... Regarding the Linux code, I don't exactly understand what functionality you want to implement here...
gpesquero reviewed 2023-01-23 21:08:24 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:08:24 +00:00
Author
Member

Modified !!

Modified !!
gpesquero reviewed 2023-01-23 21:09:14 +00:00
platform/measurement_utils.cpp
@ -28,0 +30,4 @@
return ToStringPrecisionLocale(loc, d, pr);
}
gpesquero commented 2023-01-23 21:09:13 +00:00
Author
Member

Modified !!

Modified !!
gpesquero reviewed 2023-01-23 21:15:48 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:15:48 +00:00
Author
Member

Yes, instead of std::replace() we could assume that the decimal separator is at out.size() - 2. I will change the code accordingly...
And yes, 3.0 is printed as 3.0

Yes, instead of `std::replace()` we could assume that the decimal separator is at `out.size() - 2`. I will change the code accordingly... And yes, 3.0 is printed as 3.0
gpesquero reviewed 2023-01-23 21:40:23 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:40:23 +00:00
Author
Member

And regarding the unit tests, please confirm that the idea is to add to measurement_tests.cpp tests of the FormatDistanceImpl() function to check the functionality of the decimal and grouping separators replacement based on different locales.

And regarding the unit tests, please confirm that the idea is to add to `measurement_tests.cpp` tests of the `FormatDistanceImpl()` function to check the functionality of the decimal and grouping separators replacement based on different locales.
gpesquero reviewed 2023-01-23 21:41:06 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:41:05 +00:00
Author
Member

Corrected !!

Corrected !!
gpesquero reviewed 2023-01-23 21:44:40 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:44:40 +00:00
Author
Member

I've added #ifdef so the decimal and grouping separators and "manually" replaced only on Android OS.
I've also added a comment explaining this.

I've added `#ifdef` so the decimal and grouping separators and "manually" replaced only on Android OS. I've also added a comment explaining this.
gpesquero reviewed 2023-01-23 21:47:49 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:47:48 +00:00
Author
Member

The added #ifdef applies for both decimal and grouping separator...

The added `#ifdef` applies for both decimal and grouping separator...
gpesquero reviewed 2023-01-23 21:54:12 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:54:11 +00:00
Author
Member

Yes, I can change the code so we can handle two or mode grouping separators...

Yes, I can change the code so we can handle two or mode grouping separators...
gpesquero reviewed 2023-01-23 21:56:56 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-23 21:56:55 +00:00
Author
Member

Modified !!

Modified !!
gpesquero reviewed 2023-01-27 00:29:30 +00:00
platform/measurement_utils.cpp
@ -32,0 +49,4 @@
else
{
// Value with no decimals. Check if it's equal or bigger than 10000 to
// insert the grouping (thousands) separator characters.
gpesquero commented 2023-01-27 00:29:29 +00:00
Author
Member

Done!! I've also moved this piece of code to the function ToStringPrecision() so speed strings are also localized...

Done!! I've also moved this piece of code to the function `ToStringPrecision()` so speed strings are also localized...
gpesquero reviewed 2023-01-27 00:30:47 +00:00
platform/locale.mm
@ -4,11 +4,28 @@
namespace platform
{
gpesquero commented 2023-01-27 00:30:47 +00:00
Author
Member

Modified code to use NSString directly...

Modified code to use NSString directly...
gpesquero commented 2023-01-27 11:47:13 +00:00
Author
Member
Copy link

One of the checks on macOS of my last commit has failed... I don't know the reason why...

One of the checks on macOS of my last commit has failed... I don't know the reason why...
gpesquero reviewed 2023-02-10 21:56:28 +00:00
platform/locale.mm
@ -4,11 +4,28 @@
namespace platform
{
gpesquero commented 2023-02-10 21:56:28 +00:00
Author
Member

Implemented use of char(0) for empty group separator

Implemented use of `char(0)` for empty group separator
biodranik (Migrated from github.com) reviewed 2023-02-18 07:58:24 +00:00
android/jni/app/organicmaps/platform/Language.cpp
biodranik (Migrated from github.com) commented 2023-02-18 07:15:19 +00:00 
  static jmethodID const getDecimalSeparatorId = jni::GetStaticMethodID(env, g_utilsClazz, "getDecimalSeparator", "()C");
```suggestion static jmethodID const getDecimalSeparatorId = jni::GetStaticMethodID(env, g_utilsClazz, "getDecimalSeparator", "()C"); ```
biodranik (Migrated from github.com) commented 2023-02-18 07:15:51 +00:00 
  char const decimalSeparator = env->CallStaticCharMethod(g_utilsClazz, getDecimalSeparatorId);
```suggestion char const decimalSeparator = env->CallStaticCharMethod(g_utilsClazz, getDecimalSeparatorId); ```
biodranik (Migrated from github.com) commented 2023-02-18 07:16:13 +00:00 
  static jmethodID const getGroupingSeparatorId = jni::GetStaticMethodID(env, g_utilsClazz, "getGroupingSeparator", "()C");
```suggestion static jmethodID const getGroupingSeparatorId = jni::GetStaticMethodID(env, g_utilsClazz, "getGroupingSeparator", "()C"); ```
biodranik (Migrated from github.com) commented 2023-02-18 07:17:10 +00:00 
  char const groupingSeparator = env->CallStaticCharMethod(g_utilsClazz, getGroupingSeparatorId);;
```suggestion char const groupingSeparator = env->CallStaticCharMethod(g_utilsClazz, getGroupingSeparatorId);; ```
iphone/Maps/main.mm
biodranik (Migrated from github.com) commented 2023-02-18 07:19:34 +00:00

Doesn't it work on a mac? If it doesn't, then ifdef should also be above near the include. If it does, then ifdef is not needed.

Doesn't it work on a mac? If it doesn't, then ifdef should also be above near the include. If it does, then ifdef is not needed.
platform/measurement_utils.cpp
biodranik (Migrated from github.com) commented 2023-02-18 07:58:20 +00:00

Can it be done in main.mm?

Can it be done in main.mm?
gpesquero reviewed 2023-02-19 15:49:06 +00:00
iphone/Maps/main.mm
gpesquero commented 2023-02-19 15:49:06 +00:00
Author
Member

I first assumed that the std::locale::global(std::locale("")); call in main.mm configured the locale, so certain distance values were displayed with local decimal separator, for example in the place info panel:

place_info

But now I've realized that these values are correctly displayed with the locale decimal separator because of the use of MKDistanceFormatter that applies the localized decimal separator.

So this call to std::locale::global(std::locale("")); in main.mm can be deleted, as it doesn't make any use.

I first assumed that the `std::locale::global(std::locale(""));` call in `main.mm` configured the locale, so certain distance values were displayed with local decimal separator, for example in the place info panel: ![place_info](https://user-images.githubusercontent.com/25888296/219958694-dee50bed-0bc4-4d08-b70b-7a875ef4d278.png) But now I've realized that these values are correctly displayed with the locale decimal separator because of the use of [MKDistanceFormatter](https://developer.apple.com/documentation/mapkit/mkdistanceformatter) that applies the localized decimal separator. So this call to `std::locale::global(std::locale(""));` in `main.mm` can be deleted, as it doesn't make any use.
gpesquero reviewed 2023-02-19 16:41:47 +00:00
platform/measurement_utils.cpp
gpesquero commented 2023-02-19 16:41:47 +00:00
Author
Member

I've now made further checks with this solution and I've found out that with some language/region combinations, the imbue() call crashes. For example, with "es_ES" (Spanish/Spain) locale string it works, but it crashes if I change the region to the US, locale "es_US" (Spanish/US).

I honestly don't know how to make things work with imbue(), so my proposal right now is to implement the same solution for iOS and Android for the decimal and grouping separators.

I've now made further checks with this solution and I've found out that with some language/region combinations, the `imbue()` call crashes. For example, with "es_ES" (Spanish/Spain) locale string it works, but it crashes if I change the region to the US, locale "es_US" (Spanish/US). I honestly don't know how to make things work with `imbue()`, so my proposal right now is to implement the same solution for iOS and Android for the decimal and grouping separators.
gpesquero reviewed 2023-02-19 16:49:41 +00:00
android/jni/app/organicmaps/platform/Language.cpp
gpesquero commented 2023-02-19 16:49:40 +00:00
Author
Member

OK, I've changed the code accordingly.

OK, I've changed the code accordingly.
gpesquero reviewed 2023-02-19 16:49:55 +00:00
android/jni/app/organicmaps/platform/Language.cpp
gpesquero commented 2023-02-19 16:49:54 +00:00
Author
Member

OK, I've changed the code accordingly.

OK, I've changed the code accordingly.
gpesquero reviewed 2023-02-19 16:50:06 +00:00
android/jni/app/organicmaps/platform/Language.cpp
gpesquero commented 2023-02-19 16:50:06 +00:00
Author
Member

OK, I've changed the code accordingly.

OK, I've changed the code accordingly.
gpesquero reviewed 2023-02-19 16:50:16 +00:00
android/jni/app/organicmaps/platform/Language.cpp
gpesquero commented 2023-02-19 16:50:15 +00:00
Author
Member

OK, I've changed the code accordingly.

OK, I've changed the code accordingly.
biodranik (Migrated from github.com) reviewed 2023-04-09 21:41:10 +00:00
biodranik (Migrated from github.com) left a comment

Can you please add a log and track how often is it called in some common scenarios (like searching, for example)?

Can you please add a log and track how often is it called in some common scenarios (like searching, for example)?
biodranik (Migrated from github.com) reviewed 2023-07-23 11:45:45 +00:00
biodranik (Migrated from github.com) left a comment

Am I correctly understanding that the distance is now properly formatted for each locale, but speed is not?

@AndrewShkrob Does it make sense to extend your distance class with speed support? Or would it be easier to support native speed formatting as it is done right now? Do we need to display speed values from C++ code now (or in the future)?

Am I correctly understanding that the distance is now properly formatted for each locale, but speed is not? @AndrewShkrob Does it make sense to extend your distance class with speed support? Or would it be easier to support native speed formatting as it is done right now? Do we need to display speed values from C++ code now (or in the future)?
biodranik (Migrated from github.com) reviewed 2023-08-16 20:56:23 +00:00
biodranik (Migrated from github.com) left a comment

@AndrewShkrob @vng WDYT?

@AndrewShkrob @vng WDYT?
android/jni/app/organicmaps/Framework.cpp
biodranik (Migrated from github.com) commented 2023-08-16 20:37:41 +00:00 
  jni::TScopedLocalRef const totalAscentString(env, jni::ToJavaString(env, ToStringPrecision(totalAscent, 0)));

  jni::TScopedLocalRef const totalDescentString(env, jni::ToJavaString(env, ToStringPrecision(totalDescent, 0)));
```suggestion jni::TScopedLocalRef const totalAscentString(env, jni::ToJavaString(env, ToStringPrecision(totalAscent, 0))); jni::TScopedLocalRef const totalDescentString(env, jni::ToJavaString(env, ToStringPrecision(totalDescent, 0))); ```
android/jni/app/organicmaps/platform/Language.cpp
biodranik (Migrated from github.com) commented 2023-08-16 20:39:33 +00:00

Align it with (

Align it with (
biodranik (Migrated from github.com) commented 2023-08-16 20:39:44 +00:00

ditto

ditto
iphone/CoreApi/CoreApi/Common/MWMGeoUtil.mm
biodranik (Migrated from github.com) commented 2023-08-16 20:48:47 +00:00

stringWithUTF8String

stringWithUTF8String
biodranik (Migrated from github.com) commented 2023-08-16 20:49:03 +00:00 
  auto const outString = measurement_utils::ToStringPrecision(self.value, self.value >= 10.0 ? 0 : 1);
```suggestion auto const outString = measurement_utils::ToStringPrecision(self.value, self.value >= 10.0 ? 0 : 1); ```
platform/locale.hpp
@ -14,3 +19,4 @@
Locale GetCurrentLocale();
bool GetLocale(std::string localeName, Locale& result);
} // namespace platform
biodranik (Migrated from github.com) commented 2023-08-16 20:49:52 +00:00

Can a decimal separator also be a non-ASCII symbol? Is there a full list somewhere?

Can a decimal separator also be a non-ASCII symbol? Is there a full list somewhere?
platform/platform_tests/distance_tests.cpp
@ -257,2 +278,4 @@
{Distance(109, Units::Meters), 110, Units::Meters, "110", MakeDistanceStr("110", "m")},
{Distance(991, Units::Meters), 990, Units::Meters, "990", MakeDistanceStr("990", "m")},
// From Kilometers to Kilometers
biodranik (Migrated from github.com) commented 2023-08-16 20:56:04 +00:00
  1. Can tests for a decimal separator also be added?
  2. Is it hard to add tests for altitude formatting?
  3. What if tests are run on a French locale?
1. Can tests for a decimal separator also be added? 2. Is it hard to add tests for altitude formatting? 3. What if tests are run on a French locale?
gpesquero reviewed 2023-08-16 22:26:38 +00:00
platform/locale.hpp
@ -14,3 +19,4 @@
Locale GetCurrentLocale();
bool GetLocale(std::string localeName, Locale& result);
} // namespace platform
gpesquero commented 2023-08-16 22:26:38 +00:00
Author
Member

Yes, according to this Wikipedia article, there's also an Arabic decimal separator (U+066B).

Here's a screenshot of the place info panel with locate set to Farsi/Persian, where the decimal and grouping separators are both non-ASCII chars:
Captura desde 2023-08-17 00-08-14

Yes, according to [this](https://en.wikipedia.org/wiki/Decimal_separator) Wikipedia article, there's also an Arabic decimal separator (U+066B). Here's a screenshot of the place info panel with locate set to Farsi/Persian, where the decimal and grouping separators are both non-ASCII chars: ![Captura desde 2023-08-17 00-08-14](https://github.com/organicmaps/organicmaps/assets/25888296/cceb989c-00ed-435b-9c64-a33706e18622)
gpesquero reviewed 2023-08-16 22:39:21 +00:00
android/jni/app/organicmaps/Framework.cpp
gpesquero commented 2023-08-16 22:39:21 +00:00
Author
Member

Noted! Commit will be amended accordingly...

Noted! Commit will be amended accordingly...
gpesquero reviewed 2023-08-16 22:39:31 +00:00
android/jni/app/organicmaps/platform/Language.cpp
gpesquero commented 2023-08-16 22:39:31 +00:00
Author
Member

Noted! Commit will be amended accordingly...

Noted! Commit will be amended accordingly...
gpesquero reviewed 2023-08-16 22:39:39 +00:00
android/jni/app/organicmaps/platform/Language.cpp
gpesquero commented 2023-08-16 22:39:39 +00:00
Author
Member

Noted! Commit will be amended accordingly...

Noted! Commit will be amended accordingly...
gpesquero reviewed 2023-08-16 23:03:55 +00:00
iphone/CoreApi/CoreApi/Common/MWMGeoUtil.mm
gpesquero commented 2023-08-16 23:03:54 +00:00
Author
Member

Noted! I will use stringWithUTF8String instead of stringWithCString.

Noted! I will use `stringWithUTF8String` instead of `stringWithCString`.
gpesquero reviewed 2023-08-16 23:15:12 +00:00
iphone/CoreApi/CoreApi/Common/MWMGeoUtil.mm
gpesquero commented 2023-08-16 23:15:12 +00:00
Author
Member

Noted! Commit will be amended accordingly...

Noted! Commit will be amended accordingly...
gpesquero reviewed 2023-08-17 16:58:15 +00:00
platform/platform_tests/distance_tests.cpp
@ -257,2 +278,4 @@
{Distance(109, Units::Meters), 110, Units::Meters, "110", MakeDistanceStr("110", "m")},
{Distance(991, Units::Meters), 990, Units::Meters, "990", MakeDistanceStr("990", "m")},
// From Kilometers to Kilometers
gpesquero commented 2023-08-17 16:58:15 +00:00
Author
Member
  1. Can tests for a decimal separator also be added?

Do you mean to test decimal separators on different locales? With the current implementation in ToStringPrecision(), GetCurrentLocale() is only called once at start-up, so we could only make tests with a single locale.

  1. Is it hard to add tests for altitude formatting?

We already have a couple of tests for altitude formatting in UNIT_TEST(Distance_CreateAltitudeFormatted) in distance_tests.cpp. I've included tough a third one for high altitudes in meters.

  1. What if tests are run on a French locale?

Current implementation of GetCurrentLocale() in linux does not read system's locale, and forces decimal and grouping separators to '.' and ',' respectively, so tests run flawlessly even with Spanish or French system locales.

> 1. Can tests for a decimal separator also be added? Do you mean to test decimal separators on different locales? With the current implementation in `ToStringPrecision()`, `GetCurrentLocale()` is only called once at start-up, so we could only make tests with a single locale. > 2. Is it hard to add tests for altitude formatting? We already have a couple of tests for altitude formatting in `UNIT_TEST(Distance_CreateAltitudeFormatted)` in `distance_tests.cpp`. I've included tough a third one for high altitudes in meters. > 3. What if tests are run on a French locale? Current implementation of `GetCurrentLocale()` in linux does not read system's locale, and forces decimal and grouping separators to '.' and ',' respectively, so tests run flawlessly even with Spanish or French system locales.
AndrewShkrob reviewed 2023-08-17 17:15:35 +00:00
platform/locale.mm
@ -6,3 +6,3 @@
{
Locale GetCurrentLocale()
Locale NSLocale2Locale(NSLocale *locale)
{
AndrewShkrob commented 2023-08-17 17:15:34 +00:00
Member

I'm not sure that default value for groupingSeparator is OK. In my country we don't use commas: "10 123 m" or "1234 m".

I think, it's better to leave it empty because if it happens "somehow" that there is no info about groupingSeparator, some people may be confused by the printed values.

I'm not sure that default value for `groupingSeparator` is OK. In my country we don't use commas: "10 123 m" or "1234 m". I think, it's better to leave it empty because if it happens "somehow" that there is no info about `groupingSeparator`, some people may be confused by the printed values.
AndrewShkrob reviewed 2023-08-17 17:16:08 +00:00
platform/locale_linux.cpp
@ -1,10 +1,41 @@
#include "platform/locale.hpp"
#include <locale>
AndrewShkrob commented 2023-08-17 17:16:08 +00:00
Member

Same here

Same here
AndrewShkrob approved these changes 2023-08-17 17:27:08 +00:00
gpesquero reviewed 2023-08-17 21:50:08 +00:00
platform/locale.mm
@ -6,3 +6,3 @@
{
Locale GetCurrentLocale()
Locale NSLocale2Locale(NSLocale *locale)
{
gpesquero commented 2023-08-17 21:50:08 +00:00
Author
Member

Yes, maybe an empty grouping separator may be more common across different locales, though this default value will be overwritten by system's configuration in most of the times.

Your comment also confirms that in many languages, the grouping separator is only used in values with more than 4 digits, keeping digits with 4 digits with no grouping separator ("10 123 m" and "1234 m"). Maybe we shall also keep this behavior, which is also suggested as International Standard according to this Wikipedia article about digit grouping. This International Standard also suggests to use always the narrow no-break space char as the grouping separator.

My proposal for the grouping separator would be then: set empty "" (no grouping separator) as default if the system does not provide any & apply grouping separators only on numbers with more that 4 digits.

Yes, maybe an empty grouping separator may be more common across different locales, though this default value will be overwritten by system's configuration in most of the times. Your comment also confirms that in many languages, the grouping separator is only used in values with more than 4 digits, keeping digits with 4 digits with no grouping separator ("10 123 m" and "1234 m"). Maybe we shall also keep this behavior, which is also _suggested_ as International Standard according to [this](https://en.wikipedia.org/wiki/Decimal_separator#Digit_grouping) Wikipedia article about digit grouping. This International Standard also _suggests_ to use always the narrow no-break space char as the grouping separator. My proposal for the grouping separator would be then: set empty "" (no grouping separator) as default if the system does not provide any & apply grouping separators *only* on numbers with more that 4 digits.
gpesquero reviewed 2023-08-17 21:53:31 +00:00
platform/locale_linux.cpp
@ -1,10 +1,41 @@
#include "platform/locale.hpp"
#include <locale>
gpesquero commented 2023-08-17 21:53:30 +00:00
Author
Member

Same as before, I agree to set the empty "" grouping separator as default, though this change affects the unit testing code, so I will have to change back again the unit tests for distances.

Same as before, I agree to set the empty "" grouping separator as default, though this change affects the unit testing code, so I will have to change back again the unit tests for distances.
biodranik (Migrated from github.com) reviewed 2023-08-17 22:16:47 +00:00
platform/locale.mm
@ -6,3 +6,3 @@
{
Locale GetCurrentLocale()
Locale NSLocale2Locale(NSLocale *locale)
{
biodranik (Migrated from github.com) commented 2023-08-17 22:16:47 +00:00

Thanks! Looks like a good default is:

  • Use group separator for >4 chars only
  • Use a non-breaking space

P.S. Is non-breaking space used after a number and before the unit name? Should we use it too?

Thanks! Looks like a good default is: - Use group separator for >4 chars only - Use a non-breaking space P.S. Is non-breaking space used after a number and before the unit name? Should we use it too?
biodranik (Migrated from github.com) reviewed 2023-08-17 22:18:46 +00:00
platform/platform_tests/distance_tests.cpp
@ -254,3 +264,1 @@
{Distance(101, Units::Meters), 100, Units::Meters, "100", "100 m"},
{Distance(109, Units::Meters), 110, Units::Meters, "110", "110 m"},
{Distance(991, Units::Meters), 990, Units::Meters, "990", "990 m"},
{Distance(0, Units::Meters), 0, Units::Meters, "0", MakeDistanceStr("0", "m")},
biodranik (Migrated from github.com) commented 2023-08-17 22:18:03 +00:00

If we change defaults to >4 chars for group separators, these tests will change, right?

If we change defaults to >4 chars for group separators, these tests will change, right?
platform/platform_tests/distance_tests.cpp
@ -257,2 +278,4 @@
{Distance(109, Units::Meters), 110, Units::Meters, "110", MakeDistanceStr("110", "m")},
{Distance(991, Units::Meters), 990, Units::Meters, "990", MakeDistanceStr("990", "m")},
// From Kilometers to Kilometers
biodranik (Migrated from github.com) commented 2023-08-17 22:18:42 +00:00

1/ I meant test cases where a decimal separator is used.

1/ I meant test cases where a decimal separator is used.
gpesquero reviewed 2023-08-18 23:31:15 +00:00
platform/locale.mm
@ -6,3 +6,3 @@
{
Locale GetCurrentLocale()
Locale NSLocale2Locale(NSLocale *locale)
{
gpesquero commented 2023-08-18 23:31:15 +00:00
Author
Member

@biodranik: I assume that you suggest to use as default the narrow non-breaking space (NNBSP, U+202F), instead of the standard non-breaking space (NBSP, U+00A0).

In this Wikipedia article, it's indeed stated that the NBSP's are also used between the value and the units, so yes, we can also use them there instead of the regular whitespace.

In a first commit I've set the NNBSP as default for iOS and linux and I've modified the UTs accordingly, but after pushing the commit, the iOS tests have failed because the grouping separator is configured to the ',' character. The workaround is to set the default character in linux and the UTs with the ',' character.

I've amended the commit according to these last comments.

@biodranik: I assume that you suggest to use as default the **narrow** non-breaking space (NNBSP, U+202F), instead of the **standard** non-breaking space (NBSP, U+00A0). In [this](https://en.wikipedia.org/wiki/Non-breaking_space#Uses_and_variations) Wikipedia article, it's indeed stated that the NBSP's are also used between the value and the units, so yes, we can also use them there instead of the regular whitespace. In a first commit I've set the NNBSP as default for iOS and linux and I've modified the UTs accordingly, but after pushing the commit, the iOS tests have failed because the grouping separator is configured to the ',' character. The workaround is to set the default character in linux and the UTs with the ',' character. I've amended the commit according to these last comments.
gpesquero reviewed 2023-08-18 23:32:02 +00:00
platform/platform_tests/distance_tests.cpp
@ -254,3 +264,1 @@
{Distance(101, Units::Meters), 100, Units::Meters, "100", "100 m"},
{Distance(109, Units::Meters), 110, Units::Meters, "110", "110 m"},
{Distance(991, Units::Meters), 990, Units::Meters, "990", "990 m"},
{Distance(0, Units::Meters), 0, Units::Meters, "0", MakeDistanceStr("0", "m")},
gpesquero commented 2023-08-18 23:32:01 +00:00
Author
Member

Yes, the UTs have to be changed.

Yes, the UTs have to be changed.
biodranik (Migrated from github.com) reviewed 2023-08-19 08:17:09 +00:00
biodranik (Migrated from github.com) left a comment

Good! @Jean-BaptisteC can you please test if it works with different system languages, including Arabic, Hebrew, French, German, US, Chinese, Japanese?

Good! @Jean-BaptisteC can you please test if it works with different system languages, including Arabic, Hebrew, French, German, US, Chinese, Japanese?
android/src/app/organicmaps/routing/RoutingBottomMenuController.java
biodranik (Migrated from github.com) commented 2023-08-19 08:15:00 +00:00

Is + faster/better than String.format?

Is + faster/better than String.format?
Jean-BaptisteC (Migrated from github.com) approved these changes 2023-08-19 18:36:20 +00:00
Jean-BaptisteC (Migrated from github.com) left a comment

✅ Pixel 6 - Android 13

French

Arabic

Deutsch

English (US)

Japanese

Chinese

Hebrew

Other examples:

✅ Pixel 6 - Android 13 French <img src="https://github.com/organicmaps/organicmaps/assets/87148630/a0717cfe-ebe5-41c7-8612-f9da8f95d892" height=50 /> Arabic <img src="https://github.com/organicmaps/organicmaps/assets/87148630/30ab24a6-bb5b-45f9-acb0-5babff9c0f7b" height=50 /> Deutsch <img src="https://github.com/organicmaps/organicmaps/assets/87148630/be4674dd-162d-4368-bfe9-9c4e53f7769f" height=50 /> English (US) <img src="https://github.com/organicmaps/organicmaps/assets/87148630/b9f8c647-2d4a-4933-8170-ef4138a54c89" height=50 /> Japanese <img src="https://github.com/organicmaps/organicmaps/assets/87148630/0cf201da-62c9-4368-a6cf-393945f0f523" height=50 /> Chinese <img src="https://github.com/organicmaps/organicmaps/assets/87148630/bc29a30c-55e9-4f6e-8d74-4db97379c074" height=50 /> Hebrew <img src="https://github.com/organicmaps/organicmaps/assets/87148630/ec29aadc-48bc-435f-81c1-865432a97179" height=50 /> Other examples: <img src="https://github.com/organicmaps/organicmaps/assets/87148630/a4ad6862-41d3-4386-86b8-a8f1469fc015" height=60 /> <img src="https://github.com/organicmaps/organicmaps/assets/87148630/5114c5d8-e844-4ae5-aeb3-2617000b3012" height=350 /> <img src="https://github.com/organicmaps/organicmaps/assets/87148630/8abbfef9-4204-49a7-840d-ee9a362bd6b8" height=350 />
gpesquero reviewed 2023-08-20 16:29:03 +00:00
android/src/app/organicmaps/routing/RoutingBottomMenuController.java
gpesquero commented 2023-08-20 16:29:03 +00:00
Author
Member

Yes, the + operator seems to be much faster:

First column: String.format, second column: + operator
2778346 ns, 72961 ns
1656885 ns, 341115 ns
1513307 ns, 45154 ns
1233769 ns, 32039 ns

And as we're dealing now only with strings, the code seems to be clearer than with the String.format function.

Yes, the + operator seems to be much faster: First column: String.format, second column: + operator 2778346 ns, 72961 ns 1656885 ns, 341115 ns 1513307 ns, 45154 ns 1233769 ns, 32039 ns And as we're dealing now only with strings, the code seems to be clearer than with the String.format function.
biodranik (Migrated from github.com) approved these changes 2023-08-20 23:22:26 +00:00
biodranik (Migrated from github.com) left a comment

Doing even faster string generation without iostream and reallocations would be an awesome bonus, but it's good enough already.

@vng WDYT?

Doing _even faster_ string generation without iostream and reallocations would be an awesome bonus, but it's good enough already. @vng WDYT?
biodranik (Migrated from github.com) requested changes 2023-08-20 23:23:14 +00:00
biodranik (Migrated from github.com) left a comment

Oh wait:

1  tests failed:
distance_tests.cpp::Distance_ToStringPrecisionLocale
Some tests FAILED.
Oh wait: ``` 1 tests failed: distance_tests.cpp::Distance_ToStringPrecisionLocale Some tests FAILED. ```
AndrewShkrob reviewed 2023-08-21 07:26:11 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-21 07:26:10 +00:00
Member

Let's move \u202F to a const variable at the beginning of the file:
char const kNonBreakingSpace = '\u202F'

Let's move `\u202F` to a const variable at the beginning of the file: `char const kNonBreakingSpace = '\u202F'`
AndrewShkrob reviewed 2023-08-21 07:34:54 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-21 07:34:54 +00:00
Member
  1. Let's move it to the related tests file. I guess, it's measurement_tests.cpp
  2. I don't really like this solution.
    You can mock locale as it's done for display units with ScopedSettings guard.
    Create ScopedLocale guard class, pass separators to the constructor and overwrite current locale settings. In destructor - restore locale.
    Check UNIT_TEST(Distance_CreateFormatted) as an example

This solution won't require installing additional locales to the system.

1. Let's move it to the related tests file. I guess, it's `measurement_tests.cpp` 2. I don't really like this solution. You can mock locale as it's done for display units with `ScopedSettings guard`. Create `ScopedLocale guard` class, pass separators to the constructor and overwrite current locale settings. In destructor - restore locale. Check `UNIT_TEST(Distance_CreateFormatted)` as an example This solution won't require installing additional locales to the system.
AndrewShkrob reviewed 2023-08-21 07:46:05 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-21 07:46:05 +00:00
Member

Hm, I was under the impression that we had a place to store the locale, but upon closer examination, I can only locate the GetCurrentLocale function. Given this scenario, my original suggestion will indeed not work.

Consequently, an additional implementation to store the current locale will be necessary. If you're unable to devise a solution, it might be more prudent to eliminate these tests. Depending on installed system locales for a test is generally inadvisable.

Hm, I was under the impression that we had a place to store the locale, but upon closer examination, I can only locate the `GetCurrentLocale` function. Given this scenario, my original suggestion will indeed not work. Consequently, an additional implementation to store the current locale will be necessary. If you're unable to devise a solution, it might be more prudent to eliminate these tests. Depending on installed system locales for a test is generally inadvisable.
biodranik (Migrated from github.com) reviewed 2023-08-21 18:38:47 +00:00
platform/platform_tests/distance_tests.cpp
biodranik (Migrated from github.com) commented 2023-08-21 18:38:47 +00:00

constexpr is even better:

char constexpr kNonBreakingSpace[] = " ";
constexpr is even better: ``` char constexpr kNonBreakingSpace[] = " "; ```
biodranik (Migrated from github.com) reviewed 2023-08-21 18:42:41 +00:00
platform/platform_tests/distance_tests.cpp
biodranik (Migrated from github.com) commented 2023-08-21 18:42:41 +00:00

Testing specific locales in a controlled environment is ok. You can edit our .github/workflows actions by adding sudo locale-gen ru_RU.UTF-8 commands in the beginning.

Testing specific locales in a controlled environment is ok. You can edit our .github/workflows actions by adding `sudo locale-gen ru_RU.UTF-8` commands in the beginning.
gpesquero reviewed 2023-08-21 23:35:33 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-08-21 23:35:33 +00:00
Author
Member

As the non-breaking space \u202F is used in different source files, I've included its definition in the file locale.hpp.

I cannot get it working as char, so I've declared it as std::string.

As the non-breaking space `\u202F` is used in different source files, I've included its definition in the file `locale.hpp`. I cannot get it working as `char`, so I've declared it as `std::string`.
gpesquero reviewed 2023-08-22 00:27:13 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-08-22 00:27:12 +00:00
Author
Member

I've added the sudo locale-gen xx_YY.UTF-8 commands to .github/workflows/linux_check.yaml for different languages (en, es, fr & ru) and now the linux checks seem to work Ok.

Anyhow, the macOS checks still do not work. Instead of the linux command locale-gen, in macOS it looks like the command to use is localedef, but I still cannot get it working. I will make further investigations in the incoming days, but any help is welcomed.

I've added the `sudo locale-gen xx_YY.UTF-8` commands to `.github/workflows/linux_check.yaml` for different languages (en, es, fr & ru) and now the linux checks seem to work Ok. Anyhow, the macOS checks still do not work. Instead of the linux command `locale-gen`, in macOS it looks like the command to use is `localedef`, but I still cannot get it working. I will make further investigations in the incoming days, but any help is welcomed.
AndrewShkrob reviewed 2023-08-22 16:27:24 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-22 16:27:24 +00:00
Member

That will definetly break tests on local machines without all required locales.
At least, consider covering such test cases with #if CI

    TestData testData[] = {
          { "en_US.UTF-8",     "9.8",      "12,345"},
#if CI
          { "es_ES.UTF-8",     "9,8",      "12.345"},
          { "fr_FR.UTF-8",     "9,8",      "12 345"},
          { "ru_RU.UTF-8",     "9,8",      "12 345"}
#endif
    };

We can pass CI var through cmake when it's defined in the environment. CI services like GitHub Actions always have this environment variable.
https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables

That will definetly break tests on local machines without all required locales. At least, consider covering such test cases with `#if CI` ```cpp TestData testData[] = { { "en_US.UTF-8", "9.8", "12,345"}, #if CI { "es_ES.UTF-8", "9,8", "12.345"}, { "fr_FR.UTF-8", "9,8", "12 345"}, { "ru_RU.UTF-8", "9,8", "12 345"} #endif }; ``` We can pass `CI` var through cmake when it's defined in the environment. CI services like GitHub Actions always have this environment variable. https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
AndrewShkrob reviewed 2023-08-22 16:29:26 +00:00
.github/workflows/linux-check.yaml
@ -166,0 +171,4 @@
sudo locale-gen fr_FR.UTF-8
sudo locale-gen ru_RU
sudo locale-gen ru_RU.UTF-8
sudo update-locale
AndrewShkrob commented 2023-08-22 16:29:26 +00:00
Member

I think it's better to move this part to a separate job. Probably near the dependencies job or before the tests job with a comment that it's required only for testing

I think it's better to move this part to a separate job. Probably near the dependencies job or before the tests job with a comment that it's required only for testing
gpesquero reviewed 2023-08-22 17:52:08 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-08-22 17:52:08 +00:00
Author
Member

Let me describe the latest tests and implementations that I've perfomed today:

  • Finally it was not needed to use the command localedef in macOS. I only had to rename the locales from xx_YY.UTF8 to xx_YY.UTF-8 to get them working in macOS.
  • I found out that in macOS the call to std::locale::global() does not affect the behavior of class NSLocale, so I've had to implement a new call GetLocale(std::string) in locale.mm order to obtain a new Locale struct based on the locale name in macOS.
  • Now comes the funny part: I've made tests with 4 different locales: EN, ES, FR and RU. With EN and ES there are no problems, but with FR locale the grouping separator character in Linux is a normal whitespace (\u0020), but in macOS it's a Narrow Non-Breaking Space (NNBSP, \u202F). And with RU locale, in Linux it's also a normal whitespace (\u0020), but in macOS it's a standard Non-Breaking Space (NBSP, \u00A0).

There's a mix of many different types of space characters in some locales and in different platforms, and the only possible work-around is to fill the testing code with #ifdef for the different platforms and locales.

Taking into consideration all these tests and the comment from @AndrewShkrob...

That will definetly break tests on local machines without all required locales.
At least, consider covering such test cases with #if CI

... I think that it's no worth the implementation of the UTs for the ToStringPrecision() function with different locales.

Let me know your opinion whether we shall continue to implement these UTs.

Let me describe the latest tests and implementations that I've perfomed today: - Finally it was not needed to use the command `localedef` in macOS. I only had to rename the locales from `xx_YY.UTF8` to `xx_YY.UTF-8` to get them working in macOS. - I found out that in macOS the call to `std::locale::global()` does not affect the behavior of class `NSLocale`, so I've had to implement a new call `GetLocale(std::string)` in `locale.mm` order to obtain a new Locale struct based on the locale name in macOS. - Now comes the funny part: I've made tests with 4 different locales: EN, ES, FR and RU. With EN and ES there are no problems, but with FR locale the grouping separator character in Linux is a normal whitespace (\u0020), but in macOS it's a Narrow Non-Breaking Space (NNBSP, \u202F). And with RU locale, in Linux it's also a normal whitespace (\u0020), but in macOS it's a standard Non-Breaking Space (NBSP, \u00A0). There's a mix of many different types of space characters in some locales and in different platforms, and the only possible work-around is to fill the testing code with `#ifdef` for the different platforms and locales. Taking into consideration all these tests and the comment from @AndrewShkrob... > That will definetly break tests on local machines without all required locales. > At least, consider covering such test cases with #if CI ... I think that it's no worth the implementation of the UTs for the `ToStringPrecision()` function with different locales. Let me know your opinion whether we shall continue to implement these UTs.
AndrewShkrob reviewed 2023-08-27 18:39:09 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-27 18:39:09 +00:00
Member

"100" + kNonBreakingSpace + "m"

I apologize for raising what might seem like trivial matters, but I believe it would enhance code readability to establish a distinct function. This way, we can encapsulate the use of kNonBreakingSpace and prevent its repetition on every line.

`"100" + kNonBreakingSpace + "m"` I apologize for raising what might seem like trivial matters, but I believe it would enhance code readability to establish a distinct function. This way, we can encapsulate the use of kNonBreakingSpace and prevent its repetition on every line.
gpesquero reviewed 2023-08-27 20:58:02 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-08-27 20:58:01 +00:00
Author
Member

Hi @AndrewShkrob! Please do not hesitate to make any comment about my code. You're most probably used to the coding standards/style in OM.

I've added the static function Distance::ToString() which encapsulates the usage of kNonBreakingSpace.

Please have a look at the code that I've just pushed and confirm that this approach is Ok.

Hi @AndrewShkrob! Please do not hesitate to make any comment about my code. You're most probably used to the coding standards/style in OM. I've added the `static` function `Distance::ToString()` which encapsulates the usage of `kNonBreakingSpace`. Please have a look at the code that I've just pushed and confirm that this approach is Ok.
gpesquero reviewed 2023-08-27 21:01:26 +00:00
.github/workflows/linux-check.yaml
@ -166,0 +171,4 @@
sudo locale-gen fr_FR.UTF-8
sudo locale-gen ru_RU
sudo locale-gen ru_RU.UTF-8
sudo update-locale
gpesquero commented 2023-08-27 21:01:26 +00:00
Author
Member

Hi @AndrewShkrob! I've removed the UTs for the ToStringPrecision() function, so these calls to sudo locale-gen XX_YY are no longer needed. Thanks anyway for your comment.

Hi @AndrewShkrob! I've removed the UTs for the `ToStringPrecision()` function, so these calls to `sudo locale-gen XX_YY` are no longer needed. Thanks anyway for your comment.
AndrewShkrob reviewed 2023-08-27 21:19:04 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-27 21:19:04 +00:00
Member

Certainly, there's no need for such a function inside the Distance class. Since it will solely be utilized in unit tests, I suggest moving it to platform/platform_tests/distance_tests.cpp.

Moreover, I concur that ToString might not be the most suitable name. How about renaming it to MakeDistanceStr or a similar variant?

Certainly, there's no need for such a function inside the Distance class. Since it will solely be utilized in unit tests, I suggest moving it to `platform/platform_tests/distance_tests.cpp`. Moreover, I concur that `ToString` might not be the most suitable name. How about renaming it to `MakeDistanceStr` or a similar variant?
gpesquero reviewed 2023-08-27 22:07:30 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-08-27 22:07:30 +00:00
Author
Member

Ok!! I've moved the function (now called MakeDistanceStr()) to platform/platform_tests/distance_tests.cpp.

Ok!! I've moved the function (now called `MakeDistanceStr()`) to `platform/platform_tests/distance_tests.cpp`.
AndrewShkrob reviewed 2023-08-28 08:41:21 +00:00
platform/platform_tests/distance_tests.cpp
AndrewShkrob commented 2023-08-28 08:41:21 +00:00
Member 
std::string MakeDistanceStr(std::string const & value, std::string const & unit)
```suggestion std::string MakeDistanceStr(std::string const & value, std::string const & unit) ```
gpesquero reviewed 2023-09-02 09:24:48 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-09-02 09:24:48 +00:00
Author
Member

Finally I decided to remove the UTs for the ToStringPrecision() function with different locales.

Finally I decided to remove the UTs for the `ToStringPrecision()` function with different locales.
biodranik (Migrated from github.com) reviewed 2023-09-02 20:27:23 +00:00
platform/platform_tests/distance_tests.cpp
biodranik (Migrated from github.com) commented 2023-09-02 20:27:23 +00:00

Testing it on different OSes with different locales actually was quite useful )

Testing it on different OSes with different locales actually was quite useful )
gpesquero reviewed 2023-09-03 18:59:20 +00:00
platform/platform_tests/distance_tests.cpp
gpesquero commented 2023-09-03 18:59:19 +00:00
Author
Member

Ok, I've brought back the UTs for the ToStringPrecision() function with different locales.

As the grouping separator is different for Linux and MacOS in some languages, the UNIT_TEST(ToStringPrecisionLocale) contains #ifdefs for different test results in different OS's.

I've included also a #if CI as suggested by @AndrewShkrob, so these tests are only performed in the GitHub workflows, and local tests do not require locales to be installed.

Ok, I've brought back the UTs for the `ToStringPrecision()` function with different locales. As the grouping separator is different for Linux and MacOS in some languages, the `UNIT_TEST(ToStringPrecisionLocale)` contains `#ifdefs` for different test results in different OS's. I've included also a `#if CI` as suggested by @AndrewShkrob, so these tests are only performed in the GitHub workflows, and local tests do not require locales to be installed.
gpesquero reviewed 2023-09-03 19:11:52 +00:00
.github/workflows/linux-check.yaml
@ -166,0 +171,4 @@
sudo locale-gen fr_FR.UTF-8
sudo locale-gen ru_RU
sudo locale-gen ru_RU.UTF-8
sudo update-locale
gpesquero commented 2023-09-03 19:11:52 +00:00
Author
Member

Hello again @AndrewShkrob! As I've included back again the UTs for the ToStringPrecision() function, I've included the sudo locale-gen xx_YY calls in the Github workflow for Linux, in the same place that I initially used.

You suggested to move this part to a separate job, but I'm not sure how to do it.

Currently, Linux tests are run in the last step 'Tests' of the 'linux-matrix' job.

I'd appreciate if you could provide a piece of code of your proposal for this change.

Hello again @AndrewShkrob! As I've included back again the UTs for the `ToStringPrecision()` function, I've included the `sudo locale-gen xx_YY` calls in the Github workflow for Linux, in the same place that I initially used. You suggested to move this part to a separate job, but I'm not sure how to do it. Currently, Linux tests are run in the last step 'Tests' of the 'linux-matrix' job. I'd appreciate if you could provide a piece of code of your proposal for this change.
biodranik (Migrated from github.com) reviewed 2023-09-04 00:27:10 +00:00
.github/workflows/linux-check.yaml
@ -166,0 +171,4 @@
sudo locale-gen fr_FR.UTF-8
sudo locale-gen ru_RU
sudo locale-gen ru_RU.UTF-8
sudo update-locale
biodranik (Migrated from github.com) commented 2023-09-04 00:27:10 +00:00
  1. You can move these calls to the tests section.
  2. Is there a way to detect if locale is installed and skip the test if it's not?
  3. Is there a way to get a locale-specific delimiter from a test and avoid ifdef?
1. You can move these calls to the tests section. 2. Is there a way to detect if locale is installed and skip the test if it's not? 3. Is there a way to get a locale-specific delimiter from a test and avoid ifdef?
biodranik (Migrated from github.com) reviewed 2023-09-06 21:44:05 +00:00
platform/locale_linux.cpp
biodranik (Migrated from github.com) commented 2023-09-06 21:44:04 +00:00

Why setting a global locale required? Does std::locale(localeName) work directly?

Why setting a global locale required? Does `std::locale(localeName)` work directly?
gpesquero reviewed 2023-09-06 22:06:19 +00:00
platform/locale_linux.cpp
gpesquero commented 2023-09-06 22:06:19 +00:00
Author
Member

You're right! There's no need to use std::locale::global(). Will change accordingly...

You're right! There's no need to use `std::locale::global()`. Will change accordingly...
gpesquero reviewed 2023-09-06 22:07:25 +00:00
.github/workflows/linux-check.yaml
@ -166,0 +171,4 @@
sudo locale-gen fr_FR.UTF-8
sudo locale-gen ru_RU
sudo locale-gen ru_RU.UTF-8
sudo update-locale
gpesquero commented 2023-09-06 22:07:25 +00:00
Author
Member

  1. I still don't know where you want these calls to be placed. They already are located in the 'Tests' step of the 'linux-matrix' job, and I don't see any other tests section in this 'linux-check.yaml" file. Please provide a diff file or some direct instructions (like: insert these calls in line XX of file YY), otherwise I'm lost regarding this change request.

  2. I've added some try/catch in the GetLocale() function in Linux to detect errors while loading the locales, so now the tests don't crash and we can skip the tests for the missing locales. Under macOS, GetLocale() never fails, even if a wrong locale is requested.

  3. I've removed the #ifdefs for grouping separator checking in different OS's by adding a AddGroupingSeparators() function that uses locale's grouping separator for the tests.

1. I still don't know where you want these calls to be placed. They already are located in the 'Tests' step of the 'linux-matrix' job, and I don't see any other tests section in this 'linux-check.yaml" file. Please provide a diff file or some direct instructions (like: insert these calls in line XX of file YY), otherwise I'm lost regarding this change request. 2. I've added some try/catch in the `GetLocale()` function in Linux to detect errors while loading the locales, so now the tests don't crash and we can skip the tests for the missing locales. Under macOS, `GetLocale()` never fails, even if a wrong locale is requested. 3. I've removed the `#ifdefs` for grouping separator checking in different OS's by adding a `AddGroupingSeparators()` function that uses locale's grouping separator for the tests.
gpesquero reviewed 2023-09-07 21:03:20 +00:00
platform/locale_linux.cpp
gpesquero commented 2023-09-07 21:03:20 +00:00
Author
Member

Commit amended!!

Commit amended!!
biodranik (Migrated from github.com) approved these changes 2023-09-19 08:01:22 +00:00
biodranik (Migrated from github.com) left a comment

Thank you very much for your work! And sorry for the delay. Let's see how it works in production :)

Thank you very much for your work! And sorry for the delay. Let's see how it works in production :)
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
AndrewShkrob
Jean-BaptisteC
biodranik
Labels
Clear labels   
Accessibility

issues affecting users with specific needs

  
Accessibility

issues affecting users with specific needs

  
Address

Issues with addresses, post codes, city/town/village borders

  
Address

Issues with addresses, post codes, city/town/village borders

  
Android

Android development

  
Android

Android development

  
Android Auto

Android Auto

  
Android Auto

Android Auto

  
Android Automotive (AAOS)

   
Android Automotive (AAOS)

   
API

Short Links, deep links and url schemes, including ge0 and om://

  
API

Short Links, deep links and url schemes, including ge0 and om://

  
AppGallery

Huawei AppGallery

  
AppGallery

Huawei AppGallery

  
AppStore

Apple AppStore

  
AppStore

Apple AppStore

  
Battery and Performance

Performance and battery consumption-related issues and optimizations

  
Battery and Performance

Performance and battery consumption-related issues and optimizations

  
Blocker

An issue blocking the release

  
Blocker

An issue blocking the release

  
Bookmarks and Tracks

Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export

  
Bookmarks and Tracks

Bookmarks, imported tracks, and KML, KMZ, KMB, GPX, GPZ import or export

  
Borders

Country borders and polygons

  
Borders

Country borders and polygons

  
Bug

Something isn't working

  
Bug

Something isn't working

  
Build

Compilation issue

  
Build

Compilation issue

  
CarPlay

CarPlay Integration

  
CarPlay

CarPlay Integration

  
Classificator

Classify object by types

  
Classificator

Classify object by types

  
Community

Community relations

  
Community

Community relations

  
Core

Cross-platform C++ libraries with a core functionality

  
Core

Cross-platform C++ libraries with a core functionality

  
CrashReports

Crash Reporting

  
CrashReports

Crash Reporting

  
Cycling

Bike, cycle, biking and related

  
Cycling

Bike, cycle, biking and related

  
Desktop

Desktop application

  
Desktop

Desktop application

  
DevEx

Developer Experience

  
DevEx

Developer Experience

  
DevOps

Automation and pipelines

  
DevOps

Automation and pipelines

  
dev_sandbox

A desktop app to debug rendering

  
dev_sandbox

A desktop app to debug rendering

  
Directions

Issues with next turns, labels, streets, voice instructions

  
Directions

Issues with next turns, labels, streets, voice instructions

  
Documentation

Improvements or additions to documentation

  
Documentation

Improvements or additions to documentation

  
Downloader

Map dowloader

  
Downloader

Map dowloader

  
Drape

Drape OpenGL, Vulkan and Metal graphics rendering engine

  
Drape

Drape OpenGL, Vulkan and Metal graphics rendering engine

  
Driving

Drive a car related issues

  
Driving

Drive a car related issues

  
Duplicate

This issue or pull request already exists

  
Duplicate

This issue or pull request already exists

  
Editor

OSM Editor

  
Editor

OSM Editor

  
Elevation

Isolines and altitude

  
Elevation

Isolines and altitude

  
Enhancement

New feature or request, an improvement of some existing feature

  
Enhancement

New feature or request, an improvement of some existing feature

  
Epic

A large body work

  
Epic

A large body work

  
External Map Datasets

TIGER, Ordnance Survey, ASTER, SRTM, etc.

  
External Map Datasets

TIGER, Ordnance Survey, ASTER, SRTM, etc.

  
F-Droid

F-Droid build and distribution

  
F-Droid

F-Droid build and distribution

  
Fonts

Font and text rendering

  
Fonts

Font and text rendering

  
Frequently User Reported

Most annoying UX and other issues for users reported by many people

  
Frequently User Reported

Most annoying UX and other issues for users reported by many people

  
Fund

   
Fund

   
Generator

Related to generator tool.

  
Generator

Related to generator tool.

  
Good first issue

Good for newcomers

  
Good first issue

Good for newcomers

  
Google Play

Google Play

  
Google Play

Google Play

  
GPS

Location and positioning issues

  
GPS

Location and positioning issues

  
GSoC

Ideas for Google Summer of Code

  
GSoC

Ideas for Google Summer of Code

  
iCloud

   
iCloud

   
Icons

Map and app icons

  
Icons

Map and app icons

  
iOS

iOS development

  
iOS

iOS development

  
Legal

Legal aspects

  
Legal

Legal aspects

  
Linux Desktop

Ubuntu, Gentoo, Red Hat, Debian, Cent OS, Fedora, etc.

  
Linux Desktop

Ubuntu, Gentoo, Red Hat, Debian, Cent OS, Fedora, etc.

  
Linux packaging

   
Linux packaging

   
Linux Phone

LibreM, PinePhone, Wayland and other mobile Linux versions

  
Linux Phone

LibreM, PinePhone, Wayland and other mobile Linux versions

  
Mac OS

Desktop issues related to Mac OS X, OM running on M1, M2, M3 Apple silicon, or Qt version

  
Mac OS

Desktop issues related to Mac OS X, OM running on M1, M2, M3 Apple silicon, or Qt version

  
Map Data

OpenStreetMap data related issues

  
Map Data

OpenStreetMap data related issues

  
Metro

Subway, Tube, light-rail public transport issues

  
Metro

Subway, Tube, light-rail public transport issues

  
Navigation

Issues related to the active Navigator / Navigation mode

  
Navigation

Issues related to the active Navigator / Navigation mode

  
Need Feedback

Further information is requested

  
Need Feedback

Further information is requested

  
Night Mode

Night or dark mode feature

  
Night Mode

Night or dark mode feature

  
NLnet 2024-06-281

Tasks related to NLnet 2024-06-281 project.

  
NLnet 2024-06-281

Tasks related to NLnet 2024-06-281 project.

  
No Feature Parity

A platform needs this to be implemented to have feature parity with other platforms

  
No Feature Parity

A platform needs this to be implemented to have feature parity with other platforms

  
Opening Hours

Business Hours, off time, holidays, seasonal

  
Opening Hours

Business Hours, off time, holidays, seasonal

  
Outdoors

Hiking, mtb, 4x4...

  
Outdoors

Hiking, mtb, 4x4...

  
POI Info

Feature metadata, OSM tags that are displayed in Place Page

  
POI Info

Feature metadata, OSM tags that are displayed in Place Page

  
Privacy

Privacy

  
Privacy

Privacy

  
Public Transport

Bus, trolleybus, train, tram, metro, subway, light rail

  
Public Transport

Bus, trolleybus, train, tram, metro, subway, light rail

  
Raw Idea

An idea that needs to be elaborated

  
Raw Idea

An idea that needs to be elaborated

  
Refactoring

   
Refactoring

   
Regional

This issue is different depending on the country or region

  
Regional

This issue is different depending on the country or region

  
Regression

A regression bug

  
Regression

A regression bug

  
Releases

Meta-tickets for tracking the release procedure

  
Releases

Meta-tickets for tracking the release procedure

  
RoboTest

Robo / Monkey Auto Tests

  
RoboTest

Robo / Monkey Auto Tests

  
Route Planning

Preview and plan your track

  
Route Planning

Preview and plan your track

  
Routing

Route building issues, e.g. valid route, valid ETA

  
Routing

Route building issues, e.g. valid route, valid ETA

  
Ruler

Helicopter routing, "as a crow flies"

  
Ruler

Helicopter routing, "as a crow flies"

  
Search

Search

  
Search

Search

  
Security

Security improvements

  
Security

Security improvements

  
Styles

Map drawing styles

  
Styles

Map drawing styles

  
Tests

   
Tests

   
Track Recording

Track Recording

  
Track Recording

Track Recording

  
Translations

Localization and translations issues

  
Translations

Localization and translations issues

  
TTS

Text-to-Speech

  
TTS

Text-to-Speech

  
UI

User interface issues

  
UI

User interface issues

  
UX

User eXperience, an issue with usability

  
UX

User eXperience, an issue with usability

  
Walk Navigation

Foot pedestrian mode

  
Walk Navigation

Foot pedestrian mode

  
Watches

Android Watches or Apple Watches support

  
Watches

Android Watches or Apple Watches support

  
Web

https://organicmaps.app/

  
Web

https://organicmaps.app/

  
Wikipedia

Issues related to Wiki articles embedded in OM data files.

  
Wikipedia

Issues related to Wiki articles embedded in OM data files.

  
Windows

Windows

  
Windows

Windows

  
Won't fix

Unfeasible or impossible to resolve

  
Won't fix

Unfeasible or impossible to resolve

  
World Map

Issues about improving the base map of the whole world that is bundled with the app

  
World Map

Issues about improving the base map of the whole world that is bundled with the app

No labels
Accessibility
Accessibility
Address
Address
Android
Android
Android Auto
Android Auto
Android Automotive (AAOS)
Android Automotive (AAOS)
API
API
AppGallery
AppGallery
AppStore
AppStore
Battery and Performance
Battery and Performance
Blocker
Blocker
Bookmarks and Tracks
Bookmarks and Tracks
Borders
Borders
Bug
Bug
Build
Build
CarPlay
CarPlay
Classificator
Classificator
Community
Community
Core
Core
CrashReports
CrashReports
Cycling
Cycling
Desktop
Desktop
DevEx
DevEx
DevOps
DevOps
dev_sandbox
dev_sandbox
Directions
Directions
Documentation
Documentation
Downloader
Downloader
Drape
Drape
Driving
Driving
Duplicate
Duplicate
Editor
Editor
Elevation
Elevation
Enhancement
Enhancement
Epic
Epic
External Map Datasets
External Map Datasets
F-Droid
F-Droid
Fonts
Fonts
Frequently User Reported
Frequently User Reported
Fund
Fund
Generator
Generator
Good first issue
Good first issue
Google Play
Google Play
GPS
GPS
GSoC
GSoC
iCloud
iCloud
Icons
Icons
iOS
iOS
Legal
Legal
Linux Desktop
Linux Desktop
Linux packaging
Linux packaging
Linux Phone
Linux Phone
Mac OS
Mac OS
Map Data
Map Data
Metro
Metro
Navigation
Navigation
Need Feedback
Need Feedback
Night Mode
Night Mode
NLnet 2024-06-281
NLnet 2024-06-281
No Feature Parity
No Feature Parity
Opening Hours
Opening Hours
Outdoors
Outdoors
POI Info
POI Info
Privacy
Privacy
Public Transport
Public Transport
Raw Idea
Raw Idea
Refactoring
Refactoring
Regional
Regional
Regression
Regression
Releases
Releases
RoboTest
RoboTest
Route Planning
Route Planning
Routing
Routing
Ruler
Ruler
Search
Search
Security
Security
Styles
Styles
Tests
Tests
Track Recording
Track Recording
Translations
Translations
TTS
TTS
UI
UI
UX
UX
Walk Navigation
Walk Navigation
Watches
Watches
Web
Web
Wikipedia
Wikipedia
Windows
Windows
Won't fix
Won't fix
World Map
World Map
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: organicmaps/organicmaps-tmp#4029
No description provided.
Delete branch "decimal"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?