Expressions #10654

tobrun · 2017-12-07T15:11:26Z

This PR adds binding integration for expressions from #9439

Symbol layer example:

    mapboxMap.addLayer(new SymbolLayer(LAYER_ID, SOURCE_ID)
      .withProperties(

        // icon configuration
        iconImage(get(FEATURE_ID)),
        iconAllowOverlap(false),
        iconSize(
          division(get(FEATURE_RANK), 2)
        ),
        iconAnchor(ICON_ANCHOR_BOTTOM),

        // text field configuration
        textField(
          concat(
            upcase("a "),
            get(FEATURE_TYPE),
            downcase(" IN "),
            get(FEATURE_REGION)
          )
        ),
        textSize(
          product(get(FEATURE_RANK), pi())
        ),
        textAnchor(TEXT_ANCHOR_TOP)
      )
    );

Todo:

Closes #9803

cc @mapbox/android @anandthakker

ThibaudM · 2017-12-11T16:40:03Z

Could you also add a getFilter() accessor?
An implementation was proposed in #9077 but has been rejected.

1ec5 · 2017-12-12T02:15:53Z

#9077 was declined because this PR will obsolete the underlying implementation details of that PR, but #7978 continues to track implementing a getFilter() accessor for parity with the other platforms.

tobrun · 2017-12-12T19:53:17Z

Noting here that I would prefer doing expression filter support as tailwork. Would love to get some feedback on this PR first before moving forward on that. This PR is ready for review.

cc @ivovandongen @anandthakker

anandthakker · 2017-12-12T20:00:20Z

Noting here that I would prefer doing expression filter support as tailwork

👍, especially since expression-based filters are still to be done in core

tobrun · 2017-12-12T20:10:19Z

Ok, that explains a couple of things :) I initially thought I had to wire those up myself but now seeing that symbol_layer_impl.cpp etc is found in core.

anandthakker

Looks great!

