Skip to content

Remove unnecessary Navigator status pulling from updateRouteLeg method#3269

Closed
S2Ler wants to merge 2 commits into
mainfrom
s2ler/remove-sync-nav-status-from-leg-change
Closed

Remove unnecessary Navigator status pulling from updateRouteLeg method#3269
S2Ler wants to merge 2 commits into
mainfrom
s2ler/remove-sync-nav-status-from-leg-change

Conversation

@S2Ler
Copy link
Copy Markdown
Contributor

@S2Ler S2Ler commented Aug 18, 2021

After navigator.changeRouteLeg, a new navigator status will be generated with origin equal to changeRouteLeg where we update indexes anyway.

Related comment: #3265 (comment)

S2Ler added 2 commits August 18, 2021 14:54
After navigator.changeRouteLeg, a new navigator status will be
generated with origin equal to changeRouteLeg where we update indexes
anyway.
@S2Ler S2Ler added this to the v2.0.0 (GA) milestone Aug 18, 2021
@S2Ler S2Ler requested review from a team and 1ec5 August 18, 2021 11:57
@S2Ler S2Ler self-assigned this Aug 18, 2021
}
private func updateRouteLeg(to legIndex: Int) {
precondition(legIndex >= 0, "Leg index can't be negative")
navigator.changeRouteLeg(forRoute: 0, leg: UInt32(legIndex))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the navigator responds to the leg change and updates the navigator status very quickly and synchronously, RouteController responds to the status update asynchronously. This could violate the developer’s expectation, because updateRouteLeg(to:) is called from the public Router.advanceLegIndex() method, which looks like a synchronous method to the developer. The developer may be following up with operations that depend on the status being updated, for example to update UI to reflect the new leg.

To set the right expectations, I think Router.advanceLegIndex() will need to be renamed Router.advanceLegIndex(completionHandler:). Alternatively, we could leave the redundant getStatus() in place if MapboxNavigationNative considers it harmless.

@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Sep 9, 2021
@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Sep 17, 2021

#3342 removes the synchronous getStatus() call. The only other thing in this PR is the assertion that the leg index is nonnegative. However, that’s more of a precaution than an expected failure mode, since the developer no longer has the option to set an arbitrary leg index.

@1ec5 1ec5 closed this Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backwards incompatible changes that break backwards compatibility of public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants