Update InstructionView with BannerMilestone only with callback #969

danesfeder · 2018-05-23T20:43:09Z

Noticed this while testing:

This PR ensures we only update the InstructionView when the milestone listener is fired and not with every update which is what was happening previously.

I also added the maneuver type and modifier from the BannerText object as well.

Guardiola31337

When you say

This PR ensures we only update the InstructionView when the milestone listener is fired and not with every update which is what was happening previously.

What do you mean?
Removing updateManeuverView from

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

Lines 176 to 183 in ee41dd3

    
           navigationViewModel.instructionModel.observe(owner, new Observer<InstructionModel>() { 
        
             @Override 
        
             public void onChanged(@Nullable InstructionModel model) { 
        
               if (model != null) { 
        
                 updateInstructionData(model); 
        
               } 
        
             } 
        
           });

Also added some nit picks/questions.

Guardiola31337 · 2018-05-28T11:49:52Z

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

@@ -207,7 +206,7 @@ public void onChanged(@Nullable Boolean isOffRoute) {
    });

    // ViewModel set - click listeners can be set now


NIT unnecessary comment

Guardiola31337 · 2018-05-28T11:58:51Z

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

@@ -559,11 +554,11 @@ private void addBottomSheetListener() {
    }
  }

-  private void initClickListeners() {
+  private void initializeClickListeners() {
    if (getContext().getResources().getConfiguration().orientation == Configuration.ORIENTATION_LANDSCAPE) {


What about extracting an explaining variable here and simplify the expression?

Addressed in #973

Guardiola31337 · 2018-05-28T12:28:05Z

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

@@ -221,19 +220,17 @@ public void onChanged(@Nullable Boolean isOffRoute) {
  public void update(RouteProgress routeProgress) {


Why are we exposing this using @SuppressWarnings("UnusedDeclaration")? 🤔

No usages found in All Places

Could you clarify how this would be used?

This API is used when InstructionView is used without a NavigationViewModel. If you wanted to inflate the view separate of NavigationView, you would use this API with a ProgressChangeListener to provide it data.

Gotcha. Thanks for clarifying 👍

Guardiola31337 · 2018-05-28T12:30:50Z

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

@@ -682,7 +677,8 @@ private void distanceText(InstructionModel model) {
   *
   * @param model provides instruction text
   */
-  private void updateTextInstruction(InstructionModel model) {
+  private void updateBannerData(InstructionModel model) {
+    updateManeuverView(model);


Wondering if we should extract this call from here and make it explicit wherever we call updateBannerData. At a first glance updateBannerData and updateManeuverView seem completely different things to me. Could you clarify?

The ManeuverView renders the arrow based on the String maneuver type + modifier data. This is provided in the InstructionModel - maybe updateManeuverViewData would be more clear here?

So the ManeuverView is part of the Banner, right? In terms of Views (overall layout), could you clarify what's the Instruction, Banner, Maneuver, etc.? I think it's confusing to me because I don't have the big picture of how it's all glued together.

danesfeder · 2018-05-29T19:54:35Z

@Guardiola31337 yeah there are two different models that populate the InstructionView: InstructionModel and BannerInstructionModel - the BannerInstructionModel is created from the BannerInstructionMilestone, so it only fires 2-3 times a step depending on the API data. The InstructionModel fires for every Location update and updates more specific info like distance / time remaining.

Guardiola31337 · 2018-06-04T10:20:27Z

...in/java/com/mapbox/services/android/navigation/ui/v5/instruction/BannerInstructionModel.java

@@ -11,21 +11,33 @@

 public class BannerInstructionModel extends InstructionModel {

-  private BannerInstructionMilestone milestone;
+  private BannerText primaryBannerText;


This is not necessary. This field is already declared in its parent InstructionModel.

Guardiola31337 · 2018-06-04T10:20:41Z

...in/java/com/mapbox/services/android/navigation/ui/v5/instruction/BannerInstructionModel.java

@@ -11,21 +11,33 @@

 public class BannerInstructionModel extends InstructionModel {

-  private BannerInstructionMilestone milestone;
+  private BannerText primaryBannerText;
+  private BannerText secondaryBannerText;


Same here ☝️

Guardiola31337 · 2018-06-04T10:22:11Z

...in/java/com/mapbox/services/android/navigation/ui/v5/instruction/BannerInstructionModel.java


  public BannerInstructionModel(Context context, BannerInstructionMilestone milestone,
                                RouteProgress progress, Locale locale, @NavigationUnitType.UnitType int unitType) {
    super(context, progress, locale, unitType);
-    this.milestone = milestone;
+    primaryBannerText = milestone.getBannerInstructions().primary();
+    secondaryBannerText = milestone.getBannerInstructions().secondary();
  }

  @Override
  BannerText getPrimaryBannerText() {


This is not necessary. This method no needs to be overridden, it's already declared in its parent InstructionModel.

Guardiola31337 · 2018-06-04T10:22:25Z

...in/java/com/mapbox/services/android/navigation/ui/v5/instruction/BannerInstructionModel.java

  }

  @Override
  BannerText getPrimaryBannerText() {
-    return milestone.getBannerInstructions().primary();
+    return primaryBannerText;
  }

  @Override
  BannerText getSecondaryBannerText() {


Same here ☝️

Guardiola31337 · 2018-06-05T11:50:00Z

I've noticed the same during my last test drive 👀

Hopefully this PR will solve these weird resizing issues 🤞

danesfeder · 2018-06-05T20:13:59Z

@Guardiola31337 anything else needed to address here?

Guardiola31337

@Guardiola31337 anything else needed to address here?

One last minor comment not blocking the PR. Great job @danesfeder 👏

Guardiola31337 · 2018-06-06T08:09:38Z

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

@@ -222,13 +221,12 @@ public void onChanged(@Nullable Boolean isOffRoute) {
  public void update(RouteProgress routeProgress) {
    if (routeProgress != null && !isRerouting) {
      InstructionModel model = new InstructionModel(getContext(), routeProgress, language, unitType);
-      updateViews(model);
-      updateTextInstruction(model);
+      updateDataFromInstructionModel(model);


Minor comment - what about updateDataFromInstruction(model) and updateDataFromBannerInstruction(model)? I think it'd be better for consistency and it reads well. What do you think?

@Guardiola31337 yeah I like that naming a lot better, thanks 💯

…eived

danesfeder added bug Defect to be fixed. navigation-ui labels May 23, 2018

danesfeder self-assigned this May 23, 2018

danesfeder requested review from Guardiola31337 and devotaaabel May 23, 2018 20:43

danesfeder added this to the 0.14.0 milestone May 23, 2018

danesfeder added the ✓ ready for review label May 25, 2018

Guardiola31337 reviewed May 28, 2018

View reviewed changes

danesfeder force-pushed the dan-intsruction-auto branch from ee41dd3 to f26ef50 Compare May 29, 2018 19:50

danesfeder modified the milestones: 0.14.0, 0.15.0 May 30, 2018

Guardiola31337 reviewed Jun 4, 2018

View reviewed changes

danesfeder force-pushed the dan-intsruction-auto branch from f26ef50 to bd7b430 Compare June 5, 2018 12:33

danesfeder force-pushed the dan-intsruction-auto branch from bd7b430 to f874b55 Compare June 5, 2018 20:14

Guardiola31337 approved these changes Jun 6, 2018

View reviewed changes

Update InstructionView with BannerMilestone only when callback is rec…

f74183d

…eived

danesfeder force-pushed the dan-intsruction-auto branch from f874b55 to f74183d Compare June 6, 2018 12:51

danesfeder merged commit fbda855 into master Jun 6, 2018

danesfeder deleted the dan-intsruction-auto branch June 6, 2018 13:23

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

Update InstructionView with BannerMilestone only with callback #969

Update InstructionView with BannerMilestone only with callback #969

danesfeder commented May 23, 2018

Guardiola31337 left a comment

Guardiola31337 May 28, 2018

Guardiola31337 May 28, 2018

danesfeder May 29, 2018

Guardiola31337 May 28, 2018

danesfeder May 29, 2018

Guardiola31337 Jun 4, 2018

Guardiola31337 May 28, 2018

danesfeder May 29, 2018

Guardiola31337 Jun 4, 2018

danesfeder Jun 5, 2018 •

edited

Loading

danesfeder commented May 29, 2018

Guardiola31337 Jun 4, 2018

Guardiola31337 Jun 4, 2018

Guardiola31337 Jun 4, 2018

Guardiola31337 Jun 4, 2018

Guardiola31337 commented Jun 5, 2018

danesfeder commented Jun 5, 2018

Guardiola31337 left a comment

Guardiola31337 Jun 6, 2018

danesfeder Jun 6, 2018

	navigationViewModel.instructionModel.observe(owner, new Observer<InstructionModel>() {
	@Override
	public void onChanged(@Nullable InstructionModel model) {
	if (model != null) {
	updateInstructionData(model);
	}
	}
	});

		@@ -207,7 +206,7 @@ public void onChanged(@Nullable Boolean isOffRoute) {
		});

		// ViewModel set - click listeners can be set now

		@@ -221,19 +220,17 @@ public void onChanged(@Nullable Boolean isOffRoute) {
		public void update(RouteProgress routeProgress) {

Update InstructionView with BannerMilestone only with callback #969

Update InstructionView with BannerMilestone only with callback #969

Conversation

danesfeder commented May 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danesfeder Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

danesfeder commented May 29, 2018

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

Guardiola31337 commented Jun 5, 2018

danesfeder commented Jun 5, 2018

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

danesfeder Jun 5, 2018 •

edited

Loading