Locale distance formatter #668

devotaaabel · 2018-01-25T16:35:28Z

Refactored the distance formatter for simplicity, and also added locale to options, so that the numbers can be formatted properly for the given locale. I also added a test to verify that the refactored code behavior matched the previous behavior, and to test the locale formatting. Unit type is still specified in the options separately. We could potentially use the locale to pick the unit type if no unit type is specified.

Output from tests:

Meters	Locale	Formatted String
18124.65	en_US	11 mi
18124.65	fr_FR	18 km
109.73	en_US	350 ft
109.73	fr_FR	100 m
9812.33	fr_FR	9,8 km
9812.33	en_US	6.1 mi
13.71	fr_FR	50 m
13.71	en_US	50 ft

…way, the special handling for returning the unspanned string can be removed

…ring

…nce-formatter

ericrwolfe · 2018-01-25T18:33:27Z

...-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/DistanceUtils.java

+    UNIT_STRINGS.put(UNIT_KILOMETERS, "km");
+    UNIT_STRINGS.put(UNIT_METERS, "m");
+    UNIT_STRINGS.put(UNIT_MILES, "mi");
+    UNIT_STRINGS.put(UNIT_FEET, "ft");


Keep in mind that we'll eventually need to localize these for certain languages, so if we can make the "km", "m", "mi", "ft" here easily localizable then 👍:
1,2 km (fr-FR) = 1.2公里 (zh-CN) = 1,2 км (ru-RU)

