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.

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

fixed endnavigation issue #69

Merged
merged 4 commits into from
Jun 5, 2017
Merged

fixed endnavigation issue #69

merged 4 commits into from
Jun 5, 2017

Conversation

cammace
Copy link
Contributor

@cammace cammace commented Jun 5, 2017

Closes #68

Fixes issue with navigationSession using old reference of service even after the service is destroyed.

@cammace cammace added bug Defect to be fixed. enhancement labels Jun 5, 2017
@cammace cammace added this to the v0.3.0 milestone Jun 5, 2017
@cammace cammace self-assigned this Jun 5, 2017
@cammace cammace requested a review from zugaldia June 5, 2017 18:34

@Override
public void onProgressChange(Location location, RouteProgress routeProgress) {

Copy link
Member

Choose a reason for hiding this comment

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

nit: extra line


// If the user arrives at the final destination, end the navigation session.
if (routeProgress.getAlertUserLevel() == NavigationConstants.ARRIVE_ALERT_LEVEL) {
System.out.println("working");
Copy link
Member

Choose a reason for hiding this comment

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

remove or replace with Timber

@@ -438,7 +455,7 @@ public void onServiceConnected(ComponentName componentName, IBinder service) {
if (alertLevelChangeListeners != null) {
navigationService.setAlertLevelChangeListeners(alertLevelChangeListeners);
}

progressChangeListeners.add(MapboxNavigation.this);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right, the object is implementing a listener that is keeping track of. Also, the next line is always true because there always is at least one ProgressChangeListener. Could you elaborate why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The if null checks were leftovers from before when we didn't add the listeners to a list, I've removed them since the list are always set in the constructor. The MapboxNavigation objects listening in to get notified when the user arrives at the destination. When this occurs, the sessions, automatically ended. Otherwise, the service would continue running in the background even though the navigation sessions over.

I'm not really happy with the current setup with everything being passed inside the onConnected callback since this is only called once when the service is first created, but I'll need more time to refactor.

@@ -403,7 +420,7 @@ public float getUserOriginBearing() {
* @since 0.3.0
*/
public MapboxNavigationOptions getMapboxNavigationOptions() {
return options;
return isServiceAvailable() ? navigationService.getMapboxNavigationOptions() : options;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have two different copies of options?

@cammace cammace merged commit 10f85a8 into master Jun 5, 2017
@cammace cammace deleted the cam-68-fix branch June 5, 2017 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants