Skip to content

Conversation

@danesfeder
Copy link
Contributor

@danesfeder danesfeder commented Apr 15, 2019

Description

While testing #1829, we noticed that we can get "stuck" in a routing state, where subsequent requests are ignored. I.e. - stuck in constant state of loading after an off-route is detected.

Goal

The goal of this PR is to create a timeout that accompanies the routing state. So, even if we haven't received a response, we should try again and cancel the existing call. The idea is, with the second try, if connectivity has changed, the ConnectivityStatusProvider should pick up on that change (and hit offline for example).

Implementation

Added a new package-private class RouteCallStatus that will monitor response received / time since the call to the current time. This class is used in NavigationViewRouter.

Testing

Please describe the manual tests that you ran to verify your changes

  • I have tested via a test drive (we should do both for this)
    • Simulation/mock location app
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code

@danesfeder danesfeder added bug Defect to be fixed. ✓ ready for review labels Apr 15, 2019
@danesfeder danesfeder added this to the v0.36.0 milestone Apr 15, 2019
@danesfeder danesfeder self-assigned this Apr 15, 2019
@codecov-io
Copy link

codecov-io commented Apr 15, 2019

Codecov Report

Merging #1888 into master will increase coverage by 0.11%.
The diff coverage is 81.39%.

@@             Coverage Diff              @@
##             master    #1888      +/-   ##
============================================
+ Coverage     34.41%   34.53%   +0.11%     
- Complexity     1046     1057      +11     
============================================
  Files           257      258       +1     
  Lines          8799     8821      +22     
  Branches        666      666              
============================================
+ Hits           3028     3046      +18     
  Misses         5506     5506              
- Partials        265      269       +4

@Guardiola31337
Copy link
Contributor

I got stuck in the offline to online transition 👀

dynamic_offline_stuck_offline_online

I believe this is happening because we're not updating RouteCallStatus for the offline scenario 👀 we're not doing anything in

@Override
public void onRouteFound(@NonNull DirectionsRoute offlineRoute) {
router.updateCurrentRoute(offlineRoute);
}
@Override
public void onError(@NonNull OfflineError error) {
router.onRequestError(error.getMessage());
}

@danesfeder danesfeder force-pushed the dan-routing-timeout branch from 7548779 to 4be8d70 Compare April 16, 2019 13:09
@danesfeder
Copy link
Contributor Author

@Guardiola31337 yeah, great catch! I updated and added tests for those missing calls 👍

@danesfeder danesfeder force-pushed the dan-routing-timeout branch 3 times, most recently from 279fdce to 6d2f927 Compare April 16, 2019 20:47
@danesfeder danesfeder force-pushed the dan-routing-timeout branch from 6d2f927 to 33c9841 Compare April 16, 2019 21:01
@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Apr 16, 2019

We've tested latest changes using a mock location app and it's working pretty good now 🎉

Let's do some more testing on the field and merge here. Excellent work @danesfeder

@danesfeder danesfeder merged commit 1828b38 into master Apr 17, 2019
@danesfeder danesfeder deleted the dan-routing-timeout branch April 17, 2019 17:12
@Guardiola31337 Guardiola31337 mentioned this pull request Apr 17, 2019
11 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.

3 participants