Skip to content
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

Electronic horizon position notification should directly contain pointer to edge tree #2906

Closed
1ec5 opened this issue Mar 31, 2021 · 4 comments · Fixed by #2949
Closed

Electronic horizon position notification should directly contain pointer to edge tree #2906

1ec5 opened this issue Mar 31, 2021 · 4 comments · Fixed by #2949
Assignees
Labels
op-ex Refactoring, Tech Debt or any other operational excellence work. release blocker Needs to be resolved before the release. topic: location
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Mar 31, 2021

As of #2834, the electronicHorizonDidUpdatePosition notification’s user info dictionary’s treeKey is set to an instance of the ElectronicHorizon struct, whose only purpose is to hold the starting ElectronicHorizon.Edge of the tree. This is redundant. Assuming we don’t intend to add more properties to MapboxNavigationNative.ElectronicHorizon in the future, we should eliminate the struct and place the starting edge directly in the user info dictionary. The extensive struct documentation comment should be split between ElectronicHorizon.Edge, ElectronicHorizonOptions, and treeKey. Nested types like ElectronicHorizon.Edge should be renamed to no longer nest underneath ElectronicHorizon.

/ref #2834 (comment)
/cc @mapbox/navigation-ios

@1ec5 1ec5 added op-ex Refactoring, Tech Debt or any other operational excellence work. topic: location Needs estimate labels Mar 31, 2021
@1ec5 1ec5 added this to the v2.0.0 (RC) milestone Mar 31, 2021
@1ec5
Copy link
Contributor Author

1ec5 commented Apr 1, 2021

Assuming we don’t intend to add more properties to MapboxNavigationNative.ElectronicHorizon in the future

@mapbox/navnative is this a reasonable assumption? Are there any other reasons for the indirection around the ElectronicHorizon type?

@1ec5 1ec5 added release blocker Needs to be resolved before the release. and removed Needs estimate labels Apr 1, 2021
@SiarheiFedartsou
Copy link
Contributor

Assuming we don’t intend to add more properties to MapboxNavigationNative.ElectronicHorizon in the future

@mapbox/navnative is this a reasonable assumption? Are there any other reasons for the indirection around the ElectronicHorizon type?

The main idea why we don't send ElectronicHorizonEdge directly, but have this ElectronicHorizon is that we potentially may add some fields to ElectronicHorizon itself, which may be helpful for end user. Tbh I don't have examples of those, but who knows? :)

@truburt truburt added wip and removed wip labels Apr 2, 2021
@1ec5
Copy link
Contributor Author

1ec5 commented Apr 20, 2021

The main idea why we don't send ElectronicHorizonEdge directly, but have this ElectronicHorizon is that we potentially may add some fields to ElectronicHorizon itself, which may be helpful for end user. Tbh I don't have examples of those, but who knows? :)

Most likely we’d add any such fields to the notification itself rather than a separate ElectronicHorizon type, but if necessary we do have the flexibility to put an object of a different type in the notification, because notifications aren’t strongly typed.

@azarovalex
Copy link
Contributor

PR merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
op-ex Refactoring, Tech Debt or any other operational excellence work. release blocker Needs to be resolved before the release. topic: location
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants