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

Request location updates for AndroidLocationEngine #602

Merged
merged 1 commit into from
Jul 26, 2018
Merged

Request location updates for AndroidLocationEngine #602

merged 1 commit into from
Jul 26, 2018

Conversation

LukasPaczos
Copy link
Contributor

Fixes #601.

@LukasPaczos LukasPaczos added the location-layer-plugin Issues that deal with the location layer module label Jul 23, 2018
@LukasPaczos LukasPaczos added this to the location-layer-0.7.1 milestone Jul 23, 2018
danesfeder
danesfeder approved these changes Jul 23, 2018
View reviewed changes
Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

@LukasPaczos thanks for the quick work on this and great catch @Guardiola31337 🚀

@Guardiola31337
Copy link
Contributor

Run into the following leak while testing OP changes (LocationLayerMapChangeActivity from the test app) 👀

llp_android_location_engine_leak

@LukasPaczos
Copy link
Contributor Author

Any reproduction steps @Guardiola31337?

@Guardiola31337
Copy link
Contributor

Also noticed that LocationLayerPlugin#getLastKnownLocation is always returning null if you're stale when using AndroidLocationEngine. This could be LocationManager#getLastKnownLocation malfunctioning though. I'll keep digging.

cc @electrostat

@Guardiola31337
Copy link
Contributor

Any reproduction steps

Yeah, closing and reopening the Activity a couple of times causes the leak 👀

llp_android_location_engine_leak

@LukasPaczos LukasPaczos force-pushed the 601-android-location-engine-activation branch from 1dbbc19 to 3159c36 Compare July 23, 2018 14:57
@LukasPaczos
Copy link
Contributor Author

Leak fixed with 3159c36, thanks @Guardiola31337!

@Guardiola31337 Guardiola31337 mentioned this pull request Jul 23, 2018
Closed
Guardiola31337
Guardiola31337 approved these changes Jul 23, 2018
View reviewed changes
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.

I'm still experiencing #602 (comment) but couldn't reproduce the leak anymore so 🚀

Thanks @LukasPaczos

@Guardiola31337 Guardiola31337 mentioned this pull request Jul 23, 2018
Closed
@Guardiola31337
Copy link
Contributor

I'm still experiencing #602 (comment)

I cut a follow up ticket to keep digging 👉 mapbox/mapbox-events-android#187

cc @electrostat

@LukasPaczos LukasPaczos merged commit ed2eb2d into master Jul 26, 2018
@LukasPaczos LukasPaczos deleted the 601-android-location-engine-activation branch July 26, 2018 13:28
@danesfeder danesfeder mentioned this pull request Jul 26, 2018
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Guardiola31337 Guardiola31337 Guardiola31337 approved these changes

@danesfeder danesfeder danesfeder approved these changes

@tobrun tobrun Awaiting requested review from tobrun

Assignees
No one assigned
Labels
location-layer-plugin Issues that deal with the location layer module
Projects
None yet
Milestone
location-layer-0.7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@LukasPaczos @Guardiola31337 @danesfeder