Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve geocoder #4723

Merged
merged 35 commits into from
Jan 5, 2017
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
89fcd98
Support custom geocoder
erikmaarten Dec 5, 2016
6bfbc9d
Add example of how to create make a custom geocoder
erikmaarten Dec 5, 2016
60db026
Add support for geocoder suggestions
erikmaarten Dec 6, 2016
fb19346
Enable keyboard navigation for geocoder suggestions
erikmaarten Dec 6, 2016
9f84da3
Hide/show geocoder suggestions
erikmaarten Dec 6, 2016
bc8ed9a
Adjust suggestions container scroll when selecting with arrow keys
erikmaarten Dec 6, 2016
da2b6cb
Use textInput binding instead of onKeyUp for updating search input value
erikmaarten Dec 6, 2016
202eee3
Clean up tests
erikmaarten Dec 6, 2016
d2ef80c
Clean up and deprecate unused properties
erikmaarten Dec 6, 2016
7680bdc
Fix failing tests
erikmaarten Dec 6, 2016
8d04944
Clean up GeocoderService API
erikmaarten Dec 7, 2016
9124a6b
Add geocoder services
erikmaarten Dec 7, 2016
ba11113
Add support for multiple geocoders
erikmaarten Dec 7, 2016
48111c6
Update tests
erikmaarten Dec 7, 2016
3d4071f
Use promises instead of callbacks in geocoder service API
erikmaarten Dec 7, 2016
b94d7be
Update geocoder interface to reflect that geocoders can send back res…
erikmaarten Dec 7, 2016
9152955
Merge in master
erikmaarten Dec 8, 2016
4e58f4b
Add deprecation/change notes
erikmaarten Dec 8, 2016
a68cbc5
Reject promises when geocoding fails
erikmaarten Dec 8, 2016
93876bc
Address review comments
erikmaarten Dec 8, 2016
2849a18
Fix CHANGES.md
pjcozzi Dec 8, 2016
1fc73d4
Rename LongLatGeocoderService to CartographicGeocoderService
erikmaarten Dec 13, 2016
b06cfa1
Update doc for cartographic geocoder
erikmaarten Dec 13, 2016
e99f838
Remove Nominatim geocoder from core and show only as a Sandcastle exa…
erikmaarten Dec 13, 2016
0846a89
Add geocoder-related classes, interface and example to changes list
erikmaarten Dec 13, 2016
b72ae90
Clean up documentation
erikmaarten Dec 13, 2016
94eecbf
knockout es5 makes everything difficult
Dec 13, 2016
1b66af9
fix specs
Dec 13, 2016
2659c7f
Remove unused property displayName
erikmaarten Dec 14, 2016
f10c75f
chain geocoder promises
Dec 16, 2016
ff5b0ea
update sandcastle example
Dec 19, 2016
7d52c00
cleanup
Jan 4, 2017
1d34c97
Merge branch 'master' into improve-geocoder
Jan 4, 2017
97f10a7
fix search dropdown
Jan 4, 2017
7c8f5f4
Update release numbers since this was delayed
erikmaarten Jan 5, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions Apps/Sandcastle/gallery/Custom Geocoder.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
<meta name="description" content="Cluster labels, billboards and points.">
<meta name="cesium-sandcastle-labels" content="Tutorials,Showcases">
<title>Cesium Demo</title>
<script type="text/javascript" src="../Sandcastle-header.js"></script>
<script type="text/javascript" src="../../../ThirdParty/requirejs-2.1.20/require.js"></script>
<script type="text/javascript">
require.config({
baseUrl : '../../../Source',
waitSeconds : 60
});
</script>
</head>
<body class="sandcastle-loading" data-sandcastle-bucket="bucket-requirejs.html">
<style>
@import url(../templates/bucket.css);
#toolbar {
background: rgba(42, 42, 42, 0.8);
padding: 4px;
border-radius: 4px;
}
#toolbar input {
vertical-align: middle;
padding-top: 2px;
padding-bottom: 2px;
}
</style>
<div id="cesiumContainer" class="fullSize"></div>
<div id="loadingOverlay"><h1>Loading...</h1></div>
<script id="cesium_sandcastle_script">
function startup(Cesium) {
'use strict';
//Sandcastle_Begin

var viewer = new Cesium.Viewer('cesiumContainer', {
geocoder: new Cesium.OpenStreetMapNominatimGeocoderService()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure about including this in core Cesium? Are people asking for it? Is it just increasing the API surface area? Would it be better suited as an example of a custom geocoder in this Sandcastle example as this name states?

});

//Sandcastle_End
Sandcastle.finishedLoading();
}
if (typeof Cesium !== "undefined") {
startup(Cesium);
} else if (typeof require === "function") {
require(["Cesium"], startup);
}
</script>
</body>
</html>
4 changes: 3 additions & 1 deletion CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ Change Log
==========

