Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cache sorting order of dataset metas #6757

Merged
merged 2 commits into from
Dec 14, 2019
Merged

Cache sorting order of dataset metas #6757

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbed0ee
Cache sorting order of dataset metas
kurkle Nov 16, 2019
a8b976e
Cache dataset visibility
kurkle Nov 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions src/core/core.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
var me = this;
var newControllers = [];
var datasets = me.data.datasets;
var sorted = me._sortedMetasets = [];
var i, ilen;

for (i = 0, ilen = datasets.length; i < ilen; i++) {
Expand All @@ -433,6 +434,7 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
meta.order = dataset.order || 0;
meta.index = i;
meta.label = '' + dataset.label;
meta.visible = me.isDatasetVisible(i);

if (meta.controller) {
meta.controller.updateIndex(i);
Expand All @@ -446,8 +448,11 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
meta.controller = new ControllerClass(me, i);
newControllers.push(meta.controller);
}
sorted.push(meta);
Copy link
Contributor

@benmccann benmccann Nov 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should check if the dataset is visible here so that we don't have to do it each time _getSortedDatasetMetas is called

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily need a new property visible cause that's basically just !hidden, right?

I was wondering if we could do the filtering of only visible datasets here to cache that as well, but on second look it seems like that's not possible because the legend shows the labels for visible and non-visible datasets

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chart.js/src/core/core.controller.js

Lines 897 to 899 in eb0751c

// meta.hidden is a per chart dataset hidden flag override with 3 states: if true or false,
// the dataset.hidden value is ignored, else if null, the dataset hidden state is returned.
return typeof meta.hidden === 'boolean' ? !meta.hidden : !this.data.datasets[datasetIndex].hidden;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the use case where datasets are shared between charts, @etimberg any insights?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure where we modify the data object, but stopping that sounds like another potentially good thing to change

Copy link
Member

@etimberg etimberg Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps 'modify' is the wrong word, but we attach extra info to the data object and so systems that treat that as immutable are unhappy

Chart.js/src/core/core.controller.js

Line 863 in e5b03a0

dataset._meta = {};

I used chart.js in a React project recently and tried writing my own wrapper. I had a look at the source code of react-chartjs-2 and they have to do a lot of work to handle the fact that the data object gets mutated. Dropping that would significantly simplify things

Copy link
Contributor

@benmccann benmccann Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to treat options as immutable as well. Here's a PR for fixing dataset._meta: #6778

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the meta.hidden and dataset.hidden are both needed. dataset.hidden is provided in dataset configuration (loaded from storage maybe) and meta.hidden is used to override that, when clicking legend (avoiding touching the provided configuration).

Now this could be changed to read the dataset.hidden to meta.hidden at initializion and only use meta.hidden runtime. Provide hide() and show() for developer access. But not in the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should visible be made private (i.e. _visible) since we may change it in the future? (maybe even worth adding a TODO to make the change you suggested)

}

sorted.sort(compare2Level('order', 'index'));

return newControllers;
},

Expand Down Expand Up @@ -703,13 +708,15 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
* @private
*/
transition: function(easingValue) {
var me = this;
const me = this;
var i, ilen;

if (!me._animationsDisabled) {
for (i = 0, ilen = (me.data.datasets || []).length; i < ilen; ++i) {
if (me.isDatasetVisible(i)) {
me.getDatasetMeta(i).controller.transition(easingValue);
const metas = me._getSortedDatasetMetas();
for (i = 0, ilen = metas.length; i < ilen; ++i) {
let meta = metas[i];
if (meta.visible) {
meta.controller.transition(easingValue);
}
}
}
Expand All @@ -727,18 +734,17 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
*/
_getSortedDatasetMetas: function(filterVisible) {
var me = this;
var datasets = me.data.datasets || [];
var metasets = me._sortedMetasets;
var result = [];
var i, ilen;

for (i = 0, ilen = datasets.length; i < ilen; ++i) {
if (!filterVisible || me.isDatasetVisible(i)) {
result.push(me.getDatasetMeta(i));
for (i = 0, ilen = metasets.length; i < ilen; ++i) {
const meta = metasets[i];
if (!filterVisible || meta.visible) {
result.push(meta);
}
}

result.sort(compare2Level('order', 'index'));

return result;
},

Expand Down Expand Up @@ -882,13 +888,7 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
},

getVisibleDatasetCount: function() {
var count = 0;
for (var i = 0, ilen = this.data.datasets.length; i < ilen; ++i) {
if (this.isDatasetVisible(i)) {
count++;
}
}
return count;
return this._getSortedVisibleDatasetMetas().length;
},

isDatasetVisible: function(datasetIndex) {
Expand Down