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

Update Maps SDK 7.x and events 4.x with new location APIs #1615

Merged
merged 1 commit into from
Jan 14, 2019

Conversation

danesfeder
Copy link
Contributor

@danesfeder danesfeder commented Dec 6, 2018

Closes #1121 Closes #928

This PR brings in the new Location APIs from events 4.x. This is a decent refactor as we use location services a lot and in almost every activity of the test app.

TODO:

  • Fix-up remaining test activities
  • Add / update tests
  • Bring in correct Maps SDK dependency

@danesfeder danesfeder added ⚠️ DO NOT MERGE PR should not be merged! navigation-core backwards incompatible Requires a SEMVER major version change. labels Dec 6, 2018
@danesfeder danesfeder self-assigned this Dec 6, 2018
@danesfeder
Copy link
Contributor Author

@andrlee I'm cutting this a bit prematurely to get eyes on it as soon as possible. Please try to ignore the mess.

@danesfeder danesfeder added this to the v0.25.0 milestone Dec 7, 2018
@Guardiola31337
Copy link
Contributor

Bring in correct Maps SDK dependency

Actually, this is blocked upstream until the Maps SDK 7.0.0 major version (including the new Location APIs from Events 4.0.0) is available i.e. this work may be slipped out from the upcoming 0.25.0 release because we are going to also address other Maps SDK breaking changes.

@LukasPaczos
Copy link

Just wanted to flag mapbox/mapbox-events-android#312 as a potential issue that will need a workaround until fixed upstream. Workaround tracked for the Maps SDK in mapbox/mapbox-gl-native#13587.

@danesfeder danesfeder modified the milestones: v0.25.0, v0.26.0, v0.27.0 Dec 18, 2018
@danesfeder danesfeder changed the title [WIP] Update to events 4.x with new location APIs [WIP] Update Maps SDK 7.x and events 4.x with new location APIs Jan 7, 2019
@Guardiola31337 Guardiola31337 force-pushed the dan-update-location branch 2 times, most recently from d85b27b to e5d0b1c Compare January 8, 2019 13:01
@danesfeder danesfeder force-pushed the dan-update-location branch from e5d0b1c to 40b7d47 Compare January 8, 2019 14:12
@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Jan 8, 2019

While testing this, I run into some issues.

TODO here:

  • 👀 leaks originated by new location engine
    • NavigationLauncherActivity
    • ComponentNavigationActivity
  • Revisit ReplayRouteLocationEngine implementation
    • EndNavigationActivity
java.lang.Exception: location can't be null
            at com.mapbox.services.android.navigation.v5.location.replay.ReplayRouteLocationEngine.getLastLocation(ReplayRouteLocationEngine.java:102)
java.lang.Exception: No route found to replay.
            at com.mapbox.services.android.navigation.v5.location.replay.ReplayRouteLocationEngine.requestLocationUpdates(ReplayRouteLocationEngine.java:115)
java.lang.Exception: location can't be null
            at com.mapbox.services.android.navigation.v5.location.replay.ReplayRouteLocationEngine.getLastLocation(ReplayRouteLocationEngine.java:102)
  • RerouteActivity - doesn’t allow re-routes (ReplayRouteLocationEngine related)
  • WaypointNavigationActivity - doesn’t navigate to next destination (ReplayRouteLocationEngine related)
  • MockNavigationActivity doesn’t fetch routes (calculateRoute is commented out) - code needs to be adapted to new location lib changes locationEngine.getLastLocation() is not available anymore)

@danesfeder danesfeder force-pushed the dan-update-location branch from c687ea0 to a9e8c36 Compare January 8, 2019 20:26
@danesfeder
Copy link
Contributor Author

@Guardiola31337 I think I've fixed everything not related to ReplayRouteLocationEngine. All leaks were addressed with WeakReference to my knowledge.

I removed the ReplayRouteLocationEngine#moveTo API. I've not sure if it makes sense anymore given the new LocationEngine API. It was preventing RerouteActivity from working properly.

I found this while testing DualNavigationMapActivity:

ezgif com-video-to-gif

As discussed, have a stab at the ReplayRouteLocationEngine APIs - I believe that's one of the last problems for us to solve here 🎉

@danesfeder danesfeder force-pushed the dan-update-location branch from f430dbd to 373b4b4 Compare January 8, 2019 22:39

@SuppressLint("MissingPermission")
private void requestInitialLocationUpdates(LocationEngine locationEngine, LocationEngineRequest request) {
locationEngine.getLastLocation(callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens in the situation in which the last location is really far from where you actually are? Could this end up in a "undesired" flying effect? I'm wondering if we should / could remove it 🤔

@Guardiola31337
Copy link
Contributor

Run into the following leak while testing EndNavigationActivity 👀

leak_end_navigation_activity

Not sure if caused by the changes introduced here. If not, we should cut a separate ticket to investigate further.

@danesfeder danesfeder force-pushed the dan-update-location branch 10 times, most recently from b177645 to 588888e Compare January 11, 2019 17:01
@danesfeder
Copy link
Contributor Author

Per discussion with @Guardiola31337, we have one remaining issue regarding allowing users to zoom in too closely to the map. He's going to pick that up and then bring this PR home 🎉

The rest of the issues we found while testing seem unrelated to the Maps update and we will ticket as necessary.

@Guardiola31337 Guardiola31337 force-pushed the dan-update-location branch 2 times, most recently from 5f151db to 28fc5db Compare January 14, 2019 12:01
@Guardiola31337
Copy link
Contributor

we have one remaining issue regarding allowing users to zoom in too closely to the map

This is now fixed 🎉

Noting that the zoom levels in the LocationComponent aren't restricted (by default) anymore. This was disabled in mapbox/mapbox-gl-native#13097 / mapbox/mapbox-gl-native#13425

Now you should tune the the max / min zoom level of the map yourself with MapboxMapOptions#maxZoomPreference / MapboxMapOptions#minZoomPreference or MapboxMap#setMaxZoomPreference / MapboxMap#setMinZoomPreference Thanks @LukasPaczos for clarifying 🙏

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

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

Let's merge here 🎉

Great work @danesfeder

@jpcod3s
Copy link

jpcod3s commented Jan 21, 2019

image
it keeps showing this on android studio 3.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible Requires a SEMVER major version change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants