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.

Sign up for GitHub

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

Jump to bottom

Remove offline #2509

Merged
merged 12 commits into from
Aug 24, 2020
Merged

Remove offline #2509

merged 12 commits into from
Aug 24, 2020

Conversation

IodaMikeMapbox
Copy link
Contributor

@IodaMikeMapbox IodaMikeMapbox commented Jul 30, 2020 •
edited
Loading

In preparation for the OfflineService to land in the Maps and Nav native libs, and platform delivery in Nav SDK v1.1, we should remove all of the legacy offline APIs that were implemented as part of the Nav UI SDK v0.x.

The pr removes:

  • NavigationDirections
  • offline settings from the example app, but leaves settings screen. Settings button will be disabled if there is no settings to show on the settings screen

Resolves mapbox/navigation-sdks#490

@IodaMikeMapbox IodaMikeMapbox self-assigned this Jul 30, 2020
Udumft
Udumft reviewed Jul 31, 2020
View reviewed changes
Copy link
Contributor

@Udumft Udumft left a comment

Choose a reason for hiding this comment

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

A few comments, otherwise LGTM

Example/ViewController.swift Outdated Show resolved Hide resolved
Example/ViewController.swift Show resolved Hide resolved
MaximAlien
MaximAlien reviewed Aug 3, 2020
View reviewed changes
Copy link
Contributor

@MaximAlien MaximAlien left a comment

Choose a reason for hiding this comment

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

I can see that let MBSelectedOfflineVersion = "MBSelectedOfflineVersion" is never used in Example. Do we still need it?

@MaximAlien
Copy link
Contributor

Also just to make sure: I can see that there are several offline related entries (OFFLINE_) in Localizable.strings. We'll use the same ones in upcoming offline service implementation?

@1ec5
Copy link
Contributor

1ec5 commented Aug 4, 2020

Also just to make sure: I can see that there are several offline related entries (OFFLINE_) in Localizable.strings. We'll use the same ones in upcoming offline service implementation?

I think most of them won’t be relevant to the offline service feature, since the user won’t have the opportunity to choose an arbitrary bounding box to download.

@asinghal22
Copy link

I am not sure why this is part of v1.0 scope. Removing legacy should be part of v1.1. Am I missing something here. @1ec5

@IodaMikeMapbox
Copy link
Contributor Author

I can see that let MBSelectedOfflineVersion = "MBSelectedOfflineVersion" is never used in Example. Do we still need it?

removed

Also just to make sure: I can see that there are several offline related entries (OFFLINE_) in Localizable.strings. We'll use the same ones in upcoming offline service implementation?

removed

@IodaMikeMapbox
Copy link
Contributor Author

I am not sure why this is part of v1.0 scope. Removing legacy should be part of v1.1. Am I missing something here. @1ec5

@asinghal22
We should remove all of the legacy offline APIs that were implemented as part of the Nav UI SDK v0.x. This will give us a clear marketing strategy and avoid confusion/breaking changes when v1.1 rolls out. What's important, those legacy features were never released publicly, so we are not lowering the bar.

For more info please check mapbox/navigation-sdks#484

@1ec5 1ec5 mentioned this pull request Aug 5, 2020
Merged
@asinghal22
Copy link

We should remove all of the legacy offline APIs that were implemented as part of the Nav UI SDK v0.x. This will give us a clear marketing strategy and avoid confusion/breaking changes when v1.1 rolls out. What's important, those legacy features were never released publicly, so we are not lowering the bar.

How about this link?

https://docs.mapbox.com/ios/navigation/overview/offline-routing/#offline-routing-in-the-navigation-sdk-example-app

@1ec5
Copy link
Contributor

1ec5 commented Aug 5, 2020

@asinghal22 is correct, we did publicly release a version of offline navigation on iOS. What we never released on iOS was the hybrid navigation feature. In any event, if we don’t remove this offline navigation functionality ahead of v1.0, we won’t be able to remove it until v2.0, and that may prevent us from introducing a planned revamped offline navigation feature in v1.1.

