organicmaps/organicmaps-tmp

Archived

[android] Enable detection of system locale change #7779

Open

gpesquero wants to merge 4 commits from gpesquero/separators into master

gpesquero commented

2024-04-01 23:36:12 +00:00

Member

This PR enables the detection of system locale change for Android, so there's no need to restart the app to display the correct decimal or grouping separator if there's a change of the Android device locale.

This PR also includes a commit for the core part, that adds a global pointer to store the current system locale. This change will add a few microsecs of overhead during runtime, but on the other side it will also enable to have unit tests for distance and altitude calls (to be implemented in PR organicmaps/organicmaps#6470).

This PR enables the detection of system locale change for Android, so there's no need to restart the app to display the correct decimal or grouping separator if there's a change of the Android device locale. This PR also includes a commit for the `core` part, that adds a global pointer to store the current system locale. This change will add a few microsecs of overhead during runtime, but on the other side it will also enable to have unit tests for distance and altitude calls (to be implemented in PR https://git.omaps.dev/organicmaps/organicmaps/pulls/6470).

rtsisyk reviewed 2024-04-01 23:36:12 +00:00

biodranik (Migrated from github.com) reviewed 2024-04-01 23:36:12 +00:00

biodranik (Migrated from github.com) reviewed 2024-04-02 21:40:16 +00:00

android/app/src/main/cpp/app/organicmaps/util/Language.cpp

biodranik (Migrated from github.com) commented

2024-04-02 21:30:07 +00:00

nit: do not specify unused variable names.

Java_app_organicmaps_util_Language_nativeRefreshSystemLocale(JNIEnv *, jclass)

nit: do not specify unused variable names. ```suggestion Java_app_organicmaps_util_Language_nativeRefreshSystemLocale(JNIEnv *, jclass) ```

android/app/src/main/java/app/organicmaps/MwmActivity.java

biodranik (Migrated from github.com) commented

2024-04-02 21:30:45 +00:00

nit: Aren't Java variables null by default?

nit: Aren't Java variables null by default?

android/app/src/main/java/app/organicmaps/MwmActivity.java

biodranik (Migrated from github.com) commented

2024-04-02 21:32:30 +00:00

Is onSafeCreate called only once per lifetime of the activity? If yes, then is this check really needed?

Is onSafeCreate called only once per lifetime of the activity? If yes, then is this check really needed?

biodranik (Migrated from github.com) commented

2024-04-02 21:33:15 +00:00

nit:

      mLocaleChangeReceiver = new BroadcastReceiver()
      {

nit: ```suggestion mLocaleChangeReceiver = new BroadcastReceiver() { ```

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

biodranik (Migrated from github.com) commented

2024-04-02 21:36:37 +00:00

The pointer is not needed.

static Locale g_systemLocale;

The pointer is not needed. ```suggestion static Locale g_systemLocale; ```

biodranik (Migrated from github.com) commented

2024-04-02 21:37:07 +00:00

Can it be also updated from iOS?

Can it be also updated from iOS?

biodranik (Migrated from github.com) commented

2024-04-02 21:37:36 +00:00

  SetSystemLocale(GetCurrentLocale());

```suggestion SetSystemLocale(GetCurrentLocale()); ```

biodranik (Migrated from github.com) commented

2024-04-02 21:38:59 +00:00

void SetSystemLocale(platform::Locale newLocale)
{
  g_systemysLocale = newLocale;
}

```suggestion void SetSystemLocale(platform::Locale newLocale) { g_systemysLocale = newLocale; } ```

biodranik (Migrated from github.com) commented

2024-04-02 21:39:32 +00:00

  SetSystemLocale(GetCurrentLocale());

```suggestion SetSystemLocale(GetCurrentLocale()); ```

biodranik (Migrated from github.com) commented

2024-04-02 21:39:53 +00:00

  return ToStringPrecisionLocale(g_systemLocale, d, pr);

```suggestion return ToStringPrecisionLocale(g_systemLocale, d, pr); ```

platform/measurement_utils.hpp

					
				@ -64,5 +64,9 @@ std::string OSMDistanceToMetersString(std::string const & osmRawValue,

				                                      bool supportZeroAndNegativeValues = true,

biodranik (Migrated from github.com) commented

2024-04-02 21:40:12 +00:00

void SetSystemLocale(platform::Locale newLocale);

```suggestion void SetSystemLocale(platform::Locale newLocale); ```

gpesquero reviewed 2024-04-03 19:45:39 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-04-03 19:45:39 +00:00

Author

Member

@biodranik: Please be aware that, in navigation mode, this ToStringPrecisionLocale() function is called 60 times per second: 20 Hz due to compass update and x3 values to format (total distance, next turn distance and speed), so we shall not make calls to GetCurrentLocale() this often.

That's why I was using a pointer to store the system locale. It's first set to null and it's only initialized once. The only overhead that we were adding is the check if the pointer is null.

If you don't like using pointers, then at least we shall use an additional static boolean variable to make a only-one-time call to GetCurrentLocale(), adding a minimum overhead:

static boolean firstTime = true;

if (firstTime)
{
  SetSystemLocale(GetCurrentLocale());

  firstTime = false;
}

I will amend the commit using this last approach.

@biodranik: Please be aware that, in navigation mode, this `ToStringPrecisionLocale()` function is called 60 times per second: 20 Hz due to compass update and x3 values to format (total distance, next turn distance and speed), so we shall not make calls to `GetCurrentLocale()` this often. That's why I was using a pointer to store the system locale. It's first set to null and it's only initialized once. The only overhead that we were adding is the check if the pointer is null. If you don't like using pointers, then at least we shall use an additional static boolean variable to make a only-one-time call to `GetCurrentLocale()`, adding a minimum overhead: ``` static boolean firstTime = true; if (firstTime) { SetSystemLocale(GetCurrentLocale()); firstTime = false; } ``` I will amend the commit using this last approach.

biodranik (Migrated from github.com) reviewed 2024-04-04 22:11:46 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

biodranik (Migrated from github.com) commented

2024-04-04 22:11:46 +00:00

OMG that is an issue then. There is no need to ~~format strings~~ call refresh so often. Any ideas on how to avoid calling it 60 times per second?

OMG that is an issue then. There is no need to ~format strings~ call refresh so often. Any ideas on how to avoid calling it 60 times per second?

biodranik (Migrated from github.com) reviewed 2024-04-04 22:25:25 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

biodranik (Migrated from github.com) commented

2024-04-04 22:25:25 +00:00

I'm starting to think that this code has become too overcomplicated. Let's think about how can we simplify it and avoid unnecessary overhead. Note that calling JNI 60 times per second is very expensive.

Also calling C++ to call Java from it to get locale to init in back in C++ looks very weird and complex. A cleaner solution would just set it once when necessary from Java, if we stick to this approach.

Can we simplify this whole code by implementing it natively in Java and iOS? Any other ideas?

I'm starting to think that this code has become too overcomplicated. Let's think about how can we simplify it and avoid unnecessary overhead. Note that calling JNI 60 times per second is very expensive. Also calling C++ to call Java from it to get locale to init in back in C++ looks very weird and complex. A cleaner solution would just set it once when necessary from Java, if we stick to this approach. Can we simplify this whole code by implementing it natively in Java and iOS? Any other ideas?

gpesquero reviewed 2024-04-04 23:22:26 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-04-04 23:22:26 +00:00

Author

Member

We already did the trade-off analysis of the different possible implementations for the locale distance/speed formatting in PR organicmaps/organicmaps#4029 (comment), and we came with the solution of implementing the formatting in the c++ side, due to these main reasons:

Faster formatting in c++ than in Java/iOS side.
Unique place for formatting implementation, otherwise we would have to make implementations for every platform (Android, iOS, macOS, Linux, Windows).

Regarding the 60 times per second update, before this PR we were calling the GetCurrentLocale() function only once (at app startup) as we were using a local static variable inside of the ToStringPrecisionLocale() function.

With the amend that I've just made 1 hour ago, a boolean variable just prevents from calling this functions more than once.

From my POV, the code is OK as it is now. The only remaining issue in this PR is to check if we can also detect system locale changes in iOS. I will investigate it.

We already did the trade-off analysis of the different possible implementations for the locale distance/speed formatting in PR https://git.omaps.dev/organicmaps/organicmaps/pulls/4029#issuecomment-1346347205, and we came with the solution of implementing the formatting in the c++ side, due to these main reasons: * Faster formatting in c++ than in Java/iOS side. * Unique place for formatting implementation, otherwise we would have to make implementations for every platform (Android, iOS, macOS, Linux, Windows). Regarding the 60 times per second update, before this PR we were calling the `GetCurrentLocale()` function only once (at app startup) as we were using a local static variable inside of the `ToStringPrecisionLocale()` function. With the amend that I've just made 1 hour ago, a boolean variable just prevents from calling this functions more than once. From my POV, the code is OK as it is now. The only remaining issue in this PR is to check if we can also detect system locale changes in iOS. I will investigate it.

gpesquero reviewed 2024-04-14 20:38:01 +00:00

android/app/src/main/java/app/organicmaps/MwmActivity.java

gpesquero commented

2024-04-14 20:38:01 +00:00

Author

Member

Removed default null assignment...

Removed default null assignment...

gpesquero reviewed 2024-04-14 20:39:34 +00:00

android/app/src/main/java/app/organicmaps/MwmActivity.java

gpesquero commented

2024-04-14 20:39:34 +00:00

Author

Member

Yes, onSafeCreate() is only called once for the activity, so I've removed the check...

Yes, `onSafeCreate()` is only called once for the activity, so I've removed the check...

gpesquero reviewed 2024-04-14 22:02:46 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-04-14 22:02:45 +00:00

Author

Member

Can it be also updated from iOS?

@kirylkaveryn, @fabwu: PTAL at the implementation to detect locale change for iOS. I've tried to make some tests with a simulator, but everytime that I change de iOS language/locate, the app is restarted so the currentLocaleDidChange function is never called...

> Can it be also updated from iOS? @kirylkaveryn, @fabwu: PTAL at the implementation to detect locale change for iOS. I've tried to make some tests with a simulator, but everytime that I change de iOS language/locate, the app is restarted so the `currentLocaleDidChange` function is never called...

biodranik (Migrated from github.com) reviewed 2024-04-14 22:22:07 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

biodranik (Migrated from github.com) commented

2024-04-14 22:22:07 +00:00

@gpesquero right, I forgot that on iOS all apps are restarted when the locale is changed, so it is not needed.

Are there any alternative solutions to fix unit tests? These locales are complicating the code a lot...

@gpesquero right, I forgot that on iOS all apps are restarted when the locale is changed, so it is not needed. Are there any alternative solutions to fix unit tests? These locales are complicating the code a lot...

gpesquero reviewed 2024-04-14 22:55:54 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-04-14 22:55:54 +00:00

Author

Member

Are there any alternative solutions to fix unit tests? These locales are complicating the code a lot...

Doing all the locale formatting in the Android/iOS part will make all the c++ code and related unit tests much cleaner, but we may encounter performance issues if we do this quite often distance & speed localized formatting using non-native functions (remember the checks that I did in organicmaps/organicmaps#4029 (comment)).

Let me make a few tests & measurements with a simple formatting in Android and making a performance comparison vs c++ implementation. I will get back to you once I made the tests.

We shall also review the update rate of the distance & speed while routing. Right now they are updated 20 times per second; IMO, this update rate shall be lower (5 times per second?)

> Are there any alternative solutions to fix unit tests? These locales are complicating the code a lot... Doing all the locale formatting in the Android/iOS part will make all the c++ code and related unit tests much cleaner, but we may encounter performance issues if we do this _quite often_ distance & speed localized formatting using non-native functions (remember the checks that I did in https://git.omaps.dev/organicmaps/organicmaps/pulls/4029#issuecomment-1646971596). Let me make a few tests & measurements with a simple formatting in Android and making a performance comparison vs c++ implementation. I will get back to you once I made the tests. We shall also review the update rate of the distance & speed while routing. Right now they are updated 20 times per second; IMO, this update rate shall be lower (5 times per second?)

fabwu (Migrated from github.com) reviewed 2024-04-15 03:10:06 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

fabwu (Migrated from github.com) commented

2024-04-15 03:10:05 +00:00

@gpesquero If I understand this correctly you need to change the locale for unit tests, so why don't you add a default parameter to ToStringPrecision() and overwrite the locale in the test?

@gpesquero If I understand this correctly you need to change the locale for unit tests, so why don't you add a default parameter to `ToStringPrecision()` and overwrite the locale in the test?

gpesquero reviewed 2024-04-15 19:44:30 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-04-15 19:44:30 +00:00

Author

Member

@gpesquero If I understand this correctly you need to change the locale for unit tests, so why don't you add a default parameter to ToStringPrecision() and overwrite the locale in the test?

Hi @fabwu: We already have 2 possible ways to call ToStringPrecision(): one with locale as an argument and the other uses an internal variable to store the system local that it's only updated once. This PR just adds a global variable to store the system locale, and that would allow us to change the locale during unit tests for Distance/Speed calls from c++, Android & iOS.

> @gpesquero If I understand this correctly you need to change the locale for unit tests, so why don't you add a default parameter to `ToStringPrecision()` and overwrite the locale in the test? Hi @fabwu: We already have 2 possible ways to call `ToStringPrecision()`: one with locale as an argument and the other uses an internal variable to store the system local that it's only updated once. This PR just adds a global variable to store the system locale, and that would allow us to change the locale during unit tests for Distance/Speed calls from c++, Android & iOS.

gpesquero reviewed 2024-04-15 19:51:03 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-04-15 19:51:03 +00:00

Author

Member

@gpesquero right, I forgot that on iOS all apps are restarted when the locale is changed, so it is not needed.

I've removed the locale change detection for iOS.

> @gpesquero right, I forgot that on iOS all apps are restarted when the locale is changed, so it is not needed. I've removed the locale change detection for iOS.

rtsisyk requested changes 2024-05-25 15:22:00 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

rtsisyk commented

2024-05-25 15:21:57 +00:00

Owner

This pattern looks highly controversial. Why don't call SetSystemLocale() during initialization of the app before calling ToStringPrecision()?

This pattern looks highly controversial. Why don't call SetSystemLocale() during initialization of the app before calling ToStringPrecision()?

rtsisyk approved these changes 2024-08-24 10:20:07 +00:00

rtsisyk left a comment

Owner

LGTM with comments

LGTM with comments

android/app/src/main/java/app/organicmaps/MwmActivity.java

rtsisyk commented

2024-08-24 10:17:07 +00:00

Owner

This receiver probably will be more appropriate in MwmApplication instead of MwmActivity.

This receiver probably will be more appropriate in MwmApplication instead of MwmActivity.

gpesquero reviewed 2024-08-26 20:21:03 +00:00

platform/measurement_utils.cpp

					
				@ -19,2 +19,4 @@

				namespace measurement_utils

				{

				// Global Locale variable to store system locale.

				static platform::Locale g_systemLocale =

gpesquero commented

2024-08-26 20:21:03 +00:00

Author

Member

@rtsisyk: I've removed this firstTime boolean variable and added Locale initialization at app start-up.

@rtsisyk: I've removed this `firstTime` boolean variable and added Locale initialization at app start-up.

gpesquero reviewed 2024-08-26 20:22:20 +00:00

android/app/src/main/java/app/organicmaps/MwmActivity.java

gpesquero commented

2024-08-26 20:22:20 +00:00

Author

Member

This receiver probably will be more appropriate in MwmApplication instead of MwmActivity.

@rtsisyk: Moved Locale initialization and BroadcastReceiver to MwmApplication.

> This receiver probably will be more appropriate in MwmApplication instead of MwmActivity. @rtsisyk: Moved Locale initialization and `BroadcastReceiver` to `MwmApplication`.

biodranik (Migrated from github.com) reviewed 2024-08-26 20:39:21 +00:00

platform/measurement_utils.cpp

biodranik (Migrated from github.com) commented

2024-08-26 20:39:21 +00:00

The previous implementation of a function allowed to unit-test it nicely. Would it be better to pass the g_systemLocale as a parameter?

The previous implementation of a function allowed to unit-test it nicely. Would it be better to pass the g_systemLocale as a parameter?

biodranik (Migrated from github.com) reviewed 2024-08-26 20:41:21 +00:00

platform/measurement_utils.cpp

biodranik (Migrated from github.com) commented

2024-08-26 20:41:21 +00:00

How will it work on iOS?

How will it work on iOS?

gpesquero reviewed 2024-08-27 20:06:55 +00:00

platform/measurement_utils.cpp

gpesquero commented

2024-08-27 20:06:55 +00:00

Author

Member

The previous implementation of a function allowed to unit-test it nicely. Would it be better to pass the g_systemLocale as a parameter?

I've returned to the previous implementation, leaving the function ToStringPrecisionLocale() and also I've added unit tests for the function ToStringPrecision().

I've had to set the default grouping separator as comma in order for the distance_tests to work.

> The previous implementation of a function allowed to unit-test it nicely. Would it be better to pass the g_systemLocale as a parameter? I've returned to the previous implementation, leaving the function `ToStringPrecisionLocale()` and also I've added unit tests for the function `ToStringPrecision()`. I've had to set the default grouping separator as `comma` in order for the `distance_tests` to work.

gpesquero reviewed 2024-08-27 20:10:48 +00:00

platform/measurement_utils.cpp

gpesquero commented

2024-08-27 20:10:48 +00:00

Author

Member

On iOS we already confirmed that the app is re-started when the locale is changed, so we decided above to remove locale change detection in iOS.
On iOS, the locale is set in cpp only at app start-up.

On iOS we already confirmed that the app is re-started when the locale is changed, so we decided [above](https://git.omaps.dev/organicmaps/organicmaps/pulls/7779#discussion_r1566352616) to remove locale change detection in iOS. On iOS, the locale is set in cpp only at app start-up.

biodranik (Migrated from github.com) reviewed 2024-08-27 20:57:33 +00:00

biodranik (Migrated from github.com) left a comment

Thanks for reminding about the iOS!

Thanks for reminding about the iOS!

platform/measurement_utils.cpp

biodranik (Migrated from github.com) commented

2024-08-27 20:53:46 +00:00

static platform::Locale g_systemLocale =
{

C++20 allows to specify which struct members are initialized, for clarity.

Did you also test the desktop version? How will it behave with "undefined" locale values? On desktops (Mac, Linux) locale is properly set when the app is launched.

```suggestion static platform::Locale g_systemLocale = { ``` C++20 allows to specify which struct members are initialized, for clarity. Did you also test the desktop version? How will it behave with "undefined" locale values? On desktops (Mac, Linux) locale is properly set when the app is launched.

platform/measurement_utils.cpp

					
				@ -21,0 +37,4 @@

				{

				  SetSystemLocale(platform::GetCurrentLocale());

				}

biodranik (Migrated from github.com) commented

2024-08-27 20:56:35 +00:00

The previous approach initialized locale on startup without any additional calls/refreshes. Now what happens on iOS or Linux or Mac?

The previous approach initialized locale on startup without any additional calls/refreshes. Now what happens on iOS or Linux or Mac?

gpesquero reviewed 2024-08-28 19:52:11 +00:00

platform/measurement_utils.cpp

					
				@ -21,0 +37,4 @@

				{

				  SetSystemLocale(platform::GetCurrentLocale());

				}

gpesquero commented

2024-08-28 19:52:11 +00:00

Author

Member

Now what happens on iOS or Linux or Mac?

For iOS I've already included a call to measurement_utils::RefreshSystemLocale() in main.mm, but we would have to include additional calls for desktop Linux/Mac (and also for Android Auto?).

Instead of having to add this locale init calls for each environment, the best would be to make it in a single point (as we're doing now with the local static variable).

My first proposal in this PR was to use a pointer for the global locale variable, but it was dismissed:

std::string ToStringPrecision(double d, int pr)
{
  if (pSysLocale == null)
    RefreshSystemLocale();
    
  return ToStringPrecisionLocale(*pSysLocale, d, pr);
}

One other proposal was to use a boolean variable to init the locale only once, but it was also dismissed:

static bool firstTime = true;

// Get current system locale only once.
if (firstTime)
  RefreshSystemLocale();

One third proposal that I'm doing here is to check if the language code of the sys locale is empty:

std::string ToStringPrecision(double d, int pr)
{
  if (g_systemLocale.m_language.empty())
    RefreshSystemLocale();

  return ToStringPrecisionLocale(*pSysLocale, d, pr);
}

> Now what happens on iOS or Linux or Mac? For iOS I've already included a call to `measurement_utils::RefreshSystemLocale()` in `main.mm`, but we would have to include additional calls for desktop Linux/Mac (and also for Android Auto?). Instead of having to add this locale init calls for each environment, the best would be to make it in a single point (as we're doing now with the local static variable). My first proposal in this PR was to use a pointer for the global locale variable, but it was dismissed: ``` std::string ToStringPrecision(double d, int pr) { if (pSysLocale == null) RefreshSystemLocale(); return ToStringPrecisionLocale(*pSysLocale, d, pr); } ``` One other proposal was to use a boolean variable to init the locale only once, but it was also dismissed: ``` static bool firstTime = true; // Get current system locale only once. if (firstTime) RefreshSystemLocale(); ``` One third proposal that I'm doing here is to check if the language code of the sys locale is empty: ``` std::string ToStringPrecision(double d, int pr) { if (g_systemLocale.m_language.empty()) RefreshSystemLocale(); return ToStringPrecisionLocale(*pSysLocale, d, pr); } ```

gpesquero reviewed 2024-08-29 18:15:32 +00:00

platform/measurement_utils.cpp

gpesquero commented

2024-08-29 18:15:32 +00:00

Author

Member

I've checked the Linux desktop version and the distance formatting does not seem to follow system's locale (neither with the previous nor with the new implementation). That problem for desktop shall be managed in a separate issue...

I've checked the Linux desktop version and the distance formatting does not seem to follow system's locale (neither with the previous nor with the new implementation). That problem for desktop shall be managed in a separate issue...

biodranik (Migrated from github.com) reviewed 2024-08-29 19:42:58 +00:00

platform/measurement_utils.cpp

					
				@ -21,0 +37,4 @@

				{

				  SetSystemLocale(platform::GetCurrentLocale());

				}

biodranik (Migrated from github.com) commented

2024-08-29 19:42:58 +00:00

An unnecessary condition check on every call is not a good solution.

What's wrong with the old approach of initializing the global variable, but making it non-const, and updating it from the UI only when necessary/when it really changed?

An unnecessary condition check on every call is not a good solution. What's wrong with the old approach of initializing the global variable, but making it non-const, and updating it from the UI only when necessary/when it really changed?

gpesquero reviewed 2024-08-29 20:56:00 +00:00

platform/measurement_utils.cpp

					
				@ -21,0 +37,4 @@

				{

				  SetSystemLocale(platform::GetCurrentLocale());

				}

gpesquero commented

2024-08-29 20:56:00 +00:00

Author

Member

What's wrong with the old approach of initializing the global variable, but making it non-const, and updating it from the UI only when necessary/when it really changed?

I have no problem with keeping the global variable. Right now we're initializing it on both iOS and Android.

For Linux/macOS desktop, further checks shall be made on a separate issue after this PR is merged.

> What's wrong with the old approach of initializing the global variable, but making it non-const, and updating it from the UI only when necessary/when it really changed? I have no problem with keeping the global variable. Right now we're initializing it on both iOS and Android. For Linux/macOS desktop, further checks shall be made on a separate issue after this PR is merged.

gpesquero reviewed 2024-10-20 20:21:47 +00:00

platform/measurement_utils.cpp

					
				@ -21,0 +37,4 @@

				{

				  SetSystemLocale(platform::GetCurrentLocale());

				}

gpesquero commented

2024-10-20 20:21:47 +00:00

Author

Member

Now what happens on iOS or Linux or Mac?

I've also implemented the locale initialization at app start-up on Linux.

> Now what happens on iOS or Linux or Mac? I've also implemented the locale initialization at app start-up on Linux.

This repo is archived. You cannot comment on pull requests.

No reviewers

No labels

Android Automotive (AAOS)

Android Automotive (AAOS)

Battery and Performance

Battery and Performance

Bookmarks and Tracks

Bookmarks and Tracks

External Map Datasets

External Map Datasets

Frequently User Reported

Frequently User Reported

Good first issue

Good first issue

Linux packaging

Linux packaging

NLnet 2024-06-281

NLnet 2024-06-281

No Feature Parity

No Feature Parity

Public Transport

Public Transport

Track Recording

Track Recording

Walk Navigation

Walk Navigation

No milestone

No project

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#7779

No description provided.