Default Text Instructions #98

cammace · 2017-06-27T14:30:02Z

No description provided.

cammace · 2017-06-27T15:53:23Z

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/NavigationEngine.java

@@ -94,7 +93,9 @@ void onLocationChanged(DirectionsRoute directionsRoute, Location location) {
    for (Milestone milestone : milestones) {
      if (milestone.isOccurring(previousRouteProgress, routeProgress)) {
        for (MilestoneEventListener listener : milestoneEventListeners) {
-          listener.onMilestoneEvent(routeProgress, INSTRUCTION_STRING, milestone.getIdentifier());
+          // Create a new DefaultInstruction based on the current RouteProgress and Milestone identifier
+          DefaultInstruction instruction = new DefaultInstruction(routeProgress, milestone.getIdentifier());


Should be the generic Instruction class like so: Instruction instruction = new DefaultInstruction(. This way in the future, if a dev wants to implement their own instruction engine, they can (though we currently don't offer a way to pass in custom implementations)

cammace · 2017-06-27T15:57:11Z

...vigation/src/main/java/com/mapbox/services/android/navigation/v5/instruction/LengthUnit.java

@@ -0,0 +1,97 @@
+package com.mapbox.services.android.navigation.v5.instruction;
+
+public class LengthUnit {


This entire class can be removed in favor of TurfHelpers.convertDistance() added in this PR: mapbox/mapbox-java#481

danesfeder · 2017-06-27T18:19:37Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+  }
+
+  private String buildDepartureInstruction(RouteProgress progress) {
+    if (progress.getCurrentLegProgress().getUpComingStep().getDistance() > MINIMUM_UPCOMING_STEP_DISTANCE) {


@cammace can you check my logic here? I had interpreted it as

if the next step > 15 meters, announce normally, if < 15 meters, use the combo announcement

yep looks good.

cammace · 2017-06-27T19:01:21Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+  private static final String IN_STRING_FORMAT = "In %s %s";
+  private static final String THEN_STRING_FORMAT = "%s then %s";
+  private static final String THEN_IN_STRING_FORMAT = "%s then in %s %s";
+  private static final String CONTINUE_STRING_FORMAT = "Continue on %s for %s";


these strings should go into the string.xml file so we can handle translations.

cammace · 2017-06-27T19:43:59Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+  }
+
+  private String buildArrivalInstruction(RouteProgress progress) {
+    String instruction = progress.getCurrentLegProgress().getUpComingStep().getManeuver().getInstruction();


getUpComingStep needs to be checked if null since there's a chance it could be.

- passing to MilestoneEventListeners for possible voicing on client side

- Fix issues per Cam code review

… use upcoming step for default instruction

… than 15 meters long

cammace · 2017-06-28T02:31:27Z

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

+  private String instruction;
+
+  public DefaultInstruction(RouteProgress routeProgress, int identifier) {
+    super(routeProgress, identifier);


Remove abstract constructor and super()

cammace · 2017-06-28T02:33:07Z

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

+  }
+
+  private String createInstruction(RouteProgress routeProgress, int identifier) {
+    if (defaultInstructionEngine.get(identifier) != null) {


this null checks to make sure the identifier matches one of the keys inside defaultInstructionEngine correct? Line comment would be nice explaining this

cammace · 2017-06-28T02:36:34Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+import java.text.DecimalFormat;
+import java.util.Locale;
+
+class DefaultInstructionEngine extends SparseArray<DefaultInstructionEngine.InstructionBuilder> {


Since we have a set number of key-value pairs, should we define the sparse array size or is this not possible? See the SparseArray constructor.

cammace · 2017-06-28T02:38:16Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+    initDefaultBuilders();
+  }
+
+  private void initDefaultBuilders() {


We should ticket out and work in a different branch how we could expose a way for developers to add their own instructions specific to their custom milestone. My understanding is right now if the user wants to add an instruction they'd need to implement their own instructionEngine or handle the string building inside MilestoneEvent callback.

@cammace yeah, that's correct - I'll create a ticket

cammace · 2017-06-28T02:46:02Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+
+  /**
+   * Creates a {@link String} with the current step name and distance remaining
+   * Example: "Continue on Main St. for 3.2 miles"


We should look into better handling the distance values, we might want to further round 3.2 miles to 3 miles since the driver most likely won't be able to distinguish the 0.2-mile difference. No need to fix in this PR but opening a ticket might be a good idea.

cammace · 2017-06-28T02:50:58Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/instruction/Instruction.java

+
+import com.mapbox.services.android.navigation.v5.RouteProgress;
+
+public abstract class Instruction {


Javadoc using @since 0.4.0 tag

cammace · 2017-06-28T02:51:04Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/instruction/Instruction.java

+  Instruction(RouteProgress routeProgress, int identifier) {
+  }
+
+  public abstract String getInstruction();


Javadoc using @since 0.4.0 tag

cammace · 2017-06-28T02:53:12Z

...ain/java/com/mapbox/services/android/navigation/v5/instruction/DefaultInstructionEngine.java

+import java.text.DecimalFormat;
+import java.util.Locale;
+
+class DefaultInstructionEngine extends SparseArray<DefaultInstructionEngine.InstructionBuilder> {


Even though the class is only visible inside the package, it might be good to add some basic Javadoc here including the @since 0.4.0 tag.

cammace · 2017-06-28T02:54:02Z

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

+
+import com.mapbox.services.android.navigation.v5.RouteProgress;
+
+public class DefaultInstruction extends Instruction {


Javadoc using @since 0.4.0 tag

cammace · 2017-06-28T02:54:11Z

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

+  private DefaultInstructionEngine defaultInstructionEngine;
+  private String instruction;
+
+  public DefaultInstruction(RouteProgress routeProgress, int identifier) {


Javadoc using @since 0.4.0 tag

danesfeder · 2017-06-28T13:35:47Z

@cammace ready for another round of 👀

- Also include check for empty step name or rounded user distance to 0 (occurring sometimes on departure)

cammace · 2017-06-28T15:49:49Z

Looks good the merge @danesfeder

danesfeder · 2017-06-28T16:42:01Z

🚀

cammace added enhancement ⚠️ DO NOT MERGE PR should not be merged! labels Jun 27, 2017

cammace added this to the v0.4.0 milestone Jun 27, 2017

cammace assigned cammace and danesfeder Jun 27, 2017

cammace commented Jun 27, 2017

View reviewed changes

danesfeder force-pushed the text-instructions branch from 43ec813 to 66972fd Compare June 27, 2017 18:16

danesfeder reviewed Jun 27, 2017

View reviewed changes

cammace commented Jun 27, 2017

View reviewed changes

danesfeder changed the title ~~text instructions~~ Default Text Instructions Jun 27, 2017

Cameron Mace and others added 6 commits June 27, 2017 16:24

initial instruction setup

a2a387e

Setup for DefaultInstruction class and String instruction creation

082006b

- passing to MilestoneEventListeners for possible voicing on client side

Add instruction builders for each default milestone

2f10359

- Fix issues per Cam code review

Check upcoming step distance when building departure instruction

30f63a2

Improve logic for return strings based on current step

1dd0774

Use current step for fallback on arrival if upcoming step is null and…

888da4c

… use upcoming step for default instruction

danesfeder force-pushed the text-instructions branch from 6b0014b to 888da4c Compare June 27, 2017 20:25

danesfeder added 2 commits June 27, 2017 17:04

Add javadoc comments

611d739

Urgent instructions use then string format when upcoming step is less…

bbe852d

… than 15 meters long

cammace commented Jun 28, 2017

View reviewed changes

Add javadoc tags and changes per review requests

882b4c3

Clean up code / javadoc

0aed2d5

- Also include check for empty step name or rounded user distance to 0 (occurring sometimes on departure)

cammace added ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jun 28, 2017

danesfeder approved these changes Jun 28, 2017

View reviewed changes

danesfeder merged commit 45cda51 into master Jun 28, 2017

danesfeder deleted the text-instructions branch June 28, 2017 16:41

1ec5 mentioned this pull request Jul 1, 2017

DefaultInstructionEngine is unlocalizable #114

Closed

cammace mentioned this pull request Aug 1, 2017

Release 0.4.0 #152

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default Text Instructions #98

Default Text Instructions #98

cammace commented Jun 27, 2017

cammace Jun 27, 2017

cammace Jun 27, 2017

danesfeder Jun 27, 2017

cammace Jun 27, 2017

cammace Jun 27, 2017

cammace Jun 27, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

danesfeder Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

cammace Jun 28, 2017

danesfeder commented Jun 28, 2017

cammace commented Jun 28, 2017

danesfeder commented Jun 28, 2017

		@@ -0,0 +1,97 @@
		package com.mapbox.services.android.navigation.v5.instruction;

		public class LengthUnit {


		import com.mapbox.services.android.navigation.v5.RouteProgress;

		public abstract class Instruction {


		import com.mapbox.services.android.navigation.v5.RouteProgress;

		public class DefaultInstruction extends Instruction {

Default Text Instructions #98

Default Text Instructions #98

Conversation

cammace commented Jun 27, 2017

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

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 commented Jun 28, 2017

cammace commented Jun 28, 2017

danesfeder commented Jun 28, 2017