Improve geocoder #4723

erikmaarten · 2016-12-06T21:16:23Z

Adresses parts of #1262.

Support for custom geocoders
Support for automatic suggestions

Something I haven't quite decided on is whether there should be different methods for getting geocoder search suggestions and the actual geocoding. This would be necessary if a certain service has different API endpoints for this, or if they otherwise need to be handled differently. It would probably be better to keep this separate.

A custom geocoder is expected to look like:

{
    geocode: function(query) {...},
    getSuggestions: function(query) {...} // Currently not used -- should we have this?
}

The default geocoder in Cesium remains largely unchanged in this PR.

I've marked the GeocoderViewModel properties key and url for deprecation. I think these don't make sense to expose as public properties since they are implementation details (and may not exist) of the custom geocoder provided.

pjcozzi · 2016-12-07T00:57:13Z

I've marked the GeocoderViewModel properties key and url for deprecation.

Please submit an issue for this: https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#deprecation-and-breaking-changes

pjcozzi · 2016-12-07T01:00:34Z

Apps/Sandcastle/gallery/Custom Geocoder.html

+                            bboxDegrees = resultObject.boundingbox;
+                            return {
+                                displayName: resultObject.display_name,
+                                bbox: {


bbox is part of the new public Cesium API, right? This is part of the "class" a geocode implementation returns.

It should following Cesium conventions:

Avoid abbreviations, probably call it rectangle

Use a Cesium type, probably Rectangle

Use radians. This will likely require most users to convert, e.g., using fromDegrees, but it will be consistent with the rest of the API.

Thanks for the review @pjcozzi. Yes, bbox will be part of the public API, I'll have this fixed.

hpinkos · 2016-12-07T05:46:18Z

@erikmaarten this is a great start, but I think there are a few changes we can make here to improve the API.

Create an GeocoderService interface that describes the properties and functions needed by a custom geocoding service (Feel free to use a better name if you can think of one. Unfortunately we're already using Geocoder for the widget name.) For example, here is the interface we created to describe an ImageryProvider: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/ImageryProvider.js
Create BingMapsGeocoderService to abstract the bing maps geocoder functionality out of GeocoderViewModel
Make GeocoderViewModel take an optional GeocoderService parameter. If none is specified, use new BingMapsGeocoderService()
Possibly create OpenStreetMapGeocoderService using the functionality from your sandcastle example, then update the sandcastle example to pretty much be viewer.geocoder.geocoderService = new OpenStreetMapGeocoderService()

This will make it really easy to plug in different services in the future. For example, I know people on the forum were asking how to use the Mapbox geocoder as well. We could eventually add MapboxGeocoderService to make it easy for users to plug that in instead. This would also make it brainless to switch the default in case we ever decide to stop using Bing.

Also, if you could, it would be better to make the custom geocoder changes and the autocomplete changes in separate PRs so we can review each individually

…ults that destinations of type Rectangle or Cartesian3

erikmaarten · 2016-12-08T00:45:56Z

@hpinkos Ready for review.

hpinkos · 2016-12-08T17:19:21Z

Source/Core/OpenStreetMapNominatimGeocoderService.js

+                        )
+                    };
+                });
+            })


Missing semicolon

hpinkos · 2016-12-08T17:20:45Z

CHANGES.md

