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

[core] [android] - configurable API endpoint #6309

Merged
merged 1 commit into from
Sep 16, 2016
Merged

[core] [android] - configurable API endpoint #6309

merged 1 commit into from
Sep 16, 2016

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Sep 12, 2016

This PR enables configuration of a custom URL base endpoint.
Review c++: @jfirebaugh @tmpsantos
Review java: @zugaldia

Side notes vs GL-JS implementation:

  • I went for using APIBaseURL over APIURL for readability
  • I initially mimicked the Config class concept but removed it after discussion with @tmpsantos

@tobrun tobrun added Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl labels Sep 12, 2016
@tobrun tobrun added this to the android-v4.2.0 milestone Sep 12, 2016
jfirebaugh
jfirebaugh reviewed Sep 12, 2016
View reviewed changes
platform/default/online_file_source.cpp
@@ -152,30 +152,34 @@ OnlineFileSource::~OnlineFileSource() = default;

std::unique_ptr<AsyncRequest> OnlineFileSource::request(const Resource& resource, Callback callback) {
Resource res = resource;

if(apiBaseURL.empty()){
Copy link
Contributor

Choose a reason for hiding this comment

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

Set the default value to mbgl::util::API_BASE_URL instead of checking for the empty string.

@tobrun
Copy link
Member Author

tobrun commented Sep 12, 2016

Thanks @jfirebaugh for 👀 , the remarks have been fixed in ad5f363.
I will now look into resolving this for the other bindings.

zugaldia
zugaldia reviewed Sep 12, 2016
View reviewed changes
...orm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/MapboxMapOptions.java
@@ -957,6 +978,8 @@ public boolean equals(Object o) {
if (!Arrays.equals(myLocationBackgroundPadding, options.myLocationBackgroundPadding))
return false;
if (style != null ? !style.equals(options.style) : options.style != null) return false;
if (apiBaseUrl != null ? !apiBaseUrl.equals(options.apiBaseUrl) : options.apiBaseUrl != null)
Copy link
Member

Choose a reason for hiding this comment

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

@tobrun This is very hard to read, could you split it up?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is generated code from intelij or actually it's copied code from intelij. The correct way should be to regenerate the hashcode/equals method.. Will pick this up with other remarks but I'm not taking readability into account with generated code.

Copy link
Member

Choose a reason for hiding this comment

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

@tobrun Sorry, I missed this was generated code. Scratch my comment.

@tmpsantos
Copy link
Contributor

This PR is missing unit tests for a new core API. ;-)

@tobrun tobrun force-pushed the tobrun-configurable-base-endpoint branch from 011ef9e to 4db1f6d Compare September 14, 2016 17:54
@tobrun
Copy link
Member Author

tobrun commented Sep 14, 2016

@tmpsantos I fixed up the comments and added a tests in 4db1f6d.

@tobrun tobrun force-pushed the tobrun-configurable-base-endpoint branch from 4db1f6d to a65b937 Compare September 15, 2016 16:53
@1ec5 1ec5 mentioned this pull request Sep 15, 2016
Closed
@1ec5
Copy link
Contributor

1ec5 commented Sep 15, 2016

Tracking the necessary iOS/macOS SDK changes in #6346.

@tobrun tobrun force-pushed the tobrun-configurable-base-endpoint branch from a65b937 to ee14d3b Compare September 15, 2016 18:29
@tobrun
Copy link
Member Author

tobrun commented Sep 15, 2016

Need to fix the tests for glfw, the tests are missing runloop

@tobrun
Copy link
Member Author

tobrun commented Sep 15, 2016

@tmpsantos thanks for the hint on RunLoop.
This is now implemented in 5825a6c and the tests are passing 😎

tmpsantos
tmpsantos reviewed Sep 16, 2016
View reviewed changes
Copy link
Contributor

@tmpsantos tmpsantos left a comment

Choose a reason for hiding this comment

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

Thanks for this and thanks for also adding new tests!

test/storage/online_file_source.cpp
@@ -409,10 +409,9 @@ TEST(OnlineFileSource, ChangeAPIBaseURL){
util::RunLoop loop;
OnlineFileSource fs;

loop.run();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great if you could split the PR in two or three commits:

  • One adding the API to core
  • One adding the API to android
  • One adding the tests

This makes reviewing a lot easier, since reviewing is usually done "per commit". You can do this next time, but for now I'm fine squashing everything into a single commit. The last two are fixing something on the first one, so they don't really add value to the PR.

@tobrun tobrun force-pushed the tobrun-configurable-base-endpoint branch from 5825a6c to 02d8e0e Compare September 16, 2016 00:31
tmpsantos
tmpsantos reviewed Sep 16, 2016
View reviewed changes
test/util/mapbox.cpp
@@ -10,85 +11,85 @@ using namespace mbgl;
TEST(Mapbox, SourceURL) {
EXPECT_EQ(
"https://api.mapbox.com/v4/user.map.json?access_token=key&secure",
mbgl::util::mapbox::normalizeSourceURL("mapbox://user.map", "key"));
mbgl::util::mapbox::normalizeSourceURL(util::API_BASE_URL, "mapbox://user.map", "key"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, extra tab

@tobrun tobrun force-pushed the tobrun-configurable-base-endpoint branch from 02d8e0e to 57665c7 Compare September 16, 2016 01:08
@tobrun tobrun merged commit 9ef6544 into master Sep 16, 2016
@jfirebaugh jfirebaugh deleted the tobrun-configurable-base-endpoint branch September 16, 2016 18:01
@incanus incanus mentioned this pull request Oct 14, 2016
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tmpsantos tmpsantos tmpsantos approved these changes

Assignees
No one assigned
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl
Projects
None yet
Milestone
android-v4.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tobrun @tmpsantos @1ec5 @zugaldia @jfirebaugh