Add support of reading WMTS Get Cap document #3026

htulipe · 2014-12-08T20:58:48Z

This commit fixe #2721. It still lacks the possibility to create layer from the GetCap document. It also lacks the reading of TMS limits.

elemoine · 2014-12-09T08:38:39Z

+1 on merging this in. @fgravin, do you agree with reviewing this and tell if the format would work for you?

fgravin · 2014-12-09T09:31:21Z

Yes i will review this shortly.
Thanks for the work.

fgravin · 2014-12-09T16:39:03Z

Thanks for this work.
The parser works well and i like the output format. Unfortunatly, the behavior is too different from the ol.format.WMSCapabilities:
You use a lot of goog.object.set to define the output format as you want instead of basing your object on the XML structure. I find it good personnally, but it is not how other formats seemed to have been designed, specially the WMSCapabilities one.

For example the xml above:

<Layer>
<ows:Title>Blue Marble Next Generation</ows:Title>
<ows:Abstract>Blue Marble Next Generation NASA Product

gives you a

layers: [{
    title: 'Blue Marble Next Generation',
    abstract: 'Blue Marble Next Generation NASA'
}]

while the WMS one follows more xml structure with

Layer: [{
    Title: 'Blue Marble Next Generation',
    Abstract: 'Blue Marble Next Generation NASA'
}]

Check name and capitals.

I think we must be consistent between formats, and use more xml.format methods (like ol.xml.pushParseAndPop) to build the output structure.

htulipe · 2014-12-09T16:44:35Z

I actually copy/pasted the test from ol2's wmts format and worked on make those tests pass. The structure you get is more or less the one that ol2 would gave you.

That being said, I get your point about consistency. I will refactor that, this is not much.

Thanks for your input.

elemoine · 2014-12-09T16:52:00Z

test/spec/ol/format/wmts/ogcsample.xml

+        </TileMatrixSet>
+	</Contents>
+	<ServiceMetadataURL xlink:href="http://www.maps.bob/wmts/1.0.0/WMTSCapabilities.xml"/>
+</Capabilities>


No new line at end of file.

elemoine · 2014-12-09T16:54:26Z

That being said, I get your point about consistency. I will refactor that, this is not much.

Thanks. At this point I think it makes sense that the WMTS GetCapabilities format be consistent with the WMS GetCapabilities format.

Thanks @fgravin for the review.

fgravin · 2014-12-10T16:02:20Z

src/ol/format/wmtscapabilitiesformat.js

