Remove origin, destination, unit type and locale from nav options #965

devotaaabel · 2018-05-23T14:50:58Z

Closes #924 along with possibly some voice/locale related bugs

Guardiola31337

When testing this PR I found 👇

Selected route not restored after updating the settings (NavigationViewActivity).

👀 #800 (EmbeddedNavigationActivity).

When clicking the top banner (list of instructions displayed) the speed view is visible (EmbeddedNavigationActivity).

In certain scenarios when a reroute occurs, noticed that the speech player changed when it shouldn't (couldn't reproduce but worth noting).

Also added some nit picks/questions.

Guardiola31337 · 2018-05-28T14:31:43Z

...mapbox/services/android/navigation/testapp/activity/navigationui/NavigationViewActivity.java

  }

-  private Locale getLocale() {
+  private String getUnitType() {


get/set nomenclature is hijacked by the Java language and should be only used when defining POJOs/DTOs.

Guardiola31337 · 2018-05-28T14:31:56Z

...mapbox/services/android/navigation/testapp/activity/navigationui/NavigationViewActivity.java

  }

-  @NavigationUnitType.UnitType
-  private int getUnitType() {
+  private String getLanguage() {


Same here ☝️

Guardiola31337 · 2018-05-28T14:32:13Z

...mapbox/services/android/navigation/testapp/activity/navigationui/NavigationViewActivity.java

+      language = localeUtils.getDeviceLanguage(this);
+    }
+
+    return language;
  }

  private boolean getShouldSimulateRoute() {


Same here ☝️

Guardiola31337 · 2018-05-28T14:38:09Z

...ervices/android/navigation/testapp/activity/navigationui/NavigationViewSettingsActivity.java

@@ -9,10 +11,20 @@
 import com.mapbox.services.android.navigation.testapp.R;

 public class NavigationViewSettingsActivity extends PreferenceActivity {
+  SharedPreferences.OnSharedPreferenceChangeListener listener;


Why is this package-private?

Guardiola31337 · 2018-05-28T14:39:01Z

...ervices/android/navigation/testapp/activity/navigationui/NavigationViewSettingsActivity.java

@@ -9,10 +11,20 @@
 import com.mapbox.services.android.navigation.testapp.R;

 public class NavigationViewSettingsActivity extends PreferenceActivity {
+  SharedPreferences.OnSharedPreferenceChangeListener listener;
+  public static final String UNIT_TYPE_CHANGED = "unit_type_changed";


Why are we exposing UNIT_TYPE_CHANGED and LANGUAGE_CHANGED publicly? Both could be package-private.

Guardiola31337 · 2018-05-28T16:34:49Z

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

   * @param unitType to use, or NONE_SPECIFIED to use default for locale country
   */
-  public DistanceUtils(Context context, @NonNull Locale locale, @NavigationUnitType.UnitType int unitType) {
+  public DistanceUtils(Context context, String language, @DirectionsCriteria.VoiceUnitCriteria String unitType) {


Why removing @NonNull annotation? See comment above ☝️

Guardiola31337 · 2018-05-28T16:35:08Z

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

    unitStrings.put(UNIT_KILOMETERS, context.getString(R.string.kilometers));
    unitStrings.put(UNIT_METERS, context.getString(R.string.meters));
    unitStrings.put(UNIT_MILES, context.getString(R.string.miles));
    unitStrings.put(UNIT_FEET, context.getString(R.string.feet));

+


NIT unnecessary new line

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


 import java.util.Locale;

-import static com.mapbox.services.android.navigation.v5.navigation.NavigationUnitType.TYPE_IMPERIAL;
-import static com.mapbox.services.android.navigation.v5.navigation.NavigationUnitType.TYPE_METRIC;
-
 public class LocaleUtils {


Guardiola31337 · 2018-05-28T16:39:24Z

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

-  public static Locale getNonNullLocale(Context context, Locale locale) {
-    if (locale == null) {
-      return getDeviceLocale(context);
+  public String getNonNullLocale(Context context, String language) {


We should rename this method, it's not returning a Locale anymore.

Guardiola31337 · 2018-05-28T16:44:13Z

.../src/test/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRouteTest.java

+  private LocaleUtils localeUtils;
+
+  @Before
+  public void setup() {


In general, we should avoid inline setup 👀 https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=65
Could you extract this block of code into private "factory" methods and use them only in the tests that actually need it/across the rest of the tests?
IMO, it reflects better what's going on in the test and you don't break the triple A "rule".

danesfeder

Some comments added in addition to @Guardiola31337's. Great work on untangling this 🍝 - in the end, this will make localization a lot more straightforward for our users.

danesfeder · 2018-05-29T14:53:32Z

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

-    locale = LocaleUtils.getNonNullLocale(getApplication(), options.locale());
-    unitType = options.unitType();
+  private void initLocaleInfo(NavigationUiOptions options) {
+    language = new LocaleUtils().getNonNullLocale(getApplication(), options.directionsRoute().voiceLanguage());


Can we also update this naming to initLanguage or intializeLanguage as the underlying code has changed a bit here?

danesfeder · 2018-05-29T14:55:09Z

.../java/com/mapbox/services/android/navigation/ui/v5/instruction/InstructionStepResources.java

+  private boolean shouldDistanceUtilsBeInitialized(String language,
+                                                   @DirectionsCriteria.VoiceUnitCriteria String unitType) {
+    return distanceUtils == null ||  !this.language.equals(language) || !this.unitType.equals(unitType);
+


NIT - new line added

danesfeder · 2018-05-29T14:55:58Z

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

@@ -330,26 +329,26 @@ public void showInstructionList() {
  /**
   * Sets the locale to use for languages and default unit type


Comment can be updated to reflect new code

danesfeder · 2018-05-29T14:58:24Z

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

-      distanceUtils = new DistanceUtils(context, locale, unitType);
-      this.locale = locale;
+                          String language, @DirectionsCriteria.VoiceUnitCriteria String unitType) {
+    if (distanceUtils == null || !this.language.equals(language) || !this.unitType.equals(unitType)) {


Can we update this line to include the same boolean function you created in https://github.com/mapbox/mapbox-navigation-android/pull/965/files#diff-ac7c5e067e3f4755a545253204a39851R92?

danesfeder · 2018-05-29T15:36:14Z

...-ui/src/main/java/com/mapbox/services/android/navigation/ui/v5/voice/MapboxSpeechPlayer.java

    setupCaches(context);
    instructionQueue = new ConcurrentLinkedQueue();
    voiceInstructionLoader = VoiceInstructionLoader.builder()
-      .language(locale.toString())
+      .language(language)


💯 I believe this was the source of some of the Locale related voice bugs we were seeing

.../java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationNotification.java

-    if (locale == null) {
-      locale = LocaleUtils.getDeviceLocale(context);
-    }
+    RouteOptions routeOptions = mapboxNavigation.getRoute().routeOptions();


danesfeder · 2018-05-29T15:46:22Z

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationRoute.java

+  /**
+   * Helper class to reduce redundant logging code when no other action is taken in onFailure
+   */
+  public abstract static class SimplifiedCallback implements Callback<DirectionsResponse> {


I agree with @Guardiola31337 here - our developers looks at the test app for examples of how to use NavigationRoute and this may end up creating some confusion.

danesfeder

Great work @devotaaabel 👍

…usion with MapboxNavigationOptions

… RouteOptions

… opened

Danny-James · 2018-06-18T09:19:36Z

@Guardiola31337 @danesfeder do we have any instructions on now setting the locale and unit type since this has been removed from the NavigationOptions?

danesfeder · 2018-06-18T12:15:17Z

@Danny-James In 0.14.0 this is now done at the Directions API level, please consult the release notes for further detail there > https://github.com/mapbox/mapbox-navigation-android/releases/tag/v0.14.0

devotaaabel added the ⚠️ DO NOT MERGE PR should not be merged! label May 23, 2018

devotaaabel added this to the 0.14.0 milestone May 23, 2018

devotaaabel self-assigned this May 23, 2018

devotaaabel requested review from Guardiola31337 and danesfeder May 23, 2018 14:50

devotaaabel changed the title ~~Devota remove origin, destination, unit type and locale from nav options~~ Remove origin, destination, unit type and locale from nav options May 23, 2018

devotaaabel force-pushed the devota-remove-origin-destination-from-nav-options branch from 5194ce1 to b3de512 Compare May 25, 2018 17:48

devotaaabel removed the ⚠️ DO NOT MERGE PR should not be merged! label May 25, 2018

danesfeder force-pushed the devota-remove-origin-destination-from-nav-options branch from 658424f to 71e6da2 Compare May 25, 2018 19:47

Guardiola31337 requested changes May 28, 2018

View reviewed changes

danesfeder reviewed May 29, 2018

View reviewed changes

devotaaabel force-pushed the devota-remove-origin-destination-from-nav-options branch 2 times, most recently from 2e3c693 to a0c1303 Compare May 29, 2018 20:37

devotaaabel mentioned this pull request May 30, 2018

Clean test app examples #714

Closed

devotaaabel force-pushed the devota-remove-origin-destination-from-nav-options branch from a0c1303 to bc958eb Compare May 30, 2018 18:06

danesfeder approved these changes May 30, 2018

View reviewed changes

Devota Aabel added 13 commits May 31, 2018 10:13

Refactored NavigationOptions to be NavigationUiOptions to reduce conf…

0b4020e

…usion with MapboxNavigationOptions

Started refactoring locale and unit type to use the strings passed in…

49c7a0e

… RouteOptions

More cleanup and got navigation view demo working

213fc99

Added fix for re-fetching route in NavigationView after settings were…

5297865

… opened

Cleanup

d8ae317

Fixed EmbeddedActivity

6146389

Fixed EmbeddedExample

cc3df3f

Fixed WaypointNavigationActivity

1c7c833

Updated NavigationFragment and NavigationConstants

bc1ee8b

Cleanup

de1723d

Added SimplifiedCallback to simplify callback code for routes

9932889

Fixed checkstyle issues

0dd21cd

Fixed crash

d216cff

Devota Aabel added 10 commits May 31, 2018 10:23

Updated naming to keep consisten

75c7767

Refactoring and fixed crash from NavigationViewActivity

d4c816c

Removed outdated code

d29bd90

Added startActivityForResult for NavigationViewActivity settings

623c905

Adjusted NavigaitonRoute.builder to take in a context

adcad6a

Fixed compile errors

caceb7a

Made LocaleUtils non-static to fix tests

ca13736

Fixed failing tests

d130188

Fixed crash

97aed1f

Refactored to make it clearer what default means

2193690

Guardiola31337 approved these changes May 31, 2018

View reviewed changes

Devota Aabel added 4 commits May 31, 2018 10:26

Addressed most of PR comments

4ca1d53

Added null check for constructor items for DistanceUtils

f24e5f6

Made method package private for testing

ab4657d

Added off-route call

7389074

devotaaabel force-pushed the devota-remove-origin-destination-from-nav-options branch from 41212e9 to 7389074 Compare May 31, 2018 14:26

devotaaabel merged commit 7a3a704 into master May 31, 2018

devotaaabel deleted the devota-remove-origin-destination-from-nav-options branch May 31, 2018 14:56

danesfeder mentioned this pull request May 31, 2018

Release v0.14.0 #982

Closed

13 tasks

danesfeder mentioned this pull request Jun 21, 2018

Release v0.15.0 #1049

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove origin, destination, unit type and locale from nav options #965

Remove origin, destination, unit type and locale from nav options #965

devotaaabel commented May 23, 2018 •

edited

Loading

Guardiola31337 left a comment

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

This comment was marked as resolved.

This comment was marked as resolved.

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

danesfeder left a comment

danesfeder May 29, 2018

danesfeder May 29, 2018

danesfeder May 29, 2018

danesfeder May 29, 2018

danesfeder May 29, 2018

This comment was marked as resolved.

danesfeder May 29, 2018

danesfeder left a comment

Danny-James commented Jun 18, 2018

danesfeder commented Jun 18, 2018 •

edited

Loading

		@@ -330,26 +329,26 @@ public void showInstructionList() {
		/**
		* Sets the locale to use for languages and default unit type

Remove origin, destination, unit type and locale from nav options #965

Remove origin, destination, unit type and locale from nav options #965

Conversation

devotaaabel commented May 23, 2018 • edited Loading

Guardiola31337 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

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

This comment was marked as resolved.

This comment was marked as resolved.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

danesfeder left a comment

Choose a reason for hiding this comment

Danny-James commented Jun 18, 2018

danesfeder commented Jun 18, 2018 • edited Loading

devotaaabel commented May 23, 2018 •

edited

Loading

danesfeder commented Jun 18, 2018 •

edited

Loading