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

[android] Integration of the new events library #10999

Merged
merged 7 commits into from
Feb 13, 2018

Conversation

Guardiola31337
Copy link
Contributor

@Guardiola31337 Guardiola31337 added Android Mapbox Maps SDK for Android telemetry Integration with Mapbox Telemetry libraries ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jan 23, 2018
@Guardiola31337 Guardiola31337 added this to the android-v6.0.0 milestone Jan 23, 2018
@Guardiola31337 Guardiola31337 self-assigned this Jan 23, 2018
@zugaldia
Copy link
Member

@Guardiola31337 @electrostat Parallel to this PR, @osana is working on integrating the MAS 3.x on #10920 and I believe she's encountering some (old) telemetry integration issues. Would you recommend working on MAS 3.x integration as branched work from this PR, or should we continue with the current approach on #10920?

@osana
Copy link
Contributor

osana commented Jan 23, 2018

@zugaldia @electrostat @Guardiola31337 @cammace @tobrun

Turned out that the issue I had was related to my JNI code but the error message said that it was not finding some class related to GoogleLocationEngine. I should be good for now. If needed I will cherry-pick my change into the a child branch of this one.

@Guardiola31337
Copy link
Contributor Author

Would you recommend working on MAS 3.x integration as branched work from this PR, or should we continue with the current approach on #10920?

So far, we needed to create MathUtils (not available in the old library anymore) and used it in CameraPosition and MapGestureDetector.

So, whatever is easier for you ¯\_(ツ)_/¯

@@ -36,6 +35,8 @@
private String accessToken;
private Boolean connected;
private LocationEngine locationEngine;
@SuppressLint("StaticFieldLeak")
private static MapboxTelemetry telemetry;
Copy link
Member

Choose a reason for hiding this comment

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

any background on why this should be static?

// TODO transform.getZoom() may cause a NullPointerException
MapState twoFingerTap = new MapState((float) latLng.getLatitude(), (float) latLng.getLongitude(), (float)
transform.getZoom());
twoFingerTap.setGesture("TwoFingerTap");
Copy link
Member

Choose a reason for hiding this comment

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

do we have any gesture constants? eg. for TwoFingerTap etc.

MapboxEvent.GESTURE_TWO_FINGER_SINGLETAP, transform));
MapEventFactory mapEventFactory = new MapEventFactory();
LatLng latLng = projection.fromScreenLocation(new PointF(event.getX(), event.getY()));
// TODO transform.getZoom() may cause a NullPointerException
Copy link
Member

Choose a reason for hiding this comment

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

on top of this todo, would it be possible hardening add same checks as with #10959 ?

