Shared data between charts #2346

simonbrunel · 2016-04-24T13:17:17Z

Important: This PR breaks all tests because now it requires to work on a real chart instance (not a fake chart/context). Modifying all tests will require an important effort, so I prefer to be sure we want to merge these changes before rewriting all tests.

Note: Since this PR is pretty big and touches almost all files, it would be great to consider merging it as soon as possible to avoid too many conflicts with upcoming contributions.

This PR allows sharing config.data between multiple chart instances (charts can be of different types). For details, please review commit messages which should be enough to understand changes. But basically, I moved all meta info under dataset._meta, which is a map from chart id to meta info (chart id is now an integer to speed up lookups).

left: master | right: PR

Also changed (fixed) the polar show/hide animations as talk on Slack:

coveralls · 2016-04-24T13:21:27Z

Coverage decreased (-30.3%) to 41.652% when pulling 4935990 on simonbrunel:shared-data into c1d7725 on nnnick:master.

etimberg · 2016-04-24T13:38:29Z

src/controllers/controller.line.js

 		addElementAndReset: function(index) {
-			this.getDataset().metaData = this.getDataset().metaData || [];


Is the meta.data array defined somewhere else?

Accessing meta info requires to use the getDatasetMeta() (or getMeta() on a dataset controller), which will build the meta object (so the meta.data array).

Actually, we should never access the _meta object directly, but always rely on the get(Dataset)Meta() accessors. This to ensure that meta are initialized, but also in the case we decide to change where we store these meta info.

Ok, perfect. I haven't gotten that far down yet so that jumped out at me.

etimberg · 2016-04-24T14:08:51Z

The data_label_combo-bar-line.html sample will need an update. Other than that, the samples work for adding data, removing data, adding datasets, and removing datasets. All of the legends work as expected. Tooltips still work for all charts

simonbrunel · 2016-04-24T14:23:38Z

Yes, I plan to fix this sample while working on the tests

coveralls · 2016-04-24T14:24:01Z

Coverage decreased (-30.3%) to 41.632% when pulling 517d9a9 on simonbrunel:shared-data into ae33b1e on nnnick:master.

simonbrunel · 2016-04-25T17:52:24Z

@etimberg, can you please review my fix in the last commit?

coveralls · 2016-04-25T17:54:43Z

Changes Unknown when pulling c2c09b6 on simonbrunel:shared-data into * on chartjs:master*.

etimberg · 2016-04-25T18:00:10Z

@simonbrunel looks good

tannerlinsley · 2016-04-25T18:00:29Z

This is great!

coveralls · 2016-04-25T18:06:26Z

Changes Unknown when pulling ac7e267 on simonbrunel:shared-data into * on chartjs:master*.

simonbrunel · 2016-04-25T18:10:28Z

@etimberg @tannerlinsley @zachpanz88: commit ac7e267 is a rewrite of some tests using the new meta data structure. It would be nice to review it because I had to change a lot of code and might have change the tests logic.

etimberg · 2016-04-25T18:45:09Z

@simonbrunel I'll have a detailed look tonight.

etimberg · 2016-04-25T18:48:01Z

At first glance the test changes look good 👍

coveralls · 2016-04-25T21:39:03Z

Changes Unknown when pulling d74e97c on simonbrunel:shared-data into * on chartjs:master*.

Meta info are now scoped by chart and moved under the dataset._meta map { chart.id -> meta }. Meta for a specific chart (and dataset) can be accessed using chart.getDatasetMeta(datasetIndex) or from the dataset controller using getMeta(). Note that helpers.uid() now generates an int (instead of a string) to make lookups in the _meta map faster.

Dataset effective type is now stored under meta.type, allowing many charts to share the same dataset but with different types. Also move dataset.bar flag to meta.bar.

Introduced a new meta.hidden 3 states flag (null|true|false) to be able to override dataset.hidden when interacting with the chart (i.e., true or false to ignore the dataset.hidden value). This is required in order to be able to correctly share dataset.hidden between multiple charts. For example: 2 charts are sharing the same data and dataset.hidden is initially false: the dataset will be displayed on both charts because meta.hidden is null. If the user clicks the legend of the first chart, meta.hidden is changed to true and the dataset is only hidden on the first chart. If dataset.hidden changes, only the second chart will have the dataset visibility updated and that until the user click again on the first chart legend, switching the meta.hidden to null.

New Chart.Element.hidden bool flag storing the visibility state of its associated data. Since elements belong to a specific chart, this change allows to manage data visibility per chart (e.g. when clicking the legend of some charts). This commit also changes (fixes?) the polar chart animation when data visibility changes. Previous implementation was affected by an edge effect due to the use of NaN as hidden implementation.

Fix access of uninitialized meta data while calculating circumference in the polar area chart by caching the number of visible elements in the update() method. Also make the calculateTotal() of the doughnut chart tolerant of uninitialized meta data.

Since we changed the way how meta data are stores, now unit tests need to work on real Chart instances. This commit brings some helpers to inject/cleanup HTML canvas and it's wrapper into/from the DOM.

Because of differences between testing platforms, introduce a new matcher for (floating) pixel values comparison (currently 2 pixels tolerance).

Also replace the 2 spaces indentation in controller.bar.tests.js by tabs to match the overall code style.

coveralls · 2016-04-26T10:52:03Z

Coverage decreased (-1.7%) to 70.295% when pulling e0353da on simonbrunel:shared-data into b615fe8 on chartjs:master.

coveralls · 2016-04-27T17:50:25Z

Coverage increased (+1.8%) to 73.776% when pulling 83b98dd on simonbrunel:shared-data into b615fe8 on chartjs:master.

coveralls · 2016-04-27T18:02:49Z

Coverage decreased (-0.9%) to 71.032% when pulling eb14481 on simonbrunel:shared-data into cbb88be on chartjs:master.

simonbrunel · 2016-04-27T18:05:15Z

As agreed with @etimberg, I disabled failing tests to be able to merge that PR. The list includes the following suites (can be updated in gulpfile.js):

controller.line.tests.js (@zachpanz88)
controller.radar.tests.js (@etimberg)
core.layoutService.tests.js
defaultConfig.tests.js
scale.linear.tests.js (@etimberg)
scale.radialLinear.tests.js (@etimberg)
scale.time.tests.js (@etimberg)

Note: core.layoutService.tests.js has been rewritten, but is failing only in Chrome on the test server. No idea how to fix it since the difference with expected value is 10px.

etimberg reviewed Apr 24, 2016
View reviewed changes

simonbrunel force-pushed the shared-data branch from 4935990 to 517d9a9 Compare April 24, 2016 14:19

simonbrunel force-pushed the shared-data branch from ac7e267 to d74e97c Compare April 25, 2016 21:35

simonbrunel force-pushed the shared-data branch from d74e97c to 6b12796 Compare April 25, 2016 22:18

etimberg mentioned this pull request Apr 25, 2016

Time scale now compensates for rounded tick units #2371

Merged

simonbrunel force-pushed the shared-data branch 3 times, most recently from 49e935b to dee0485 Compare April 25, 2016 23:46

simonbrunel added 6 commits April 26, 2016 12:46

Handle dataset type per chart

f3457c9

Dataset effective type is now stored under meta.type, allowing many charts to share the same dataset but with different types. Also move dataset.bar flag to meta.bar.

New test helpers to acquire and release charts

8869564

Since we changed the way how meta data are stores, now unit tests need to work on real Chart instances. This commit brings some helpers to inject/cleanup HTML canvas and it's wrapper into/from the DOM.

simonbrunel added 2 commits April 26, 2016 12:46

New toBeCloseToPixel jasmine matcher

8b156bd

Because of differences between testing platforms, introduce a new matcher for (floating) pixel values comparison (currently 2 pixels tolerance).

Fix bar, bubble, doughnut, and polarArea tests

e0353da

Also replace the 2 spaces indentation in controller.bar.tests.js by tabs to match the overall code style.

simonbrunel force-pushed the shared-data branch from dee0485 to e0353da Compare April 26, 2016 10:48

Fix helpers, layoutService and logarithmic tests

a93b3f4

Fix legend tests and disable other failing tests

eb14481

simonbrunel force-pushed the shared-data branch from 83b98dd to eb14481 Compare April 27, 2016 17:58

etimberg merged commit 7c6a14c into chartjs:master Apr 27, 2016

simonbrunel deleted the shared-data branch April 27, 2016 20:05

panzarino mentioned this pull request Apr 28, 2016

Update line tests to match new metadata system #2411

Merged

kurkle mentioned this pull request May 8, 2019

Remove unnecessary checks for hidden data #6255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared data between charts #2346

Shared data between charts #2346

simonbrunel commented Apr 24, 2016 •

edited

Loading

coveralls commented Apr 24, 2016

etimberg Apr 24, 2016

simonbrunel Apr 24, 2016 •

edited

Loading

simonbrunel Apr 24, 2016

etimberg Apr 24, 2016

etimberg commented Apr 24, 2016

simonbrunel commented Apr 24, 2016

coveralls commented Apr 24, 2016

simonbrunel commented Apr 25, 2016

coveralls commented Apr 25, 2016

etimberg commented Apr 25, 2016

tannerlinsley commented Apr 25, 2016

coveralls commented Apr 25, 2016

simonbrunel commented Apr 25, 2016

etimberg commented Apr 25, 2016

etimberg commented Apr 25, 2016

coveralls commented Apr 25, 2016

coveralls commented Apr 26, 2016

coveralls commented Apr 27, 2016

coveralls commented Apr 27, 2016

simonbrunel commented Apr 27, 2016 •

edited by etimberg

Loading

		addElementAndReset: function(index) {
		this.getDataset().metaData = this.getDataset().metaData \|\| [];

Shared data between charts #2346

Shared data between charts #2346

Conversation

simonbrunel commented Apr 24, 2016 • edited Loading

coveralls commented Apr 24, 2016

etimberg Apr 24, 2016

Choose a reason for hiding this comment

simonbrunel Apr 24, 2016 • edited Loading

Choose a reason for hiding this comment

simonbrunel Apr 24, 2016

Choose a reason for hiding this comment

etimberg Apr 24, 2016

Choose a reason for hiding this comment

etimberg commented Apr 24, 2016

simonbrunel commented Apr 24, 2016

coveralls commented Apr 24, 2016

simonbrunel commented Apr 25, 2016

coveralls commented Apr 25, 2016

etimberg commented Apr 25, 2016

tannerlinsley commented Apr 25, 2016

coveralls commented Apr 25, 2016

simonbrunel commented Apr 25, 2016

etimberg commented Apr 25, 2016

etimberg commented Apr 25, 2016

coveralls commented Apr 25, 2016

coveralls commented Apr 26, 2016

coveralls commented Apr 27, 2016

coveralls commented Apr 27, 2016

simonbrunel commented Apr 27, 2016 • edited by etimberg Loading

simonbrunel commented Apr 24, 2016 •

edited

Loading

simonbrunel Apr 24, 2016 •

edited

Loading

simonbrunel commented Apr 27, 2016 •

edited by etimberg

Loading