Allow custom parameters in the template url of an UrlTemplateImageryP… #5696

oterral · 2017-07-31T14:56:48Z

…rovider.

This is a proposal to add flexibility in the UrlTemplateImageryProvider.

This PR allows to add a custom tags in the template url.

For example with a {Time} tag in the template:

var positron = new Cesium.UrlTemplateImageryProvider({
    url : 'http://mywmt/{Time}/{x}/{y}/{z}.{format}`',
    customTags : {
        Time: function() {
            return '19381231'; 
        }
    }
});

hpinkos · 2017-07-31T15:17:29Z

Thanks @oterral! This is a great idea. Can you add unit tests?
@ggetz do you want to review this?

ggetz

Thanks @oterral! Please add unit tests and add this to CHANGES.md.

ggetz · 2017-07-31T15:24:54Z

Source/Scene/UrlTemplateImageryProvider.js

@@ -132,6 +132,13 @@ define([
     *        source does not support picking features or if you don't want this provider's features to be pickable. Note
     *        that this can be dynamically overridden by modifying the {@link UriTemplateImageryProvider#enablePickFeatures}
     *        property.
+     * @param {Object} [options.customTags] Allow to replace custom keywords in the URL template. The object must have strings as keys and functions as values.
+              For example:


See the Documentation guide for examples so this is formatted nicely in the reference documentation.

ggetz · 2017-07-31T15:39:28Z

@oterral You also have some eslint errors that need fixing.

oterral · 2017-08-02T06:34:25Z

Thx for the review. I'm on it.

oterral · 2017-08-02T11:35:24Z

Should be good now.

ggetz

Thanks @oterral, a few more comments.

ggetz · 2017-08-02T13:46:38Z

CHANGES.md

@@ -1,6 +1,10 @@
 Change Log
 ==========

+### 1.37 - 2017


This will be 2017-09-01

ggetz · 2017-08-02T13:49:00Z

Specs/Scene/UrlTemplateImageryProviderSpec.js

+
+        return pollToPromise(function() {
+            return provider.ready;
+        }).then(function() {console.error('spyon');


Remove the console.error

shunter · 2017-08-02T14:05:23Z

Conventionally in templating engines, the curly braces are not specified as part of the variable name. This might be easier to use if it were like:

new Cesium.UrlTemplateImageryProvider({
    url : 'http://mywmt/{Time}/{x}/{y}/{z}.{format}`',
    customTags : {
        Time: function() {
            return '19381231'; 
        }
    }
});

mramato · 2017-08-02T14:14:04Z

I agree with @shunter.

Also, this needs to be carefully documented because unless the result of the supplied function is constant in regards to the rest of url, users might receive unexpected results. For example, if the result of Time changes, none of the previously fetched tiles will get automatically refreshed. Of course this might be able to tie in with @tfili's recent changes regarding dynamic images, perhaps he could chime in here.

oterral · 2017-08-03T07:10:21Z

@shunter good idea .

@mramato of course it will be better if the layer could refresh itself on each change. If someone can give me guidance. I can do it in another PR.

hpinkos · 2017-09-15T20:44:22Z

@oterral there is a merge conflict, please merge in master when you have a chance. Thanks!

@mramato @shunter are there other chances that need to be made in this PR?

…rovider

mramato · 2017-09-18T14:05:51Z

Source/Scene/UrlTemplateImageryProvider.js

@@ -568,6 +578,16 @@ define([
            }
            that._credit = credit;

+            if (properties.customTags) {


This should be if(defined(properties.customTags))

mramato · 2017-09-18T14:06:12Z

Source/Scene/UrlTemplateImageryProvider.js

+                for (var tag in properties.customTags) {
+                    if (properties.customTags.hasOwnProperty(tag)) {
+                        var targetTag = '{' + tag + '}';
+                        tags[targetTag] = properties.customTags[tag]; //eslint-disable-line no-use-before-define


I'm surprised these eslint directives are needed. What does it thing is being used before define?

Here was the messages:

/home/travis/build/AnalyticalGraphicsInc/cesium/Source/Scene/UrlTemplateImageryProvider.js 585:25 error 'tags' was used before it was defined no-use-before-define 586:25 error 'pickFeaturesTags' was used before it was defined no-use-before-define

Let's remove the directives and simple move tags and pickFeaturesTags to the top of the file.

mramato · 2017-09-18T14:10:44Z

@mramato of course it will be better if the layer could refresh itself on each change. If someone can give me guidance. I can do it in another PR.

I don't think this is easily possible with the current implementation because there's no way for us to know when a property has changed. We would need some sort of higher level construct that notifies us when a function would have a different value for the same input. @tfili might have some ideas here, but I think they could be added in a backwards compatible way in the future. So I have no objections to this PR going in largely as is if @hpinkos is happy with it.

oterral · 2017-09-19T15:25:18Z

@mramato use of defined() added

mramato · 2017-09-19T15:33:33Z

Source/Scene/UrlTemplateImageryProvider.js

@@ -568,6 +578,16 @@ define([
            }
            that._credit = credit;

+            if (defined(properties.customTags)) {


As a general rule, we try to keep property access to a minimum, rather than have properties.customTags multiple times, extract a var customTags = properties.customTags variable and use it throughout.

mramato · 2017-09-19T15:36:58Z

Source/Scene/UrlTemplateImageryProvider.js

+                for (var tag in properties.customTags) {
+                    if (properties.customTags.hasOwnProperty(tag)) {
+                        var targetTag = '{' + tag + '}';
+                        tags[targetTag] = properties.customTags[tag]; //eslint-disable-line no-use-before-define


I just realized there is a major issue here. Since tags and pickFeaturesTags used to be readonly, the same set is shared across multiple instances. However, now we are modifying tags and pickFeaturesTags which means that the modified version is used by all instances. Shouldn't these objects be per-instance now?

The tags and pickFeaturesTags are the same for all objects.
Maybe I can just append the customTags values to tags values in a new object and pass this new object to urlTemplateToParts.

hpinkos · 2017-10-04T20:35:57Z

@oterral is this ready? Next time make a comment after you make a commit. Otherwise we don't get notified that a pull request is ready for another look

hpinkos · 2017-10-20T20:08:44Z

Thanks for all the work you put into this @oterral! It looks good to me.

oterral · 2017-11-07T13:14:13Z

Thx for the rebase @hpinkos

ggetz requested changes Jul 31, 2017

View reviewed changes

oterral force-pushed the time branch 6 times, most recently from 54f6997 to 7092a04 Compare August 2, 2017 11:35

ggetz requested changes Aug 2, 2017

View reviewed changes

oterral force-pushed the time branch 2 times, most recently from 78209b8 to 64ff4fa Compare August 3, 2017 07:08

oterral force-pushed the time branch from 2322f47 to c5ea382 Compare September 18, 2017 07:09

Allow custom parameters in the template url of an UrlTemplateImageryP…

e726b0d

…rovider

mramato reviewed Sep 18, 2017

View reviewed changes

oterral force-pushed the time branch from c5ea382 to cdd987b Compare September 19, 2017 15:02

Remove use of curly braces

8566cf2

oterral force-pushed the time branch from cdd987b to 8566cf2 Compare September 19, 2017 15:24

mramato reviewed Sep 19, 2017

View reviewed changes

Don't override tags and pickFeaturesTags objects

26ec8de

oterral force-pushed the time branch from 8fb0660 to 26ec8de Compare September 20, 2017 09:06

oterral mentioned this pull request Sep 20, 2017

3d add zeitreise to 3d geoadmin/mf-geoadmin3#3887

Closed

hpinkos added 2 commits October 20, 2017 15:59

Merge remote-tracking branch 'upstream/master' into time

2ed8e9f

fix changes

d6abaf1

hpinkos merged commit 5d499c5 into CesiumGS:master Oct 20, 2017

oterral deleted the time branch November 7, 2017 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom parameters in the template url of an UrlTemplateImageryP… #5696

Allow custom parameters in the template url of an UrlTemplateImageryP… #5696

oterral commented Jul 31, 2017 •

edited

Loading

hpinkos commented Jul 31, 2017

ggetz left a comment

ggetz Jul 31, 2017

ggetz commented Jul 31, 2017

oterral commented Aug 2, 2017

oterral commented Aug 2, 2017

ggetz left a comment

ggetz Aug 2, 2017

ggetz Aug 2, 2017

oterral Aug 3, 2017

shunter commented Aug 2, 2017 •

edited

Loading

mramato commented Aug 2, 2017

oterral commented Aug 3, 2017

hpinkos commented Sep 15, 2017

mramato Sep 18, 2017

mramato Sep 18, 2017

oterral Sep 19, 2017

mramato Sep 19, 2017

mramato commented Sep 18, 2017

oterral commented Sep 19, 2017

mramato Sep 19, 2017

oterral Sep 20, 2017

mramato Sep 19, 2017

oterral Sep 20, 2017

hpinkos commented Oct 4, 2017

hpinkos commented Oct 20, 2017

oterral commented Nov 7, 2017

Allow custom parameters in the template url of an UrlTemplateImageryP… #5696

Allow custom parameters in the template url of an UrlTemplateImageryP… #5696

Conversation

oterral commented Jul 31, 2017 • edited Loading

hpinkos commented Jul 31, 2017

ggetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Jul 31, 2017

oterral commented Aug 2, 2017

oterral commented Aug 2, 2017

ggetz 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

shunter commented Aug 2, 2017 • edited Loading

mramato commented Aug 2, 2017

oterral commented Aug 3, 2017

hpinkos commented Sep 15, 2017

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

mramato commented Sep 18, 2017

oterral commented Sep 19, 2017

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

hpinkos commented Oct 4, 2017

hpinkos commented Oct 20, 2017

oterral commented Nov 7, 2017

oterral commented Jul 31, 2017 •

edited

Loading

shunter commented Aug 2, 2017 •

edited

Loading