Style.java #13484

tobrun · 2018-11-30T14:49:36Z

This PR introduces the concept of Style.java to mimic style.hpp from core. All style related methods are moved to the style class vs mapboxmap. OnMapReady will now be invoked when the map surface is created. Styles are composed together using a builder class and allow for a more flexible build up of your style.

For example the following code snippet allows you to only define a background and symbol layer with a geojson source (this without loading another style as mapbox-streets).

mapboxMap.setStyle(new Style.Builder()
         .withLayer(
           new BackgroundLayer(“bg”)
             .withProperties(
               backgroundColor(rgb(120, 161, 226))
             )
         )
         .withLayer(
           new SymbolLayer(“symbols-layer”, “symbols-source”)
             .withProperties(
               iconImage(“test-icon”)
             )
         )
         .withSource(
           new GeoJsonSource(“symbols-source”, testPoints)
         )
         .withImage(“test-icon”, markerImage)
       );

Generates the following output:

Todo:

add javadoc style.java
add sdf support to Style.Builder#withImage()
add below/above layer support to Style.Builder
fix LocationComponentTest
decouple style from NativeMapView

Tailwork after merge:

LukasPaczos

This looks great, thanks a lot for the huge lift @tobrun!
A couple of notes:

It'd be great to check the code for missing nullability annotations.
Can we guard all of the add/remove methods on the Style object from being called before the style has been loaded? We can go as far as throwing an exception. Exposing the Style#styleLoaded flag could be useful as well.
Since we are coupling the concept of adding images via the Style, would it make sense to remove all of the images added with this style during the cleanup? Or should we rather separate those concepts?
Assuming there is no other way of handling the detached state besides the flag, would there be a way of keeping this flag in the core, and having it set on all of the sources within the style during the cleanup? This would guard all of the sources/layers, not only those added in the runtime.

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

LukasPaczos · 2018-12-03T12:51:06Z

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

