[android] - transition options for layer properties #8509
Conversation
|
@tobrun, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh to be potential reviewers. |
tobrun
force-pushed
the
tvn-transitionoptions-property
branch
2 times, most recently
from
7181f73
to
22758cf
Compare
|
|
||
| mbgl::style::TransitionOptions options = layer.as<mbgl::style::FillLayer>()->FillLayer::getFillOpacityTransition(); | ||
| jlong durations[2]; | ||
| durations[0] = options.duration.value_or(mbgl::Duration::zero()).count(); |
There was a problem hiding this comment.
Could we just return the actual object in a typed manner. Don't think there is a noticeable overhead in creating an object instead of an array and filling it up and then creating the object from that jvm side.
| METHOD(&CircleLayer::getCircleStrokeWidth, "nativeGetCircleStrokeWidth"), | ||
|
|
There was a problem hiding this comment.
NIT. Could you remove the blank lines?
| jni::Object<jni::ObjectTag> FillLayer::getFillOpacity(jni::JNIEnv& env) { | ||
| using namespace mbgl::android::conversion; | ||
| Result<jni::jobject*> converted = convert<jni::jobject*>(env, layer.as<mbgl::style::FillLayer>()->FillLayer::getFillOpacity()); | ||
| return jni::Object<jni::ObjectTag>(*converted); | ||
| } | ||
|
|
||
| jni::Array<jni::jlong> FillLayer::getFillOpacityTransition(jni::JNIEnv& env) { |
There was a problem hiding this comment.
Same here. Let's just use the typed object
|
|
||
| mbgl::style::TransitionOptions options = layer.as<mbgl::style::FillLayer>()->FillLayer::getFillTranslateTransition(); | ||
| jlong durations[2]; | ||
| durations[0] = options.duration.value_or(mbgl::Duration::zero()).count(); |
There was a problem hiding this comment.
Can we use the actual unit instead of Duration so it's clear on first glance what we're talking about?
| } | ||
|
|
||
| void FillLayer::setFillTranslateTransition(jni::JNIEnv& env, jlong duration, jlong delay) { | ||
| (void) env; |
There was a problem hiding this comment.
Can you remove this line and instead remove the parameter name?
| @@ -40,84 +41,251 @@ namespace android { | |||
| return jni::Object<jni::ObjectTag>(*converted); | |||
| } | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Redundant blank lines
tobrun
force-pushed
the
tvn-transitionoptions-property
branch
3 times, most recently
from
7188b40
to
1a0b019
Compare
tobrun
removed
the
⚠️ DO NOT MERGE
| <% if (property.transition) { -%> | ||
| void set<%- camelize(property.name) %>Transition(jni::JNIEnv&, jlong duration, jlong delay); | ||
| jni::Object<TransitionOptions> get<%- camelize(property.name) %>Transition(jni::JNIEnv&); | ||
| <% } -%> |
There was a problem hiding this comment.
Very small nit; one empty line here would be just perfection
tobrun
force-pushed
the
tvn-transitionoptions-property
branch
from
1a0b019
to
ea0fb93
Compare
[android] fixup highlevel bindings for transition options finish integration
tobrun
force-pushed
the
tvn-transitionoptions-property
branch
from
ea0fb93
to
94a7146
Compare
Closes #8015
This PR adds TransitionOptions to the Android bindings for Layer Properties that support it
Todo to finish:
jni::Array<jni::jlong>return type withjni::Object<jni::ObjectTag>jni::Object<TransformOptions>cc @zugaldia @ericrwolfe