[GEOT-6494] MBStyle extension #2743

jodygarnett · 2020-01-13T07:49:55Z

Reviewing mbstyle codebase, cleaning up reports and checking code coverage.

https://osgeo-org.atlassian.net/browse/GEOT-6493

[GEOT-6503] Extend SLD to support background fill specification at the UserStyle level #2775 background color
MBStyle, use alpha also from RGBA color spec #2782 opacity fix

Working on:

Update pom.xml and move to extension
Documentation
Test coverage
Include BSD license for documentation

Checklist

For all pull requests:

Confirm you have read the contribution guidelines
You have sent a Contribution Licence Agreement (CLA) as necessary (not required for small changes, e.g., fixing typos in documentation)
Make sure the first PR targets the master branch, eventual backports will be managed later. This can be ignored if the PR is fixing an issue that only happens in a specific branch, but not in newer ones.
The changes are not breaking the build in downstream projects using SNAPSHOT dependencies, GeoWebCache and GeoServer.

The following are required only for core and extension modules (they are welcomed, but not required, for unsupported modules):

There is a ticket in Jira describing the issue/improvement/feature (a notable exemptions is, changes not visible to end users)
PR for bug fixes and small new features are presented as a single commit
Commit message must be in the form "[GEOT-XYZW] Title of the Jira ticket"
New unit tests have been added covering the changes
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
This PR passes the QA checks (QA checks results will be reported by travis-ci after opening this PR)
Documentation has been updated accordingly.

Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.

aaime · 2020-01-13T08:08:19Z

@jodygarnett I see this is just a review, which is fine. Could you make sure all PRs against the module are merged before moving it? I have one out, one in the making.

modules/unsupported/mbstyle/src/main/java/org/geotools/mbstyle/layer/CircleMBLayer.java

jodygarnett · 2020-01-13T18:19:24Z

@aaime wrote:

Could you make sure all PRs against the module are merged before moving it?

Yes I would like to clean up what I can here, and then do a separate PR to move.

jodygarnett · 2020-01-31T21:53:52Z

@jodygarnett I see this is just a review, which is fine. Could you make sure all PRs against the module are merged before moving it? I have one out, one in the making.

@aaime do you have any additional PRs against this module for me to review?

aaime · 2020-01-31T21:59:23Z

Yes there are two, I pinged you in both already, the background one and the exponential math one Il ven 31 gen 2020, 22:53 Jody Garnett <notifications@github.com> ha scritto:

…

@jodygarnett <https://github.com/jodygarnett> I see this is just a review, which is fine. Could you make sure all PRs against the module are merged before moving it? I have one out, one in the making. @aaime <https://github.com/aaime> do you have any additional PRs against this module for me to review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2743?email_source=notifications&email_token=AACPOOOGVFRWPLKWNIPYO63RASMXDA5CNFSM4KF5ZQU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKQD6BI#issuecomment-580927237>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACPOOPRUZ4XMT4VGVF7E7DRASMXDANCNFSM4KF5ZQUQ> .

aaime · 2020-02-08T13:37:35Z

@jodygarnett I don't have time to work on the alpha issue, please go ahead with the review and move, I'll resume later.

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett · 2020-02-09T17:31:43Z

docs/user/extension/mbstyle/index.rst

-  Color
+Quoting `MapBox Style Specification <https://www.mapbox.com/mapbox-gl-js/style-spec/>`__ :
+
+  ``Color``


@tbarsballe this section has some duplication with spec/types.rst, do you know any background to untangle?

Reading the content, it looks like this was an earlier summary of the specification