anandthakker · 2017-12-12T20:10:42Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+public class Expression {
+
+  private final String operator;
+  private final Object[] values;


The word "value" gets so easily overloaded -- would it make sense to call this arguments, which is how we refer to them in the style spec as well?

Intuitively, I'd have expected this to be Expression[] rather than Object[]. I'm guessing it's Object[] so that string/number/boolean literals can be used directly?

Yes, that is correct, capturing from chat that I will look into the feasibility to use something as Expression instead.

anandthakker · 2017-12-13T13:40:59Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   */
+  public static Expression var(Object... expressions) {
+    return new Expression("var", expressions);
+  }


The variable name argument must be a string literal -- it can't be a subexpression -- so it might make sense to remove this overload.

can the string literal come from another expression?

eg.
var(get("featureKey"))

If that is the case I will remove the varargs approach and expose the following:

var(Expression expression)

var(String string)

Nope -- "literal" here means it can't be the output of an expression, but has to be a string directly.

anandthakker · 2017-12-13T13:41:33Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param stops      pair of input and output values
+   * @return expression
+   */
+  public static Expression step(Expression expression, Object... stops) {


Maybe rename expression to input for consistency with docs?

anandthakker · 2017-12-13T13:42:23Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param stops         pair of input and output values
+   * @return expression
+   */
+  public static Expression interpolate(Expression interpolation, Number number, Object... stops) {


Similar to step: maybe rename number to input?

anandthakker · 2017-12-13T13:45:58Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param stops         pair of input and output values
+   * @return expression
+   */
+  public static Expression interpolate(Expression interpolation, Expression number, Object... stops) {


I could see the case for creating a separate Interpolation type for the first argument here. (So linear(), exponential(), and cubicBezier() would return an Interpolation value.) Not sure whether it's actually better to do so, just mentioning it as an alternative.

One benefit of doing it that way is that users couldn't do something weird like sum(linear(), 2). As it stands now, if they did this, they'd get an error message that linear is an unknown expression operator -- which might be confusing to them, since the linear() method claims to produce an expression.

ivovandongen · 2017-12-13T15:19:10Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/layers/Layer.java

@@ -27,9 +28,17 @@ public void setProperties(@NonNull PropertyValue<?>... properties) {
    for (PropertyValue<?> property : properties) {
      Object converted = convertValue(property.value);
      if (property instanceof PaintPropertyValue) {
-        nativeSetPaintProperty(property.name, converted);
+        if(converted instanceof Expression) {


Could this be part of convertValue()? (And space between if and ()

wow, don't know how I could have missed that

tobrun · 2017-12-15T13:10:19Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   *
+   * @param <T>
+   */
+  private static class ExpressionValue<T> extends Expression<T> {


ExpressionLiteral might be a better name for this class.

ivovandongen · 2017-12-15T14:04:37Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+ * a combination of the zoom level and individual feature properties.
+ * </p>
+ */
+public class Expression<T> {


Document <T>

ivovandongen · 2017-12-15T14:04:56Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+ */
+public class Expression<T> {
+
+  private String operator;


Can both be final so that the object is immutable?

ivovandongen · 2017-12-15T14:05:17Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+  private String operator;
+  private Expression[] arguments;
+
+  Expression() {


Not used except by the subclass? If it initializes the members to null, the object can be immutable

ivovandongen · 2017-12-15T15:55:30Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   */
+  private static class ExpressionValue<T> extends Expression<T> {
+
+    private Object object;


Should this be <T> instead of Object?

yes this should be enforced

ivovandongen · 2017-12-15T15:56:54Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param expression an expression to convert to a color
+   * @return expression
+   */
+  public static Expression toRgba(Expression<Color> expression) {


Missing generic specifier (<Color>) on return type?

Correct about missing specifier, this case it should be <Array> shown in style spec with ["to-rgba", color]: array<number, 4>).

ivovandongen · 2017-12-15T15:57:53Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param input boolean input
+   * @return expression
+   */
+  public static Expression<Boolean> not(Boolean input) {


should this be boolean instead? Otherwise, we should indicate nullability restrictions

correct, all boolean input should primitive

also went through the api and applied @NonNull were needed

ivovandongen · 2017-12-15T16:00:06Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param expression an expression object or expression string
+   * @return expression
+   */
+  public static Expression<Number> length(Expression expression) {


Is it preferable to use Expression<?> over Expression here?

ivovandongen · 2017-12-15T16:02:36Z

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/expressions/Expression.java

+   * @param operator  the expression operator
+   * @param arguments expressions input
+   */
+  public Expression(@NonNull String operator, @Nullable Expression... arguments) {


Should we use @Size(min=1) on varargs here?

There are cases where the size may be 0, eg: ["pi"]: number or ["properties"]: object. I wasn't aware of this annotation, will try using it in other varargs occurrences in the class.

ivovandongen

Some commented code can be removed. Otherwise 👍

ivovandongen · 2017-12-18T17:28:50Z

...KTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/RuntimeStyleActivity.java

      )
-    ));
+    );


To be removed right?

ivovandongen · 2017-12-18T17:29:23Z

...stApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/style/SymbolGeneratorActivity.java

+    } else if (item.getItemId() == R.id.menu_action_filter) {
+      SymbolLayer layer = mapboxMap.getLayerAs(LAYER_ID);
+      layer.setFilter(Filter.eq(FEATURE_RANK, 1));
+      //layer.setFilter(eq(get(FEATURE_RANK), 1));


To be removed?

1ec5 · 2018-01-06T12:17:37Z

#9077 was declined because this PR will obsolete the underlying implementation details of that PR, but #7978 continues to track implementing a getFilter() accessor for parity with the other platforms.

Make that #10720.

tobrun added the Android Mapbox Maps SDK for Android label Dec 7, 2017

tobrun added this to the android-v6.0.0 milestone Dec 7, 2017

tobrun self-assigned this Dec 7, 2017

tobrun force-pushed the tvn-expressions-wip branch 4 times, most recently from 71c779e to ec425ff Compare December 12, 2017 19:48

tobrun requested review from ivovandongen and anandthakker December 12, 2017 19:53

tobrun force-pushed the tvn-expressions-wip branch from ec425ff to 98597e1 Compare December 12, 2017 20:02

anandthakker reviewed Dec 13, 2017

View reviewed changes

ivovandongen reviewed Dec 13, 2017

View reviewed changes

tobrun force-pushed the tvn-expressions-wip branch from 98597e1 to 49af916 Compare December 13, 2017 17:17

tobrun commented Dec 15, 2017

View reviewed changes

ivovandongen reviewed Dec 15, 2017

View reviewed changes

tobrun force-pushed the tvn-expressions-wip branch from f57fde0 to f408c00 Compare December 16, 2017 11:20

This was referenced Dec 16, 2017

Add expression filter support #10720

Closed

Add expression accessors #10721

Closed

Add expression instrumentation tests #10722

Closed

ivovandongen approved these changes Dec 18, 2017

View reviewed changes

[android] - add binding integration for expressions

862eaa0

tobrun force-pushed the tvn-expressions-wip branch from f408c00 to 862eaa0 Compare December 19, 2017 06:03

tobrun merged commit 757cc0f into master Dec 19, 2017

tobrun deleted the tvn-expressions-wip branch December 19, 2017 07:55

1ec5 mentioned this pull request Dec 20, 2017

Show the contour-label symbol in meters or feet #5018

Closed

4 tasks

tobrun mentioned this pull request Jan 26, 2018

Release Android v6.0.0-beta.1 #11060

Closed

23 tasks

Expressions #10654

Expressions #10654

Conversation

tobrun commented Dec 7, 2017 • edited Loading

Symbol layer example:

Todo:

ThibaudM commented Dec 11, 2017

1ec5 commented Dec 12, 2017

tobrun commented Dec 12, 2017

anandthakker commented Dec 12, 2017

tobrun commented Dec 12, 2017 • edited Loading

anandthakker 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

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 Dec 16, 2017 • 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

tobrun Dec 16, 2017 • edited Loading

Choose a reason for hiding this comment

ivovandongen 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

1ec5 commented Jan 6, 2018

tobrun commented Dec 7, 2017 •

edited

Loading

tobrun commented Dec 12, 2017 •

edited

Loading

tobrun Dec 16, 2017 •

edited

Loading

tobrun Dec 16, 2017 •

edited

Loading