-
+* Deprecated
+    * The properties `url` and `key` will be removed from `GeocoderViewModel` in 1.30. This properties will be available on geocoder services that support them, like `BingMapsGeocoderService`;
+* Added support for custom geocoder services [4723](https://github.com/AnalyticalGraphicsInc/cesium/pull/4723).


Add a # before 4723

hpinkos · 2016-12-08T17:21:09Z

Source/Core/BingMapsGeocoderService.js

+    /**
+     * Provides geocoding through Bing Maps.
+     * @alias BingMapsGeocoderService
+     *


Add @params doc

hpinkos · 2016-12-08T17:21:52Z

Source/Core/BingMapsGeocoderService.js

+        options = defaultValue(options, defaultValue.EMPTY_OBJECT);
+        this._canceled = false;
+
+        this._url = 'https://dev.virtualearth.net/REST/v1/Locations';


Should this be defaultValue(options.url, url)?

I'm not sure it should be possible to supply a different URL, because then it's not really the same geocoder service anymore

hpinkos · 2016-12-08T17:24:45Z

Source/Core/GeocoderService.js

+         *
+         */
+        displayName : {
+            get : DeveloperError.throwInstantiationError


Should this be here? It doesn't look like BingMapsGeocoderService uses this property

This isn't actually being used at the moment, so perhaps I should remove it, but I thought it would be useful to have a name for each service for the case where somebody wanted to show to end users which results come from which geocoders. But that may be a premature consideration since it's not actually used... what do you think, should I just get rid of it?

If this isn't being used, you should get rid of it. Thanks

hpinkos · 2016-12-16T22:33:27Z

@mramato ready for another review. I fix the problem you found, and also re-did the way the GeocoderViewModel was handling the promises for multiple geocoders. Instead of running geocoderService.geocode on each geocoder service, it treats it like a priority queue. It keeps asking down the chain until one of the geocoders returns a successful result.

@erikmaarten you should review the changes I made in f10c75f with regards to promises to see how I changed them together. Your use of when.all and creating an extra when.defer wasn't necessary, especially since in your last then you were just taking the first result returned.

erikmaarten · 2016-12-17T11:12:24Z

Thanks @mramato for reviewing.

@hpinkos that's a nice refactor, but the functionality is not what I intended, if I'm reading the code correctly. It's good to avoid superfluous requests, but now users will be forced to wait for requests to start and fail in turn. If a more specifically scoped geocoder (like a geocoder with radio station codes as mentioned in the forums) is higher in priority, which I think would be normal, than a general geocoder, then users of that app would often have to wait for one extra network request (slow in the context of users interacting with the geocoder search bar), or more if more geocoders are used.

So while it makes technical sense to avoid requests that aren't necessary, doing the extra network requests upfront makes for better UX. A middle ground might also be sensible if we anticipate large numbers of geocoders in the same app: chaining the requests, but with chunks of five requests each time or something like that.

pjcozzi · 2016-12-20T15:00:30Z

Is this ready? If we want to ship this in 1.29, we should merge it this week.

erikmaarten · 2016-12-20T19:33:33Z

@mramato had some comments on the interface design that I've replied to, and @hpinkos asked him for a follow-up review of the last changes, but since he's out for the holidays we won't be able to get any updates from him there.

@hpinkos have you seen my comments? Would like to hear what you think. Also, since my comments don't affect the interface, we could just settle with the current functionality and see how people actually use and then adapt to that.

hpinkos · 2016-12-21T17:13:06Z

It's good to avoid superfluous requests, but now users will be forced to wait for requests to start and fail in turn

I get what you're saying here. I did make it a bit more synchronous by waiting for the previous call to complete before making the next one. I was thinking the first one would often be the geocode that we ended up using, so I didn't see the point of making calls to the other geocoders and waiting for them all to either resolve or fail.

I'm really not sure which is best, so I'll wait to hear what @mramato thinks before changing things more

pjcozzi · 2017-01-04T14:08:28Z

What is the plan with this pull request?

hpinkos · 2017-01-04T16:02:59Z

@mramato can you take a look at this?

mramato · 2017-01-04T16:10:49Z

On my list for later today.

mramato · 2017-01-04T21:50:56Z

Start the Cesium Viewer
Click on the magnifying glass (not the text box, just the icon part)
Start typing and autocomplete doesn't trigger.

If you click inside the text field at any point, autocomplete starts to work as expected.

mramato · 2017-01-04T21:53:17Z

Source/Core/CartographicGeocoderService.js

+        }
+        //>>includeEnd('debug');
+
+        try {


Sorry if this has been covered, but what part of this try block can cause an exception?

mramato · 2017-01-04T22:09:21Z

Source/Widgets/Geocoder/GeocoderViewModel.js


        var that = this;
+
+        this._suggestionsVisible = knockout.pureComputed(function () {
+            var suggestions = knockout.getObservable(that, '_suggestions');


My knockout-es5 is a little rusty, but is this knockout.getObservable needed here? I think you can just use that._suggestions directly and everything will work. CC @shunter

I wasn't sure it would trigger the pureComputed properly otherwise, but I didn't test it to see

mramato · 2017-01-04T22:14:11Z

Source/Widgets/Geocoder/GeocoderViewModel.js

+         */
+        this.autoComplete = defaultValue(options.autocomplete, true);
+
+        this._focusTextbox = false;


This shouldn't be private if it's used in the bindings.

Every other thing we're binding to below is privage

mramato · 2017-01-04T22:14:31Z

Source/Widgets/Geocoder/GeocoderViewModel.js

        }
    });

+    GeocoderViewModel.prototype.destroy = function() {


Add documentation.

mramato · 2017-01-04T22:31:25Z

Source/Widgets/Geocoder/GeocoderViewModel.js

+        viewModel._geocodePromise = promise;
+        promise
+            .then(function (result) {
+                if (promise.cancel || promise !== viewModel._geocodePromise) {


I don't think the second condition here can ever be true, since it's impossible to start a second geocode while a current geocode is in process.

That would be for if the geocode was triggered again before the first geocode resolved

Maybe I'm missing something, but how is that possible with the code as currently written?

True. I was thinking it would happen if you hit the geocode button a few times, but in that case promise.cancel would be true. I just kept this here because that's how it was working before, but I agree that it doesn't seem necessary.

mramato · 2017-01-04T22:34:01Z

Source/Widgets/Geocoder/GeocoderViewModel.js

+                    nextPromise.otherwise(function (err) {
+                        return {state: 'rejected', reason: err};
+                    });
+                } else if (defined(nextPromise.catch)) {


I assume you are doing this to try and handle multiple promise implementations, but I think we should just simplify the code and always assume the promise was created with our version of when. Otherwise bad things can happen anyway, for example Bluebird promises have a cancel function, which would end up making your code think that cancel has been called on successful geocodes because you glob your own cancel onto the promise as a boolean. (hopefully that make sense).

We we upgrade to bluebird, these problems should go away, but we should just assume everyone is using Cesium's own when library for the time being.

mramato · 2017-01-04T22:34:57Z

CHANGES.md

@@ -2,7 +2,13 @@ Change Log
 ==========

 ### 1.29 - 2017-01-02
-
+* Deprecated


Merge in master and move this to 1.30.

mramato · 2017-01-04T22:35:43Z

Thanks again @erikmaarten, I don't want to bikeshed this too much since it clearly works well (except in the couple cases I mentioned). Once my last round of comments are addressed we can merge this up to master.

hpinkos · 2017-01-04T22:37:18Z

@erikmaarten I'll take care of cleaning this up

# Conflicts: # CHANGES.md

hpinkos · 2017-01-04T23:07:21Z

@mramato ready

erikmaarten · 2017-01-05T09:42:10Z

Thanks @hpinkos for taking care of this.

@mramato do you have any opinion on #4723 (comment)?

I updated the release numbers here to deprecate those properties in 1.30 and remove them in 1.31 since we didn't make it in time for the 1.29 release. I also updated the deprecation issue.

mramato · 2017-01-05T15:39:09Z

Thanks @hpinkos for taking care of this.

@mramato do you have any opinion on #4723 (comment)?

I updated the release numbers here to deprecate those properties in 1.30 and remove them in 1.31 since we didn't make it in time for the 1.29 release. I also updated the deprecation issue.

Ultimately, I think the use of multiple geocoders will be uncommon, so the point is probably moot. However, I agree that having a slow geocoder first could end up slowing down the entire operation in that use case, so firing multiple requests and letting the first one that returns "win" may be a good idea.

We should probably also make the limit of 5 an option in the view model so that users can allow more returns if desired.

I'm going to merge this as-is for now since it's in good shape, but if you (@erikmaarten) want to open another PR right away to add back the gecode "race" then we'll merge that too (and it will be easier to review since it will be a smaller change).

Thanks again!

erikmaarten added 10 commits December 5, 2016 15:55

Support custom geocoder

89fcd98

Add example of how to create make a custom geocoder

6bfbc9d

Add support for geocoder suggestions

60db026

Enable keyboard navigation for geocoder suggestions

fb19346

Hide/show geocoder suggestions

9f84da3

Adjust suggestions container scroll when selecting with arrow keys

bc8ed9a

Use textInput binding instead of onKeyUp for updating search input value

da2b6cb

Clean up tests

202eee3

Clean up and deprecate unused properties

d2ef80c

Fix failing tests

7680bdc

pjcozzi reviewed Dec 7, 2016

View reviewed changes

erikmaarten added 6 commits December 7, 2016 10:52

Clean up GeocoderService API

8d04944

Add geocoder services

9124a6b

Add support for multiple geocoders

ba11113

Update tests

48111c6

Use promises instead of callbacks in geocoder service API

3d4071f

Update geocoder interface to reflect that geocoders can send back res…

b94d7be

…ults that destinations of type Rectangle or Cartesian3

erikmaarten mentioned this pull request Dec 7, 2016

Remove deprecated properties in GeocoderViewModel in 1.31 #4730

Closed

erikmaarten added 2 commits December 7, 2016 19:35

Merge in master

9152955

Add deprecation/change notes

4e58f4b

erikmaarten mentioned this pull request Dec 8, 2016

Geocoder lon/lat bug #4713

Closed

Reject promises when geocoding fails

a68cbc5

hpinkos reviewed Dec 8, 2016

View reviewed changes

Source/Core/OpenStreetMapNominatimGeocoderService.js

)

};

});

})