spec/*.rst is a direct copy of the MapBox spect with GeoTools supported version added.
This index page tries to provide a more concise overview / quickstart, and pulls some of the more top-level information from the spec. It could maybe do with some rewording, but should still keep all that info in it.

okay I removed the duplication, and made this focus on the example snippets

jodygarnett · 2020-02-09T19:10:39Z

The integration tests show an unrelated failure in GeoServer GeoWebCache (GWC) Module - unable to delete a folder on disk.

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett · 2020-02-09T21:24:55Z

docs/user/welcome/license.rst

-* Code contributed by government members is often contributed as public domain
-* To facilitate adopting of the GeoTools library the example code used in the
-  user guide is also public domain
-


Moved this to the respective headings.

aaime · 2020-02-10T10:14:00Z

modules/plugin/pom.xml

    <module>property</module>
    <module>shapefile</module>
    <module>svg</module>
-    <module>coverage-multidim</module>
-    <module>geopkg</module>


So this change kicked geopkg out of the build? Ideally a module promotion should just do that, promote the module, all the other improvements should be in a different commit (if not different PR), because they are not related, and are easy to get lost in the movement of so many files (github reports 200+ files changed in this PR). Thankfully I spotted this one at the source...

Thanks for the catch Andrea, I appreciate it.
I initially tried moving this to plugin, but it did not fit (since it defines new API).

aaime · 2020-02-10T10:16:13Z

pom.xml

@@ -1577,6 +1577,13 @@
        <artifactId>jt-shadedrelief</artifactId>
        <version>${jaiext.version}</version>
      </dependency>
+
+      <!-- Third-party -->
+      <dependency>


Ah, so mbstyle is using json-simple to parse JSON? The library is completely dead, has not seen releases since 2012.
Not holding it as a blocker for the module, but wondering, is there any plan to switch to something supported? (it would not be the first dependency having this issue mind, I'm really just asking).

I am not aware why this was chosen, just annoyed it was not in the dependency management section.

Didn't we talk this out with GeoMesa folks to select a decent JSON library?

There was a discussion on dependencies for gt-main, and we ended up using jackson for that. But mbstyles is older, and used the same library as the semi-defunct gt-geojson (which I hope is gonna be replaced by the gt-jacksondatastore one day or the other...)

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett · 2020-02-11T19:14:24Z

Travis is failing on documentation toctree use of caption directive, must be an older version of sphinx.

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett · 2020-02-13T11:18:39Z

This was approved by PMC:

Andrea Aime +1
Ian Turton +1
Jody Garnett +1
Nuno Oliveira +1
Simone Giannecchini +1
Torben Barsballe +1

jodygarnett · 2020-02-13T11:19:36Z

There is no need to backport at this time.

aaime reviewed Jan 13, 2020

View reviewed changes

modules/unsupported/mbstyle/src/main/java/org/geotools/mbstyle/layer/CircleMBLayer.java Outdated Show resolved Hide resolved

jodygarnett changed the title ~~mbstyle review~~ [GEOT-6493] MBStyle plugin available Jan 13, 2020

jodygarnett changed the title ~~[GEOT-6493] MBStyle plugin available~~ [GEOT-6494] Review MBStyle plugin Jan 13, 2020

jodygarnett force-pushed the mbstyle_QA branch 2 times, most recently from fcf2ea7 to bd649fb Compare January 31, 2020 21:51

jodygarnett added 5 commits February 8, 2020 17:15

cleaning up javadoc warnings

dc46d3d

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

[GEOT-6494] updated test coverage instructions

cb5eb11

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

[GEOT-6494] addressing javadoc warnings

b871343

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

Formatting clean up

edfa57a

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

javadoc warnings

a6b1fc8

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett force-pushed the mbstyle_QA branch from b7b6eaa to a6b1fc8 Compare February 8, 2020 16:16

initial move of mbstyle extension and documentation

874d4a0

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett marked this pull request as ready for review February 9, 2020 17:12

jodygarnett added 2 commits February 9, 2020 18:28

fix build

7cdbcef

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

break spec into pages

fd6541d

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett changed the title ~~[GEOT-6494] Review MBStyle plugin~~ [GEOT-6494] MBStyle extension Feb 9, 2020

jodygarnett commented Feb 9, 2020

View reviewed changes

Clean up license references in documentation

52e0f6f

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett commented Feb 9, 2020

View reviewed changes

aaime reviewed Feb 10, 2020

View reviewed changes

revise based on feedback

9fea6a4

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

remove toctree caption

d468fc4

Signed-off-by: Jody Garnett <jody.garnett@gmail.com>

jodygarnett merged commit b709df9 into geotools:master Feb 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GEOT-6494] MBStyle extension #2743

[GEOT-6494] MBStyle extension #2743

jodygarnett commented Jan 13, 2020 •

edited

Loading

aaime commented Jan 13, 2020

jodygarnett commented Jan 13, 2020

jodygarnett commented Jan 31, 2020

aaime commented Jan 31, 2020 via email

aaime commented Feb 8, 2020

jodygarnett Feb 9, 2020 •

edited

Loading

tbarsballe Feb 10, 2020

jodygarnett Feb 11, 2020

jodygarnett commented Feb 9, 2020 •

edited

Loading

jodygarnett Feb 9, 2020

aaime Feb 10, 2020

jodygarnett Feb 11, 2020

aaime Feb 10, 2020 •

edited

Loading

jodygarnett Feb 11, 2020

aaime Feb 11, 2020

jodygarnett commented Feb 11, 2020

jodygarnett commented Feb 13, 2020

jodygarnett commented Feb 13, 2020

[GEOT-6494] MBStyle extension #2743

[GEOT-6494] MBStyle extension #2743

Conversation

jodygarnett commented Jan 13, 2020 • edited Loading

Checklist

aaime commented Jan 13, 2020

jodygarnett commented Jan 13, 2020

jodygarnett commented Jan 31, 2020

aaime commented Jan 31, 2020 via email

aaime commented Feb 8, 2020

jodygarnett Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

tbarsballe Feb 10, 2020

Choose a reason for hiding this comment

jodygarnett Feb 11, 2020

Choose a reason for hiding this comment

jodygarnett commented Feb 9, 2020 • edited Loading

jodygarnett Feb 9, 2020

Choose a reason for hiding this comment

aaime Feb 10, 2020

Choose a reason for hiding this comment

jodygarnett Feb 11, 2020

Choose a reason for hiding this comment

aaime Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

jodygarnett Feb 11, 2020

Choose a reason for hiding this comment

aaime Feb 11, 2020

Choose a reason for hiding this comment

jodygarnett commented Feb 11, 2020

jodygarnett commented Feb 13, 2020

jodygarnett commented Feb 13, 2020

jodygarnett commented Jan 13, 2020 •

edited

Loading

jodygarnett Feb 9, 2020 •

edited

Loading

jodygarnett commented Feb 9, 2020 •

edited

Loading

aaime Feb 10, 2020 •

edited

Loading