+ol.format.WMTSCapabilities.readTileMatrixSet_ = function(node, objectStack) {
+  var value = ol.xml.pushParseAndPop({},
+      ol.format.WMTSCapabilities.TMS_PARSERS_, node, objectStack);
+  if (!goog.isDef(value) || !goog.isDef(value.identifier)) {


This can't work in advanced mode :

In TMS_PARSERS_ you set the identifier as

'Identifier': ol.xml.makeObjectPropertySetter( ol.format.XSD.readString, 'identifier')

You tell the compiler to set this property name explicitly to 'identifier' but when you call the property via value.identifier the compiler renames it so the reference is lost.

I am really not familiar with google closure. If I understand well I must use something more like this:

if (!goog.isDef(value) || !goog.isDef(value.get('identifier'))) { }

Is that right?

fgravin · 2014-12-10T16:08:47Z

The format doesn't work in advanced build mode. I added some comments to point some errors on. I think that a better use of ol.xml.makeObjectPropertyPusher ol.xml.makeObjectPropertySetter etc.. will avoid you to manipule the objects as you do, avoiding some compilation issues.

You should test your format in built mode too.

htulipe · 2014-12-10T16:33:37Z

Dumb question maybe: how can I run my unit tests with the "built" mode?

htulipe · 2014-12-13T15:52:34Z

Okay I updated the PR with you guys feedback. I also added an example inspired by WMS GetCap. Ready to review.

fgravin · 2014-12-16T09:28:17Z

Hi, thanks, i'll take time to review this in the week.

fgravin · 2014-12-16T09:32:20Z

You can't launch your tests against build mode.
You can try to make a small example and include the last built version you got, or work with the host-examples workflow.

htulipe · 2014-12-16T09:39:15Z

Ok so that's what I did. I hosted my new example with host-examples, the parser was working.

htulipe · 2015-01-12T15:02:21Z

Hi guys, any update on this PR?

fgravin · 2015-01-13T17:43:04Z

Hi, i'll try to review it and test it on friday.

sarametz · 2015-01-16T04:07:55Z

Hi, I've created another PR that builds on this. It alters the WMTS source optionsFromCapabilties function to work with the new variable names, which allows for WMTS sources to be created from a capabilities document. Could you guys please have a look at #3142?

fgravin · 2015-01-16T08:29:31Z

@htulipe Thanks for your work. I reviewed and tested it back against your changes and i find it pretty good. It works in build mode, and the capabilities structure fits better the XML document and the WMSCapabities format.

Just few minor comments would be to be sure you respect case for all XML elements.
I suggest you to open your result object, and watch all property name against the XML tags, to be sure it fits well.
(HTTP, GET, ServiceIdentification, OperationsMetadata etc..) I'm not sure the DCP.HTTP.GET is an array by the way (it's not the case for WMS Capabitliies).

You can test the format against http://sdi.georchestra.org/geoserver/gwc/service/wmts?REQUEST=GetCapabilities&SERVICE=WMTS&VERSION=1.0.0 that have a pretty complete content.

Thanks

htulipe · 2015-01-16T09:30:17Z

Hi @fgravin, thanks for the feedback. I am guessing you are refering to following XML tags:

ServiceIdentification
ServiceProvider
OperationsMetadata

Those tags are read from owsformat.js (a previous contribution of mine I made in anticipation of WMTS format). That format was designed following philosophy explained in my previous comment!. I am perfectly OK for refactoring that format as well but it will induce backward compatibility break (since owsformat was released on 3.0.0).

That being said, I am not sure that owsformat is widely used, so the compatibility break won't make too much damage. Anyhow, it is not my place to make that decision. What do you think?

elemoine · 2015-01-19T07:38:43Z

src/ol/format/wmtscapabilitiesformat.js

+
+/**
+ * @classdesc
+ * Format for reading WMTS capabilities data


Point missing at end of sentence.

elemoine · 2015-01-19T07:47:37Z

src/ol/format/wmtscapabilitiesformat.js

+    goog.object.set(resource, 'template', template);
+  }
+  if (goog.isDef(resourceType)) {
+    goog.object.set(resource, 'resourceType', resourceType);


Use bracket notation instead.

elemoine · 2015-01-19T07:50:51Z

I've added a couple of new, pretty minor, comments. I'll merge this when they are addressed. Thanks for your effort @htulipe. And thanks @fgravin for reviewing this again.

sarametz · 2015-01-19T22:37:52Z

I've updated PR #3142 with changes that address the comments made by @elemoine. The difference between the two branches can be seen here. Please have a look if you have time.

sarametz · 2015-01-21T00:47:07Z

I found a bug while testing #3142 that also affects this PR. When using the built ol.js rather than ol-debug.js, the gets within operationsMetadata had their constraints variable renamed. This was due to dot notation being used in ol.format.OWS.readConstraint_

I have fixed this in #3142 with commit df5c799

htulipe · 2015-01-25T18:15:26Z

Okay I updated the PR with @elemoine comments. I did not made any modification regarding your comment @fgravin (for me it's outside of this PR scope, please see my comment about that).

elemoine · 2015-02-02T17:02:22Z

I am a bit lost here. Which branch should be reviewed? @htulipe's or @sarametz's? I'd like to review the most up-to-date branch, and I don't want to review "the ability to create WMTS source from capabilities" and "WMTS GetFeatureInfo" for now. This should be addressed with separate PRs. Thanks.

htulipe · 2015-02-02T17:15:04Z

This PR is independent of any other PR in the repo and was forked directly from master. I can not talk for @sarametz but I think her work needs my PR to be approved and merge first. Hence, if your question is what PR is the first in the stack, it is this one.

htulipe · 2015-02-02T17:16:41Z

As a reminder, I integrated your comments but I did not integrated last @fgravin comments regarding OWS format. I am still waiting to hear from him.

sarametz · 2015-02-02T19:30:30Z

My branch is "the ability to create WMTS source from capabilities" and requires @htulipe's changes in this PR so this should be merged first.
However, there is a bug in this PR that has been addressed in my branch (see #3026 (comment) above)

htulipe · 2015-02-02T19:49:57Z

If there is a bug in OWS format it must be fixed but it is outside of the scope of this PR. This PR can be merged and then another one can focus on refactoring/fixing OWS format.

fgravin · 2015-02-03T08:15:20Z

@htulipe could you provide more information about your contribution on OWSFormat ? What this format is used for ? Is it marked as stable etc.. ?

htulipe · 2015-02-03T08:48:40Z

The PR is #1863 You can find extended information on comments and commit message. Here is a sum up:

In OL2, WMTS GetCap was read in one module mixing OWS tags and WMTS tags. Since OWS tags are not only related to WMTS documents (other standard might use it), it make sense to have 2 distinct parsers.

With that consideration, I wanted to make a ol.format.OWS module and then a WMTS parser that would rely on the former. The former is in #1863 and the latter is in this PR.

Regarding the implementation details, I relied on OL2 unit tests. Hence the returned objects look just the same as in OL2. While you made a #3026 (comment), about this, the OWS format was merged as is.

Please let me know if you need other details.

fgravin · 2015-02-03T09:00:20Z

Ok it's clear thanks. Is OWSFormat used in other formats than WTMS right now ?
I think it makes sense to follow same philosophy as other formats in ol3, and also get quite same structure object like the WMS reader.
So I think you should change object property names to match XML tags, case sensitive at least.

htulipe · 2015-02-03T09:08:28Z

Right now it is not used anywhere else. To be confirmed but I think that OWS format was not marked @stable in previous releases, hence there would be no backward compatibility break. I will do the refactor at noon.

This commit fixe openlayers#2721. It still lacks the possibility to create layer from the GetCap document. It also lacks the reading of TMS limits.

htulipe · 2015-02-03T13:59:59Z

Ok I updated the PR with modification to OWS format. Regarding the Get attribute it is an array because the XSD specifies that HTTP tag is

<choice maxOccurs="unbounded">

Hence, it is possible to have several Get attributes.

Ref.: http://schemas.opengis.net/ows/1.1.0/owsOperationsMetadata.xsd

fgravin · 2015-02-04T12:31:36Z

Ok thanks, looks good to me.

elemoine · 2015-02-04T12:32:29Z

Looks good to me too. Thanks for your continued effort on this. That's great.

Add support of reading WMTS Get Cap document

htulipe · 2015-02-04T16:42:29Z

Woo, a day to be remembered! Thanks guy for your reviews.

htulipe mentioned this pull request Dec 8, 2014

WMTSCapabilities in 3.0 final #2721

Closed

elemoine reviewed Dec 9, 2014
View reviewed changes

fgravin reviewed Dec 10, 2014
View reviewed changes

htulipe force-pushed the wmts-getcap branch from 2646545 to 4f991f2 Compare December 13, 2014 15:42

tschaub modified the milestone: v3.2.0 Dec 22, 2014

sarametz mentioned this pull request Jan 16, 2015

WMTS Get Cap document with updated WMTS.optionsFromCapabilities function #3142

Merged

sarametz mentioned this pull request Jan 19, 2015

Wmts getFeatureInfo with REST functionality #3150

Closed

elemoine reviewed Jan 19, 2015
View reviewed changes

sarametz added a commit to sarametz/ol3 that referenced this pull request Jan 19, 2015

Refactored wmtscapabilities after review on openlayers#3026

b0f4355

htulipe force-pushed the wmts-getcap branch 2 times, most recently from fe09eeb to 2708926 Compare January 25, 2015 17:51

htulipe force-pushed the wmts-getcap branch from 2708926 to 24be5ee Compare February 3, 2015 13:44

Add support of WMTS Get Cap read

87644a1

This commit fixe openlayers#2721. It still lacks the possibility to create layer from the GetCap document. It also lacks the reading of TMS limits.

htulipe force-pushed the wmts-getcap branch from 24be5ee to 87644a1 Compare February 3, 2015 13:46

elemoine pushed a commit that referenced this pull request Feb 4, 2015

Merge pull request #3026 from htulipe/wmts-getcap

2d3e5d2

Add support of reading WMTS Get Cap document

elemoine merged commit 2d3e5d2 into openlayers:master Feb 4, 2015

htulipe deleted the wmts-getcap branch February 4, 2015 16:42

davidoesch mentioned this pull request Mar 4, 2015

Add WMTS Support in Import geoadmin/mf-geoadmin3#1654

Closed

Uh oh!

Add support of reading WMTS Get Cap document #3026

Add support of reading WMTS Get Cap document #3026

Uh oh!

Conversation

htulipe commented Dec 8, 2014

Uh oh!

elemoine commented Dec 9, 2014

Uh oh!

fgravin commented Dec 9, 2014

Uh oh!

fgravin commented Dec 9, 2014

Uh oh!

htulipe commented Dec 9, 2014

Uh oh!

elemoine Dec 9, 2014

Choose a reason for hiding this comment

Uh oh!

elemoine commented Dec 9, 2014

Uh oh!

fgravin Dec 10, 2014

Choose a reason for hiding this comment

Uh oh!

htulipe Dec 10, 2014

Choose a reason for hiding this comment

Uh oh!

fgravin commented Dec 10, 2014

Uh oh!

htulipe commented Dec 10, 2014

Uh oh!

htulipe commented Dec 13, 2014

Uh oh!

fgravin commented Dec 16, 2014

Uh oh!

fgravin commented Dec 16, 2014

Uh oh!

htulipe commented Dec 16, 2014

Uh oh!

htulipe commented Jan 12, 2015

Uh oh!

fgravin commented Jan 13, 2015

Uh oh!

sarametz commented Jan 16, 2015

Uh oh!

fgravin commented Jan 16, 2015

Uh oh!

htulipe commented Jan 16, 2015

Uh oh!

elemoine Jan 19, 2015

Choose a reason for hiding this comment

Uh oh!

elemoine Jan 19, 2015

Choose a reason for hiding this comment

Uh oh!

elemoine commented Jan 19, 2015

Uh oh!

sarametz commented Jan 19, 2015

Uh oh!

sarametz commented Jan 21, 2015

Uh oh!

htulipe commented Jan 25, 2015

Uh oh!

elemoine commented Feb 2, 2015

Uh oh!

htulipe commented Feb 2, 2015

Uh oh!

htulipe commented Feb 2, 2015

Uh oh!

sarametz commented Feb 2, 2015

Uh oh!

htulipe commented Feb 2, 2015

Uh oh!

fgravin commented Feb 3, 2015

Uh oh!

htulipe commented Feb 3, 2015

Uh oh!

fgravin commented Feb 3, 2015

Uh oh!

htulipe commented Feb 3, 2015

Uh oh!