MapEventFactory mapEventFactory = new MapEventFactory();
LatLng latLng = projection.fromScreenLocation(new PointF(event.getX(), event.getY()));
// TODO transform.getZoom() may cause a NullPointerException
MapState twoFingerTap = new MapState((float) latLng.getLatitude(), (float) latLng.getLongitude(), (float)
Copy link
Member

Choose a reason for hiding this comment

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

Would it makes sense changing MapState to double instead of float?

Guardiola31337 added a commit that referenced this pull request Jan 29, 2018
@@ -65,7 +65,9 @@ public static synchronized Mapbox getInstance(@NonNull Context context, @NonNull
}

ConnectivityReceiver.instance(appContext);
Timber.d("instance: connectivity receiver started");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How adding a log solves a JNI bug?
In any case, we should remove these logs before merging because they'd clutter Logcat unnecessarily.

@Guardiola31337
Copy link
Contributor Author

@tobrun
All the comments are addressed in https://github.com/mapbox/mapbox-gl-native/tree/pg-fix-events-lib-integration-comments, right now build is failing because changes depend on mapbox/mapbox-events-android#60 and mapbox/mapbox-events-android#61 when those get merged, we should cherry-pick 0dd1937 in and this PR should be ready to go 🚀

cc @electrostat

Guardiola31337 added a commit that referenced this pull request Jan 29, 2018
Guardiola31337 added a commit that referenced this pull request Jan 29, 2018
Copy link

@electrostat electrostat left a comment

Choose a reason for hiding this comment

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

Looks Good.
@tobrun give it a once over, if you can. Then I think we are good to go on this one.

Copy link
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.

Some comments to the PR, non-blocking, can be addressed in follow up tickets if necessary.

@@ -0,0 +1,79 @@
package com.mapbox.mapboxsdk.utils;

// TODO Remove this class if we finally include it within MAS 3.x (GeoJSON)
Copy link
Member

Choose a reason for hiding this comment

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

Remove or ticket.

Choose a reason for hiding this comment

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

Ticketed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@electrostat

Ticketed

This should be ticketed in this repo 👉 https://github.com/mapbox/mapbox-gl-native/issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up ticket 👉 #11182

/**
* Returns the location engine used by the SDK.
*
* @return the location engine configured
*/
// TODO Do we need to expose this?
Copy link
Member

Choose a reason for hiding this comment

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

Remove or ticket.

Choose a reason for hiding this comment

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

Ticketed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@electrostat

Ticketed

This should be ticketed in this repo 👉 https://github.com/mapbox/mapbox-gl-native/issues

@@ -61,7 +61,8 @@ dependencies {
implementation dependenciesList.supportRecyclerView
implementation dependenciesList.supportDesign

implementation dependenciesList.lost
// implementation dependenciesList.lost
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated line.

@@ -9,6 +9,7 @@ ext {

versions = [
mapboxServices: '2.2.9',
mapboxTelemetryVersion: '3.0.0-SNAPSHOT',
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to rely on a SNAPSHOT? If so, upgrading to a stable version needs to be ticketed.

@@ -42,6 +43,7 @@ ext {
supportRecyclerView : "com.android.support:recyclerview-v7:${versions.supportLib}",

lost : "com.mapzen.android:lost:${versions.lost}",
gmsLocation : 'com.google.android.gms:play-services-location:11.0.4',
Copy link
Member

Choose a reason for hiding this comment

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

We should use ${versions.*} for consistency here too.

@@ -25,6 +25,8 @@ android {
minSdkVersion androidVersions.minSdkVersion
targetSdkVersion androidVersions.targetSdkVersion
buildConfigField "String", "GIT_REVISION_SHORT", String.format("\"%s\"", getGitRevision())
buildConfigField "String", "MAPBOX_SDK_IDENTIFIER", String.format("\"%s\"", "mapbox-maps-android")
buildConfigField "String", "MAPBOX_SDK_VERSION", String.format("\"%s\"", project.VERSION_NAME)
buildConfigField "String", "MAPBOX_VERSION_STRING", String.format("\"Mapbox/%s\"", project.VERSION_NAME)
Copy link
Member

Choose a reason for hiding this comment

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

Is MAPBOX_VERSION_STRING still being used or was it replaced by MAPBOX_SDK_VERSION?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tobrun
Copy link
Member

tobrun commented Feb 13, 2018

can we update this PR with telem beta version and merge this?

@Guardiola31337
Copy link
Contributor Author

can we update this PR with telem beta version and merge this?

bcf3d42 addresses ☝️

I guess that we’re moving forward and we’ll remove MathUtils (#10999 (comment)) later, when mapbox/mapbox-java#719 lands and a new beta of MAS 3.0 is available, right?

@Guardiola31337 Guardiola31337 merged commit 44ca0a5 into master Feb 13, 2018
@Guardiola31337 Guardiola31337 deleted the pg-events-lib-integration branch February 13, 2018 10:31
@Guardiola31337
Copy link
Contributor Author

I guess that we’re moving forward and we’ll remove MathUtils (#10999 (comment)) later, when mapbox/mapbox-java#719 lands and a new beta of MAS 3.0 is available, right?

Follow up ticket 👉 #11182

Guardiola31337 added a commit that referenced this pull request Feb 13, 2018
* [android] integration of the new events library

* JNI Bug

- current build with JNI bug

* fix #10999 comments

* Clean-up

- clean-up timbers and test code

* [android] fix sdk identifier and sdk version

* [android] merge from master (MAS 3.0)

* [android] bump events lib version to 3.0.0-beta.1 and remove never used methods from math utils class
Guardiola31337 added a commit that referenced this pull request Feb 13, 2018
* [android] integration of the new events library

* JNI Bug

- current build with JNI bug

* fix #10999 comments

* Clean-up

- clean-up timbers and test code

* [android] fix sdk identifier and sdk version

* [android] merge from master (MAS 3.0)

* [android] bump events lib version to 3.0.0-beta.1 and remove never used methods from math utils class
@tobrun tobrun mentioned this pull request Feb 13, 2018
23 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold telemetry Integration with Mapbox Telemetry libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants