Format expression Android bindings #12985

LukasPaczos · 2018-09-28T17:38:50Z

Refs #12624, exposes format expression on Android.

Setting the text-field using the expression, or plain text, works as expected, however, getting the text-field property back from core reveals a couple of issues:

Plain text is returned as expected.
Any expression, other than format, is returned wrapped by format expression. This includes tokens (which are beforehand converted from plain text to expressions and this is expected).
When value is passed initially as the format expression, it's being returned as String, which is expected because of this implementation.

Currently, above issues cause a bunch of tests to fail.

/cc @ChrisLoer

LukasPaczos · 2018-09-28T18:11:20Z

Solution to above issues will require introducing a Formatted type on the Java side, and possibly separate getters and setters of text-field property for plain text and Expression + Formatted.

LukasPaczos · 2018-10-11T18:50:48Z

This PR introduces Formatted (equivalent of the style-spec type) java object which is returned in, also newly added, SymbolLayer#getFormattedTextField value field for constant text-field properties. In order not to break semver, the SymbolLayer#getTextField value field still returns plain text in the form of String.

Expression fields returned by SymbolLayer#getFormattedTextField and SymbolLayer#getTextField both return the original Expression wrapped in Expression#format expression.

Follow up work needed after this PR lands is exposing PropertyFactory#textField method that takes Formatted as an argument and all the c++ conversion code needed. When major version release comes, we can remove SymbolLayer#getFormattedTextField and make SymbolLayer#getTextField return Formatted value type.

ChrisLoer · 2018-10-11T20:14:55Z

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

+   * Text is required to be a string literal, font-scale a number literal and text-font an array literal that holds
+   * names of main and backup fonts.
+   * All of them are required to be passed, but font-scale and text-font can be nullified
+   * and will default to 1.0 and an empty array respectively.


"default to 1.0 and an empty array respectively."

Instead of describing the defaults in implementation terms here, I think it would be better to describe the conceptual effect, which is that if "font-scale" or "text-font" are not set, the section will use the base "text-size" or "text-font (respectively) defined for the symbol.

ChrisLoer · 2018-10-11T20:15:51Z

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

+   * and accepts unlimited numbers of formatted sections.
+   * <p>
+   * Each sections consist of 3 elements, in this order: text, font-scale and text-font.
+   * Text is required to be a string literal, font-scale a number literal and text-font an array literal that holds


"text-font" is required to be an array literal. "text" only requires an expression that returns a string and "font-scale" requires an expression that returns a number.

ChrisLoer · 2018-10-11T20:19:39Z

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

+   * "format" expressions can be used, for example, with the {@link PropertyFactory#textField(Expression)}
+   * and accepts unlimited numbers of formatted sections.
+   * <p>
+   * Each sections consist of 3 elements, in this order: text, font-scale and text-font.


How hard would it be to make the Java interface conform to the underlying expression format of using an array of text/option pairs? The problem with expanding the current "options" argument into two separate arguments here is that it will break backwards compatibility when/if we add support for further options (there are several that could conceivably be added, the immediate one we have a request for is to add "text-color" as a format option).

Thanks for the heads up! I'll try to future-proof the builder.

...rm/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/layers/layer.java.ejs

ChrisLoer · 2018-10-11T20:37:28Z

...ndroid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/layers/PropertyValue.java

@@ -62,7 +62,7 @@ public Expression getExpression() {
        ? Expression.Converter.convert((JsonArray) value)
        : (Expression) value;
    } else {
-      Logger.w(TAG, "not a expression, try value");
+      Logger.w(TAG, "not an expression, try value");


Took me a moment to understand "try value"/"try expression" -- would it read easier to say "try getValue()"/"getExpression()"?

ChrisLoer · 2018-10-11T20:47:01Z

...xGLAndroidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/layer.junit.ejs

@@ -133,7 +131,6 @@ public class <%- camelize(type) %>LayerTest extends BaseActivityTest {
    });
  }

-


nit: This makes the diff bigger than it needs to be.

ChrisLoer · 2018-10-11T20:52:37Z

