From 396f2e28dd27d514f7882c3918103b12764038de Mon Sep 17 00:00:00 2001 From: Andy Allan Date: Fri, 23 Jun 2017 14:03:57 +0100 Subject: [PATCH] Rework coordinates to avoid scientific formatting of small numbers. Fixes #1509 --- app/models/node.rb | 4 ++-- app/views/notes/_note.json.jsonify | 2 +- lib/bounding_box.rb | 8 +++---- lib/geo_record.rb | 19 +++++++++++++-- lib/potlatch.rb | 4 ++-- test/controllers/api_controller_test.rb | 4 ++-- test/controllers/changeset_controller_test.rb | 24 +++++++++---------- test/controllers/relation_controller_test.rb | 8 +++---- test/lib/bounding_box_test.rb | 16 ++++++------- test/models/note_test.rb | 8 +++++++ test/models/old_node_test.rb | 8 +++++++ test/models/tracepoint_test.rb | 8 +++++++ 12 files changed, 76 insertions(+), 37 deletions(-) diff --git a/app/models/node.rb b/app/models/node.rb index fb9bdf68b..f4367e459 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -185,8 +185,8 @@ class Node < ActiveRecord::Base add_metadata_to_xml_node(el, self, changeset_cache, user_display_name_cache) if visible? - el["lat"] = format("%.7f", lat) - el["lon"] = format("%.7f", lon) + el["lat"] = lat.to_s + el["lon"] = lon.to_s end add_tags_to_xml_node(el, node_tags) diff --git a/app/views/notes/_note.json.jsonify b/app/views/notes/_note.json.jsonify index 5e3ac518e..b96439922 100644 --- a/app/views/notes/_note.json.jsonify +++ b/app/views/notes/_note.json.jsonify @@ -2,7 +2,7 @@ json.type "Feature" json.geometry do json.type "Point" - json.coordinates [ note.lon, note.lat ] + json.coordinates [ note.lon.to_f, note.lat.to_f ] end json.properties do diff --git a/lib/bounding_box.rb b/lib/bounding_box.rb index 11e831cfe..51972efbc 100644 --- a/lib/bounding_box.rb +++ b/lib/bounding_box.rb @@ -137,10 +137,10 @@ class BoundingBox # there are two forms used for bounds with and without an underscore, # cater for both forms eg minlon and min_lon def add_bounds_to(hash, underscore = "") - hash["min#{underscore}lat"] = min_lat.to_s - hash["min#{underscore}lon"] = min_lon.to_s - hash["max#{underscore}lat"] = max_lat.to_s - hash["max#{underscore}lon"] = max_lon.to_s + hash["min#{underscore}lat"] = format("%.7f", min_lat) + hash["min#{underscore}lon"] = format("%.7f", min_lon) + hash["max#{underscore}lat"] = format("%.7f", max_lat) + hash["max#{underscore}lon"] = format("%.7f", max_lon) hash end diff --git a/lib/geo_record.rb b/lib/geo_record.rb index 09ced9729..e4a66f932 100644 --- a/lib/geo_record.rb +++ b/lib/geo_record.rb @@ -1,4 +1,19 @@ +require "delegate" + module GeoRecord + # Ensure that when coordinates are printed that they are always in decimal degrees, + # and not e.g. 4.0e-05 + # Unfortunately you can't extend Numeric classes directly (e.g. `Coord < Float`). + class Coord < DelegateClass(Float) + def initialize(obj) + super(obj) + end + + def to_s + format("%.7f", self) + end + end + # This scaling factor is used to convert between the float lat/lon that is # returned by the API, and the integer lat/lon equivalent that is stored in # the database. @@ -31,11 +46,11 @@ module GeoRecord # Return WGS84 latitude def lat - latitude.to_f / SCALE + Coord.new(latitude.to_f / SCALE) end # Return WGS84 longitude def lon - longitude.to_f / SCALE + Coord.new(longitude.to_f / SCALE) end end diff --git a/lib/potlatch.rb b/lib/potlatch.rb index 898a5a07f..76944e394 100644 --- a/lib/potlatch.rb +++ b/lib/potlatch.rb @@ -85,7 +85,7 @@ module Potlatch a + 0.chr + 0.chr + 9.chr when String 2.chr + encodestring(n) - when Numeric + when Numeric, GeoRecord::Coord 0.chr + encodedouble(n) when NilClass 5.chr @@ -107,7 +107,7 @@ module Potlatch # Encode number as eight-byte double precision float def self.encodedouble(n) - [n].pack("G") + [n.to_f].pack("G") end # Encode number as four-byte long diff --git a/test/controllers/api_controller_test.rb b/test/controllers/api_controller_test.rb index 61763d128..b07eb68c9 100644 --- a/test/controllers/api_controller_test.rb +++ b/test/controllers/api_controller_test.rb @@ -70,7 +70,7 @@ class ApiControllerTest < ActionController::TestCase end assert_response :success, "Expected scucess with the map call" assert_select "osm[version='#{API_VERSION}'][generator='#{GENERATOR}']", :count => 1 do - assert_select "bounds[minlon='#{minlon}'][minlat='#{minlat}'][maxlon='#{maxlon}'][maxlat='#{maxlat}']", :count => 1 + assert_select "bounds[minlon='#{format('%.7f', minlon)}'][minlat='#{format('%.7f', minlat)}'][maxlon='#{format('%.7f', maxlon)}'][maxlat='#{format('%.7f', maxlat)}']", :count => 1 assert_select "node[id='#{node.id}'][lat='#{format('%.7f', node.lat)}'][lon='#{format('%.7f', node.lon)}'][version='#{node.version}'][changeset='#{node.changeset_id}'][visible='#{node.visible}'][timestamp='#{node.timestamp.xmlschema}']", :count => 1 do # This should really be more generic assert_select "tag[k='#{tag.k}'][v='#{tag.v}']" @@ -141,7 +141,7 @@ class ApiControllerTest < ActionController::TestCase get :map, :bbox => "179.998,89.998,179.999.1,89.999" assert_response :success, "The map call should have succeeded" assert_select "osm[version='#{API_VERSION}'][generator='#{GENERATOR}']", :count => 1 do - assert_select "bounds[minlon='179.998'][minlat='89.998'][maxlon='179.999'][maxlat='89.999']", :count => 1 + assert_select "bounds[minlon='179.9980000'][minlat='89.9980000'][maxlon='179.9990000'][maxlat='89.9990000']", :count => 1 assert_select "node", :count => 0 assert_select "way", :count => 0 assert_select "relation", :count => 0 diff --git a/test/controllers/changeset_controller_test.rb b/test/controllers/changeset_controller_test.rb index c12f1d902..abdd4cdaa 100644 --- a/test/controllers/changeset_controller_test.rb +++ b/test/controllers/changeset_controller_test.rb @@ -1540,10 +1540,10 @@ EOF # get the bounding box back from the changeset get :read, :id => changeset_id assert_response :success, "Couldn't read back changeset." - assert_select "osm>changeset[min_lon='1.0']", 1 - assert_select "osm>changeset[max_lon='1.0']", 1 - assert_select "osm>changeset[min_lat='2.0']", 1 - assert_select "osm>changeset[max_lat='2.0']", 1 + assert_select "osm>changeset[min_lon='1.0000000']", 1 + assert_select "osm>changeset[max_lon='1.0000000']", 1 + assert_select "osm>changeset[min_lat='2.0000000']", 1 + assert_select "osm>changeset[max_lat='2.0000000']", 1 # add another node to it with_controller(NodeController.new) do @@ -1555,10 +1555,10 @@ EOF # get the bounding box back from the changeset get :read, :id => changeset_id assert_response :success, "Couldn't read back changeset for the second time." - assert_select "osm>changeset[min_lon='1.0']", 1 - assert_select "osm>changeset[max_lon='2.0']", 1 - assert_select "osm>changeset[min_lat='1.0']", 1 - assert_select "osm>changeset[max_lat='2.0']", 1 + assert_select "osm>changeset[min_lon='1.0000000']", 1 + assert_select "osm>changeset[max_lon='2.0000000']", 1 + assert_select "osm>changeset[min_lat='1.0000000']", 1 + assert_select "osm>changeset[max_lat='2.0000000']", 1 # add (delete) a way to it, which contains a point at (3,3) with_controller(WayController.new) do @@ -1571,10 +1571,10 @@ EOF get :read, :id => changeset_id assert_response :success, "Couldn't read back changeset for the third time." # note that the 3.1 here is because of the bbox overexpansion - assert_select "osm>changeset[min_lon='1.0']", 1 - assert_select "osm>changeset[max_lon='3.1']", 1 - assert_select "osm>changeset[min_lat='1.0']", 1 - assert_select "osm>changeset[max_lat='3.1']", 1 + assert_select "osm>changeset[min_lon='1.0000000']", 1 + assert_select "osm>changeset[max_lon='3.1000000']", 1 + assert_select "osm>changeset[min_lat='1.0000000']", 1 + assert_select "osm>changeset[max_lat='3.1000000']", 1 end ## diff --git a/test/controllers/relation_controller_test.rb b/test/controllers/relation_controller_test.rb index 6aafa3aab..afdab1bf2 100644 --- a/test/controllers/relation_controller_test.rb +++ b/test/controllers/relation_controller_test.rb @@ -967,10 +967,10 @@ OSM assert_response :success, "can't re-read changeset for modify test" assert_select "osm>changeset", 1, "Changeset element doesn't exist in #{@response.body}" assert_select "osm>changeset[id='#{changeset_id}']", 1, "Changeset id=#{changeset_id} doesn't exist in #{@response.body}" - assert_select "osm>changeset[min_lon='#{bbox.min_lon}']", 1, "Changeset min_lon wrong in #{@response.body}" - assert_select "osm>changeset[min_lat='#{bbox.min_lat}']", 1, "Changeset min_lat wrong in #{@response.body}" - assert_select "osm>changeset[max_lon='#{bbox.max_lon}']", 1, "Changeset max_lon wrong in #{@response.body}" - assert_select "osm>changeset[max_lat='#{bbox.max_lat}']", 1, "Changeset max_lat wrong in #{@response.body}" + assert_select "osm>changeset[min_lon='#{format('%.7f', bbox.min_lon)}']", 1, "Changeset min_lon wrong in #{@response.body}" + assert_select "osm>changeset[min_lat='#{format('%.7f', bbox.min_lat)}']", 1, "Changeset min_lat wrong in #{@response.body}" + assert_select "osm>changeset[max_lon='#{format('%.7f', bbox.max_lon)}']", 1, "Changeset max_lon wrong in #{@response.body}" + assert_select "osm>changeset[max_lat='#{format('%.7f', bbox.max_lat)}']", 1, "Changeset max_lat wrong in #{@response.body}" end end diff --git a/test/lib/bounding_box_test.rb b/test/lib/bounding_box_test.rb index 52d0b8388..53095307d 100644 --- a/test/lib/bounding_box_test.rb +++ b/test/lib/bounding_box_test.rb @@ -265,19 +265,19 @@ class BoundingBoxTest < ActiveSupport::TestCase def test_add_bounds_to_no_underscore bounds = @bbox_from_string.add_bounds_to({}) assert_equal 4, bounds.size - assert_equal @min_lon.to_s, bounds["minlon"] - assert_equal @min_lat.to_s, bounds["minlat"] - assert_equal @max_lon.to_s, bounds["maxlon"] - assert_equal @max_lat.to_s, bounds["maxlat"] + assert_equal format("%.7f", @min_lon), bounds["minlon"] + assert_equal format("%.7f", @min_lat), bounds["minlat"] + assert_equal format("%.7f", @max_lon), bounds["maxlon"] + assert_equal format("%.7f", @max_lat), bounds["maxlat"] end def test_add_bounds_to_with_underscore bounds = @bbox_from_string.add_bounds_to({}, "_") assert_equal 4, bounds.size - assert_equal @min_lon.to_s, bounds["min_lon"] - assert_equal @min_lat.to_s, bounds["min_lat"] - assert_equal @max_lon.to_s, bounds["max_lon"] - assert_equal @max_lat.to_s, bounds["max_lat"] + assert_equal format("%.7f", @min_lon), bounds["min_lon"] + assert_equal format("%.7f", @min_lat), bounds["min_lat"] + assert_equal format("%.7f", @max_lon), bounds["max_lon"] + assert_equal format("%.7f", @max_lat), bounds["max_lat"] end def test_to_scaled diff --git a/test/models/note_test.rb b/test/models/note_test.rb index 0abd0136e..77ed7f1b2 100644 --- a/test/models/note_test.rb +++ b/test/models/note_test.rb @@ -64,4 +64,12 @@ class NoteTest < ActiveSupport::TestCase comment = create(:note_comment, :author_ip => IPAddr.new("192.168.1.1")) assert_equal IPAddr.new("192.168.1.1"), comment.note.author_ip end + + # Ensure the lat/lon is formatted as a decimal e.g. not 4.0e-05 + def test_lat_lon_format + note = build(:note, :latitude => 0.00004 * GeoRecord::SCALE, :longitude => 0.00008 * GeoRecord::SCALE) + + assert_equal "0.0000400", note.lat.to_s + assert_equal "0.0000800", note.lon.to_s + end end diff --git a/test/models/old_node_test.rb b/test/models/old_node_test.rb index 8b0540957..42097c853 100644 --- a/test/models/old_node_test.rb +++ b/test/models/old_node_test.rb @@ -59,6 +59,14 @@ class OldNodeTest < ActiveSupport::TestCase assert_in_delta 76.543 * OldNode::SCALE, node.longitude, 0.000001 end + # Ensure the lat/lon is formatted as a decimal e.g. not 4.0e-05 + def test_lat_lon_xml_format + old_node = build(:old_node, :latitude => 0.00004 * OldNode::SCALE, :longitude => 0.00008 * OldNode::SCALE) + + assert_match /lat="0.0000400"/, old_node.to_xml.to_s + assert_match /lon="0.0000800"/, old_node.to_xml.to_s + end + def test_node_tags node_v1 = create(:old_node, :version => 1) node_v2 = create(:old_node, :node_id => node_v1.node_id, :version => 2) diff --git a/test/models/tracepoint_test.rb b/test/models/tracepoint_test.rb index 45dd7496f..ae258f43a 100644 --- a/test/models/tracepoint_test.rb +++ b/test/models/tracepoint_test.rb @@ -7,4 +7,12 @@ class TracepointTest < ActiveSupport::TestCase tracepoint.timestamp = nil assert !tracepoint.valid? end + + # Ensure the lat/lon is formatted as a decimal e.g. not 4.0e-05 + def test_lat_lon_xml_format + tracepoint = build(:tracepoint, :latitude => 0.00004 * GeoRecord::SCALE, :longitude => 0.00008 * GeoRecord::SCALE) + + assert_match /lat="0.0000400"/, tracepoint.to_xml_node.to_s + assert_match /lon="0.0000800"/, tracepoint.to_xml_node.to_s + end end -- 2.43.2