GNOME Bugzilla – Bug 742374
Refresh from overpass if place is stale
Last modified: 2015-01-07 11:23:55 UTC
So if a place has been in the placeStore for > threshhold we could refresh it from overpass. I will attach patches that do so for a threshold of 7 days. Even if we do this we still risk having stale information since not all our data comes from Overpass. What we really would need is for geocode-glib to get a new constructor: geocode_reverse_new_for_osm_id or something similar, that takes osm_id and osm_type and reverse geocodes the place for us according to: http://wiki.openstreetmap.org/wiki/Nominatim#Parameters_2
Created attachment 293812 [details] [review] placeStore: Make updatePlace public
Created attachment 293813 [details] [review] placeStore: Add isStale method
Created attachment 293814 [details] [review] searchResultBubble: Refresh place if stale
Review of attachment 293812 [details] [review]: ACK
Review of attachment 293813 [details] [review]: Some small comments. ::: src/placeStore.js @@ +33,3 @@ const _ICON_SIZE = 20; +const _ONE_DAY = 1000 * 60 * 60 * 24; // one day in ms +const _STALE_THRESHHOLD = 7; // mark the osm information as stale after a week THRESHOLD (you got an extra H there). @@ +242,3 @@ + let now = new Date().getTime(); + let days = Math.round(Math.abs(now - added) / _ONE_DAY); + if (days >= _STALE_THRESHHOLD) Same here then :) Also not sure about the math. Currently isStale tests for places older than 6.5 days (because of the Math.round() call). Maybe just skip the Math.round()-call? @@ +245,3 @@ + return true; + else + return false; Style-nit: > return (days >= _STALE_THRESHOLD);
Review of attachment 293814 [details] [review]: Looks good, just a style nit. ::: src/searchResultBubble.js @@ +67,3 @@ + Application.placeStore.updatePlace(this.place); + }).bind(this)); + } I think we should swap these if branches. So that it is like this: // If the place is stale, update from Overpass if (Application.placeStore.isStale(this.place)) { overpass.addInfo(this.place, (function(status, code) { this._populate(this.place); Application.placeStore.updatePlace(this.place); }).bind(this)); } else { let place = Application.placeStore.get(this.place); this._populate(place); } (this makes the comment apply to the code directly below.)
If you agree with the comments above, feel free to just push after fixing them. :)
Thanks!