platform/android/src/style/formatted.cpp

+        auto section = value.sections.at(i);
+        auto text = jni::Make<jni::String>(env, section.text);
+
+        auto fontScale = 1.0;


nit: auto here is hard to read (although not technically ambiguous obviously since the compiler can figure it out). I would prefer explicitly declaring the type as double. There's a rule of thumb that you should restrict using auto to cases where the type is visible on the same line -- we definitely don't live up to that rule, but it's good to keep in the back of your mind! Basically, auto is good when it saves lots of repetitive typing, but it's bad when it prevents you from having to think about what type you're working with.

...roid/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/style/types/FormattedSection.java

ChrisLoer · 2018-10-11T21:03:26Z

platform/android/src/style/formatted.cpp

+            fontScale = section.fontScale.value();
+        }
+
+        auto fontStack = jni::Array<jni::String>::New(env, 0);


When I saw FormattedSection::getFontStack() above and saw that it was nullable, I assumed that "null" was being used to represent "not set". But it looks here (and now I notice in the tests) that text-font: null will cause getFontStack() to return new String[] {}. That doesn't seem right -- an empty array would signify "use this FontStack that doesn't have any fonts in it" instead of "use the default font stack".

Good point, I'm going to return null instead of an empty array for un-set stack.

platform/android/src/style/formatted.cpp

tobrun · 2018-10-12T14:09:09Z

.../MapboxGLAndroidSDK/src/test/java/com/mapbox/mapboxsdk/style/expressions/ExpressionTest.java

+      }
+    };
+    Object[] actual = format(
+      literal("test"), null, literal("awesome"),


"awesome" should be new String[]{"awesome"}
same applies for other occurrences of font-stack in the unit tests

great catch!

tobrun · 2018-10-12T14:25:11Z

Thanks for running with this @LukasPaczos and @ChrisLoer for the thorough review and great insights!

Took the code for a spin and seems to be working well:

Above is the a the expression copied from the unit test with some valid fonts (+ putting them in an array)

            format(
              literal("test"), null, literal(new String[] {"DIN Offc Pro Regular", "Arial Unicode MS Regular"}),
              literal("test2"), literal(1.5), null,
              literal("test3"), null, null,
              literal("test4"), literal(1.5), literal(new String[] {"DIN Offc Pro Regular", "Arial Unicode MS Regular"})
            )

An idea to improve the ease of use of this API we could expose format specific expression:

            format(
              formatEntry("test", new String[] {"DIN Offc Pro Regular", "Arial Unicode MS Regular"}),
              formatEntry("test2", 1.5),
              formatEntry("test3"),
              formatEntry("test4"), 1.5, new String[] {"DIN Offc Pro Regular", "Arial Unicode MS Regular"})
            )

This makes it slightly easier to integrate and would remove the necessity of adding null values such an improvement can be done as tail work. I'm 👍 on this PR, will leave it to @ChrisLoer for the actual 👍.

For converting to Java types, flatten 'text-field' back to a string.

LukasPaczos · 2018-10-23T15:57:36Z

This one is ready for another round @tobrun @ChrisLoer. I addressed the comments and opted for a more typed builder with FormatEntry type suggested by @tobrun and extended the idea to include FormatOption which also prepares the code to accept a new set of options in the future.

LukasPaczos · 2018-10-23T15:59:08Z

923dfa7 also updates the local style that we are using for testing which was causing issues when setting a text-field property.

LukasPaczos · 2018-10-23T17:39:39Z

8395a5c renames format options to avoid a clash with PropertyFactory#textFont and adds the example in the test activity.

ChrisLoer

Looking good to me! Will defer to @tobrun on final review.

ChrisLoer · 2018-10-23T18:19:04Z

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

+   *         formatFontScale(2.0),
+   *         formatTextFont(new String[] {"DIN Offc Pro Regular", "Arial Unicode MS Regular"})
+   *       ),
+   *       formatEntry(concat(literal("\n"), get("description_property")), formatFontScale(1.5)),


