From aaeca5b5346b01d7ef26a301492804d36049eff8 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 9 Oct 2013 10:06:28 -0700 Subject: [PATCH] Use reverse geocoders for any lat/lon queries This simplifies the implementation, allows code sharing with "Where am I?", and produces friendlier results for lat/lon searches (actual reverse geocode results rather than a raw lat/lon display). --- app/assets/javascripts/index/search.js | 1 + app/controllers/geocoder_controller.rb | 115 ++++++++------------ app/views/geocoder/description.html.erb | 13 --- app/views/geocoder/search.html.erb | 2 +- app/views/site/_search.html.erb | 2 +- config/locales/en.yml | 3 +- config/routes.rb | 6 +- test/functional/geocoder_controller_test.rb | 104 ++++++++++-------- 8 files changed, 108 insertions(+), 138 deletions(-) delete mode 100644 app/views/geocoder/description.html.erb diff --git a/app/assets/javascripts/index/search.js b/app/assets/javascripts/index/search.js index 2f92e6282..dc4df821e 100644 --- a/app/assets/javascripts/index/search.js +++ b/app/assets/javascripts/index/search.js @@ -24,6 +24,7 @@ function initializeSearch(map) { $("#sidebar_title").html(I18n.t('site.sidebar.search_results')); $("#sidebar_content").load($(this).attr("action"), { query: $("#query").val(), + zoom: map.getZoom(), minlon: bounds.getWest(), minlat: bounds.getSouth(), maxlon: bounds.getEast(), diff --git a/app/controllers/geocoder_controller.rb b/app/controllers/geocoder_controller.rb index 8d29ad505..ac6a2013c 100644 --- a/app/controllers/geocoder_controller.rb +++ b/app/controllers/geocoder_controller.rb @@ -7,24 +7,21 @@ class GeocoderController < ApplicationController before_filter :authorize_web before_filter :set_locale - before_filter :convert_latlon, :only => [:search] def search - @query = params[:query] - @sources = Array.new + normalize_params - @query.sub(/^\s+/, "") - @query.sub(/\s+$/, "") - - if @query.match(/^[+-]?\d+(\.\d*)?\s*[\s,]\s*[+-]?\d+(\.\d*)?$/) - @sources.push "latlon" - elsif @query.match(/^\d{5}(-\d{4})?$/) + @sources = [] + if params[:lat] && params[:lon] + @sources.push "osm_nominatim_reverse" + @sources.push "geonames_reverse" + elsif params[:query].match(/^\d{5}(-\d{4})?$/) @sources.push "us_postcode" @sources.push "osm_nominatim" - elsif @query.match(/^(GIR 0AA|[A-PR-UWYZ]([0-9]{1,2}|([A-HK-Y][0-9]|[A-HK-Y][0-9]([0-9]|[ABEHMNPRV-Y]))|[0-9][A-HJKS-UW])\s*[0-9][ABD-HJLNP-UW-Z]{2})$/i) + elsif params[:query].match(/^(GIR 0AA|[A-PR-UWYZ]([0-9]{1,2}|([A-HK-Y][0-9]|[A-HK-Y][0-9]([0-9]|[ABEHMNPRV-Y]))|[0-9][A-HJKS-UW])\s*[0-9][ABD-HJLNP-UW-Z]{2})$/i) @sources.push "uk_postcode" @sources.push "osm_nominatim" - elsif @query.match(/^[A-Z]\d[A-Z]\s*\d[A-Z]\d$/i) + elsif params[:query].match(/^[A-Z]\d[A-Z]\s*\d[A-Z]\d$/i) @sources.push "ca_postcode" @sources.push "osm_nominatim" else @@ -33,35 +30,6 @@ class GeocoderController < ApplicationController end end - def search_latlon - # get query parameters - query = params[:query] - - # create result array - @results = Array.new - - # decode the location - if m = query.match(/^\s*([+-]?\d+(\.\d*)?)\s*[\s,]\s*([+-]?\d+(\.\d*)?)\s*$/) - lat = m[1].to_f - lon = m[3].to_f - end - - # generate results - if lat < -90 or lat > 90 - @error = "Latitude #{lat} out of range" - render :action => "error" - elsif lon < -180 or lon > 180 - @error = "Longitude #{lon} out of range" - render :action => "error" - else - @results.push({:lat => lat, :lon => lon, - :zoom => POSTCODE_ZOOM, - :name => "#{lat}, #{lon}"}) - - render :action => "results" - end - end - def search_us_postcode # get query parameters query = params[:query] @@ -224,14 +192,7 @@ class GeocoderController < ApplicationController render :action => "error" end - def description - @sources = Array.new - - @sources.push({ :name => "osm_nominatim" }) - @sources.push({ :name => "geonames" }) - end - - def description_osm_nominatim + def search_osm_nominatim_reverse # get query parameters lat = params[:lat] lon = params[:lon] @@ -245,9 +206,16 @@ class GeocoderController < ApplicationController # parse the response response.elements.each("reversegeocode/result") do |result| + lat = result.attributes["lat"].to_s + lon = result.attributes["lon"].to_s + object_type = result.attributes["osm_type"] + object_id = result.attributes["osm_id"] description = result.get_text.to_s - @results.push({:prefix => "#{description}"}) + @results.push({:lat => lat, :lon => lon, + :zoom => zoom, + :name => description, + :type => object_type, :id => object_id}) end render :action => "results" @@ -256,7 +224,7 @@ class GeocoderController < ApplicationController render :action => "error" end - def description_geonames + def search_geonames_reverse # get query parameters lat = params[:lat] lon = params[:lon] @@ -271,7 +239,10 @@ class GeocoderController < ApplicationController response.elements.each("geonames/countrySubdivision") do |geoname| name = geoname.get_text("adminName1").to_s country = geoname.get_text("countryName").to_s - @results.push({:prefix => "#{name}, #{country}"}) + @results.push({:lat => lat, :lon => lon, + :zoom => GEONAMES_ZOOM, + :name => name, + :suffix => ", #{country}"}) end render :action => "results" @@ -323,25 +294,29 @@ private return URI.escape(query, Regexp.new("[^#{URI::PATTERN::UNRESERVED}]", false, 'N')) end - def convert_latlon - @query = params[:query] + def normalize_params + query = params[:query] + return unless query - if latlon = @query.match(/^([NS])\s*(\d{1,3}(\.\d*)?)\W*([EW])\s*(\d{1,3}(\.\d*)?)$/).try(:captures) # [NSEW] decimal degrees - params[:query] = nsew_to_decdeg(latlon) - elsif latlon = @query.match(/^(\d{1,3}(\.\d*)?)\s*([NS])\W*(\d{1,3}(\.\d*)?)\s*([EW])$/).try(:captures) # decimal degrees [NSEW] - params[:query] = nsew_to_decdeg(latlon) + query.strip! - elsif latlon = @query.match(/^([NS])\s*(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?\W*([EW])\s*(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?$/).try(:captures) # [NSEW] degrees, decimal minutes - params[:query] = ddm_to_decdeg(latlon) - elsif latlon = @query.match(/^(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?\s*([NS])\W*(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?\s*([EW])$/).try(:captures) # degrees, decimal minutes [NSEW] - params[:query] = ddm_to_decdeg(latlon) + if latlon = query.match(/^([NS])\s*(\d{1,3}(\.\d*)?)\W*([EW])\s*(\d{1,3}(\.\d*)?)$/).try(:captures) # [NSEW] decimal degrees + params.merge!(nsew_to_decdeg(latlon)).delete(:query) + elsif latlon = query.match(/^(\d{1,3}(\.\d*)?)\s*([NS])\W*(\d{1,3}(\.\d*)?)\s*([EW])$/).try(:captures) # decimal degrees [NSEW] + params.merge!(nsew_to_decdeg(latlon)).delete(:query) - elsif latlon = @query.match(/^([NS])\s*(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]?\W*([EW])\s*(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]?$/).try(:captures) # [NSEW] degrees, minutes, decimal seconds - params[:query] = dms_to_decdeg(latlon) - elsif latlon = @query.match(/^(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]\s*([NS])\W*(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]?\s*([EW])$/).try(:captures) # degrees, minutes, decimal seconds [NSEW] - params[:query] = dms_to_decdeg(latlon) - else - return + elsif latlon = query.match(/^([NS])\s*(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?\W*([EW])\s*(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?$/).try(:captures) # [NSEW] degrees, decimal minutes + params.merge!(ddm_to_decdeg(latlon)).delete(:query) + elsif latlon = query.match(/^(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?\s*([NS])\W*(\d{1,3})°?\s*(\d{1,3}(\.\d*)?)?['′]?\s*([EW])$/).try(:captures) # degrees, decimal minutes [NSEW] + params.merge!(ddm_to_decdeg(latlon)).delete(:query) + + elsif latlon = query.match(/^([NS])\s*(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]?\W*([EW])\s*(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]?$/).try(:captures) # [NSEW] degrees, minutes, decimal seconds + params.merge!(dms_to_decdeg(latlon)).delete(:query) + elsif latlon = query.match(/^(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]\s*([NS])\W*(\d{1,3})°?\s*(\d{1,2})['′]?\s*(\d{1,3}(\.\d*)?)?["″]?\s*([EW])$/).try(:captures) # degrees, minutes, decimal seconds [NSEW] + params.merge!(dms_to_decdeg(latlon)).delete(:query) + + elsif latlon = query.match(/^\s*([+-]?\d+(\.\d*)?)\s*[\s,]\s*([+-]?\d+(\.\d*)?)\s*$/) + params.merge!({:lat => latlon[1].to_f, :lon => latlon[3].to_f}).delete(:query) end end @@ -354,7 +329,7 @@ private captures[0].downcase != 's' ? lat = captures[1].to_f : lat = -(captures[1].to_f) captures[3].downcase != 'w' ? lon = captures[4].to_f : lon = -(captures[4].to_f) end - return "#{lat}, #{lon}" + {:lat => lat, :lon => lon} end def ddm_to_decdeg(captures) @@ -366,7 +341,7 @@ private captures[0].downcase != 's' ? lat = captures[1].to_f + captures[2].to_f/60 : lat = -(captures[1].to_f + captures[2].to_f/60) captures[4].downcase != 'w' ? lon = captures[5].to_f + captures[6].to_f/60 : lon = -(captures[5].to_f + captures[6].to_f/60) end - return "#{lat}, #{lon}" + {:lat => lat, :lon => lon} end def dms_to_decdeg(captures) @@ -378,7 +353,7 @@ private captures[0].downcase != 's' ? lat = captures[1].to_f + (captures[2].to_f + captures[3].to_f/60)/60 : lat = -(captures[1].to_f + (captures[2].to_f + captures[3].to_f/60)/60) captures[5].downcase != 'w' ? lon = captures[6].to_f + (captures[7].to_f + captures[8].to_f/60)/60 : lon = -(captures[6].to_f + (captures[7].to_f + captures[8].to_f/60)/60) end - return "#{lat}, #{lon}" + {:lat => lat, :lon => lon} end end diff --git a/app/views/geocoder/description.html.erb b/app/views/geocoder/description.html.erb deleted file mode 100644 index 018f77765..000000000 --- a/app/views/geocoder/description.html.erb +++ /dev/null @@ -1,13 +0,0 @@ -<% @sources.each do |source| %> - <% if source[:types] %> -

<%= raw(t("geocoder.description.title.#{source[:name]}", :types => t("geocoder.description.types.#{source[:types]}"))) %>

- <% else %> -

<%= raw(t("geocoder.description.title.#{source[:name]}")) %>

- <% end %> -
- <%= image_tag "searching.gif", :class => "search_searching" %> -
- -<% end %> diff --git a/app/views/geocoder/search.html.erb b/app/views/geocoder/search.html.erb index 8674c9580..6e25588b6 100644 --- a/app/views/geocoder/search.html.erb +++ b/app/views/geocoder/search.html.erb @@ -4,6 +4,6 @@ <%= image_tag "searching.gif", :class => "search_searching" %> <% end %> diff --git a/app/views/site/_search.html.erb b/app/views/site/_search.html.erb index c208b3219..e2f00082c 100644 --- a/app/views/site/_search.html.erb +++ b/app/views/site/_search.html.erb @@ -10,7 +10,7 @@

<%= raw(t 'site.search.search_help') %> - <%= link_to t('site.search.where_am_i'), { :controller => :geocoder, :action => :description }, { :id => "describe_location", :title => t('site.search.where_am_i_title') } %> + <%= link_to t('site.search.where_am_i'), { :controller => :geocoder, :action => :search }, { :id => "describe_location", :title => t('site.search.where_am_i_title') } %>

<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index d582c62ac..a1938079f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -462,12 +462,13 @@ en: geocoder: search: title: - latlon: 'Results from Internal' us_postcode: 'Results from Geocoder.us' uk_postcode: 'Results from NPEMap / FreeThe Postcode' ca_postcode: 'Results from Geocoder.CA' osm_nominatim: 'Results from OpenStreetMap Nominatim' geonames: 'Results from GeoNames' + osm_nominatim_reverse: 'Results from OpenStreetMap Nominatim' + geonames_reverse: 'Results from GeoNames' search_osm_nominatim: prefix_format: "%{name}" prefix: diff --git a/config/routes.rb b/config/routes.rb index 26bf71917..445c0353f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -214,15 +214,13 @@ OpenStreetMap::Application.routes.draw do # geocoder match '/geocoder/search' => 'geocoder#search', :via => :post - match '/geocoder/search_latlon' => 'geocoder#search_latlon', :via => :get match '/geocoder/search_us_postcode' => 'geocoder#search_us_postcode', :via => :get match '/geocoder/search_uk_postcode' => 'geocoder#search_uk_postcode', :via => :get match '/geocoder/search_ca_postcode' => 'geocoder#search_ca_postcode', :via => :get match '/geocoder/search_osm_nominatim' => 'geocoder#search_osm_nominatim', :via => :get match '/geocoder/search_geonames' => 'geocoder#search_geonames', :via => :get - match '/geocoder/description' => 'geocoder#description', :via => :post - match '/geocoder/description_osm_nominatim' => 'geocoder#description_osm_nominatim', :via => :get - match '/geocoder/description_geonames' => 'geocoder#description_geonames', :via => :get + match '/geocoder/search_osm_nominatim_reverse' => 'geocoder#search_osm_nominatim_reverse', :via => :get + match '/geocoder/search_geonames_reverse' => 'geocoder#search_geonames_reverse', :via => :get # export match '/export/start' => 'export#start', :via => :get diff --git a/test/functional/geocoder_controller_test.rb b/test/functional/geocoder_controller_test.rb index bbb1b0f01..51fc2020a 100644 --- a/test/functional/geocoder_controller_test.rb +++ b/test/functional/geocoder_controller_test.rb @@ -11,10 +11,6 @@ class GeocoderControllerTest < ActionController::TestCase { :path => "/geocoder/search", :method => :post }, { :controller => "geocoder", :action => "search" } ) - assert_routing( - { :path => "/geocoder/search_latlon", :method => :get }, - { :controller => "geocoder", :action => "search_latlon" } - ) assert_routing( { :path => "/geocoder/search_us_postcode", :method => :get }, { :controller => "geocoder", :action => "search_us_postcode" } @@ -35,18 +31,13 @@ class GeocoderControllerTest < ActionController::TestCase { :path => "/geocoder/search_geonames", :method => :get }, { :controller => "geocoder", :action => "search_geonames" } ) - - assert_routing( - { :path => "/geocoder/description", :method => :post }, - { :controller => "geocoder", :action => "description" } - ) assert_routing( - { :path => "/geocoder/description_osm_nominatim", :method => :get }, - { :controller => "geocoder", :action => "description_osm_nominatim" } + { :path => "/geocoder/search_osm_nominatim_reverse", :method => :get }, + { :controller => "geocoder", :action => "search_osm_nominatim_reverse" } ) assert_routing( - { :path => "/geocoder/description_geonames", :method => :get }, - { :controller => "geocoder", :action => "description_geonames" } + { :path => "/geocoder/search_geonames_reverse", :method => :get }, + { :controller => "geocoder", :action => "search_geonames_reverse" } ) end @@ -61,8 +52,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal code, assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -77,8 +70,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "50.06773, 14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -93,8 +88,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "50.06773, -14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta -14.37742, @controller.params[:lon] end end @@ -109,8 +106,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "-50.06773, 14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta -50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -125,8 +124,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "-50.06773, -14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta -50.06773, @controller.params[:lat] + assert_in_delta -14.37742, @controller.params[:lon] end end @@ -144,8 +145,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_latlon_equal_round "50.06773, 14.37742", assigns(:query), 5 + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -163,8 +166,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_latlon_equal_round "50.06773, -14.37742", assigns(:query), 5 + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta -14.37742, @controller.params[:lon] end end @@ -182,8 +187,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_latlon_equal_round "-50.06773, 14.37742", assigns(:query), 5 + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta -50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -201,8 +208,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_latlon_equal_round "-50.06773, -14.37742", assigns(:query), 5 + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta -50.06773, @controller.params[:lat] + assert_in_delta -14.37742, @controller.params[:lon] end end @@ -219,8 +228,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "50.06773, 14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -237,8 +248,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "50.06773, -14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta 50.06773, @controller.params[:lat] + assert_in_delta -14.37742, @controller.params[:lon] end end @@ -255,8 +268,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "-50.06773, 14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta -50.06773, @controller.params[:lat] + assert_in_delta 14.37742, @controller.params[:lon] end end @@ -273,8 +288,10 @@ class GeocoderControllerTest < ActionController::TestCase ].each do |code| post :search, :query => code assert_response :success - assert_equal ['latlon'], assigns(:sources) - assert_equal "-50.06773, -14.37742", assigns(:query) + assert_equal ['osm_nominatim_reverse', 'geonames_reverse'], assigns(:sources) + assert_nil @controller.params[:query] + assert_in_delta -50.06773, @controller.params[:lat] + assert_in_delta -14.37742, @controller.params[:lon] end end @@ -324,13 +341,4 @@ class GeocoderControllerTest < ActionController::TestCase assert_response :success assert_equal ['osm_nominatim'], assigns(:sources) end - -private - - ## - # this is a test helper for rounding latlon strings to a specified precision, e.g., at a precision - # of 5, "50.06773333333334, -14.377416666666667" will become "50.06773, -14.37742" - def assert_latlon_equal_round(expected, actual, precision) - assert_equal expected.split(',').map {|i| i.to_f.round(precision)}.join(', '), actual.split(',').map {|i| i.to_f.round(precision)}.join(', ') - end end -- 2.39.5