update style generation code for new style spec enum values docs by incanus · Pull Request #6508 · mapbox/mapbox-gl-native

incanus · 2016-09-29T00:02:59Z

Downstream work for mapbox/mapbox-gl-style-spec#510.

core
Darwin
Android

mention-bot · 2016-09-29T00:03:01Z

@incanus, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @mikemorris and @yhahn to be potential reviewers.

incanus · 2016-09-29T00:39:21Z

Only thing I'm not crazy about with e9be66b is failure to fully get NS_ENUM values, e.g.:

typedef NS_ENUM(NSUInteger, MGLSymbolStyleLayerIconRotationAlignment) {
    /**
     When `symbolPlacement` is set to `point`, aligns icons east-west. When `symbolPlacement` is set to `line`, aligns icon x-axes with the line.
     */
    MGLSymbolStyleLayerIconRotationAlignmentMap,
    /**
     Produces icons whose x-axes are aligned with the x-axis of the viewport, regardless of the value of `symbolPlacement`.
     */
    MGLSymbolStyleLayerIconRotationAlignmentViewport,
    /**
     When `symbolPlacement` is set to `point`, this is equivalent to `viewport`. When `symbolPlacement` is set to `line`, this is equivalent to `map`.
     */
    MGLSymbolStyleLayerIconRotationAlignmentAuto,
};

Note the point, line, viewport, etc.

Do we get this nitpicky?

1ec5 · 2016-09-29T01:37:33Z

Do we get this nitpicky?

Yes. The existing logic is robust enough to handle values of the property currently being described (first-person values), but not enough to handle values of another property being referenced (third-person values).

1ec5 · 2016-09-29T01:39:31Z

We can maybe detect constructions such as “x is set to y” and find candidate values accordingly.

Getting this right will ensure that jazzy can autolink the referenced symbols. I think autolinking is important for enabling users to effectively navigate our documentation, which is spread out among many headers or many webpages.

