This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
Android - Runtime styling Api #5610
Labels
Android
Mapbox Maps SDK for Android
Milestone
Comments
|
/cc @mapbox/mobile |
|
Ok, so we have a decision to make as to the level of typing we want in this api. The core api is beautifully typed, which is great to work with. For the android side of things there are some considerations though:
The other end of the spectrum is a non-typed api (or very basic at least). Passing mostly Strings and values around. This is how the QT api is shaping up, and it's nice and small (see for example setLayoutProperty()). As the type checking is done on the core side anyways, there is not much lost here, except for code completion / compile time checking of correct property types. A nice compromise might be, to offer some factory methods to construct properties (as we still want to avoid Enums). This would aid in compiler assistance of type checking per property. This can easily be generated. For example: |
Right. Do we foresee that the style API is something that devs would call so often that we'd notice the performance impact?
I personally like this approach, not only for consistency with Qt and because, as you say, checks are gonna happen anyway at the core level, but also because 1) it's easier to implement (and explain) and 2) it leaves the door open to a higher level API built on top of this one (e.g. factory methods) in the future should customers require it. |
Another plus of this approach is as the spec changes, you get new properties, types, etc for free. |
ivovandongen
added a commit
that referenced
this issue
Jul 8, 2016
|
I've added an example of the api we could use if we type the properties (Only layout properties done for this example). See the commit for details. So, the main entry point for the user would look like:
Where the LayoutProperty can be retrieved from LayoutPropertyFactory. Both are generated with ejs templates. Advantages:
Disadvantages:
|
Could animating properties from Android animators be a valid use case? If so, then yes. |
|
Quick comments:
|
|
@jfirebaugh Thanks for the comments. I'll work that in. About the JNI part; we've been limiting the amount of callbacks into the runtime lately (see #5103). But since we don't expose that part of the api, we can go either way depending on any performance issues. |
|
For continuity, the iOS / mac OS Styling API is simultaneously being worked on in #5626. |
ivovandongen
added a commit
that referenced
this issue
Jul 11, 2016
ivovandongen
added a commit
that referenced
this issue
Jul 11, 2016
ivovandongen
added a commit
that referenced
this issue
Jul 11, 2016
ivovandongen
added a commit
that referenced
this issue
Jul 12, 2016
ivovandongen
added a commit
that referenced
this issue
Jul 12, 2016
Closed
|
With the excellent guidance from @jfirebaugh I've added peer classes on the java and c++ side that keep state in sync on both sides using the high-level wrappers. This is important as we want to be able to create new layers from Java (in the addLayer() case) as well as from c++ (for the getLayer() case. Atm there is no type hierarchy yet on either side, that's the next step. One issue atm is that the art Finalizer runs on a different thread than the thread on which we first created the c++ Layer object. This leads to exceptions when the garbage collector runs since a reference is being held to the JNIEnv to delete global references: |
ivovandongen
added a commit
that referenced
this issue
Aug 1, 2016
ivovandongen
added a commit
that referenced
this issue
Aug 1, 2016
ivovandongen
added a commit
that referenced
this issue
Aug 1, 2016
|
Finished implementation and unit tests. Just up to two remaining issues: Random crash on addLayer Crash when manipulation the style in the |
|
First issue was resolved by rebasing master. Second issue still remains. To be more specific, the crash happens when adding a Layer in the onMapReady callback. I've added a test case here to reproduce the issue (thanks @cammace) |
ivovandongen
added a commit
that referenced
this issue
Aug 2, 2016
|
Merged into master |
|
@ivovandongen Assuming the bug mentioned in #6014 is real (in which case we'll need to ticket it separately), otherwise this feature is fully built out, right? Can we close this ticket? |
|
Thought it was closed already. #5852 was the last missing piece. |
ghost
mentioned this issue
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Now that the Runtime styling Api is taking shape in core, we can get started on the android side of things.
Major parts:
It's been suggested to generate the Java bindings from the style specification. We can use the same approach as taken for the node bindings, using ejs templates or go for a more Java central approach by using a gradle plugin that transforms json into java classes, like jsonschema2pojo. Depending on integration in the main build or the Android sub-build (gradle).
Because the types (and manipulation of the types) need to go through jni, it's best to make the api flat (no 1-n relations). All manipulation/querying goes through the
MapboxMapobject. For some of the types we can use subclasses or type IntDef's if the types have little specialisation.Possible api
Layer Types (generated):
BackgroundLayerCircleLayerFillLayerLineLayerRasterLayerSymbolLayerRelated types:
LayoutProperty- Each layer supports a set of layout propertiesPaintProperty- Each layer supports a set of paint propertiesFilter- Each layer can have a filterLayer methods on
MapboxMap:setFilter()addLayer()removeLayer()setPaintProperty()Source types:
GeoJsonSourceRasterSourceVectorSourceSource methods on
MapboxMapaddSource()removeSource()The text was updated successfully, but these errors were encountered: