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.

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)
+      @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
 
       response.headers["Content-Disposition"] = "attachment; filename=\"tracks.gpx\""
 
-      render :xml => doc.to_s
+      render :formats => [:gpx]
     end
   end
 end

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