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

[android] Adding builder pattern for LocationComponent activation #13941

Merged

Conversation

langsmith
Copy link
Contributor

This pr adds a builder to activate the LocationComponent and resolves #13931.

@langsmith langsmith added refactor Android Mapbox Maps SDK for Android in progress labels Feb 18, 2019
@langsmith langsmith self-assigned this Feb 18, 2019
@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch 2 times, most recently from 8b6931e to 6ae496d Compare February 18, 2019 23:16
@langsmith
Copy link
Contributor Author

Would love 👁👁 from @mapbox/maps-android

I'm not sure the best way to handle all of the nasty logic inside of activateLocationComponent(). Or should I be managing some of this upstream in LocationComponentActivationOptions and/or the builder?

I'm definitely checking locationComponentActivationOptions.styleRes() incorrectly at the moment. I noticed that if no style resource is passed through to the LocationComponentActivationOptions builder, it's 0 as default.

At least there are promising signs of (some) things working in the test app 👇

ezgif com-resize 1

@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch 2 times, most recently from 7e2a513 to d378b2a Compare February 19, 2019 19:22
@langsmith
Copy link
Contributor Author

Ok @tobrun and @LukasPaczos . Changes have been made based on your feedback.

LocationComponentOptions.createFromAttributes() seems to be returning a null LocationComponentOptions here and here. I've got 0️⃣ clue why 🤔. Thoughts? Context isn't null in either of those two spots, so that's not the culprit. Do I have to do more than just pass in R.style.mapbox_LocationComponent or locationComponentActivationOptions.styleRes()?

@LukasPaczos
Copy link
Member

LocationComponentOptions.createFromAttributes() seems to be returning a null ...

The NPE you are seeing is thrown in here because you haven't initialized the pieces yet and the locationLayerController is null. A strategy for initialization via the builder should always be to fetch the context and style, then determine styling options and call initialize with those in place. Afterward, set the engine request and the engine itself.

@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch from cd093d6 to 5f6f986 Compare February 21, 2019 20:07
@langsmith
Copy link
Contributor Author

@LukasPaczos and @tobrun , refactoring based on your latest review is complete. I've also:

  • adjusted all of the other location package test app examples to use the LocationComponentActivationOptions builder.

  • refactored all of the Espresso tests in LocationComponentTest , so that they use the LocationComponentActivationOptions

@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch 4 times, most recently from 2226db8 to 36e1b80 Compare February 22, 2019 01:08
@langsmith
Copy link
Contributor Author

crashes are Firebase instrumentation around LocationEngine logic 🤔

@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch from c6a233a to ec83e2a Compare February 22, 2019 07:53
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

The activation looks overly complicated to me, here's my proposition, let me know what you think:

  /**
   * This method initializes the component and needs to be called before any other operations are performed.
   * Afterwards, you can manage component's visibility by {@link #setLocationComponentEnabled(boolean)}.
   *
   * @param activationOptions a fully built {@link LocationComponentActivationOptions} object
   */
  public void activateLocationComponent(@NonNull LocationComponentActivationOptions
                                          activationOptions) {
    LocationComponentOptions options = activationOptions.locationComponentOptions();
    if (options == null) {
      int styleRes = activationOptions.styleRes();
      if (styleRes == 0) {
        styleRes = R.style.mapbox_LocationComponent;
      }
      options = LocationComponentOptions.createFromAttributes(activationOptions.context(), styleRes);
    }

    // Initialize the LocationComponent with Context, the map's `Style`, and either custom LocationComponentOptions
    // or backup options created from default/custom attributes
    initialize(activationOptions.context(), activationOptions.style(), options);

    // Apply the LocationComponent styling
    // TODO avoid doubling style initialization
    applyStyle(options);

    // Set the LocationEngine request if one was given to LocationComponentActivationOptions
    LocationEngineRequest locationEngineRequest = activationOptions.locationEngineRequest();
    if (locationEngineRequest != null) {
      setLocationEngineRequest(locationEngineRequest);
    }

    // Set the LocationEngine if one was given to LocationComponentActivationOptions
    LocationEngine locationEngine = activationOptions.locationEngine();
    if (locationEngine != null) {
      setLocationEngine(locationEngine);
    } else if (activationOptions.useDefaultLocationEngine()) {
      initializeLocationEngine(activationOptions.context());
    } else {
      setLocationEngine(null);
    }
  }

@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch 4 times, most recently from c41d10f to 48d01ff Compare February 22, 2019 20:20
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Good progress @langsmith!
There are still docs missing in the Builder class. It'd be great to add tests verifying if there's an exception thrown when the provided style is not fully loaded and if the useDefaultLocationEngine flag is set to true by default.

All instrumentation tests are finally passing. As we discussed offline, some of the instrumentation tests were passing in null for the LocationEngine, but I didn't use .locationEngine(null) when I refactored the tests to use the builder. My most recent commit was to add .locationEngine(null) to the tests that needed it.

Adding only .locationEngine(null) is still initializing the default engine, you'll need to add .useDefaultLocationEngine(false) instead, otherwise, the tests might become flaky.

@langsmith
Copy link
Contributor Author

Ok @LukasPaczos , all feedback from your last review has been addressed.

It'd be great to add tests verifying if there's an exception thrown when the provided style is not fully loaded and if the useDefaultLocationEngine flag is set to true by default.

21bf960#diff-14c9705b4a36f28b8485da302a5703c1

Adding only .locationEngine(null) is still initializing the default engine, you'll need to add .useDefaultLocationEngine(false) instead, otherwise, the tests might become flaky.

21bf960#diff-722a0b240a9c5f84fadee9bb40f558d5

@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch 4 times, most recently from 9009499 to 1cade17 Compare March 4, 2019 18:03
@langsmith langsmith force-pushed the ls-builder-pattern-for-android-LocationComponent-activation branch from ea4ffb4 to 00673b6 Compare March 5, 2019 17:47
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

:shipit:

@LukasPaczos LukasPaczos added this to the release-l milestone Mar 5, 2019
@langsmith langsmith merged commit 7e48249 into master Mar 5, 2019
@langsmith langsmith deleted the ls-builder-pattern-for-android-LocationComponent-activation branch March 5, 2019 19:05
@carstenhag
Copy link

Hey, just FYI: The deprecated notices (what to use instead) are not visible when just opening the file via Android Studio. So I'm only seeing @Deprecated, but not what to use instead, like this:

    /** @deprecated */
    @Deprecated
    @RequiresPermission(
        anyOf = {"android.permission.ACCESS_FINE_LOCATION", "android.permission.ACCESS_COARSE_LOCATION"}
    )
    public void activateLocationComponent(@NonNull Context context, @NonNull Style style, @StyleRes int styleRes) {
        this.activateLocationComponent(context, style, LocationComponentOptions.createFromAttributes(context, styleRes));
    }

Not sure how this is solved. I would probably need to download the sources, right? There must be a better way! :)

@tobrun
Copy link
Member

tobrun commented Mar 27, 2019

@carstenhag the javadoc with @deprecated does contain the alternative method to use. There is not much else we can do than follow standard deprecation setup. Please let us know if you find another way!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce builder pattern for LocationComponent activation
4 participants