Add 12/24 hour format Navigation View Option #805
Conversation
Guardiola31337
force-pushed
the
pg-740-navigation-view-option-12-24-time
branch
from
918a50f
to
2eb1e84
Compare
There was a problem hiding this comment.
Just some cleanup questions / comments.
One thing that's we also have to look at here is MapboxNavigationNotification which also uses TimeUtils#formatArrivalTime. We might have to move the timeFormatType to MapboxNavigationOptions so it can be used for both the core + UI libraries. The notification is currently created in the core library for the NavigationService
| @@ -371,12 +374,15 @@ public void startNavigation(NavigationViewOptions options) { | |||
|
|
|||
| private void setLocale(NavigationViewOptions options) { | |||
There was a problem hiding this comment.
I think it would be good to refactor this method, as it's called setLocale but it's setting Locale, unitType, and timeFormatType. For readability, it may be good to extract these each into their own private method.
| boolean isDeviceTwentyFourHourFormat = DateFormat.is24HourFormat(getApplication()); | ||
| String arrivalTime = formatTime(time, (int) routeProgress.durationRemaining(), timeFormatType, | ||
| isDeviceTwentyFourHourFormat); | ||
| summaryModel.setValue(new SummaryModel(getApplication(), routeProgress, locale, unitType, arrivalTime)); |
There was a problem hiding this comment.
Would it make sense to move the arrival time formatting into the SummaryModel itself? Application and routeProgress are already being passed. I think it would clean up onProgressChange
It would also remove the duplicate code found in SummaryBottomSheet
| @@ -40,7 +45,10 @@ | |||
| private ProgressBar rerouteProgressBar; | |||
| private boolean isRerouting; | |||
| private Locale locale; | |||
| private @NavigationUnitType.UnitType int unitType = NavigationUnitType.NONE_SPECIFIED; | |||
| @NavigationUnitType.UnitType | |||
| private int unitType = NavigationUnitType.NONE_SPECIFIED; | |||
There was a problem hiding this comment.
I think it makes sense to align how both the unitType and timeFormatType are both set to NONE_SPECIFIED initially. Currently the time format is set in the NavigationViewOptions#builder(), while unitType is set here in SummaryBottomSheet
There was a problem hiding this comment.
Actually, I went ahead and removed the unitType initialization here. It's not necessary because we're already initializing it in MapboxNavigationOptions#builder() 👀
Guardiola31337
force-pushed
the
pg-740-navigation-view-option-12-24-time
branch
2 times, most recently
from
988ed70
to
d4cd5da
Compare
Yeah, I noticed as well. BTW I think we should do that (consolidate Addressed the rest of the comments. This is ready for a second round of 👀 |
Guardiola31337
force-pushed
the
pg-740-navigation-view-option-12-24-time
branch
from
d4cd5da
to
34f21e6
Compare
NavigationViewOptionIf the dev doesn't provide a
NavigationTimeFormat.TypeexplicitlyNavigationTimeFormat.NONE_SPECIFIEDis set (default) and the device's 24-hour format setting (DateFormat.is24HourFormat(Context)) is used.Fixes #740
cc @danesfeder @devotaaabel