Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Navigation Service, Part 1 #1602
Navigation Service, Part 1 #1602
Changes from 6 commits
57ed105
ef09aac
23818fc
a9611d1
8f47670
099ddc4
b24c8bf
518a89e
8e44849
d169b06
3a80e14
cedabcb
230351f
8c04240
7f5aa7f
089a39a
9c5b70d
950cd68
d49ebdc
3e8edde
d6f8498
8a8fc79
7b8ed23
e454202
45795e4
a139f89
4fc2e2a
297f7b4
ee3cd30
cec16b6
b5c0c03
fdcb350
9c7c9f9
2049a30
41265b1
89c73cf
2e161df
6bccfd6
edbb56d
0994b2b
e9a7781
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 PR introduces some significant new concepts that will affect most non-trivial usage of the navigation SDK. So we’ll need an update to the changelog that enumerates the publicly visible changes (or links to a PR description that does so).
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.
nit: no need to override the default implementation for delegate with the same implementation as the superclass already provides.
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.
Using protocol composition as a substitute for explicitly plumbing through delegate methods normally has one major flaw: since the umbrella protocol is just a passthrough for other protocols, it can’t give the delegate any additional context in the form of method arguments.
In this case, the delegate has no clue which NavigationService is calling the delegate methods, only the router or tunnel intersection manager. This makes a many-to-one (delegate-to-delegator) relationship impractical. The delegate would have to compare
routeController
tonavigationService.router
if it already has a reference tonavigationService
. Otherwise, it would have to rely on a backpointer (!) fromRouteController
toNavigationService
, which itself could violate a many-to-one (navigation service–to–router) relationship.In other words, suppose I registered to be the delegate for (at least one) navigation service. When I get delegate methods back, I should know which of the navigation services is messaging me. I shouldn’t have to infer it based on the identity of some navigation service’s router.
The classical approach would be to plumb delegate methods through the implementer of this protocol, adding a
navigationService
argument. If that is undesirable, then make NavigationServiceDelegate a type alias, removeNavigationService.delegate
, and make application code talk directly to NavigationService’s router and tunnel intersection manager. I took the latter approach in #1601 in order to more loosely couple the map and navigation SDKs, but it does come at the cost of exposing implementation details.This may not be a practical concern. For all I know, there may be only one relevant navigation service and the application may never need to talk to both simultaneously. But there’s nothing in the public interface that allows the developer to rely on that fact.
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.
@1ec5 You're totally right here, and
NavigationService
is only composed as-is for purposes of convenience. I am planning on doing exactly that you suggested as part of therefactor delegation protocols
task.