Skip to content

Conversation

@JunDai
Copy link
Contributor

@JunDai JunDai commented May 8, 2020

Description

As we discussed in the #2928 that we want to rename the callbacks to be more clearer in terms of waypoint and final destination.

Fix #2933

  • I have added any issue links
  • I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
  • I have added the appropriate milestone and project boards

Implementation

  • Rename all the stop to waypoint
  • Rename all the route to finalDestination

Testing

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

  • I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
  • I have tested via a test drive, or a 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
  • I have updated the CHANGELOG including this PR

@JunDai JunDai added ✓ ready for review backwards incompatible Requires a SEMVER major version change. labels May 8, 2020
@JunDai JunDai self-assigned this May 8, 2020
@JunDai JunDai force-pushed the jd-quick-fix branch 2 times, most recently from 07b8f69 to 8c15070 Compare May 8, 2020 22:34
@JunDai JunDai marked this pull request as ready for review May 8, 2020 22:34
@codecov-io
Copy link

codecov-io commented May 8, 2020

Codecov Report

Merging #2941 into master will increase coverage by 0.01%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #2941      +/-   ##
============================================
+ Coverage     36.44%   36.45%   +0.01%     
  Complexity     2203     2203              
============================================
  Files           552      552              
  Lines         19873    19873              
  Branches       1873     1873              
============================================
+ Hits           7242     7245       +3     
+ Misses        11798    11796       -2     
+ Partials        833      832       -1     

@kmadsen
Copy link
Contributor

kmadsen commented May 9, 2020

💯 thanks i was just looking to do this

@langsmith
Copy link
Contributor

I’m a bit nervous about my original suggestion of stop –> waypoint (onStopArrival() –> onWaypointArrival()) (made in #2933 (comment)) because the observer fires when a new leg is ready https://github.com/mapbox/mapbox-navigation-android/pull/2941/files#diff-029f44fcc6851301b568c61d9091c471R44 .

Based on https://docs.mapbox.com/android/navigation/overview/route-progress/#information-about-progress and other docs, it seems that when a new leg is ready to be travelled can be different than when the device has arrived at a stop or at a waypoint. Is that correct? If the onWaypointArrival() is actually firing when a new leg is ready to be traveled on, then the method should be onNewLegReady() or something?

Comment on lines +46 to +49
* This example shows how to use the Navigation SDK's [FasterRouteObserver]
* and display the observer's faster routes that are returned if the
* device goes off of the original [DirectionsRoute].
*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

address @langsmith 's comment in the previous PR #2947
#2947 (comment)

@JunDai
Copy link
Contributor Author

JunDai commented May 11, 2020

I’m a bit nervous about my original suggestion of stop –> waypoint (onStopArrival() –> onWaypointArrival()) (made in #2933 (comment)) because the observer fires when a new leg is ready https://github.com/mapbox/mapbox-navigation-android/pull/2941/files#diff-029f44fcc6851301b568c61d9091c471R44 .

@langsmith , it's a great point. In terms of the naming, I think we might want to add another callback for the nextRouteLegStart which to replace the current onWaypointArrival call. And fire the onWaypointArrival callback inside of:

    private fun doOnWaypointArrival(routeLegProgress: RouteLegProgress) {
        onWaypointArrival(routeLegProgress) // fire it here
        val moveToNextLeg = arrivalController.navigateNextRouteLeg(routeLegProgress)
        if (moveToNextLeg) {
            navigateNextRouteLeg()
        }
    }

cc: @kmadsen @Guardiola31337

@kmadsen
Copy link
Contributor

kmadsen commented May 11, 2020

Yeah I also agree with the inaccuracy of onStopArrival|onWaypointArrival.

I think we might want to add another callback for the nextRouteLegStart which to replace the current onWaypointArrival call.

@JunDai also take a note that, a very similar callback exists in the ArrivalController. EDIT: sorry that's what your comment is referencing :). But that is not called once, it is called many times

fun navigateNextRouteLeg(routeLegProgress: RouteLegProgress): Boolean

so maybe the best name for onWaypointArrival, is actually onNavigateNextRouteLeg()

@JunDai
Copy link
Contributor Author

JunDai commented May 12, 2020

@langsmith / @kmadsen : feel free to check the PR again. thanks!

@JunDai JunDai merged commit e92d4b7 into master May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards incompatible Requires a SEMVER major version change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor ArrivalObserver's methods to be clearer

4 participants