Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor route logic #30

Merged
merged 14 commits into from
May 15, 2017
Merged

Refactor route logic #30

merged 14 commits into from
May 15, 2017

Conversation

cammace
Copy link

@cammace cammace commented May 4, 2017

This PR is intended to fix a number of issues, mainly around the internal method monitorStepProgress which was difficult to write unit test for. It has now been broken up into separate methods that have one specific job, this is allowing me to write more unit test.

cc: @mapbox/navigation

@cammace cammace added enhancement ⚠️ DO NOT MERGE PR should not be merged! labels May 4, 2017
@cammace cammace added this to the v0.2.0 milestone May 4, 2017
@cammace cammace self-assigned this May 4, 2017
@cammace cammace requested a review from zugaldia May 4, 2017 20:58
@cammace
Copy link
Author

cammace commented May 11, 2017

Refactoring's mostly complete, just need to fix up checkstyle issues and document the classes/methods.

@zugaldia ready for the review so we can do the 0.1.1 release.

@cammace cammace mentioned this pull request May 11, 2017
Closed
zugaldia
zugaldia requested changes May 12, 2017
View reviewed changes
Copy link
Member

@zugaldia zugaldia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It overall looks so much cleaner than before, I really like where this is going. Let's go ahead with fixing checkstyle, adding docs and making sure that every new class has a corresponding new test class.

navigation/app/build.gradle Outdated
@@ -35,7 +35,7 @@ dependencies {
compile 'com.android.support.constraint:constraint-layout:1.0.2'

// Mapbox Maps SDK
compile ('com.mapbox.mapboxsdk:mapbox-android-sdk:5.0.2@aar') {
compile ('com.mapbox.mapboxsdk:mapbox-android-sdk:5.1.0-beta.1@aar') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace beta.2 as of 3 minutes ago :)

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/MapboxNavigation.java Outdated
private CopyOnWriteArrayList<NavigationEventListener> navigationEventListeners;
private CopyOnWriteArrayList<ProgressChangeListener> progressChangeListeners;
private CopyOnWriteArrayList<OffRouteListener> offRouteListeners;
private List<AlertLevelChangeListener> alertLevelChangeListeners;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace Why the change? Any drawbacks from keeping the previous type?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessary at least for now since we aren't passing these list to a different thread.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace We aren't but the developer might work with different threads?

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/NavigationService.java Outdated
@@ -36,6 +33,7 @@
* @since 0.1.0
*/
@Experimental
@SuppressWarnings( {"MissingPermission"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of attaching this to the whole class, let's put on the line(s) where it's actually need to avoid masking other instances.

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/NavigationService.java
@@ -79,8 +77,7 @@ public void setupNotification(Activity activity) {
// Sets up the top bar notification
notifyBuilder = new NotificationCompat.Builder(this)
.setContentTitle("Mapbox Navigation")
.setContentText("Distance: " + routeProgress.getCurrentLegProgress().getCurrentStepProgress()
.getDistanceRemaining())
.setContentText("navigating")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we provide a more descriptive text by default? Is there an API for the dev to update this text? (can be ticketed separately)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Planned on overhauling the notification stuff in a separate PR, The notification isn't even working right now during a navigation session...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cammace Sounds good, let's ticket it separately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticketed in #25

...android-navigation/src/main/java/com/mapbox/services/android/navigation/v5/SnapLocation.java Outdated
return location;
}

// TODO split this method
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ticket or remove if done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticketed in #39

...avigation/src/main/java/com/mapbox/services/android/navigation/v5/LocationUpdatedThread.java
double secondsToEndOfStep = userSnapToStepDistanceFromManeuver / location.getSpeed();
boolean courseMatchesManeuverFinalHeading = false;

// TODO set to eventually adjust for different direction profiles.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ticket or remove if done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

support for different direction profiles is already ticked in #24

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/NavigationService.java Outdated
}

// TODO use this to update notification
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ticket or remove if done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted on #25

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/NavigationService.java Outdated
} else {
// TODO throw exception here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ticket or remove if done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NavigationException has been added and is now being thrown here.

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/NavigationService.java Outdated
} else {
locationUpdatedThread.quit();
}

// Lower accuracy to minimize battery usage while not in navigation mode.
// TODO restore accuracy state to what user had before nav session
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ticket or remove if done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ticketed in #38 and removed.

...-navigation/src/test/java/com/mapbox/services/android/navigation/v5/RouteControllerTest.java Outdated
Assert.assertEquals(NavigationConstants.MEDIUM_ALERT_LEVEL, alertLevel);
}

// TODO improve increaseIndex for testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ticket or remove if done.

@cammace cammace mentioned this pull request May 12, 2017
Closed
@cammace cammace force-pushed the cam-20-refactor branch from b4c610f to ac7dafa Compare May 13, 2017 01:06
@cammace cammace changed the title [WIP] Refactor route logic Refactor route logic May 13, 2017
@cammace cammace added ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels May 13, 2017
@cammace
Copy link
Author

cammace commented May 15, 2017

@zugaldia ready for a second pass

@cammace cammace merged commit 6dc3a29 into master May 15, 2017
@cammace cammace deleted the cam-20-refactor branch May 15, 2017 16:32
@cammace cammace mentioned this pull request May 15, 2017
Closed
@1ec5 1ec5 mentioned this pull request Jun 14, 2017
Closed
@zimiole zimiole mentioned this pull request Dec 10, 2019
Closed
@EricGeiler EricGeiler mentioned this pull request Dec 7, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zugaldia zugaldia zugaldia approved these changes

Assignees

@cammace cammace

Labels
None yet
Projects
None yet
Milestone
v0.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@cammace @zugaldia