+    }
+
+    // TODO add SDF support!
+    public Builder withImage(String name, Bitmap image) {


Now that I think about it, it'd be great to expose a Drawable overload of this method and generate a bitmap out of it just before adding it to the map to possibly minimize the time we are keeping bitmaps in the Java memory.

Let's ticket this as follow up work once the base version of this has landed

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

LukasPaczos · 2018-12-03T13:12:37Z

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

@@ -1139,7 +1062,6 @@ public void cancelAllVelocityAnimations() {
    OnCameraIsChangingListener, OnCameraDidChangeListener {

    private final List<OnMapReadyCallback> onMapReadyCallbackList = new ArrayList<>();
-    private boolean initialLoad = true;

    MapCallback() {


I believe MapCallback can be reworked to use a provided MapboxMap object and avoid null checks.

I wasn't able to workaround the nullability, MapCallback needs to be created when MapView is created and at that point MapboxMap isn't created yet. This is needed to hook up the onMapReadyCallbackList. Only way forward afaik is to decouple MapCallback into a couple of concrete classes that have different lifespan/scopes.

I don't feel this is worth picking up now but we should ticket this out and pick up in near future. This is an internal refactor that won't impact the public API.

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

LukasPaczos · 2018-12-03T13:39:56Z

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

@@ -1337,6 +1345,11 @@ public void run() {
    });
  }

+  // TODO remove dependency of Style on NativeMapView


I'm in favor of this. Maybe we can move it to the MapboxMap?

This must addressed before merging this PR! Still need to look into this.

LukasPaczos · 2018-12-03T14:49:17Z

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

+     * @param image the image to be added
+     * @return this
+     */
+    public Builder withImage(String id, Bitmap image) {


We are missing withImages method.

Atm we also don't support withLayers or withSources either but could make sense.

Maybe postponing this into a follow up ticket as well? trying to cut some corners to merge this before our branch cut.

Sounds good.

tobrun · 2018-12-04T16:13:06Z

t'd be great to check the code for missing nullability annotations.

✅

Can we guard all of the add/remove methods on the Style object from being called before the style has been loaded? We can go as far as throwing an exception. Exposing the Style#styleLoaded flag could be useful as well.

I went with a different approach, style remains null until it has fully loaded (see #13484 (comment)).

Since we are coupling the concept of adding images via the Style, would it make sense to remove all of the images added with this style during the cleanup? Or should we rather separate those concepts?

This would be indeed a good idea, this would also allow us to call recycle on the bitmaps passed to the builder.

Assuming there is no other way of handling the detached state besides the flag, would there be a way of keeping this flag in the core, and having it set on all of the sources within the style during the cleanup? This would guard all of the sources/layers, not only those added in the runtime.

Yes, 💯 this needs to be added to the jni layer and not java.

tobrun · 2018-12-04T16:47:58Z

With 134a8bb4b4281020486b3ac5b7c98dae7931bb1a I'm removing the dependency of Style.java on NativeMapView (see #13484 (comment)). This however also results in adding MapView as dependency on MapboxMap, which I'm not happy about either. MapboxMap can't be used as it doesn't have access to the required listeners..

Alternative might be to introduce a new interface that is used in NativeMapView to at least decouple Style.java from NativeMapView but I'm torn...

tobrun · 2018-12-05T14:26:31Z

Revisited #13484 (comment), with ac6f912

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

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

.../MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationLayerController.java

platform/android/MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/maps/StyleTest.kt

...estApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/utils/OnMapReadyIdlingResource.java

...ndroid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponent.java

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

…sition to Style.java

...ndroid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponent.java

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

…gurations to style.java

…ts for non existing style and multiple style loading, revisit code comments

…llyLoaded

…ntimeStyleTest when removing layer at, add javadoc to isFullyLoaded

…ll invocations, add test

LukasPaczos

I've looked through all of the demo activities and it seems like most of them could use a local Style field to avoid all of the null warnings from the IDE. This is not a high priority and can be tackled separately.

What I also noticed, is that setting a style via MapboxMapOptions is not really useful anymore as we are not restoring the style anyway. Can we drop that option as a part of this PR?

Otherwise, it looks great!

...estApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/imagegenerator/PrintActivity.java

.../src/main/java/com/mapbox/mapboxsdk/testapp/activity/location/LocationMapChangeActivity.java

...tApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/location/LocationModesActivity.java

...estApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/maplayout/LocalGlyphActivity.java

LukasPaczos

I think we're done here, thanks for the awesome work @tobrun! 🎉

tobrun added the Android Mapbox Maps SDK for Android label Nov 30, 2018

tobrun added this to the android-v7.0.0-iowaska milestone Nov 30, 2018

tobrun self-assigned this Nov 30, 2018

tobrun force-pushed the tvn-style-java branch from be10a00 to cc338db Compare November 30, 2018 14:55

tobrun requested a review from LukasPaczos November 30, 2018 15:13

tobrun force-pushed the tvn-style-java branch from 3dac5d4 to 041ad82 Compare December 3, 2018 14:23

LukasPaczos reviewed Dec 3, 2018

View reviewed changes

tobrun force-pushed the tvn-style-java branch 2 times, most recently from 1159877 to ac6f912 Compare December 5, 2018 13:21

tobrun force-pushed the tvn-style-java branch 2 times, most recently from c6e386f to ab15eba Compare December 6, 2018 16:01

This was referenced Dec 6, 2018

Style.Builder#withImage with Drawable #13516

Closed

Style.Builder#with{plural} #13517

Closed

Style.java mapcallback refactor #13518

Closed

tobrun force-pushed the tvn-style-java branch from a8987d7 to b8dc3ed Compare December 6, 2018 17:32

tobrun added the beta blocker Blocks the next beta release label Dec 7, 2018

LukasPaczos reviewed Dec 7, 2018

View reviewed changes

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/maps/Style.java Show resolved Hide resolved

tobrun force-pushed the tvn-style-java branch from 9f254c5 to bdbac82 Compare December 10, 2018 10:03

tobrun added 2 commits December 10, 2018 11:48

[android] - Style.java

bf8b886

[android] - cleanup old style java constants file, migrate style tran…

a42eac3

…sition to Style.java

LukasPaczos reviewed Dec 10, 2018

View reviewed changes

tobrun added 4 commits December 10, 2018 11:48

[android] - move all layer, source, image, light and transition confi…

d1a593f

…gurations to style.java

[android] - remove style loaded listener

799709a

[android] - expose style builder

3a66f4e

[android] - move all style methods to Style.java

f2b678c

tobrun and others added 17 commits December 10, 2018 11:49

[android] - fixup location component test to use initialize components

8ec664e

[android] - update CircleLayer to be compatible

1d21852

[android] - infer nullify on Style.java

c53cfca

[android] - limit visibility, clear callbacks

6f1baf2

[android] - introduce StyleCallback, make parent class for StateCallback

efa5ace

[android] - cleanup images

985283e

[android] move style callback invocation to MapboxMap

e6bb308

[android] - use builder json/url for constructing style object

b54775a

[android] - un-ignore location tests

99dc208

[android] fix LocationComponent unit test with Style.java object

d9a93e6

[android] - add missing javadoc on public API methods around Style

f4aea7a

[android] - add unit tests for Style.java API

209e103

[android] - update sanity test to Style.java

11062cc

[android] - update unit test to match NativeMapView#isDestroyed

85b79ce

[android] - make style part of location component activation, add tes…

db8cb59

…ts for non existing style and multiple style loading, revisit code comments

[android] - optimise loaded style for location component, expose isFu…

a85263b

…llyLoaded

[android] - throw exception when another style is loading, correct Ru…

9535604

…ntimeStyleTest when removing layer at, add javadoc to isFullyLoaded

tobrun force-pushed the tvn-style-java branch from bdbac82 to 9535604 Compare December 10, 2018 11:03

[android] - introduce validateState, add illegal state exception to a…

c01b259

…ll invocations, add test

LukasPaczos reviewed Dec 10, 2018

View reviewed changes

[android] - remove style config from mapboxMapOptions and attributes

3578d84

LukasPaczos approved these changes Dec 10, 2018

View reviewed changes

tobrun merged commit cbcd087 into master Dec 10, 2018

tobrun deleted the tvn-style-java branch December 10, 2018 15:59

This was referenced Dec 10, 2018

Style.java #9338

Closed

Decouple style loading from OnMapReady #12219

Closed

Cherry pick style.java, return boolean from removeSource/Layer #13533

Merged

LukasPaczos mentioned this pull request Jan 2, 2019

Harden detached sources/layers #10947

Closed

tobrun mentioned this pull request Mar 6, 2020

Move detached layer state handling to jni mapbox/mapbox-gl-native-android#252

Open

LukasPaczos mentioned this pull request May 24, 2021

Cache style layers and sources mapbox/mapbox-maps-android#364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style.java #13484

Style.java #13484

tobrun commented Nov 30, 2018 •

edited

Loading

LukasPaczos left a comment

LukasPaczos Dec 3, 2018

tobrun Dec 4, 2018 •

edited

Loading

LukasPaczos Dec 3, 2018

tobrun Dec 4, 2018 •

edited

Loading

tobrun Dec 4, 2018

LukasPaczos Dec 3, 2018

tobrun Dec 4, 2018

LukasPaczos Dec 3, 2018

tobrun Dec 4, 2018

tobrun Dec 4, 2018

LukasPaczos Dec 4, 2018

tobrun commented Dec 4, 2018

tobrun commented Dec 4, 2018

tobrun commented Dec 5, 2018

LukasPaczos left a comment

LukasPaczos left a comment

Style.java #13484

Style.java #13484

Conversation

tobrun commented Nov 30, 2018 • edited Loading

LukasPaczos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun Dec 4, 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

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

tobrun commented Dec 4, 2018

tobrun commented Dec 4, 2018

tobrun commented Dec 5, 2018

LukasPaczos left a comment

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

tobrun commented Nov 30, 2018 •

edited

Loading

tobrun Dec 4, 2018 •

edited

Loading

tobrun Dec 4, 2018 •

edited

Loading