### 1.29 - 2017-01-02

* Deprecated
Copy link
Contributor

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.

* 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).
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a # before 4723

* Added the ability to blend a `Model` with a color/translucency. Added `color`, `colorBlendMode`, and `colorBlendAmount` properties to `Model`, `ModelGraphics`, and CZML. Added `ColorBlendMode` enum. [#4547](https://github.com/AnalyticalGraphicsInc/cesium/pull/4547)
* Fixed tooltips for gallery thumbnails in Sandcastle [#4702](https://github.com/AnalyticalGraphicsInc/cesium/pull/4702)
* Fixed texture rotation for `RectangleGeometry` [#2737](https://github.com/AnalyticalGraphicsInc/cesium/issues/2737)
Expand Down
104 changes: 104 additions & 0 deletions Source/Core/BingMapsGeocoderService.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/*global define*/
define([
'./BingMapsApi',
'./defaultValue',
'./defineProperties',
'./loadJsonp',
'./Rectangle',
], function(
BingMapsApi,
defaultValue,
defineProperties,
loadJsonp,
Rectangle) {
'use strict';

var url = 'https://dev.virtualearth.net/REST/v1/Locations';

/**
* Provides geocoding through Bing Maps.
* @alias BingMapsGeocoderService
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Add @params doc

*/
function BingMapsGeocoderService(options) {
options = defaultValue(options, defaultValue.EMPTY_OBJECT);
this._canceled = false;

this._url = 'https://dev.virtualearth.net/REST/v1/Locations';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this._key = BingMapsApi.getKey(options.key);

this.autoComplete = defaultValue(options.autoComplete, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a Geocoder option instead of a widget-level option? It seems odd to make the decision at the low-level, non-UX part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if a particular GeocoderService can technically be used for autocomplete, an app developer may want to turn it off. If we have Bing and Nominatim, for example, the Nominatim terms of service might be violated when an app uses autocomplete due to the greater number of requests, while Bing's ToS are fine with that amount. Or if we have two geocoder services, but one's autocomplete results consistently better, we might want to turn off autocomplete for the other service to avoid polluting the autocomplete results list.

In the case of BingMapsGeocoderService autocomplete is turned off by default because the Bing Maps API is not designed to support it well.

So my intention was that the widget-level autoComplete property indicates whether the widget should enable autocomplete, and the geocoder services autoComplete property indicates whether each service supports it.

}

defineProperties(BingMapsGeocoderService.prototype, {
/**
* The URL endpoint for the Bing geocoder service
* @type {String}
Copy link
Contributor

Choose a reason for hiding this comment

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

This and below need to be marked @readonly

*/
url : {
get : function () {
return this._url;
}
},

/**
* The key for the Bing geocoder service
* @type {String}
*/
key : {
get : function () {
return this._key;
}
}
});

BingMapsGeocoderService.prototype.cancel = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation.

this._canceled = true;
};

/**
* @function
*
* @param {String} query The query to be sent to the geocoder service
* @returns {Promise<GeocoderResult[]>}
*/
BingMapsGeocoderService.prototype.geocode = function(query) {
this._canceled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

From an interface standpoint, this is kind of odd. If I do geocode twice for two different addresses and then call cancel, it cancels them both. Shouldn't the object returned from geocode have the ability to cancel, not the top-level interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

It probably makes more sense to pull the cancel functionality out of the geocode function and into the widget (since cancel doesn't really make sense for non-UX usage.


Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we don't check that query is defined and throw a DeveloperError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed it

var key = this.key;
var promise = loadJsonp(url, {
parameters : {
query : query,
key : key
},
callbackParameterName : 'jsonp'
});

var that = this;

return promise.then(function(result) {
if (that._canceled) {
return;
}
if (result.resourceSets.length === 0) {
return [];
}

var results = result.resourceSets[0].resources;

return results.map(function (resource) {
var bbox = resource.bbox;
var south = bbox[0];
var west = bbox[1];
var north = bbox[2];
var east = bbox[3];
return {
displayName: resource.name,
destination: Rectangle.fromDegrees(west, south, east, north)
};
});
});
};

return BingMapsGeocoderService;
});
55 changes: 55 additions & 0 deletions Source/Core/GeocoderService.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*global define*/
define([
'./defineProperties',
'./DeveloperError'
], function(
defineProperties,
DeveloperError) {
'use strict';

/**
* @typedef {Object} GeocoderResult
* @property {String} displayName The display name for a location
* @property {Rectangle|Cartesian3} destination The bounding box for a location
*/

/**
* Provides geocoding through an external service. This type describes an interface and
* is not intended to be used.
* @alias GeocoderService
* @constructor
*
* @see BingMapsGeocoderService
*/
function GeocoderService () {
/**
* Indicates whether this geocoding service is to be used for autocomplete.
*
* @type {boolean}
* @default false
*/
this.autoComplete = false;
}

defineProperties(GeocoderService.prototype, {
/**
* The name of this service to be displayed next to suggestions
* in case more than one geocoder is in use
* @type {String}
*
*/
displayName : {
get : DeveloperError.throwInstantiationError
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
});

/**
* @function
*
* @param {String} query The query to be sent to the geocoder service
* @returns {Promise<GeocoderResult[]>}
*/
GeocoderService.prototype.geocode = DeveloperError.throwInstantiationError;

return GeocoderService;
});
54 changes: 54 additions & 0 deletions Source/Core/LongLatGeocoderService.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*global define*/
define([
'./Cartesian3',
'./defaultValue',
'../ThirdParty/when'
], function(
Cartesian3,
defaultValue,
when) {
'use strict';

/**
* Provides geocoding through Bing Maps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Update doc.

* @alias LongLatGeocoderService
*
Copy link
Contributor

Choose a reason for hiding this comment

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

@params doc

Copy link
Contributor Author

@erikmaarten erikmaarten Dec 8, 2016

Choose a reason for hiding this comment

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

I realized options aren't actually used for this service, so I just removed them instead

*/
function LongLatGeocoderService(options) {
options = defaultValue(options, defaultValue.EMPTY_OBJECT);
this.autoComplete = false;
}

LongLatGeocoderService.prototype.cancel = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this function to the GeocoderService interface if it is required to be implemented

};

/**
* @function
*
* @param {String} query The query to be sent to the geocoder service
* @returns {Promise<GeocoderResult[]>}
*/
LongLatGeocoderService.prototype.geocode = function(query, callback) {
try {
var splitQuery = query.match(/[^\s,\n]+/g);
if ((splitQuery.length === 2) || (splitQuery.length === 3)) {
var longitude = +splitQuery[0];
var latitude = +splitQuery[1];
var height = (splitQuery.length === 3) ? +splitQuery[2] : 300.0;

if (!isNaN(longitude) && !isNaN(latitude) && !isNaN(height)) {
var result = {
displayName: query,
destination: Cartesian3.fromDegrees(longitude, latitude, height)
};
return when.resolve([result]);
}
}
return when.resolve([]);
} catch (e) {
when.reject(e);
}
};

return LongLatGeocoderService;
});
59 changes: 59 additions & 0 deletions Source/Core/OpenStreetMapNominatimGeocoderService.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*global define*/
define([
'./Cartesian3',
'./defaultValue',
'./loadJson',
'./Rectangle'
], function(
Cartesian3,
defaultValue,
loadJson,
Rectangle) {
'use strict';

/**
* Provides geocoding through OpenStreetMap Nominatim.
* @alias OpenStreetMapNominatimGeocoder
*
Copy link
Contributor

Choose a reason for hiding this comment

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

@params

*/
function OpenStreetMapNominatimGeocoder(options) {
options = defaultValue(options, defaultValue.EMPTY_OBJECT);
this.displayName = defaultValue(options.displayName, 'Nominatim');
this._canceled = false;
this.autoComplete = defaultValue(options.autoComplete, true);
}

OpenStreetMapNominatimGeocoder.prototype.cancel = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

doc

this._canceled = true;
};

/**
* @function
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably don't need @function here? At least I don't think we use this in most other places

*
* @param {String} query The query to be sent to the geocoder service
* @returns {Promise<GeocoderResult[]>}
*/
OpenStreetMapNominatimGeocoder.prototype.geocode = function (input) {
var endpoint = 'http://nominatim.openstreetmap.org/search?';
var query = 'format=json&q=' + input;
var requestString = endpoint + query;
return loadJson(requestString)
.then(function (results) {
var bboxDegrees;
return results.map(function (resultObject) {
bboxDegrees = resultObject.boundingbox;
return {
displayName: resultObject.display_name,
destination: Rectangle.fromDegrees(
bboxDegrees[2],
bboxDegrees[0],
bboxDegrees[3],
bboxDegrees[1]
)
};
});
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon

};

return OpenStreetMapNominatimGeocoder;
});
26 changes: 26 additions & 0 deletions Source/Widgets/Geocoder/Geocoder.css
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,32 @@
width: 250px;
}

.cesium-viewer-geocoderContainer .search-results {
position: absolute;
background-color: black;
overflow-y: auto;
opacity: 0.8;
width: 100%;
}

.cesium-viewer-geocoderContainer .search-results ul {
list-style-type: none;
margin: 0;
padding: 0;
}

.cesium-viewer-geocoderContainer .search-results ul li {
font-size: 14px;
padding: 3px 10px;
}
.cesium-viewer-geocoderContainer .search-results ul li:hover {
cursor: pointer;
}

.cesium-viewer-geocoderContainer .search-results ul li.active {
background: #48b;
}

.cesium-geocoder-searchButton {
background-color: #303336;
display: inline-block;
Expand Down
Loading