ivovandongen · 2016-09-29T09:58:51Z

        switch (value) {
-<% for (const value of property.values) { -%>
+<% for (const value in property.values) { -%>
+          //<%- property.values[value].doc %>


This is a left-over comment?

incanus · 2016-09-29T17:42:50Z

@ivovandongen Curious about your feedback again now after 0c8cc78 and c58f90a.

incanus · 2016-09-29T18:27:46Z

Working on some final Darwin-side propertyDoc() enhancements still. I may try something similar on the Android side after, but considering we didn't even have enum values documented at all in Java until c58f90a, this may not be high priority.

1ec5 · 2016-09-29T21:03:40Z

 NS_ASSUME_NONNULL_BEGIN

+/**
+ Controls the translation reference point.


Since the enums are on a different page than the properties that use them, perhaps this documentation could mention those properties.

Not sure I follow @1ec5.

For example:

Used by the MGLCircleStyleLayer.circleTranslateAnchor property.

Tail work per #6508 (comment).

1ec5 · 2016-09-29T21:19:03Z

+ */
 typedef NS_ENUM(NSUInteger, MGLSymbolStyleLayerIconRotationAlignment) {
+    /**
+     When `symbolPlacement` is set to `point`, aligns icons east-west. When `symbolPlacement` is set to `line`, aligns icon x-axes with the line.


Hopefully we can rely on this “x is set to y” phrase and expand y to the right enum value.

Done pending improvements in #6508 (comment).

1ec5 · 2016-09-29T21:21:41Z

 The default value of this property is an `NSValue` object containing `NSEdgeInsetsZero` or `UIEdgeInsetsZero`. Set this property to `nil` to reset it to the default value.

- This property is only applied to the style if `iconImage` is non-`nil`, and `iconTextFit` is non-`nil`, and `textField` is non-`nil`. Otherwise, it is ignored.
+ This property is only applied to the style if `iconImage` is non-`nil`, and `textField` is non-`nil`, and `iconTextFit` is set to an `NSValue` object containing `MGLSymbolStyleLayerIconTextFitBoth`, `MGLSymbolStyleLayerIconTextFitWidth`, or `MGLSymbolStyleLayerIconTextFitHeight`. Otherwise, it is ignored.


👍 This takes care of #6448 (comment).

1ec5 · 2016-09-29T21:23:48Z

@@ -143,7 +143,7 @@ public PropertyValue<String> getFillColor() {
        return (PropertyValue<String>) new PropertyValue(nativeGetFillColor());
    }
     /**


Some funky indentation going on here.

Think I got this in caeee4f.

incanus · 2016-09-29T22:43:46Z

Got the last mile of Darwin docs in e08bc5a. Formatting rules documented in generate-style-code.js itself.

incanus · 2016-09-29T23:34:16Z

@ivovandongen To build on #6508 (comment), 1325127 brings over some of the logic from Darwin where we properly format symbol names in the PropertyFactory docs.

Before:

After:

Note how we go from referring to fill-color (GL style spec) to fillColor (Android property name).

Previously, unlike on Darwin, we didn't refer to symbols as they actually appear in the Android API, which can be confusing and lose people.

A few more questions about how much further we should go on this:

Is there a way in JavaDoc that we can cross-link these symbols for better discoverability rather than just surround them in backticks? If so, should we do this?

As of changes like mapbox/mapbox-gl-style-spec@742bc7a, we move certain documentation from the enum property names to the values. However, on Android, since this is through PropertyFactory and uses primitives like string, float, etc., we don't actually document the existence of enum values (again, unlike on Darwin; see for example for each of butt, round, and square here). I think that we should, but I'm not sure where this sort of thing would go. We should be documenting for users the range of possibilities to use in the runtime styling API.

Compare iOS:

/**
Label placement relative to its geometry.
*/
typedef NS_ENUM(NSUInteger, MGLSymbolStyleLayerSymbolPlacement) {
    /**
     The label is placed at the point where the geometry is located.
     */
    MGLSymbolStyleLayerSymbolPlacementPoint,
    /**
     The label is placed along the line of the geometry. Can only be used on `LineString` and `Polygon` geometries.
     */
    MGLSymbolStyleLayerSymbolPlacementLine,
};

To Android:

/**
* Label placement relative to its geometry.
*
* @param function a wrapper function for String
* @return property wrapper around a String function
*/
public static Property<Function<String>> symbolPlacement(Function<String> function) {
    return new LayoutProperty<>("symbol-placement", function);
}

Note how we lost context like the fact that SYMBOL_PLACEMENT_LINE can only be used on LineString and Polygon geometries, or even that SYMBOL_PLACEMENT_POINT and SYMBOL_PLACEMENT_LINE are the options here.

/cc @tobrun @zugaldia

1ec5 · 2016-09-30T00:00:37Z

+global.propertyDoc = function (propertyName, property, layerType) {
+    // Match references to other property names & values. 
+    // Requires the format 'When `foo` is set to `bar`,'.
+    let doc = property.doc.replace(/When `(.+?)` is set to `(.+?)`,/g, function (m, peerPropertyName, propertyValue, offset, str) {


Is "When" needed? If so, can it be case-insensitive using the i flag? Is the comma needed? I think we should try to avoid coupling too tightly to how things are worded right now, because the style spec repo doesn't have any documentation that would inform a contributor that they need to worry about these details.

I tried without the When and comma, but ran into issues that crossed sentence boundaries. I can try to refine it further.

If the +? is matching too greedily for some reason, you can replace . with:

[^`]

Specifically I tried things like .[^\.]+? in the regex to avoid sentence-spanning over periods. Will take another crack.

> "When `foo` is set to `bar`, `baz` is set to `boo`.".match(/`(.+?)` is set to `(.+?)`/g) [ "`foo` is set to `bar`", "`baz` is set to `boo`" ] > "When `foo` is set to `bar`, `baz` becomes `boo`. When `moo` is set to `goo`, `oog` becomes `oop`.".match(/`(.+?)` is set to `(.+?)`/g) [ "`foo` is set to `bar`", "`baz` becomes `boo`. When `moo` is set to `goo`" ] > "When `foo` is set to `bar`, `baz` becomes `boo`. When `moo` is set to `goo`, `oog` becomes `oop`.".match(/`([^`]+?)` is set to `([^`]+?)`/g) [ "`foo` is set to `bar`", "`moo` is set to `goo`" ]

Tail work per #6508 (comment).

1ec5 · 2016-09-30T00:09:48Z

Is there a way in JavaDoc that we can cross-link these symbols for better discoverability rather than just surround them in backticks? If so, should we do this?

{@SNAKES_ON_A_LAYER}

We should be documenting for users the range of possibilities to use in the runtime styling API.

How about a <dl> in the existing comments for the property factory methods?

ivovandongen · 2016-09-30T09:09:40Z

+    /**
+     * Layer visibility
+     */
    public static final String VISIBLE = "visible";


@incanus If you document in this way, it is only applied to the first constant VISIBLE. The // I used don't show up at all in javadoc. I use those more as you'd use #pragma mark, to organize the source, not to document. If you apply javadoc, you need to do it on all constants I'm afraid.

ivovandongen · 2016-09-30T09:10:46Z

+    /**
+     * `line-cap`: The display of line endings.
+     */
+    //A cap with a squared-off end which is drawn to the exact endpoint of the line.


Same thing here. I would invert this, start the header with // and the individual comments with /** */

ivovandongen · 2016-09-30T09:13:04Z

Is there a way in JavaDoc that we can cross-link these symbols for better discoverability rather than just surround them in backticks? If so, should we do this?

http://docs.oracle.com/javase/8/docs/technotes/tools/windows/javadoc.html#CHDDIECH

incanus · 2016-09-30T20:54:04Z

Still pushing on Android finishing work here per discussion with @ivovandongen and more learnin' in the JavaDocs department.

incanus · 2016-09-30T21:02:09Z

The JavaDoc stuff gets tricky since we don't know the argument quantity or types, which only lets us do something like {@link symbolPlacement} and not the {@link symbolPlacement(string)} that is required for actual linking. Per http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html we could just use <code>symbolPlacement</code> instead. Doing some experiments.

incanus · 2016-09-30T21:35:24Z

I'm going to flag some things above in response to comments as tail work. The main goal here is to get use of enum docs on the books and improve over current master. Then we can incrementally make these all better across all platforms.

incanus · 2016-09-30T21:37:21Z

We should be documenting for users the range of possibilities to use in the runtime styling API.

How about a <dl> in the existing comments for the property factory methods?

Tail work per #6508 (comment).

Adds `doc` property to all enum `values`, moving them from JSON arrays to objects. Downstream mobile SDK work in mapbox/mapbox-gl-native#6508. Downstream Studio work in mapbox/studio#7765. Fixes a typo in `icon-text-fit`. Also updates validation, docs generation, and tests.

incanus · 2016-09-30T22:11:50Z

Tail work tickets: #6540, #6541, #6542.

@1ec5 Mind reviewing again and getting this ready to 🚢?

1ec5 · 2016-09-30T22:13:50Z

The Qt build failed; looks like a spurious core test failure:

[ RUN      ] Timer.StartOverrides
../../../test/util/timer.test.cpp:165: Failure
Expected: (totalTime) <= (expectedTotalTime * 1.2), actual: 8-byte object <B4-05 00-00 00-00 00-00> vs 8-byte object <00-00 00-00 00-80 76-40>
[  FAILED  ] Timer.StartOverrides (1469 ms)

incanus · 2016-09-30T22:30:29Z

Restarted tests and all now pass. Working on merge now.

bump style spec and properly iterate enum values

66557b2

incanus added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android documentation release blocker Blocks the next final release runtime styling labels Sep 29, 2016

incanus added this to the ios-v3.4.0 milestone Sep 29, 2016

incanus self-assigned this Sep 29, 2016

iOS code gen updates for enum values docs

e9be66b

Android code gen updates for enum values docs

f560286

incanus added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Sep 29, 2016

document the actual enum definitions on Darwin

57420c9

incanus mentioned this pull request Sep 29, 2016

fixes #497: document enum values mapbox/mapbox-gl-style-spec#510

Merged

ivovandongen reviewed Sep 29, 2016

View reviewed changes

incanus added 3 commits September 29, 2016 10:05

remove JNI enum docs since unnecessary

0c8cc78

Merge remote-tracking branch 'origin/master' into style-spec-enum-docs

b0c7392

document individual properties & enum values in Java

c58f90a

snake case Java headings

6c906e2

1ec5 mentioned this pull request Sep 29, 2016

Doc gen handles arrays as required property values #6448

Merged

1ec5 suggested changes Sep 29, 2016

View reviewed changes

1ec5 reviewed Sep 29, 2016

View reviewed changes

format references to peer properties and values other than the current

e08bc5a

better format symbols in Android enum property docs

1325127

1ec5 suggested changes Sep 30, 2016

View reviewed changes

ivovandongen reviewed Sep 30, 2016

View reviewed changes

bump in-progress style spec

4da521e

incanus added 2 commits September 30, 2016 14:21

[android] flip comment style & [ios, android] update visibility doc

357f6a8

nicer spacing

80da0f3

Merge remote-tracking branch 'origin/master' into style-spec-enum-docs

bb12581

1ec5 mentioned this pull request Sep 30, 2016

Override style specification documentation in runtime styling API #6107

Merged

This was referenced Sep 30, 2016

cross-reference Darwin enum docs to properties that use them #6540

Closed

make Darwin enum peer cross-references more robust #6541

Closed

more clearly list Android enum values docs in JavaDoc #6542

Closed

1ec5 approved these changes Sep 30, 2016

View reviewed changes

bump style spec to one that eventually got merged

3f1b13b

incanus merged commit ee4715a into master Sep 30, 2016

incanus deleted the style-spec-enum-docs branch September 30, 2016 22:55

incanus mentioned this pull request Sep 30, 2016

Runtime styling enumerations are undocumented #5953

Closed

1ec5 removed the ✓ ready for review label Oct 5, 2016

Conversation

incanus commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Sep 29, 2016

Uh oh!

incanus commented Sep 29, 2016

Uh oh!

1ec5 commented Sep 29, 2016

Uh oh!

1ec5 commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

incanus commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

incanus commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

incanus commented Sep 29, 2016

Uh oh!

incanus commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivovandongen commented Sep 30, 2016

Uh oh!

incanus commented Sep 30, 2016

Uh oh!

incanus commented Sep 30, 2016

Uh oh!

incanus commented Sep 30, 2016

Uh oh!

incanus commented Sep 30, 2016

Uh oh!

incanus commented Sep 30, 2016

Uh oh!

incanus commented Sep 29, 2016 •

edited

Loading

incanus commented Sep 29, 2016 •

edited

Loading

incanus commented Sep 29, 2016 •

edited

Loading

1ec5 Sep 30, 2016 •

edited

Loading