Introduce abstract components #12468

tobrun · 2018-07-24T14:00:21Z

Follow up from #12449 and #12391.

This PR makes the following components configurable:

logger
http client
telemetry

tobrun · 2018-07-25T08:04:10Z

To satisfy semver, I need to refactor telemetry module into an abstract class and add static methods

LukasPaczos

Great work so far!

LukasPaczos · 2018-07-25T08:40:20Z

platform/android/MapboxGLAndroidModuleTelemetry/build.gradle

+apply plugin: 'com.android.library'
+
+android {
+    compileSdkVersion 28


Can we use dependencies.gradle references here?

LukasPaczos · 2018-07-25T08:40:48Z

...GLAndroidModuleBase/src/main/java/com/mapbox/mapboxsdk/utils/MapboxConfigurationWrapper.java

+    if (context == null) {
+      throw new IllegalStateException("MapboxConfigurationWrapper isn't initialised correctly.");
+    }
+    return instance.getBaseContext();


NIT: this can return the context variable.

LukasPaczos · 2018-07-25T08:56:14Z

...GLAndroidModuleBase/src/main/java/com/mapbox/mapboxsdk/utils/MapboxConfigurationWrapper.java

+  }
+
+  public static String getAccessToken() {
+    if (accessToken == null) {


This check and the exception that is thrown removes the possibility of passing a null access token.

there are two places in code where this is referenced:

Telemetry.java -> no issue there

Mapbox.java -> indeed an issue
added try/catch and return null in those cases.

Had to fixup Mapbox.java for unit tests, will look for a solution without the try catch

LukasPaczos · 2018-07-25T08:59:18Z

...oid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AttributionDialogManager.java

@@ -97,7 +98,7 @@ private void showTelemetryDialog() {
    builder.setPositiveButton(R.string.mapbox_attributionTelemetryPositive, new DialogInterface.OnClickListener() {
      @Override
      public void onClick(DialogInterface dialog, int which) {
-        Telemetry.enableOnUserRequest();
+        Mapbox.getTelemetry().enableOnUserRequest();


This will throw an NPE if the access token is null.

LukasPaczos · 2018-07-25T08:59:25Z

...oid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/AttributionDialogManager.java

@@ -111,7 +112,7 @@ public void onClick(DialogInterface dialog, int which) {
    builder.setNegativeButton(R.string.mapbox_attributionTelemetryNegative, new DialogInterface.OnClickListener() {
      @Override
      public void onClick(DialogInterface dialog, int which) {
-        Telemetry.disableOnUserRequest();
+        Mapbox.getTelemetry().disableOnUserRequest();


This will throw an NPE if the access token is null.

LukasPaczos · 2018-07-25T09:01:43Z

...m/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/Attribution.java

@@ -4,7 +4,7 @@

  private static final String OPENSTREETMAP = "OpenStreetMap";
  private static final String OPENSTREETMAP_ABBR = "OSM";
-  static final String TELEMETRY = "Telemetry Settings";
+  static final String TELEMETRY = "TelemetryBase Settings";


I think this is an unnecessary result of the class name change?

refactor issue, nice catch

LukasPaczos · 2018-07-25T09:02:47Z

...GLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/exceptions/MapboxConfigurationException.java

@@ -17,7 +17,7 @@
   * Creates a Mapbox configuration exception thrown by MapboxMap when the SDK hasn't been properly initialised.
   */
  public MapboxConfigurationException() {
-    super("\nUsing MapView requires calling Mapbox.getInstance(Context context, String accessToken) before "
+    super("\nUsing MapView requires calling Mapbox.initialize(Context context, String accessToken) before "


This should still point to Mapbox.getInstance.

refactor issue, nice catch

LukasPaczos · 2018-07-25T09:12:36Z

.../MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java

@@ -4,7 +4,7 @@
 import android.os.StrictMode;
 import android.text.TextUtils;
 import com.mapbox.mapboxsdk.Mapbox;
-import com.mapbox.mapboxsdk.maps.Telemetry;
+import com.mapbox.mapboxsdk.telemetry.Telemetry;


As mentioned in #12468 (comment), this and all other Telemetry.java changes are semver major.

LukasPaczos · 2018-07-25T09:20:48Z

When it comes to logging, I feel like tagging all our logs with a unified tag, like Mapbox, would be easier to browse and search for us and users. Also, we need to add a custom, Timber logger definition to the test app to leverage Timber.

LukasPaczos · 2018-07-25T09:22:59Z

...droid/MapboxGLAndroidModuleHttp/src/main/java/com/mapbox/mapboxsdk/http/HttpRequestImpl.java

+
+  static void log(int type, String errorMessage) {
+    if (logEnabled) {
+      Timber.log(type, errorMessage);


Can we rely on the Logger here instead of Timber? A user might want to use our http module with a different LoggerDefinition.

yes, timber also removed from build.gradle

zugaldia · 2018-07-25T13:26:56Z

When it comes to logging, I feel like tagging all our logs with a unified tag, like Mapbox, would be easier to browse and search for us and users.

A compromise between granularity and consistency could be to have all tags start with the same Mapbox-specific prefix (say Mbx).

tobrun · 2018-07-25T15:17:35Z

When it comes to logging, I feel like tagging all our logs with a unified tag, like Mapbox, would be easier to browse and search for us and users.

A compromise between granularity and consistency could be to have all tags start with the same Mapbox-specific prefix (say Mbx).

Yes, this is already in place with https://github.com/mapbox/mapbox-gl-native/pull/12468/files#diff-8f41632c292f442353443b7ff4520ca1R16

@zugaldia @LukasPaczos, before re-reviewing, I will be working on removing the different modules. This is not a hard requirement.

zugaldia

Small request to move all new implementations into the same package. Otherwise this is looking great, lots of moving pieces well taken care of.

zugaldia · 2018-07-25T19:50:01Z

...form/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequestImpl.java

@@ -0,0 +1,196 @@
+package com.mapbox.mapboxsdk.http;


Could we move this file to a modules folder?

Possible yes, but to completely exclude okhttp, the user would also have to exclude com.mapbox.mapboxsdk.http.HttpRequestUtil which we can't move due to semver. I'm suggesting postponing changing package names until next major release.

zugaldia · 2018-07-25T19:52:45Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Telemetry.java

@@ -1,75 +1,157 @@
 package com.mapbox.mapboxsdk.maps;


Same as before, if this is the implementation, could we move this file to a modules folder?

This is not possible due to semver

Good point - could we move everything we can (new classes) and leave the rest for 7.x?

zugaldia · 2018-07-25T19:56:16Z

.../MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java

@@ -39,14 +41,15 @@ public void onCreate() {
  private boolean initializeLeakCanary() {
    if (LeakCanary.isInAnalyzerProcess(this)) {
      // This process is dedicated to LeakCanary for heap analysis.
-      // You should not init your app in this process.
+      // You should not init your app in this process.male


Bad copypasta?

great catch

LukasPaczos · 2018-07-26T12:15:59Z

Looks good to me!

I'm leaving the final word to @zugaldia.

tobrun · 2018-07-26T18:33:25Z

@LukasPaczos @zugaldia another iteration on this PR, would love another round of 👀

jaegs

This changeset is massive which makes it a bit hard to review. Perhaps it could be broken up into smaller pieces.

Also any chance using git mv of git cp will preserve some of the blame?

jaegs · 2018-08-14T17:09:34Z

...form/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequestUtil.java

   */
+  @Deprecated
  public static void setOkHttpClient(OkHttpClient client) {


this method will require okhttp to build

this is one of the files that would need to be excluded, due to semver we can't change this

jaegs · 2018-08-14T17:10:34Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http/HttpRequest.java

+   * @param etag        http header, identifier for a specific version of a resource
+   * @param modified    http header, used to determine if a resource hasn't been modified since
+   */
+  public abstract void executeRequest(HttpRequestResponder httpRequest, long nativePtr, String resourceUrl,


"Favor composition over inheritance" is one of the rules from Effective Java

https://stackoverflow.com/questions/2399544/difference-between-inheritance-and-composition

composition is provided in the module file here. The HttpRequest class is the contract between the SDK and the alternate implementation. How would you enforce the availability of predefined methods, the sdk needs a predefined hook, either an interface or an abstract class?

An interface is generally preferable since it doesn't lock the API user into single inheritance. I think an interface should work here unless I'm missing something.

I went ahead with an interface for HttpRequest, other functionality besides executeRequest and cancel was extracted in separate classes. Looks a lot cleaner now, thank you for the clarification.

jaegs · 2018-08-14T17:14:21Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/log/Logger.java

+
+    @Override
+    public void v(String tag, String msg) {
+      Log.v(String.format(TAG_TEMPLATE, tag), msg);


this will perform string regex even if logging is off.

wasn't aware of this, having this TAG_TEMPLATE formatting was only a nice to have, will remove it.

jaegs · 2018-08-14T17:18:04Z

...droid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/module/http/HttpRequestImpl.java

+      httpRequest.handleFailure(type, errorMessage);
+    }
+
+    private void logFailure(int type, String errorMessage, String requestUrl) {


why is this method specific to OKHttp?

move it up to the base class, though will apply composition to it so it will be found in a separate helper class.

jaegs · 2018-08-14T17:19:09Z

platform/android/src/logger.cpp

+    if(severity == EventSeverity::Debug){
+        auto method = _class.GetStaticMethod<Signature>(env, "d");
+        _class.Call(env, method, tag, message);
+    }else if(severity == EventSeverity::Info){


LukasPaczos

I understand the concerns from #12468 (comment) but couldn't we add the Mbx- prefix to every TAG that is currently being created in all of the classes instead?

With the current setup and the TelemetryDefinition being returned in the public API, TelemetryImpl#onAppUserTurnstileEvent and TelemetryImpl#onGestureInteraction are not package-restricted and can be possibly used to pollute our telemetry data.

LukasPaczos · 2018-08-22T09:15:17Z

...m/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/attribution/Attribution.java

@@ -4,7 +4,7 @@

  private static final String OPENSTREETMAP = "OpenStreetMap";
  private static final String OPENSTREETMAP_ABBR = "OSM";
-  static final String TELEMETRY = "Telemetry Settings";
+  static final String TELEMETRY = "TelemetryImpl Settings";


Refactor artifact?

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/ModuleProviderImpl.java

+   * Create a new concrete implementation of HttpRequest.
+   *
+   * @return a new instance of an HttpRequest
+   */


platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/ModuleProviderImpl.java

+   * Get the concrete implementation of TelemetryDefinition
+   *
+   * @return a single instance of Telemetry
+   */


LukasPaczos · 2018-08-22T09:20:16Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/ModuleProvider.java

+  HttpRequest createHttpRequest();
+
+  @Nullable
+  TelemetryDefinition obtianTelemetry();


TYPO: obtainTelemetry

LukasPaczos · 2018-08-22T09:21:32Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/ModuleProviderImpl.java

+ */
+public class ModuleProviderImpl implements ModuleProvider {
+
+  /**


How about moving this javadoc to the interface definition?

NIT: since the javadoc in the interface is identical, we can remove this one.

LukasPaczos · 2018-08-22T10:02:59Z

...id/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/module/telemetry/TelemetryImpl.java

+      Field field = interval.getClass().getDeclaredField("interval");
+      field.setAccessible(true);
+      Integer intervalValue = (Integer) field.get(interval);
+      return getInstance().setSessionIdRotationInterval(intervalValue);


This method should get the TelemetryImpl reference using the Mapbox#getTelemetry and check for nullability to avoid crashing.

LukasPaczos · 2018-08-22T10:03:06Z

...id/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/module/telemetry/TelemetryImpl.java

+   */
+  @Deprecated
+  public static void enableOnUserRequest() {
+    getInstance().setUserTelemetryRequestState(true);


This method should get the TelemetryImpl reference using the Mapbox#getTelemetry and check for nullability to avoid crashing.

LukasPaczos · 2018-08-22T10:03:12Z

...id/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/module/telemetry/TelemetryImpl.java

+   */
+  @Deprecated
+  public static void disableOnUserRequest() {
+    getInstance().setUserTelemetryRequestState(false);


This method should get the TelemetryImpl reference using the Mapbox#getTelemetry and check for nullability to avoid crashing.

LukasPaczos · 2018-08-22T10:06:33Z

.../MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/MapboxApplication.java

@@ -70,7 +73,10 @@ private void initializeMapbox() {
    String accessToken = TokenUtils.getMapboxAccessToken(getApplicationContext());
    validateAccessToken(accessToken);
    Mapbox.getInstance(getApplicationContext(), accessToken);
-    Telemetry.updateDebugLoggingEnabled(true);
+    TelemetryDefinition telemetry = Mapbox.getTelemetry();
+    if (telemetry != null) {


What do you think about removing the null-check here, or crashing the app with a clearer exception, to catch start-up telemetry regressions during internal testing?

added:

if (telemetry == null) { throw new IllegalStateException("Telemetry was unavailable during test application start."); }

LukasPaczos · 2018-08-22T10:10:43Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Telemetry.java

-    }
-  }
-
-  public static void initialize() {


This might not be impactful but breaks the semver. How about leaving an empty impl instead?

great catch!

LukasPaczos · 2018-08-22T10:52:46Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/ModuleProviderImpl.java

+ */
+public class ModuleProviderImpl implements ModuleProvider {
+
+  /**


NIT: since the javadoc in the interface is identical, we can remove this one.

LukasPaczos · 2018-08-22T10:53:00Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/ModuleProviderImpl.java

+    return new HttpRequestImpl();
+  }
+
+  /**


NIT: since the javadoc in the interface is identical, we can remove this one.

LukasPaczos

LGTM! 🚀

zugaldia

📦

tobrun added the Android Mapbox Maps SDK for Android label Jul 24, 2018

tobrun added this to the android-v6.4.0 milestone Jul 24, 2018

tobrun self-assigned this Jul 24, 2018

tobrun requested review from zugaldia and LukasPaczos July 24, 2018 14:00

tobrun force-pushed the tvn-modules branch 2 times, most recently from d57b421 to d9a24e1 Compare July 24, 2018 14:14

This was referenced Jul 24, 2018

HttpClient configuration with modules #12449

Closed

Make SDK logging abstract and configurable #12391

Closed

LukasPaczos suggested changes Jul 25, 2018

View reviewed changes

LukasPaczos reviewed Jul 25, 2018

View reviewed changes

tobrun force-pushed the tvn-modules branch 2 times, most recently from 5e8cec2 to 9606d09 Compare July 25, 2018 11:14

tobrun changed the title ~~Introduce modular components~~ Introduce abstract components Jul 25, 2018

tobrun force-pushed the tvn-modules branch from 9606d09 to 854ffc2 Compare July 25, 2018 16:22

zugaldia suggested changes Jul 25, 2018

View reviewed changes

tobrun force-pushed the tvn-modules branch from 854ffc2 to 7b82817 Compare July 26, 2018 09:55

tobrun force-pushed the tvn-modules branch 4 times, most recently from e13c993 to 4a182a7 Compare July 26, 2018 18:32

tobrun force-pushed the tvn-modules branch from 4a182a7 to b73d3ca Compare July 27, 2018 13:20

tobrun mentioned this pull request Aug 13, 2018

Remove Timber as library dependency #12399

Closed

jaegs reviewed Aug 14, 2018

View reviewed changes

tobrun force-pushed the tvn-modules branch 2 times, most recently from 802e0bf to d0fbde0 Compare August 15, 2018 10:46

LukasPaczos removed this from the android-v6.4.0 milestone Aug 15, 2018

tobrun force-pushed the tvn-modules branch 2 times, most recently from e2c0fd8 to b540403 Compare August 22, 2018 09:17

LukasPaczos reviewed Aug 22, 2018

View reviewed changes

tobrun force-pushed the tvn-modules branch 2 times, most recently from 5fe8f97 to 123d2b4 Compare August 22, 2018 10:55

LukasPaczos reviewed Aug 22, 2018

View reviewed changes

tobrun force-pushed the tvn-modules branch 3 times, most recently from 4d4d8bd to 1a4697a Compare August 22, 2018 12:20

LukasPaczos approved these changes Aug 23, 2018

View reviewed changes

[android] - modularise the sdk

367c53a

tobrun force-pushed the tvn-modules branch from 1a4697a to 367c53a Compare August 24, 2018 09:27

zugaldia approved these changes Aug 24, 2018

View reviewed changes

tobrun merged commit d297bf1 into master Aug 24, 2018

tobrun deleted the tvn-modules branch August 24, 2018 16:40

This was referenced Sep 19, 2018

[android] Debug flag indicating we want to see all the logs vs errors only #9681

Closed

Integrate plugable logger #12190

Closed

Integrate plugable http client #12191

Closed

zugaldia mentioned this pull request Nov 21, 2018

[android] Consolidate abstract components files under modules folder #13432

Closed

Introduce abstract components #12468

Introduce abstract components #12468

Conversation

tobrun commented Jul 24, 2018 • edited Loading

tobrun commented Jul 25, 2018

LukasPaczos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

LukasPaczos commented Jul 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zugaldia commented Jul 25, 2018

tobrun commented Jul 25, 2018

zugaldia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos commented Jul 26, 2018

tobrun commented Jul 26, 2018

jaegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

zugaldia left a comment

Choose a reason for hiding this comment

tobrun commented Jul 24, 2018 •

edited

Loading

LukasPaczos Jul 25, 2018 •

edited

Loading

LukasPaczos Aug 22, 2018 •

edited

Loading