This code example looks like it abruptly ends after formatFontScale(1.5)),, is that intentional?

Nice catch 👍

ChrisLoer · 2018-10-23T18:25:53Z

platform/android/src/style/position.cpp

@@ -31,5 +31,5 @@ float Position::getPolarAngle(jni::JNIEnv& env, const jni::Object<Position>& pos
    return position.Get(env, field);
 }

-} // namespace andr[oid


platform/android/src/style/formatted.cpp

...droidSDKTestApp/src/androidTest/java/com/mapbox/mapboxsdk/testapp/style/SymbolLayerTest.java

tobrun

🚀

…example

LukasPaczos · 2019-03-05T11:19:01Z

Example usage of the format expression:

mapView.getMapAsync(mapboxMap -> {
  mapboxMap.setStyle(Style.MAPBOX_STREETS, style -> {
    // create header entry based on data-driven styling
    Expression.FormatEntry header = Expression.formatEntry(
      Expression.get("header_property"),
      Expression.FormatOption.formatFontScale(Expression.get("header_scale_property")),
      Expression.FormatOption.formatTextFont(Expression.get("header_font_property")));
    
    // create hardcoded text entry
    Expression.FormatEntry text = Expression.formatEntry(
      "\nsome text",
      Expression.FormatOption.formatFontScale(1.5));
    
    // create hardcoded side-note entry
    Expression.FormatEntry sideNote = Expression.formatEntry(
      " (side-note)",
      Expression.FormatOption.formatFontScale(0.75));
    
    // build format expression out of provided entries
    Expression format = Expression.format(header, text, sideNote);
    
    // create the first feature and add header data
    Feature feature1 = Feature.fromGeometry(Point.fromLngLat(0, 45));
    feature1.addStringProperty("header_property", "Header1");
    feature1.addNumberProperty("header_scale_property", 3.5);
    
    JsonArray headerFontStack1 = new JsonArray();
    headerFontStack1.add("DIN Offc Pro Bold");
    headerFontStack1.add("Arial Unicode MS Regular");
    feature1.addProperty("header_font_property", headerFontStack1);
    
    // create the second feature and add header data
    Feature feature2 = Feature.fromGeometry(Point.fromLngLat(0, -45));
    feature2.addStringProperty("header_property", "Header2");
    feature2.addNumberProperty("header_scale_property", 3.5);
    
    // reverse the font stack
    JsonArray headerFontStack2 = new JsonArray();
    headerFontStack2.add("Arial Unicode MS Regular");
    headerFontStack2.add("DIN Offc Pro Bold");
    feature2.addProperty("header_font_property", headerFontStack2);
    
    // add the source and the layer
    Source source = new GeoJsonSource("source", FeatureCollection.fromFeatures(new Feature[] {feature1, feature2}));
    style.addSource(source);
    Layer layer = new SymbolLayer("layer", "source")
      .withProperties(
        PropertyFactory.textField(format)
      );
    style.addLayer(layer);
  });
});

LukasPaczos added Android Mapbox Maps SDK for Android in progress labels Sep 28, 2018

ChrisLoer force-pushed the formatted-text branch 2 times, most recently from 5712099 to 80014cb Compare October 3, 2018 00:04

LukasPaczos force-pushed the lp-format-expression branch 4 times, most recently from 928056e to 3d9515b Compare October 10, 2018 17:41

LukasPaczos added this to the android-v6.7.0 milestone Oct 10, 2018

ChrisLoer force-pushed the formatted-text branch 3 times, most recently from 0fc0825 to 0b1466e Compare October 10, 2018 23:07

LukasPaczos force-pushed the lp-format-expression branch 7 times, most recently from a267ad1 to 56e58b9 Compare October 11, 2018 18:21

LukasPaczos mentioned this pull request Oct 11, 2018

Multiple text formats for symbols via "format" expression (native port) #12624

Merged

6 tasks

LukasPaczos removed the in progress label Oct 11, 2018

LukasPaczos requested review from ChrisLoer and tobrun October 11, 2018 18:50

