History replay improvements #4342

Udumft · 2023-01-19T13:32:14Z

Description

This PR improves history replaying capabilities by exposing more public history records types and modifying ReplayLocationManager to replicate all stored history events in chronological order, including setRoute events and actually setting them to the router.

Implementation

Made PushRecordHistoryEvent public; refactored ReplayLocationManager to work with events feed and added a listener to report occurred events to.

…y to allow history parser to show this event to users; modified ReplayLocationManager to be able to replay history events in chronological order, including listening to the events; added default history events listener to MapboxNavigationService that will also set route updates from history to router.

mapbox-github-ci-issues-public-1 · 2023-01-19T13:36:08Z

API compatibility report for MapboxCoreNavigation: 🔴

Removed Decls

Constructor MapboxNavigationService.init(history:customRoutingProvider:credentials:eventsManagerType:routerType:customActivityType:) has been removed
Constructor ReplayLocationManager.init(history:) has been removed

Class Inheritance Change

Constructor ReplayLocationManager.init(history:listener:) has been added as a designated initializer to an open class

codecov · 2023-01-19T13:42:48Z

Codecov Report

Merging #4342 (0f2f091) into main (163474e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #4342   +/-   ##
=======================================
  Coverage   60.59%   60.59%           
=======================================
  Files         189      189           
  Lines       21115    21115           
=======================================
  Hits        12795    12795           
  Misses       8320     8320

Udumft · 2023-01-19T15:11:47Z

Breaking changes comments:

Constructor MapboxNavigationService.init(history:customRoutingProvider:credentials:eventsManagerType:routerType:customActivityType:) has been removed
Constructor ReplayLocationManager.init(history:) has been removed

Marked as breaking change since it has new argument with a default value => accepting these changes

Constructor ReplayLocationManager.init(history:listener:) has been added as a designated initializer to an open class

Reverted.

S2Ler

This is a good improvement to history capabilities of the SDK. However, I have some feedback and suggestions on specific areas that I think could be improved. I hope my comments were helpful.

Sources/MapboxCoreNavigation/NavigationService.swift

Sources/MapboxCoreNavigation/ReplayLocationManager.swift

S2Ler · 2023-01-20T02:53:13Z

Sources/MapboxCoreNavigation/HistoryEvent.swift

