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

Nav Native API update #1547

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Nav Native API update #1547

merged 1 commit into from
Nov 26, 2018

Conversation

devotaaabel
Copy link
Contributor

@devotaaabel devotaaabel commented Nov 21, 2018

This PR Updates the API so that OfflineNavigator#retrieveRouteFor takes in a callback instead of blocking the UI thread. This PR also renames a few things.

This is the current structure of navigation (MapboxNavigation is the only public facing API):

  • MapboxNavigation -> MapboxNavigator -> Navigator

To stick with this structure, this is what I am proposing for offline navigation (MapboxOfflineRouter is the only public facing API:

  • MapboxOfflineRouter -> OfflineNavigator -> Navigator.

-> in this case denotes a "has a" relationship. If anyone has any opinions about naming, please weigh in.

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@devotaaabel thanks for running with this and getting the route retrieval off of the UI thread 💯

I only have one question regarding naming - would it make sense to rename MapboxOfflineNavigation to MapboxOfflineRouter or OfflineRouter? The class is really only concerned with fetching a DirectionsRoute offline. The main concern being developers will be confused looking at MapboxOfflineNavigation vs. MapboxNavigation - thinking that MapboxOfflineNavigation will contain another set of route-following APIs that mirror MapboxNavigation.

Only other comment up for discussion is the abstraction work with CallbackAsyncTask. Would it make sense to do the same for the ConfigureRouterTask? It feels like a good opportunity to align that work as they are both working the the OfflineNavigator in the background and both have callbacks.

Although I guess this might be tricky because the CallbackAsyncTask.Callback<R> wants to return a result type and the OnOfflineDataInitialized callback only returns when the data is ready and doesn't return anything.

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.

@devotaaabel this is looking good, great work!

Just some general comments / questions.

Isn't CallbackAsyncTask.Callback<DirectionsRoute> leaking the underlying implementation to the developer using the SDK? Have we considered exposing callbacks that are very explicit in what they're providing (e.g. OnOfflineDataInitialized, OnOfflineRouteFound, etc.)? Right now, CallbackAsyncTask.Callback<DirectionsRoute> is being passed by the developer using the SDK and CallbackAsyncTask is only used internally (could be package-private) so the dev shouldn't have to care what that is. I'm thinking on something like OnOfflineRouteFound#onOfflineRouteFound(DirectionsRoute directionsRoute) making clear what the callback is doing / giving to you and easier to use.

@devotaaabel
Copy link
Contributor Author

@danesfeder @Guardiola31337 ready for re-review, you both had conflicting advice for the CallbackAsyncTask, so I just removed it and made the other task non-generic.

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

Thanks for working through the comments / feedback here @devotaaabel

@devotaaabel
Copy link
Contributor Author

@Guardiola31337 did you want to re-review before I merge?

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.

Could we add Javadoc to OfflineRouteFoundCallback? Other than that minor comment this looks good to me. Great work @devotaaabel

…are of retrieving the route from the navigator
@devotaaabel devotaaabel merged commit 35a168a into master Nov 26, 2018
@devotaaabel devotaaabel deleted the devota-update-offline-api branch November 26, 2018 18:45
@Guardiola31337 Guardiola31337 mentioned this pull request Nov 30, 2018
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants