Fix data ordering bug from issue 278, and clean up datasets handling

The code was declaring this.layout_.datasets as an array, assuming
numerical indices, but accessing them as an object using string labels
as keys. This caused problems with Chrome which reorders numeric-looking
string labels, for example sorting "10" before "09".

This changelist redefines datasets to be an array indexed by series
number, and maintains a separate setNames array used in those places
where it's needed.

As part of the refactoring, also change indexFromSetname to use a lookup
instead of looping over all series each time, and provide a getLabels()
convenience function.

Includes a test for this in axis_labels.js that also demonstrates the
bug.

diff --git a/auto_tests/tests/axis_labels.js b/auto_tests/tests/axis_labels.js
index 6fc54e5..02727a3 100644 (file)
--- a/auto_tests/tests/axis_labels.js
+++ b/auto_tests/tests/axis_labels.js
@@ -420,3 +420,30 @@ AxisLabelsTestCase.prototype.testGlobalFormatters = function() {
   g.setSelection(9);
   assertEquals("vf9: y:vf18", getLegend());
 };
+
+AxisLabelsTestCase.prototype.testSeriesOrder = function() {
+  var opts = {
+    width: 480,
+    height: 320
+  };
+  var data = "x,00,01,10,11\n" +
+      "0,101,201,301,401\n" +
+      "1,102,202,302,402\n" +
+      "2,103,203,303,403\n" +
+      "3,104,204,304,404\n"
+  ;
+
+  var graph = document.getElementById("graph");
+  var g = new Dygraph(graph, data, opts);
+
+  g.setSelection(2);
+  assertEquals('2: 00:103 01:203 10:303 11:403', getLegend());
+
+  // Sanity checks for indexFromSetName
+  assertEquals(0, g.indexFromSetName("x"));
+  assertEquals(1, g.indexFromSetName("00"));
+  assertEquals(null, g.indexFromSetName("abcde"));
+
+  // Verify that we get the label list back in the right order
+  assertEquals(["x", "00", "01", "10", "11"], g.getLabels());
+};

diff --git a/dygraph-canvas.js b/dygraph-canvas.js
index c0d142f..9b2be25 100644 (file)
--- a/dygraph-canvas.js
+++ b/dygraph-canvas.js
@@ -680,12 +680,7 @@ DygraphCanvasRenderer.prototype._renderLineChart = function() {
   var pointsLength = points.length;
   var point, i, j, prevX, prevY, prevYs, color, setName, newYs, err_color, rgb, yscale, axis;
 
-  var setNames = [];
-  for (var name in this.layout.datasets) {
-    if (this.layout.datasets.hasOwnProperty(name)) {
-      setNames.push(name);
-    }
-  }
+  var setNames = this.layout.setNames;
   var setCount = setNames.length;
 
   // TODO(danvk): Move this mapping into Dygraph and get it out of here.
@@ -827,8 +822,9 @@ DygraphCanvasRenderer.prototype._renderLineChart = function() {
   var afterLastIndexInSet = 0;
   var setLength = 0;
   for (i = 0; i < setCount; i += 1) {
+    firstIndexInSet = this.layout.setPointsOffsets[i];
     setLength = this.layout.setPointsLengths[i];
-    afterLastIndexInSet += setLength;
+    afterLastIndexInSet = firstIndexInSet + setLength;
     setName = setNames[i];
     color = this.colors[setName];
     var strokeWidth = this.dygraph_.attr_("strokeWidth", setName);
@@ -896,7 +892,6 @@ DygraphCanvasRenderer.prototype._renderLineChart = function() {
         }
       }
     }
-    firstIndexInSet = afterLastIndexInSet;
   }
 
   context.restore();

