From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 29 Sep 2021 14:14:53 +0000 (+0100)
Subject: Refactor tracepoint index to use an xml builder view
X-Git-Tag: live~1467^2~1
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/95e5178bfbb2a37bf26b57e4f5dfcb329dd3333c

Refactor tracepoint index to use an xml builder view

This avoids constructing xml by hand in both the controller and
the model, and opens the way for other rendering in future.

The complexity of deciding which point goes where, along with revisiting
previous tracks and tracksegs means that I've broken it down into
two parts - sorting the points into the right trksegs is done first,
before rendering them all as xml. I couldn't find a way to allow
revisiting using the builder.
---

diff --git a/app/controllers/api/tracepoints_controller.rb b/app/controllers/api/tracepoints_controller.rb
index b2d755fe6..e758d559f 100644
--- a/app/controllers/api/tracepoints_controller.rb
+++ b/app/controllers/api/tracepoints_controller.rb
@@ -33,75 +33,11 @@ module Api
       # get all the points
       ordered_points = Tracepoint.bbox(bbox).joins(:trace).where(:gpx_files => { :visibility => %w[trackable identifiable] }).order("gpx_id DESC, trackid ASC, timestamp ASC")
       unordered_points = Tracepoint.bbox(bbox).joins(:trace).where(:gpx_files => { :visibility => %w[public private] }).order("gps_points.latitude", "gps_points.longitude", "gps_points.timestamp")
-      points = ordered_points.union_all(unordered_points).offset(offset).limit(Settings.tracepoints_per_page).preload(:trace)
-
-      doc = XML::Document.new
-      doc.encoding = XML::Encoding::UTF_8
-      root = XML::Node.new "gpx"
-      root["version"] = "1.0"
-      root["creator"] = "OpenStreetMap.org"
-      root["xmlns"] = "http://www.topografix.com/GPX/1/0"
-
-      doc.root = root
-
-      # initialise these variables outside of the loop so that they
-      # stay in scope and don't get free'd up by the GC during the
-      # loop.
-      gpx_id = -1
-      trackid = -1
-      track = nil
-      trkseg = nil
-      anon_track = nil
-      anon_trkseg = nil
-      timestamps = false
-
-      points.each do |point|
-        if gpx_id != point.gpx_id
-          gpx_id = point.gpx_id
-          trackid = -1
-
-          if point.trace.trackable?
-            track = XML::Node.new "trk"
-            doc.root << track
-            timestamps = true
-
-            if point.trace.identifiable?
-              track << (XML::Node.new("name") << point.trace.name)
-              track << (XML::Node.new("desc") << point.trace.description)
-              track << (XML::Node.new("url") << url_for(:controller => "/traces", :action => "show", :display_name => point.trace.user.display_name, :id => point.trace.id))
-            end
-          else
-            # use the anonymous track segment if the user hasn't allowed
-            # their GPX points to be tracked.
-            timestamps = false
-            if anon_track.nil?
-              anon_track = XML::Node.new "trk"
-              doc.root << anon_track
-            end
-            track = anon_track
-          end
-        end
-
-        if trackid != point.trackid
-          if point.trace.trackable?
-            trkseg = XML::Node.new "trkseg"
-            track << trkseg
-            trackid = point.trackid
-          else
-            if anon_trkseg.nil?
-              anon_trkseg = XML::Node.new "trkseg"
-              anon_track << anon_trkseg
-            end
-            trkseg = anon_trkseg
-          end
-        end
-
-        trkseg << point.to_xml_node(:print_timestamp => timestamps)
-      end
+      @points = ordered_points.union_all(unordered_points).offset(offset).limit(Settings.tracepoints_per_page).preload(:trace)
 
       response.headers["Content-Disposition"] = "attachment; filename=\"tracks.gpx\""
 
-      render :xml => doc.to_s
+      render :formats => [:gpx]
     end
   end
 end
diff --git a/app/views/api/tracepoints/index.gpx.builder b/app/views/api/tracepoints/index.gpx.builder
new file mode 100644
index 000000000..291ef0d45
--- /dev/null
+++ b/app/views/api/tracepoints/index.gpx.builder
@@ -0,0 +1,79 @@
+xml.instruct!
+
+xml.gpx("version" => "1.0",
+        "creator" => "OpenStreetMap.org",
+        "xmlns" => "http://www.topografix.com/GPX/1/0") do
+  # initialise these variables outside of the loop so that they
+  # stay in scope and don't get free'd up by the GC during the
+  # loop.
+  gpx_id = -1
+  trackid = -1
+  tracks = []
+  track = nil
+  trkseg = nil
+  anon_track = nil
+  anon_trkseg = nil
+
+  @points.each do |point|
+    if gpx_id != point.gpx_id
+      gpx_id = point.gpx_id
+      trackid = -1
+
+      if point.trace.trackable?
+        track = {}
+        track["trksegs"] = []
+        tracks << track
+
+        if point.trace.identifiable?
+          track["name"] = point.trace.name
+          track["desc"] = point.trace.description
+          track["url"] = url_for(:controller => "/traces", :action => "show", :display_name => point.trace.user.display_name, :id => point.trace.id)
+        end
+      else
+        # use the anonymous track segment if the user hasn't allowed
+        # their GPX points to be tracked.
+        if anon_track.nil?
+          anon_track = {}
+          anon_track["trksegs"] = []
+          tracks << anon_track
+        end
+        track = anon_track
+      end
+    end
+
+    if trackid != point.trackid
+      if point.trace.trackable?
+        trkseg = []
+        track["trksegs"] << trkseg
+        trackid = point.trackid
+      else
+        if anon_trkseg.nil?
+          anon_trkseg = []
+          anon_track["trksegs"] << anon_trkseg
+        end
+        trkseg = anon_trkseg
+      end
+    end
+
+    trkseg << point
+  end
+
+  tracks.each do |trk|
+    xml.trk do
+      if trk.key?("name")
+        xml.name trk["name"]
+        xml.desc trk["desc"]
+        xml.url trk["url"]
+      end
+      trk["trksegs"].each do |trksg|
+        xml.trkseg do
+          trksg.each do |tracepoint|
+            xml.trkpt("lat" => tracepoint.lat.to_s, "lon" => tracepoint.lon.to_s) do
+              xml.time tracepoint.timestamp.xmlschema if tracepoint.trace.trackable?
+            end
+          end
+        end
+      end
+    end
+  end
+end