That being said, the only languages on this list (https://www.mapbox.com/api-documentation/#instructions-languages) that don't support the above hardcoded unit strings would be Chinese and Russian, so this isn't a priority we need to worry too much about right now if it turns out to be a lot of work.

@ericrwolfe good catch, should be easy.

… made DistanceUtils instantiable so that the user doesn't have to pass in fields every time

danesfeder

This is looking great so far. ❤️ the tests you added. Let's run with your idea of using SharedPreferences for the locale value. This way devs can override it on their side if they need to for whatever reason

danesfeder · 2018-01-25T21:59:08Z

.../main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationOptions.java

@@ -115,6 +121,7 @@ public static Builder builder() {
      .enableNotification(true)
      .isFromNavigationUi(false)
      .isDebugLoggingEnabled(false)
-      .unitType(NavigationUnitType.TYPE_IMPERIAL);
+      .unitType(NavigationUnitType.TYPE_IMPERIAL)
+      .locale(Locale.US);


Let's use Locale.getDefault() for the builder() here

danesfeder · 2018-01-25T22:09:00Z

.../src/main/java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionView.java

@@ -108,6 +108,7 @@ public InstructionView(Context context, @Nullable AttributeSet attrs) {

  public InstructionView(Context context, @Nullable AttributeSet attrs, int defStyleAttr) {
    super(context, attrs, defStyleAttr);
+    locale = context.getResources().getConfiguration().locale;


Replace with extraction from SharedPreferences

@danesfeder I was thinking just pass the context to DistanceUtils, and then DistanceUtils can get the locale from SharedPreferences

@devotaaabel yeah that sounds good to me - just need to make sure we aren't calling DistanceUtils somewhere where we don't have Context. Or have a backup method that can be called without context

@danesfeder To localize the strings like Eric said we'll need the context, but it shouldn't be an issue anyway

danesfeder · 2018-01-25T22:09:23Z

...i/src/main/java/com/mapbox/services/android/navigation/ui/v5/summary/SummaryBottomSheet.java

@@ -51,6 +49,7 @@ public SummaryBottomSheet(Context context, AttributeSet attrs) {

  public SummaryBottomSheet(Context context, AttributeSet attrs, int defStyleAttr) {
    super(context, attrs, defStyleAttr);
+    locale = context.getResources().getConfiguration().locale;


Same here with the SharedPref deal

danesfeder · 2018-01-25T22:15:56Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationActivity.java

@@ -118,6 +118,7 @@ private void extractConfiguration(NavigationViewOptions.Builder options) {
    MapboxNavigationOptions navigationOptions = MapboxNavigationOptions.builder()
      .unitType(preferences.getInt(NavigationConstants.NAVIGATION_VIEW_UNIT_TYPE,
        NavigationUnitType.TYPE_IMPERIAL))
+      .locale(getResources().getConfiguration().locale)


Great call here using the configuration locale here instead of Locale.getDefault()

danesfeder · 2018-01-25T22:20:50Z

...gation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationActivity.java

@@ -118,6 +118,7 @@ private void extractConfiguration(NavigationViewOptions.Builder options) {
    MapboxNavigationOptions navigationOptions = MapboxNavigationOptions.builder()
      .unitType(preferences.getInt(NavigationConstants.NAVIGATION_VIEW_UNIT_TYPE,
        NavigationUnitType.TYPE_IMPERIAL))


It'd be cool to get a default unit type based on the local here (if one isn't set in the preferences) with something like https://stackoverflow.com/questions/4898237/using-locale-settings-to-detect-wheter-to-use-imperial-units.

Current catch is the unit type isn't currently nullable in MapboxNavigationOptions, so we can switch that to @Nullable and if someone doesn't set it, we dont set the preference and get the default here when setting the default value for the SharedPreference

…special cases

…nce-formatter

danesfeder

Looks great, just a few final nit-picks then feel free to merge so you are unblocked for the voice instruction locale PR 🚀

danesfeder · 2018-02-06T14:55:02Z

...ation-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/NavigationViewModel.java

+      editor.putString(NavigationConstants.NAVIGATION_VIEW_LOCALE_COUNTRY, options.locale().getCountry());
+    }
+
+    editor.apply();


Let's extract the above lines 217-225 into a private method for readability

danesfeder · 2018-02-06T15:00:41Z

...n/java/com/mapbox/services/android/navigation/ui/v5/summary/list/InstructionListAdapter.java

    stepList = new ArrayList<>();
-    decimalFormat = new DecimalFormat(NavigationConstants.DECIMAL_FORMAT);
+    this.context = context;


Rather than store Context here as a field, would it be okay to create the instance of DistanceUtils here with the Context and assign that to a field? Context can cause leaks sometimes when storing it like this.

danesfeder · 2018-02-06T15:01:49Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/utils/LocaleUtils.java

+      return unitType;
+    }
+
+    // If unit type isn't specified, use default for locale


This is great!

…eaks

Devota Aabel added 16 commits January 24, 2018 12:27

Refactored formatting method name

6d1a6e1

Added DistanceUtilsTest

441759c

Added tests to DistanceUtilsTest

9cd072f

Set spanEnabled to true in only place that it was set to false. This …

4284cab

…way, the special handling for returning the unspanned string can be removed

Changed SpannableStringBuilder to SpannableString

d6dde29

Removed decimalFormat and spanEnabled and added locale to DistanceUtils

5728d4d

Did refactoring and simlified code, and added tests for locale

c86ac35

Updated all usages of DistanceUtils to use locale

26ba2b5

Fixed checkstyle issues

bb93d6a

Moved toString so that SummaryModel.getDistanceRemaining returns a st…

62e1fbd

…ring

Merge remote-tracking branch 'origin/master' into devota-locale-dista…

2feb26b

…nce-formatter

Made fields final

1c116c5

Removed whitespace

dd18e3d

Rearranged fields

8d47efb

Made fields final

701a226

Removed whitespace

1473409

devotaaabel requested review from cammace and danesfeder January 25, 2018 16:35

Formatted tests

4e103e1

ericrwolfe reviewed Jan 25, 2018

View reviewed changes

Devota Aabel added 2 commits January 25, 2018 14:45

Added unit abbreviations to strings.xml for future localization. Also…

db1380b

… made DistanceUtils instantiable so that the user doesn't have to pass in fields every time

Added context to all calls to DistanceUtils

21a5fbe

danesfeder suggested changes Jan 25, 2018

View reviewed changes

Devota Aabel and others added 7 commits January 25, 2018 18:16

Used only context as parameter of DistanceUtils

9d85ef5

Added special handling for locale and unit types and added tests for …

95ab03b

…special cases

Fixed checkstyle issues

98970e5

Removed unused locales

dbb24e4

Merge branch 'master' into devota-locale-distance-formatter

60daf6c

Added LocaleUtils

80d9f36

Fixed typo

1f603fa

Devota Aabel added 3 commits January 29, 2018 16:58

Added special handling for deprecated locale

d5f80b1

Merge remote-tracking branch 'origin/master' into devota-locale-dista…

06ad29e

…nce-formatter

Updated test for use of LocaleList for sdk>=N

4af1c51

danesfeder approved these changes Feb 6, 2018

View reviewed changes

devotaaabel requested a review from Guardiola31337 February 7, 2018 14:49

Devota Aabel added 3 commits February 7, 2018 13:51

Removed directionsLanguage from NavigationViewOptions

35d8bfa

Added setLocale helper method to LocaleUtils

a706541

Stopped storing context in InstructionListAdapter to prevent memory l…

c5bd115

…eaks

devotaaabel merged commit 33ff396 into master Feb 7, 2018

devotaaabel deleted the devota-locale-distance-formatter branch February 7, 2018 23:22

devotaaabel restored the devota-locale-distance-formatter branch February 8, 2018 15:34

danesfeder mentioned this pull request Feb 26, 2018

Release v0.10.0 #722

Closed

13 tasks

danesfeder deleted the devota-locale-distance-formatter branch February 28, 2018 15:31

danesfeder mentioned this pull request Mar 2, 2018

Use device Locale to infer NavigationUnitType #526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Locale distance formatter #668

Locale distance formatter #668

devotaaabel commented Jan 25, 2018 •

edited

Loading

ericrwolfe Jan 25, 2018 •

edited

Loading

devotaaabel Jan 25, 2018

danesfeder left a comment •

edited

Loading

danesfeder Jan 25, 2018

danesfeder Jan 25, 2018

devotaaabel Jan 25, 2018

danesfeder Jan 25, 2018

devotaaabel Jan 25, 2018

danesfeder Jan 25, 2018

danesfeder Jan 25, 2018

danesfeder Jan 25, 2018

danesfeder left a comment

danesfeder Feb 6, 2018

danesfeder Feb 6, 2018

danesfeder Feb 6, 2018

Locale distance formatter #668

Locale distance formatter #668

Conversation

devotaaabel commented Jan 25, 2018 • edited Loading

ericrwolfe Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danesfeder left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danesfeder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devotaaabel commented Jan 25, 2018 •

edited

Loading

ericrwolfe Jan 25, 2018 •

edited

Loading

danesfeder left a comment •

edited

Loading