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

Remove last location check from location validation #788

Merged
merged 5 commits into from
Mar 23, 2018

Conversation

danesfeder
Copy link
Contributor

@danesfeder danesfeder commented Mar 20, 2018

Found that when starting navigation, the forced location update from LocationEngine#getLastLocation would never be considered valid and we would subsequently fall back to to creating the location update from the first point on the route:

master:
ezgif com-video-to-gif

I removed the check, as this should be an edge case (I'm not even sure if the location apis would ever fire with the getLastLocation twice in a row) 🤔

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

would never be considered valid

Why? Could you clarify?

I'm not even sure if the location apis would ever fire with the getLastLocation twice in a row

Not following here. What do you mean?

return !(location.equals(locationEngine.getLastLocation())
|| (location.getSpeed() <= 0 && location.hasSpeed())
|| location.getAccuracy() >= 100);
return location != null && location.hasSpeed() && location.getAccuracy() <= 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic number 100. Location#getAccuracy returns the estimated horizontal accuracy of this location, radial, in meters what about extracting a constant and give it a name based on what getAccuracy Javadoc states?

@danesfeder danesfeder force-pushed the dan-camera-resume branch 2 times, most recently from 3fa053f to fe59295 Compare March 22, 2018 13:46
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

After CI finishes 🚀

@danesfeder danesfeder merged commit 2975aea into master Mar 23, 2018
@danesfeder danesfeder deleted the dan-camera-resume branch March 23, 2018 21:20
@danesfeder danesfeder mentioned this pull request Apr 3, 2018
13 tasks
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