Copy link

Contributor

hpinkos Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon

hpinkos reviewed Dec 8, 2016

View reviewed changes

update sandcastle example

ff5b0ea

pjcozzi added the priority - high label Jan 2, 2017

mramato reviewed Jan 4, 2017

View reviewed changes

Source/Widgets/Geocoder/GeocoderViewModel.js

}

});

GeocoderViewModel.prototype.destroy = function() {

Copy link

Member

mramato Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add documentation.

mramato reviewed Jan 4, 2017

View reviewed changes

CHANGES.md

@@ -2,7 +2,13 @@ Change Log

==========

### 1.29 - 2017-01-02

* Deprecated

Copy link

Member

mramato Jan 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge in master and move this to 1.30.

hpinkos added 3 commits January 4, 2017 17:43

cleanup

7d52c00

Merge branch 'master' into improve-geocoder

1d34c97

# Conflicts: # CHANGES.md

fix search dropdown

97f10a7

Update release numbers since this was delayed

7c8f5f4

mramato merged commit 4b612dc into CesiumGS:master Jan 5, 2017

erikmaarten deleted the improve-geocoder branch January 5, 2017 16:31

lilleyse mentioned this pull request Mar 1, 2017

Removed url and key from GeocoderViewModel #5056

Merged

Improve geocoder #4723

Improve geocoder #4723

Conversation

erikmaarten commented Dec 6, 2016 • edited

pjcozzi commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Dec 7, 2016

erikmaarten commented Dec 8, 2016

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

hpinkos commented Dec 16, 2016

erikmaarten commented Dec 17, 2016

pjcozzi commented Dec 20, 2016

erikmaarten commented Dec 20, 2016

hpinkos commented Dec 21, 2016

pjcozzi commented Jan 4, 2017

hpinkos commented Jan 4, 2017

mramato commented Jan 4, 2017

mramato commented Jan 4, 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

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

mramato commented Jan 4, 2017

hpinkos commented Jan 4, 2017

hpinkos commented Jan 4, 2017

erikmaarten commented Jan 5, 2017 • edited

mramato commented Jan 5, 2017

erikmaarten commented Dec 6, 2016 •

edited

erikmaarten commented Jan 5, 2017 •

edited