From 82c6fe4d4a5e55a9d5f7696b23a203905006d1ab Mon Sep 17 00:00:00 2001 From: Klaus Weidner Date: Mon, 6 Feb 2012 18:42:45 -0800 Subject: [PATCH] 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. --- auto_tests/tests/axis_labels.js | 27 +++++++++++++++++++++++++++ dygraph-canvas.js | 11 +++-------- dygraph-gviz.js | 8 ++------ dygraph-layout.js | 30 +++++++++++++++++++----------- dygraph.js | 37 +++++++++++++++++++++++++------------ 5 files changed, 76 insertions(+), 37 deletions(-) diff --git a/auto_tests/tests/axis_labels.js b/auto_tests/tests/axis_labels.js index 6fc54e5..02727a3 100644 --- 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 --- 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 --- 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 --- 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 --- 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]; }; /** -- 2.7.4