-
Notifications
You must be signed in to change notification settings - Fork 319
Add tunnels functionality #1392
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
Conversation
7d99640 to
cdac640
Compare
In order to keep this as simple as possible I went ahead and removed the I've edited OP to reflect this. |
danesfeder
left a comment
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.
@Guardiola31337 this is looking good, but I was wondering, would it be a good idea to contain all of the client logic needed for setting up night mode in a public class that they may use? Just a thought, I think it would make developers' lives a lot easier if we included something like this that they could use in their Activity or Fragment
| navigationView.onDestroy(); | ||
| if (isFinishing()) { | ||
| saveNightModeToPreferences(AppCompatDelegate.MODE_NIGHT_AUTO); | ||
| AppCompatDelegate.setDefaultNightMode(AppCompatDelegate.MODE_NIGHT_AUTO); |
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.
Does setting this before the Activity is destroyed still allow it to be considered on re-creation?
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.
Yes it does. Mainly because this is "per app" and in the test app closing any activity redirects you to the MainActivity i.e. the night mode is kept if you reopen the Activity.
IMO If you're referring to extracting the |
cdac640 to
9ffada8
Compare
|
@danesfeder I followed up on all your comments / concerns / questions. Ready for another round 👀 |
danesfeder
left a comment
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.
@Guardiola31337 some minor cleanup / comments - this is looking good, thanks for addressing my questions.
|
|
||
| @Override | ||
| public void onProgressChange(Location location, RouteProgress routeProgress) { | ||
| boolean inTunnel = routeProgress.inTunnel(); |
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.
This chunk of logic can be extracted into a private method. Maybe we can think of better naming for isInTunnel vs. inTunnel - it's a little confusing to determine which variable is coming from where.
| @Override | ||
| public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { | ||
| super.onViewCreated(view, savedInstanceState); | ||
| if (wasNavigationStopped()) { |
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.
For readability when the Fragment is being created, this can be extracted into a private method
| @Override | ||
| public void onProgressChange(Location location, RouteProgress routeProgress) { | ||
| boolean isInTunnel = routeProgress.inTunnel(); | ||
| boolean wasInTunnel = wasInTunnel(); |
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.
I like the variable naming here - if possible, let's us the same naming in RerouteActivity
9ffada8 to
0f031ac
Compare
|
@danesfeder this is updated and ready for another round 👀 |
danesfeder
left a comment
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.
@Guardiola31337 looks good, thanks for addressing the feedback
Fixes #835
Adds a new option (TunnelListener) toNavigationViewOptionsMapboxNavigationfor adding / removing this listener? Or even remove this listener altogether? Currently it's possible to get the sameinTunnelinformation attaching toProgressChangeListenerand querying theRouteProgress👀mapbox-navigation-android/app/src/main/java/com/mapbox/services/android/navigation/testapp/activity/RerouteActivity.java
Line 218 in 9c8d687
isNightModeEnabledand set the theme accordingly though. Kinda revert Use theme attribute to update MapView map style URL #1018TODOs
EmbeddedNavigationActivityandFragmentNavigationActivityexamples from the test app voice instructions are "duplicated" after recreating theActivity