@1ec5 1ec5 mentioned this pull request Aug 5, 2020
Closed
@1ec5 1ec5 force-pushed the ima-remove-offline-view branch 2 times, most recently from 277c5cb to a75de24 Compare August 5, 2020 03:40
@1ec5 1ec5 changed the base branch from release-v1.0-pre-registry to master August 5, 2020 20:48
This was referenced Aug 18, 2020
Merged
Closed
1ec5
1ec5 reviewed Aug 18, 2020
View reviewed changes
MapboxCoreNavigationTests/OfflineRoutingTests.swift Outdated
class OfflineRoutingTests: XCTestCase {
func testOfflineDirections() {
let bundle = Bundle(for: Fixture.self)
let tilesURL = URL(fileURLWithPath: bundle.bundlePath.appending("/tiles/liechtenstein"))
Copy link
Contributor

Choose a reason for hiding this comment

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

#2546 proposes reusing this test fixture, if not the overall test, to facilitate automated integration testing of the PassiveLocationDataSource in free driving mode.

1ec5
1ec5 reviewed Aug 19, 2020
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment •
edited by IodaMikeMapbox
Loading

Choose a reason for hiding this comment

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

@asinghal22 requested that we leave the legacy offline routing functionality in place but remove it from the officially documented public API. So we’ll need to rework this PR a bit:

  • Restore MapboxCoreNavigation/OfflineDirections.swift
  • Add :nodoc: to each of the public symbols in that file and mark each of them as deprecated, in case a developer is already using them:
    • NavigationDirectionsCompletionHandler
    • OfflineRoutingError
    • UnpackProgressHandler
    • UnpackCompletionHandler
    • OfflineRouteCompletionHandler
    • NavigationDirections
  • Revert the changes to MapboxCoreNavigation/Resources/*.lproj/Localizable.strings
  • Restore MapboxCoreNavigationTests/OfflineRoutingTests.swift
  • Ensure MapboxNavigation.xcodeproj/project.pbxproj is consistent with the files restored above

1ec5
1ec5 reviewed Aug 20, 2020
View reviewed changes
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

In addition to the feedback below, some of these removed methods are still listed in the table of contents in docs/jazzy.yml. We should also remove the entire “Offline navigation” section under the “v1.0” heading of the changelog and add an “Other changes” blurb about each of the :nodoc:’d symbols in #2509 (review) being deprecated.

Example/ViewController.swift Outdated Show resolved Hide resolved
MapboxCoreNavigation/OfflineDirections.swift Outdated Show resolved Hide resolved
MapboxCoreNavigation/OfflineDirections.swift Outdated Show resolved Hide resolved
MapboxCoreNavigation/OfflineDirections.swift Outdated Show resolved Hide resolved
1ec5
1ec5 approved these changes Aug 21, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@1ec5
Copy link
Contributor

1ec5 commented Aug 21, 2020 •
edited
Loading

To facilitate cherry-picking to the release-v1.0 branch, please squash-merge this PR. Thanks!

@1ec5 1ec5 added op-ex Refactoring, Tech Debt or any other operational excellence work. topic: directions labels Aug 21, 2020
@IodaMikeMapbox IodaMikeMapbox merged commit c14838a into master Aug 24, 2020
@IodaMikeMapbox IodaMikeMapbox deleted the ima-remove-offline-view branch August 24, 2020 12:37
This was referenced Nov 23, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ec5 1ec5 1ec5 approved these changes

@MaximAlien MaximAlien Awaiting requested review from MaximAlien

@Udumft Udumft Awaiting requested review from Udumft

Assignees

@IodaMikeMapbox IodaMikeMapbox

Labels
op-ex Refactoring, Tech Debt or any other operational excellence work. topic: directions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@IodaMikeMapbox @MaximAlien @1ec5 @asinghal22 @Udumft @YodaMike