[android] Enable detection of system locale change #7779

Open
gpesquero wants to merge 4 commits from gpesquero/separators into master
9 changed files with 72 additions and 13 deletions

View file

@ -1,4 +1,5 @@
#include "android/app/src/main/cpp/app/organicmaps/core/jni_helper.hpp"
#include "platform/measurement_utils.hpp"
#include "platform/preferred_languages.hpp"
extern "C"
@ -9,4 +10,10 @@ Java_app_organicmaps_util_Language_nativeNormalize(JNIEnv *env, jclass type, jst
std::string locale = languages::Normalize(jni::ToNativeString(env, lang));
return jni::ToJavaString(env, locale);
}
JNIEXPORT void JNICALL
Java_app_organicmaps_util_Language_nativeRefreshSystemLocale(JNIEnv *, jclass)
{
measurement_utils::RefreshSystemLocale();
}
}

View file

@ -4,7 +4,10 @@ import static app.organicmaps.location.LocationState.LOCATION_TAG;
import android.app.Activity;
import android.app.Application;
import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.content.SharedPreferences;
import android.os.Bundle;
import android.os.Handler;
@ -41,6 +44,7 @@ import app.organicmaps.settings.StoragePathManager;
import app.organicmaps.sound.TtsPlayer;
import app.organicmaps.util.Config;
import app.organicmaps.util.ConnectionState;
import app.organicmaps.util.Language;
import app.organicmaps.util.SharedPropertiesUtils;
import app.organicmaps.util.StorageUtils;
import app.organicmaps.util.ThemeSwitcher;
@ -89,6 +93,8 @@ public class MwmApplication extends Application implements Application.ActivityL
@Nullable
private WeakReference<Activity> mTopActivity;
private BroadcastReceiver mLocaleChangeReceiver;
@UiThread
@Nullable
public Activity getTopActivity()
@ -213,6 +219,22 @@ public class MwmApplication extends Application implements Application.ActivityL
Config.setStoragePath(writablePath);
Config.setStatisticsEnabled(SharedPropertiesUtils.isStatisticsEnabled(this));
// Force native system locale initialization at app start-up.
Language.nativeRefreshSystemLocale();
// Setup BroadcastReceiver to receive changes of system locale.
mLocaleChangeReceiver = new BroadcastReceiver()
{
@Override
public void onReceive(Context context, Intent intent)
{
// Refresh the native c++ system locale.
Language.nativeRefreshSystemLocale();
}
};
registerReceiver(mLocaleChangeReceiver, new IntentFilter(Intent.ACTION_LOCALE_CHANGED));
mPlatformInitialized = true;
Logger.i(TAG, "Platform initialized");
}

View file

@ -58,4 +58,7 @@ public class Language
@NonNull
public static native String nativeNormalize(@NonNull String locale);
@NonNull
public static native void nativeRefreshSystemLocale();
}

View file

@ -1,6 +1,7 @@
#import "MapsAppDelegate.h"
#import "MWMSettings.h"
#include "platform/measurement_utils.hpp"
#include "platform/platform.hpp"
int main(int argc, char * argv[])
@ -9,6 +10,9 @@ int main(int argc, char * argv[])
auto & p = GetPlatform();
LOG(LINFO, (p.Version(), "started, detected CPU cores:", p.CpuCores()));
// Force system locale initialization at app start-up.
measurement_utils::RefreshSystemLocale();
int retVal;
@autoreleasepool
{

View file

@ -18,12 +18,29 @@
namespace measurement_utils
{
// Global Locale variable to store system locale.
static platform::Locale g_systemLocale =
biodranik commented 2024-04-02 21:36:37 +00:00 (Migrated from github.com)
Review

The pointer is not needed.

static Locale g_systemLocale;
The pointer is not needed. ```suggestion static Locale g_systemLocale; ```
biodranik commented 2024-04-02 21:37:07 +00:00 (Migrated from github.com)
Review

Can it be also updated from iOS?

Can it be also updated from iOS?
biodranik commented 2024-04-02 21:37:36 +00:00 (Migrated from github.com)
Review
  SetSystemLocale(GetCurrentLocale());
```suggestion SetSystemLocale(GetCurrentLocale()); ```
biodranik commented 2024-04-02 21:38:59 +00:00 (Migrated from github.com)
Review
void SetSystemLocale(platform::Locale newLocale)
{
  g_systemysLocale = newLocale;
}
```suggestion void SetSystemLocale(platform::Locale newLocale) { g_systemysLocale = newLocale; } ```
biodranik commented 2024-04-02 21:39:32 +00:00 (Migrated from github.com)
Review
  SetSystemLocale(GetCurrentLocale());
```suggestion SetSystemLocale(GetCurrentLocale()); ```
biodranik commented 2024-04-02 21:39:53 +00:00 (Migrated from github.com)
Review
  return ToStringPrecisionLocale(g_systemLocale, d, pr);
```suggestion return ToStringPrecisionLocale(g_systemLocale, d, pr); ```
Review

@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 commented 2024-04-04 22:11:46 +00:00 (Migrated from github.com)
Review

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 commented 2024-04-04 22:25:25 +00:00 (Migrated from github.com)
Review

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?
Review

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.
Review

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 commented 2024-04-14 22:22:07 +00:00 (Migrated from github.com)
Review

@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...
Review

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 commented 2024-04-15 03:10:05 +00:00 (Migrated from github.com)
Review

@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?
Review

@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.
Review

@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.
Review

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()?
Review

@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.
{
"", // Undefined language.
"", // Undefined country.
"", // Undefined currency.
".", // Dot as default decimal separator.
"," // Comma as default grouping (thousands) separator.
};
void SetSystemLocale(platform::Locale const & locale)
{
g_systemLocale = locale;
}
void RefreshSystemLocale()
{
SetSystemLocale(platform::GetCurrentLocale());
}
biodranik commented 2024-08-27 20:56:35 +00:00 (Migrated from github.com)
Review

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?
Review

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); } ```
biodranik commented 2024-08-29 19:42:58 +00:00 (Migrated from github.com)
Review

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?
Review

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.
Review

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.
std::string ToStringPrecision(double d, int pr)
{
// We assume that the app will be restarted if a user changes device's locale.
static auto const locale = platform::GetCurrentLocale();
return ToStringPrecisionLocale(locale, d, pr);
return ToStringPrecisionLocale(g_systemLocale, d, pr);
}
std::string ToStringPrecisionLocale(platform::Locale const & loc, double d, int pr)

View file

@ -64,5 +64,9 @@ std::string OSMDistanceToMetersString(std::string const & osmRawValue,
bool supportZeroAndNegativeValues = true,
biodranik commented 2024-04-02 21:40:12 +00:00 (Migrated from github.com)
Review
void SetSystemLocale(platform::Locale newLocale);
```suggestion void SetSystemLocale(platform::Locale newLocale); ```
int digitsAfterComma = 2);
std::string ToStringPrecision(double d, int pr);
std::string ToStringPrecisionLocale(platform::Locale const & locale, double d, int pr);
std::string ToStringPrecisionLocale(platform::Locale const & loc, double d, int pr);
void SetSystemLocale(platform::Locale const & locale);
void RefreshSystemLocale();
} // namespace measurement_utils

View file

@ -181,5 +181,10 @@ UNIT_TEST(ToStringPrecisionLocale)
TEST_EQUAL(measurement_utils::ToStringPrecisionLocale(loc, d1, pr1), data.d1String, ());
TEST_EQUAL(measurement_utils::ToStringPrecisionLocale(loc, d2, pr2),
AddGroupingSeparators(d2String, loc.m_groupingSeparator), ());
measurement_utils::SetSystemLocale(loc);
TEST_EQUAL(measurement_utils::ToStringPrecision(d1, pr1), data.d1String, ());
TEST_EQUAL(measurement_utils::ToStringPrecision(d2, pr2),
AddGroupingSeparators(d2String, loc.m_groupingSeparator), ());
}
}

View file

@ -102,6 +102,9 @@ public:
int main(int argc, char * argv[])
{
// Refresh system locale for distance & speed formatting.
measurement_utils::RefreshSystemLocale();
// Our double parsing code (base/string_utils.hpp) needs dots as a floating point delimiters, not commas.
// TODO: Refactor our doubles parsing code to use locale-independent delimiters.
// For example, https://github.com/google/double-conversion can be used.

View file

@ -1,6 +1,7 @@
#include "qt/ruler.hpp"
#include "geometry/mercator.hpp"
#include "platform/distance.hpp"
#include <iomanip>
#include <string>
@ -52,13 +53,6 @@ void Ruler::SetDistance()
void Ruler::SetId()
{
std::ostringstream curValStream;
curValStream << std::fixed << std::setprecision(1);
if (m_sumDistanceM >= 1000.0)
curValStream << m_sumDistanceM / 1000.0 << " km";
else
curValStream << m_sumDistanceM << " m";
m_id = curValStream.str();
m_id = platform::Distance::CreateFormatted(m_sumDistanceM).ToString();
}
} // namespace qt