Break out events #1533

frederoni · 2018-07-02T17:55:34Z

Working towards #1500

Fix coding failures with routeLegProgress, applicationState, and startTimestamp
Refactor RouteController for testability and reusability (Vague, but I think we made good progress)
Move all event-related code out of RouteController, and test-drive it into a new private component resembling a RouteControllerEventsDelegate, who receives RouteControllerDelegate messages from the RouteController, generates telemetry events and other notifications, and passes RouteControllerDelegate messages onward

--

Jerrad's Tasks

~~Make sure reroute status hides after reroute~~ (Issue cut -- Reroute indication under instruction banner not dismissed beyond display threshold, during simulation mode. #1569)
EventsManager constructor with Router dependency injected as arg
figure out why sendCancelEvent(rating:comments:) signature is weird, also determine how EORVC feedback works (bad pattern - issue: Events manager should not rely on RouteController.deinit to send arrival messages #1563)
LoD pass
~~dedupe checkForFasterRoute and reroute~~ (unacceptable side effects)

…ctly by establishing a `Router` dependency protocol.

…o's to rid Router of tightly-coupled properties

…ving `locationManager` dependency from RouteController entirely.

…represented in a way the JSON serializer could understand

bsudekum · 2018-07-15T17:41:49Z

@JThramer Saw an issue with a few drives yesterday where the reroute status view was shown constantly although it never actually needed to reroute nor did it actually end up rerouting.

Immediate End Of Route Events

JThramer

A regression has occurred where the puck never moves on a real navigation session -- I suspect this has something to do with the routeController not emitting progress updates.

…uest.

JThramer · 2018-07-31T22:25:11Z

Issue with rerouting regression has been fixed. Still need to do one more regression test and make sure events are sending okay.

JThramer · 2018-07-31T22:26:17Z

Didn't mean to approve, just blow away the "Changes Requested." No biggie, we'll just have another approval before merge.

akitchen · 2018-08-01T01:19:43Z

MapboxCoreNavigation/RouteController.swift

-        NotificationCenter.default.addObserver(self, selector: #selector(progressDidChange(notification:)), name: .routeControllerProgressDidChange, object: self)
-        NotificationCenter.default.addObserver(self, selector: #selector(willReroute(notification:)), name: .routeControllerWillReroute, object: self)
-        NotificationCenter.default.addObserver(self, selector: #selector(didReroute(notification:)), name: .routeControllerDidReroute, object: self)
+        NotificationCenter.default.addObserver(self, selector: #selector(didRecieveTerminationWarning), name: .UIApplicationWillTerminate, object: nil)


Recieve -> Receive

akitchen · 2018-08-01T01:22:49Z

MapboxCoreNavigation/RouteController.swift

+     Ends the current navigation session.
+     */
+    @objc public func endNavigation(feedback: EndOfRouteFeedback? = nil) {
+        if !didSendCancelEvent {


This change would result in under-logging events if RouteControllers are reused. Or is that already well known not to be valid?

I'm asking partly because this is a public method which may imply a certain semantic.

AFAIK, RouteController is not designed to be reused at this time.

@akitchen hmm, so yeah in talking with @bsudekum , it sounds like it might be possible (although out of design scope) to reuse the RouteController, possibly by changing RouteController.routeProgress (as you’re replacing the thing that the Routecontroller is iterating over)… I could reset that flag on routeProgress’ didSet as a defensive measure, how's that sound?

…oop upon ending navigation.

JThramer · 2018-08-01T21:56:25Z

🎵 Scooter Gang, Scooter Gang, Scooter Gang... 🎵

🎉 After extensive ~~drive~~scoot-testing today, I have confirmed that the following events are received by the telemetry system:

navigation_cancel
navigation_reroute
navigation_feedback
navigation_arrive
navigation_depart

I'm going to call this ✅ for purposes of regression-testing telemetry events.

MapboxCoreNavigation/NavigationService.swift

MapboxCoreNavigation/EventsManager.swift

Done in error

frederoni force-pushed the events-1500 branch 2 times, most recently from 9872c38 to 4d855c2 Compare July 2, 2018 17:59

frederoni assigned frederoni and JThramer Jul 2, 2018

bsudekum mentioned this pull request Jul 3, 2018

intermittent test failures #1482

Closed

frederoni and others added 4 commits July 5, 2018 15:35

Break out events

3fa6c04

Refactor RouteController for reusability and testability

d660973

Making EventDetails an immutable struct.

0574135

Encodable EventDetails

f336968

frederoni force-pushed the events-1500 branch from 96a38ff to f336968 Compare July 5, 2018 13:35

frederoni and others added 8 commits July 5, 2018 16:06

Encodable RouteLegProgress

66cb213

Remove excessive call to resetSession

dd372cf

Reset original route

06a84ef

Outline Router and NavigationService

ea53180

Protocolize RouteController

44d4225

Break out Constants and RouteControllerDelegate

439c227

WIP - Decoupling EventsManager/EventDetails from RouteController dire…

8bfffee

…ctly by establishing a `Router` dependency protocol.

WIP - Making Router conform to CLLocationManagerDelegate, marking tod…

7b19fc8

…o's to rid Router of tightly-coupled properties

JThramer added ⚠️ DO NOT MERGE PR should not be merged! op-ex Refactoring, Tech Debt or any other operational excellence work. status: in progress topic: location topic: telemetry labels Jul 10, 2018

WIP -- Cleaning up RouteController rerouting logic in support of remo…

d3fef70

…ving `locationManager` dependency from RouteController entirely.

JThramer force-pushed the events-1500 branch from 870b00d to d3fef70 Compare July 10, 2018 01:13

Merge branch 'master' into events-1500

602e17f

JThramer force-pushed the events-1500 branch from 08a65b8 to 602e17f Compare July 13, 2018 18:19

Removing out-of-spec detail, also fixing bug where date wasn't being …

982a94c

…represented in a way the JSON serializer could understand

Jerrad Thramer and others added 3 commits July 30, 2018 17:05

Merge branch 'master' into events-1500

2948986

Merge branch 'events-1500' into jerrad/events-cancel-event

e1a9e1a

Merge pull request #1581 from mapbox/jerrad/events-cancel-event

b646fb3

Immediate End Of Route Events

JThramer suggested changes Jul 31, 2018

View reviewed changes

JThramer previously approved these changes Jul 31, 2018

View reviewed changes

Fixing issue where rerouting logic was broken by bad RouteOptions req…

4590ed9

…uest.

akitchen reviewed Aug 1, 2018

View reviewed changes

Fixing issue where init clobber causes the app to crash on infinite-l…

785dd44

…oop upon ending navigation.

Typo

9806437

frederoni force-pushed the events-1500 branch 3 times, most recently from fba5542 to ac774ff Compare August 2, 2018 12:10

Decouple EventsManager further and audit access level

250794b

frederoni force-pushed the events-1500 branch from ac774ff to 250794b Compare August 2, 2018 14:10

bsudekum reviewed Aug 2, 2018

View reviewed changes

MapboxCoreNavigation/NavigationService.swift Outdated Show resolved Hide resolved

akitchen suggested changes Aug 2, 2018

View reviewed changes

MapboxCoreNavigation/EventsManager.swift Outdated Show resolved Hide resolved

frederoni and others added 3 commits August 3, 2018 14:52

Remove dead code

0256c99

Reorganize didSendCancelEvent

7a1742b

Rename SessionState flag and move mutation to a more appropriate place

b004632

akitchen approved these changes Aug 3, 2018

View reviewed changes

JThramer added the ⏎ Ready To Merge label Aug 3, 2018

JThramer merged commit b7918a9 into master Aug 3, 2018

JThramer deleted the events-1500 branch August 3, 2018 17:28

JThramer mentioned this pull request Aug 15, 2018

Missing telemetry vales #1606

Merged

1ec5 mentioned this pull request Sep 8, 2020

Route progress classes should be codable #1940

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Break out events #1533

Break out events #1533

Uh oh!

frederoni commented Jul 2, 2018 •

edited by JThramer

Loading

Uh oh!

bsudekum commented Jul 15, 2018

Uh oh!

JThramer left a comment

Uh oh!

JThramer commented Jul 31, 2018

Uh oh!

JThramer commented Jul 31, 2018

Uh oh!

akitchen Aug 1, 2018

Uh oh!

akitchen Aug 1, 2018

Uh oh!

akitchen Aug 1, 2018

Uh oh!

JThramer Aug 1, 2018

Uh oh!

JThramer Aug 1, 2018

Uh oh!

JThramer commented Aug 1, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Break out events #1533

Break out events #1533

Uh oh!

Conversation

frederoni commented Jul 2, 2018 • edited by JThramer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsudekum commented Jul 15, 2018

Uh oh!

JThramer left a comment

Choose a reason for hiding this comment

Uh oh!

JThramer commented Jul 31, 2018

Uh oh!

JThramer commented Jul 31, 2018

Uh oh!

akitchen Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

akitchen Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

akitchen Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

JThramer Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

JThramer Aug 1, 2018

Choose a reason for hiding this comment

Uh oh!

JThramer commented Aug 1, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

frederoni commented Jul 2, 2018 •

edited by JThramer

Loading