ChrisLoer reviewed Oct 11, 2018

View reviewed changes

tobrun reviewed Oct 12, 2018

View reviewed changes

ChrisLoer force-pushed the formatted-text branch 2 times, most recently from b032b38 to fc71883 Compare October 15, 2018 19:40

LukasPaczos force-pushed the lp-format-expression branch 2 times, most recently from a4ac85b to 5ed3c05 Compare October 22, 2018 14:44

LukasPaczos changed the base branch from formatted-text to master October 22, 2018 14:45

[android] Minimal Android wrapper on 'format' expression

1685207

For converting to Java types, flatten 'text-field' back to a string.

LukasPaczos force-pushed the lp-format-expression branch 4 times, most recently from 659afbb to 923dfa7 Compare October 23, 2018 15:53

LukasPaczos added 2 commits October 23, 2018 19:14

[android] "format" expression support

bf68017

[android] update local streets style used for CI testing

edcca60

LukasPaczos force-pushed the lp-format-expression branch from 923dfa7 to edcca60 Compare October 23, 2018 17:14

ChrisLoer reviewed Oct 23, 2018

View reviewed changes

tobrun approved these changes Oct 24, 2018

View reviewed changes

LukasPaczos force-pushed the lp-format-expression branch 2 times, most recently from 6607d5b to 219c370 Compare October 24, 2018 09:50

[android] renamed format expression options, added format expression …

4b0d51e

…example

LukasPaczos force-pushed the lp-format-expression branch from 219c370 to 4b0d51e Compare October 24, 2018 10:13

LukasPaczos merged commit d5b66e5 into master Oct 24, 2018

LukasPaczos deleted the lp-format-expression branch October 24, 2018 11:35

This was referenced Oct 24, 2018

Expose Formatted text-field setter #13172

Closed

Remove plain text text-field getter #13173

Closed

LukasPaczos mentioned this pull request Nov 7, 2018

Removing plain text getter for "text-field" #13306

Merged

riastrad mentioned this pull request Mar 6, 2019

Multiple Label Styles with Format Expression mapbox/mapbox-android-demo#985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format expression Android bindings #12985

Format expression Android bindings #12985

LukasPaczos commented Sep 28, 2018

LukasPaczos commented Sep 28, 2018

LukasPaczos commented Oct 11, 2018 •

edited

ChrisLoer Oct 11, 2018

ChrisLoer Oct 11, 2018

ChrisLoer Oct 11, 2018

LukasPaczos Oct 23, 2018

ChrisLoer Oct 11, 2018

ChrisLoer Oct 11, 2018

ChrisLoer Oct 11, 2018

ChrisLoer Oct 11, 2018

LukasPaczos Oct 23, 2018 •

edited

tobrun Oct 12, 2018

LukasPaczos Oct 22, 2018

tobrun commented Oct 12, 2018

LukasPaczos commented Oct 23, 2018

LukasPaczos commented Oct 23, 2018

LukasPaczos commented Oct 23, 2018

ChrisLoer left a comment

ChrisLoer Oct 23, 2018

LukasPaczos Oct 24, 2018

ChrisLoer Oct 23, 2018

tobrun left a comment

LukasPaczos commented Mar 5, 2019

		@@ -133,7 +131,6 @@ public class <%- camelize(type) %>LayerTest extends BaseActivityTest {
		});
		}

Format expression Android bindings #12985

Format expression Android bindings #12985

Conversation

LukasPaczos commented Sep 28, 2018

LukasPaczos commented Sep 28, 2018

LukasPaczos commented Oct 11, 2018 • edited

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

LukasPaczos Oct 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobrun commented Oct 12, 2018

LukasPaczos commented Oct 23, 2018

LukasPaczos commented Oct 23, 2018

LukasPaczos commented Oct 23, 2018

ChrisLoer 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

tobrun left a comment

Choose a reason for hiding this comment

LukasPaczos commented Mar 5, 2019

LukasPaczos commented Oct 11, 2018 •

edited

LukasPaczos Oct 23, 2018 •

edited