Skip to content
This repository was archived by the owner on Aug 8, 2023. It is now read-only.

[android] Make location provider optional#9488

Merged
Guardiola31337 merged 1 commit into
masterfrom
pg-9403-location-optional
Nov 2, 2017
Merged

[android] Make location provider optional#9488
Guardiola31337 merged 1 commit into
masterfrom
pg-9403-location-optional

Conversation

@Guardiola31337
Copy link
Copy Markdown
Contributor

@Guardiola31337 Guardiola31337 commented Jul 12, 2017

In order to merge this PR mapbox-java/#502 has to be merged first.

👀 @tobrun @zugaldia

@Guardiola31337 Guardiola31337 added Android Mapbox Maps SDK for Android localization Human language support and internationalization ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jul 12, 2017
@Guardiola31337 Guardiola31337 added this to the android-v5.2.0 milestone Jul 12, 2017
zugaldia
zugaldia reviewed Jul 12, 2017
View reviewed changes
Comment thread platform/android/MapboxGLAndroidSDK/build.gradle Outdated
compile rootProject.ext.dep.timber
compile rootProject.ext.dep.okhttp3
compile(rootProject.ext.dep.lost) {
provided(rootProject.ext.dep.lost) {
Copy link
Copy Markdown
Member

@zugaldia zugaldia Jul 12, 2017

Choose a reason for hiding this comment

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

Could it be completely removed and rely solely on MAS?

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 Jul 18, 2017

Choose a reason for hiding this comment

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

@zugaldia Nope, mainly because LocationSource is part of the public API. We should deprecate LocationSource and eventually remove it, at that point we'll be able to completely remove that dependency and rely solely on MAS.

zugaldia
zugaldia reviewed Jul 12, 2017
View reviewed changes
Comment thread platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/Mapbox.java Outdated
}

Mapbox(@NonNull Context context, @NonNull String accessToken, LocationSource locationSource) {
Mapbox(@NonNull Context context, @NonNull String accessToken, LocationEngine locationSource) {
Copy link
Copy Markdown
Member

@zugaldia zugaldia Jul 12, 2017

Choose a reason for hiding this comment

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

This would be an API breaking change - let's explore if we can work it out in a way we keep public APIs untouched for now. Adding is fine, not removing or changing if we want this merged shorter term.

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 Jul 18, 2017

Choose a reason for hiding this comment

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

@zugaldia Mapbox is only used internally (note that there is no modifier) so it doesn't mean an API breaking change.

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 Jul 18, 2017

Choose a reason for hiding this comment

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

@zugaldia Problem though is public getLocationSource() which returns a LocationSource.

Copy link
Copy Markdown
Contributor Author

@Guardiola31337 Guardiola31337 Jul 18, 2017

Choose a reason for hiding this comment

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

@zugaldia I made some changes to address ☝️ without breaking semantic versioning.
👀 b7ccf16
What do you think?

zugaldia
zugaldia reviewed Jul 12, 2017
View reviewed changes
Comment thread platform/android/build.gradle Outdated
allprojects {
repositories {
jcenter()
mavenLocal()
Copy link
Copy Markdown
Member

@zugaldia zugaldia Jul 12, 2017

Choose a reason for hiding this comment

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

This needs to be removed before merging.

@Guardiola31337 Guardiola31337 force-pushed the pg-9403-location-optional branch from a4bfc86 to b7ccf16 Compare July 18, 2017 13:13
@zugaldia zugaldia mentioned this pull request Jul 21, 2017
Closed
@Guardiola31337 Guardiola31337 changed the title [android] [WIP] Make location provider optional [android] Make location provider optional Jul 25, 2017
@Guardiola31337 Guardiola31337 added ✓ ready for review ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jul 25, 2017
@tobrun
Copy link
Copy Markdown
Member

tobrun commented Aug 29, 2017

Any progress on this PR? Would love to start testing this integration.

@Guardiola31337 Guardiola31337 force-pushed the pg-9403-location-optional branch from b7ccf16 to 9dedf29 Compare September 29, 2017 11:31
@Guardiola31337
Copy link
Copy Markdown
Contributor Author

Guardiola31337 commented Sep 29, 2017 •
edited
Loading

Code has been updated to use the new APIs 👀 mapbox/mapbox-java#502 (comment) so this PR is ready for review.

@tobrun @zugaldia

@Guardiola31337 Guardiola31337 force-pushed the pg-9403-location-optional branch 4 times, most recently from abf9737 to 02167e5 Compare October 9, 2017 15:55
@zugaldia
Copy link
Copy Markdown
Member

zugaldia commented Oct 13, 2017

Code has been updated to use the new APIs 👀 mapbox/mapbox-java#502 (comment) so this PR is ready for review.

Considering that the location code in MAS is getting its own artifact, I'd suggest holding on this PR until this new artifact is ready. Unless timing doesn't align well with the new release, in that case we could revisit the approach.

@Guardiola31337 Guardiola31337 force-pushed the pg-9403-location-optional branch from 02167e5 to dbe2342 Compare November 1, 2017 20:16
zugaldia
zugaldia approved these changes Nov 1, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@zugaldia zugaldia 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 ship it as soon as the SNAPSHOT is replaced with a release.

@tobrun
Copy link
Copy Markdown
Member

tobrun commented Nov 1, 2017

🚀

@Guardiola31337 Guardiola31337 force-pushed the pg-9403-location-optional branch from dbe2342 to be72dbd Compare November 1, 2017 22:51
@Guardiola31337 Guardiola31337 force-pushed the pg-9403-location-optional branch from be72dbd to 33ee844 Compare November 1, 2017 23:03
@Guardiola31337 Guardiola31337 merged commit f13a8b0 into master Nov 2, 2017
@Guardiola31337 Guardiola31337 deleted the pg-9403-location-optional branch November 2, 2017 00:12
Guardiola31337 added a commit that referenced this pull request Nov 2, 2017
@Guardiola31337
[android] make location provider optional (#9488)
a9dd08b
Guardiola31337 added a commit that referenced this pull request Nov 2, 2017
@Guardiola31337
[android] make location provider optional (#9488) (#10354)
29a94ae
This was referenced Nov 17, 2017
Closed
Merged
Closed
@Guardiola31337 Guardiola31337 mentioned this pull request Dec 12, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@zugaldia zugaldia zugaldia approved these changes

@tobrun tobrun tobrun approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Guardiola31337 Guardiola31337

Labels

Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold localization Human language support and internationalization

Projects

None yet

Milestone

android-v5.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@Guardiola31337 @tobrun @zugaldia @lilykaiser