-    let type: String
-    let properties: String
+/// History event being pushed by the user
+public struct PushRecordHistoryEvent: HistoryEvent {


I find the naming of the struct PushRecordHistoryEvent to be a bit confusing, maybe consider renaming it to something more descriptive of its purpose. I've seen 'custom event' is used throughout of the PR indicating that maybe 'CustomHistoryEvent' is a better naming.

Naming for this event was meant to reflect HistoryRecording.pushHistoryEvent method as this is the way such events are created. I am not sure if CustomHistoryEvent would continue that relation, even though technically it is a custom history event.

I find this name difficult to read due to the usage of first push word which reads as a verb at first. Up to you to keep it that way (given that the same naming is used in NavNative) or rename to CustomHistoryEvent as verb 'custom' also used in this method comment.

S2Ler · 2023-01-20T02:59:35Z

Sources/MapboxCoreNavigation/HistoryEvent.swift

+    public let timestamp: TimeInterval
+    /// The event type in the events log for this custom event.
+    public let type: String
+    /// The data value that contains a valid JSON attached to the event.


This comment mentions the term 'valid JSON', but it's not clear what exactly is meant by this. It would be helpful if the documentation could provide more information and examples of what constitutes a 'valid JSON' in the context of the SDK.

'valid JSON' means its syntactic validity. How could I emphasize it more? There are no requirements to the contents of this data since it's filled by the user.

I now understand what you mean by 'valid JSON', thanks for clarification, but the same question might be asked by other developers. I think if we a reference that this json is from HistoryRecording.pushHistoryEvent it would greatly improve clarity.

Sources/MapboxCoreNavigation/HistoryEvent.swift

Sources/MapboxCoreNavigation/ReplayLocationManager.swift

S2Ler · 2023-01-20T03:18:52Z

Sources/MapboxCoreNavigation/ReplayLocationManager.swift

+     Events listener that will receive history events if replaying a `History`.
+     */
+    public var eventsListener: ReplayManagerHistoryEventsListener? = nil
+
    /**
     `simulatesLocation` used to indicate whether the location manager is providing simulated locations.
     - seealso: `NavigationMapView.simulatesLocation`


btw, I have never see custom seealso rendered properly by Xcode.

S2Ler · 2023-01-20T03:20:04Z

Sources/MapboxCoreNavigation/NavigationService.swift

@@ -829,6 +838,16 @@ extension MapboxNavigationService {
    }
 }

+extension MapboxNavigationService: ReplayManagerHistoryEventsListener {
+    public func onEventPublished(_ manager: ReplayLocationManager, event: HistoryEvent) {
+        if let setRouteEvent = event as? RouteAssignmentHistoryEvent {


I noticed that the code only replies to one particular event from the history, not all of them. Can you explain the reasoning behind this design choice?

This handler only tracks RouteAssignmentHistoryEvent to loop any events when route was changed (by user, via re-routing or any other way) to reproduce the history as close as possible. Other events are location updates (which are already forwarded inside the replay manager), status updates and custom user events which are not relevant for replaying history trace and thus ignored.

It would be helpful if you could add a brief comment from what you just said, as it's not immediately clear from the code. This would greatly aid in maintaining and understanding the codebase for future developers.

…breaking change by introducing new initi; style improvements

…ordHistoryEvent to UserPushedHistoryEvent

Udumft · 2023-01-20T14:46:25Z

Thanks for the feedback, I think I've now addressed it all.

Sources/MapboxCoreNavigation/HistoryEvent.swift

Sources/MapboxCoreNavigation/NavigationService.swift

Sources/MapboxCoreNavigation/ReplayLocationManager.swift

Udumft self-assigned this Jan 19, 2023

vk-NAVIOS-841-history-replay: breaking changes accepted and reverted.

a37d8d3

S2Ler reviewed Jan 20, 2023

View reviewed changes

vk-NAVIOS-841-history-replay: made events listener weak; avoided one …

068a601

…breaking change by introducing new initi; style improvements

Udumft marked this pull request as ready for review January 20, 2023 11:16

Udumft requested a review from a team January 20, 2023 11:16

github-actions bot requested review from kried and S2Ler January 20, 2023 11:17

vk-NAVIOS-841-history-replay: code comments appended; renamed PushRec…

5c5df88

…ordHistoryEvent to UserPushedHistoryEvent

kried reviewed Jan 23, 2023

View reviewed changes

S2Ler approved these changes Jan 23, 2023

View reviewed changes

vk-NAVIOS-841-history-replay: made replay events read only

0f2f091

Udumft requested a review from kried January 24, 2023 08:14

kried approved these changes Jan 24, 2023

View reviewed changes

Udumft merged commit c55cb67 into main Jan 24, 2023

Udumft deleted the vk/NAVIOS-841-history-replay branch January 24, 2023 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

History replay improvements #4342

History replay improvements #4342

Udumft commented Jan 19, 2023

mapbox-github-ci-issues-public-1 bot commented Jan 19, 2023

codecov bot commented Jan 19, 2023 •

edited

Udumft commented Jan 19, 2023

S2Ler left a comment

S2Ler Jan 20, 2023

Udumft Jan 20, 2023

S2Ler Jan 20, 2023

S2Ler Jan 20, 2023

Udumft Jan 20, 2023

S2Ler Jan 20, 2023 •

edited

S2Ler Jan 20, 2023

S2Ler Jan 20, 2023

Udumft Jan 20, 2023

S2Ler Jan 20, 2023

Udumft commented Jan 20, 2023

History replay improvements #4342

History replay improvements #4342

Conversation

Udumft commented Jan 19, 2023

Description

Implementation

mapbox-github-ci-issues-public-1 bot commented Jan 19, 2023

API compatibility report for MapboxCoreNavigation: 🔴

Removed Decls

Class Inheritance Change

codecov bot commented Jan 19, 2023 • edited

Codecov Report

Udumft commented Jan 19, 2023

S2Ler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S2Ler Jan 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Udumft commented Jan 20, 2023

codecov bot commented Jan 19, 2023 •

edited

S2Ler Jan 20, 2023 •

edited