-
Notifications
You must be signed in to change notification settings - Fork 319
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
[Android Auto] Remove MapboxNavigation
from RoutePreviewCarContext
#6226
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6226 +/- ##
=========================================
Coverage 68.75% 68.75%
Complexity 4311 4311
=========================================
Files 649 649
Lines 26022 26022
Branches 3055 3055
=========================================
Hits 17891 17891
Misses 6971 6971
Partials 1160 1160 |
carRoutesProvider.navigationRoutes.value | ||
) | ||
MapboxCarApp.updateCarAppState(ActiveGuidanceState) | ||
MapboxNavigationApp.current()?.let { mapboxNavigation -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it is important to have mapboxNavigation
here, should we use !!
instead? Or at least log a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.. thinking about this a bit and going with !!
. Going to list a couple examples where a developer could create the crash. In particular, this crash could only happen because of 3
. It is a small window of a crash, but we could potentially cure the 3
case with a reset
function 🤔.
- If you
current()!!
before thesetup
MapboxNavigationApp.current()!!.setNavigationRoutes(emptyList())
MapboxNavigationApp.setup(..)
- If you
current()!!
before theattach
lifecycleonCreate
is called
MapboxNavigationApp.attach(..)
MapboxNavigationApp.current()!!.setNavigationRoutes(emptyList())
- When you're in the middle of resetting options
val navigationOption = MapboxNavigationApp.current()!!.navigationOptions
MapboxNavigationApp.disable()
MapboxNavigationApp.current()!!.setNavigationRoutes(..)
MapboxNavigationApp.setup(navigationOption)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge here, but I think we should consider adding a ReadOnlyDelegate that makes it so current()!!
is not a common practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the ReadOnlyDelegate
#6233 😄
Description
Addressing #6141
CarSpeedLimitRenderer
toMapboxNavigationApp
CarRoutePreviewScreen
can useMapboxNavigationApp.current()
since it has single shot actionsGeoDeeplinkNavigateAction
can useMapboxNavigationApp.current()
since it has single shot actionsSkipping changelog because there is are no public api changes here.
The remaining migrations are a bit more ambiguous, this classes were some low hanging fruit.