diff --git a/dygraph-gviz.js b/dygraph-gviz.js
index b87fe80..114263a 100644 (file)
--- a/dygraph-gviz.js
+++ b/dygraph-gviz.js
@@ -67,14 +67,10 @@ Dygraph.GVizChart.prototype.getSelection = function() {
 
   if (row < 0) return selection;
 
-  var col = 1;
   var datasets = this.date_graph.layout_.datasets;
-  for (var k in datasets) {
-    if (!datasets.hasOwnProperty(k)) continue;
-    selection.push({row: row, column: col});
-    col++;
+  for (var setIdx = 0; setIdx < datasets.length; ++setIdx) {
+    selection.push({row: row, column: setIdx + 1});
   }
 
   return selection;
 };
-

diff --git a/dygraph-layout.js b/dygraph-layout.js
index 328da81..1563817 100644 (file)
--- a/dygraph-layout.js
+++ b/dygraph-layout.js
@@ -32,6 +32,7 @@
 var DygraphLayout = function(dygraph) {
   this.dygraph_ = dygraph;
   this.datasets = [];
+  this.setNames = [];
   this.annotations = [];
   this.yAxes_ = null;
 
@@ -46,7 +47,8 @@ DygraphLayout.prototype.attr_ = function(name) {
 };
 
 DygraphLayout.prototype.addDataset = function(setname, set_xy) {
-  this.datasets[setname] = set_xy;
+  this.datasets.push(set_xy);
+  this.setNames.push(setname);
 };
 
 DygraphLayout.prototype.getPlotArea = function() {
@@ -162,9 +164,8 @@ DygraphLayout.prototype._evaluateLimits = function() {
     this.minxval = this.dateWindow_[0];
     this.maxxval = this.dateWindow_[1];
   } else {
-    for (var name in this.datasets) {
-      if (!this.datasets.hasOwnProperty(name)) continue;
-      var series = this.datasets[name];
+    for (var setIdx = 0; setIdx < this.datasets.length; ++setIdx) {
+      var series = this.datasets[setIdx];
       if (series.length > 1) {
         var x1 = series[0][0];
         if (!this.minxval || x1 < this.minxval) this.minxval = x1;
@@ -212,13 +213,14 @@ DygraphLayout.prototype._evaluateLineCharts = function() {
   // for every data set since the points are added in order of the sets in
   // datasets.
   this.setPointsLengths = [];
+  this.setPointsOffsets = [];
 
-  for (var setName in this.datasets) {
-    if (!this.datasets.hasOwnProperty(setName)) continue;
-
-    var dataset = this.datasets[setName];
+  for (var setIdx = 0; setIdx < this.datasets.length; ++setIdx) {
+    var dataset = this.datasets[setIdx];
+    var setName = this.setNames[setIdx];
     var axis = this.dygraph_.axisPropertiesForSeries(setName);
 
+    this.setPointsOffsets.push(this.points.length);
     var setPointsLength = 0;
 
     for (var j = 0; j < dataset.length; j++) {
@@ -283,10 +285,10 @@ DygraphLayout.prototype.evaluateWithError = function() {
 
   // Copy over the error terms
   var i = 0;  // index in this.points
-  for (var setName in this.datasets) {
-    if (!this.datasets.hasOwnProperty(setName)) continue;
+  for (var setIdx = 0; setIdx < this.datasets.length; ++setIdx) {
     var j = 0;
-    var dataset = this.datasets[setName];
+    var dataset = this.datasets[setIdx];
+    var setName = this.setNames[setIdx];
     var axis = this.dygraph_.axisPropertiesForSeries(setName);
     for (j = 0; j < dataset.length; j++, i++) {
       var item = dataset[j];
@@ -340,7 +342,13 @@ DygraphLayout.prototype._evaluateAnnotations = function() {
  */
 DygraphLayout.prototype.removeAllDatasets = function() {
   delete this.datasets;
+  delete this.setNames;
+  delete this.setPointsLengths;
+  delete this.setPointsOffsets;
   this.datasets = [];
+  this.setNames = [];
+  this.setPointsLengths = [];
+  this.setPointsOffsets = [];
 };
 
 /**

diff --git a/dygraph.js b/dygraph.js
index 1c25f9f..758d26d 100644 (file)
--- a/dygraph.js
+++ b/dygraph.js
@@ -397,6 +397,7 @@ Dygraph.prototype.__init__ = function(div, file, attrs) {
   Dygraph.updateDeep(this.attrs_, Dygraph.DEFAULT_ATTRS);
 
   this.boundaryIds_ = [];
+  this.setIndexByName_ = {};
 
   // Create the containing DIV and other interactive elements
   this.createInterface_();
@@ -1539,11 +1540,12 @@ Dygraph.prototype.idxToRow_ = function(idx) {
     }
   }
   if (boundaryIdx < 0) return -1;
-  for (var name in this.layout_.datasets) {
-    if (idx < this.layout_.datasets[name].length) {
+  for (var setIdx = 0; setIdx < this.layout_.datasets.length; ++setIdx) {
+    var set = this.layout_.datasets[setIdx];
+    if (idx < set.length) {
       return this.boundaryIds_[boundaryIdx][0] + idx;
     }
-    idx -= this.layout_.datasets[name].length;
+    idx -= set.length;
   }
   return -1;
 };
@@ -1778,8 +1780,9 @@ Dygraph.prototype.setSelection = function(row) {
   }
 
   if (row !== false && row >= 0) {
-    for (var i in this.layout_.datasets) {
-      if (row < this.layout_.datasets[i].length) {
+    for (var setIdx = 0; setIdx < this.layout_.datasets.length; ++setIdx) {
+      var set = this.layout_.datasets[setIdx];
+      if (row < set.length) {
         var point = this.layout_.points[pos+row];
 
         if (this.attr_("stackedGraph")) {
@@ -1788,7 +1791,7 @@ Dygraph.prototype.setSelection = function(row) {
 
         this.selPoints_.push(point);
       }
-      pos += this.layout_.datasets[i].length;
+      pos += set.length;
     }
   }
 
@@ -2114,9 +2117,15 @@ Dygraph.prototype.drawGraph_ = function(clearSelection) {
   var extremes = packed[1];
   this.boundaryIds_ = packed[2];
 
+  this.setIndexByName_ = {};
+  var labels = this.attr_("labels");
+  if (labels.length > 0) {
+    this.setIndexByName_[labels[0]] = 0;
+  }
   for (var i = 1; i < datasets.length; i++) {
+    this.setIndexByName_[labels[i]] = i;
     if (!this.visibility()[i - 1]) continue;
-    this.layout_.addDataset(this.attr_("labels")[i], datasets[i]);
+    this.layout_.addDataset(labels[i], datasets[i]);
   }
 
   this.computeYAxisRanges_(extremes);
@@ -3286,15 +3295,19 @@ Dygraph.prototype.annotations = function() {
 };
 
 /**
+ * Get the list of label names for this graph. The first column is the
+ * x-axis, so the data series names start at index 1.
+ */
+Dygraph.prototype.getLabels = function(name) {
+  return this.attr_("labels").slice();
+};
+
+/**
  * Get the index of a series (column) given its name. The first column is the
  * x-axis, so the data series start with index 1.
  */
 Dygraph.prototype.indexFromSetName = function(name) {
-  var labels = this.attr_("labels");
-  for (var i = 0; i < labels.length; i++) {
-    if (labels[i] == name) return i;
-  }
-  return null;
+  return this.setIndexByName_[name];
 };
 
 /**