[patch][CLA] Refactoring to apply fixes done into default to Stacked chart

[Mathevet julien]

I refactored Default and Stacked chart.

I done this mainly to have indexed stacked chart.

When i saw code I see many duplicated line and one long function. To avoid backport indexed datas I choose to refactor. May be Default seem longer in number of line, but I think in minify is shorter. I hope code will be more understandable.

This also apply all new features made in Default which aren't backported into stacked, like animate, interpolate, and indexed datas.

[Mathevet julien]

I would say it'a mainly to have non indexed datas in stacked chart.

Main future goal it's that user could change kind of plot without changing axes and have same render.

ooops ps: ignore attachment test_anim2d.html -> use patch patchTestAnim2dWithStacked.diff

[cjolif]

Thanks a lot moogle, that's a useful patch. This is moving around a lot of code, so I'm not sure I'll be able to accept it for 1.8 so late in the cycle. I need to do a more thorough review. Just looking at the code I have a small remark, according to Dojo style guide, if must not be written without bracket. While you do have some:

if(value.y == null)value.y = 0; 

Ideally you should update the patch to fix that. See http://dojotoolkit.org/community/styleGuide

[Mathevet julien]

Ok, I will update the patch.

There also a missing thing: I have to fix StackedStats? in common. Scaler isn't set correctly and so if you if you have an x > run.data.length there are missing segments.

[cjolif]

Thanks, also if you could merge test_missingPoints_stacked.html into the existing test_missingPoints.html (just as you have done for anim2d). And avoid to use DTL in it (I'm thinking here in 2.0 where we want to minimize dependencies) it would be great!

Thanks one more time.

[Mathevet julien]

I fixed stats compute to have a correct scaler and to draw all segments. I also add patch for test missing points.

[cjolif]

patch based on moogle with only the minimal refactoring needed

[cjolif]

moogle, due to the late time in the release cycle I wanted to keep changes to the minimum, so I took your patch and kept only the minimal refactoring that was needed. Can you give it a try and let me know if that works for you? Thanks.

[Mathevet julien]

In last patch I moved stackedstats and stackefValue into new common file. Because we need into columns too (see #15353)

[Mathevet julien]

Ok, I understand. Could you at least use commonStacked ?

I will test

[cjolif]

Ok, please take my patch and include the use of commonStacked in it and re-uploaded once tested. I'll do my best to use that version.

[Mathevet julien]

Use commonStacked

[Mathevet julien]

It's ok for me. I uploaded and test minimal patch with use of commonStacked. thank's

[cjolif]

In [28565]:

fixes #15308. Implement animated, interpolated & indexed stacked charts. Includes Default base class refactoring. Thanks moogle (CLA).

[cjolif]

In [28566]:

refs #15308. Forgotten file. Thanks moogle (CLA).

[cjolif]

I'm re-opening this, it seems that test_stacked.html is broken following this commit.

[cjolif]

In [28588]:

refs #15308. This is fixing the second regession in test_stacked.html (the one that draws stacked chart after its limit).

[Mathevet julien]

Your regression fix do a regression in test_missingPoints.html .

The main problem is stacked discret values need to start to 0 to dataLength - 1. I let in patch max = indexed?Math.min(run.data.length-1, Math.ceil(this._hScaler.bounds.to-this._hScaler.bounds.from)) But I think we could set max in case of indexd to dataLength -1

[Mathevet julien]

And sorry about regression....

[Mathevet julien]

Second patch fix anim2d also and includes previous too.

[cjolif]

In [28589]:

fixes #15308. Thanks moogle (IBM, CCLA).

[cjolif]

In [28590]:

refs #15308. , -> ;

[Eugene Lazutkin]

Guys, some patches use constructs like that:

...
return typeof item == "number" || (item && !item.x);
...
if(typeof run.data[j] == "number" || !run.data[j].x){
...

Specifically I am concerned with this part: !item.x. My understanding is that you try to check if item is an object with a property x or without it. It will fail when item.x === 0.

Consider using something like "x" in item or item.hasOwnProperty("x") for that. The latter doesn't throw for truthy non-object types, and reportedly runs faster.

Another thing I've noticed is expressions like that:

v = v.y ? v.y: 0;

Usually it'll be simpler to write v = v.y || 0;, but I concede that it is a matter of taste.

Please review patches, correct them, if needed, and close the ticket.

[cjolif]

Yes you are absoluty right Eugene, too bad I missed that one, the first one definitely needs to be fixed. Moogle if you come up with something please upload it otherwise I'll do it.

[cjolif]

In [28604]:

refs #15353, #15308. Test against property being here or not correctly (+ other little cleanups).

[Mathevet julien]

thk's I was busy. Sorry about these

[Mathevet julien]

Fix in case we have non continous point on x but not interrupt by an explicit null. We have to consider them like continuous. See test test_missingPoints.html In stackedLinesChart the second curve have to be continous

[cjolif]

In [28739]:

refs #15308. Fixes for stacked line chart with non null missing data. Thanks moogle (CLA).

[cjolif]

This ticket seems to have introduced an issue. See: ​http://dojo-toolkit.33424.n3.nabble.com/Zooming-on-a-StackedArea-chart-in-1-8-td3988825.html

[cjolif]

In [29545]:

refs #15308. Improve test case to reproduce the regression introduced by this ticket.

[cjolif]

re-opening because of the regression above

[cjolif]

Here is the exact commit that created the regression ​http://trac.dojotoolkit.org/changeset/28589/dojo/dojox/trunk/charting/plot2d/Stacked.js. This was done to fix this: ​http://trac.dojotoolkit.org/ticket/15308#comment:18 so we need to be cautious.

[cjolif]

In [29546]:

refs #15308. Fix the regression without apparently creating new ones (test_missingPoints.html & test_stacked are ok as well as updated test_axisZoomControl.html).

[cjolif]

In [29547]:

refs #15308. Backport fix from trunk to 1.8

[cjolif]

The ability to use non-discrete values on column chart seem to be non working in some cases. See: ​http://dojo-toolkit.33424.n3.nabble.com/Dojo-Column-chart-why-X-values-are-not-considered-for-plotting-td3988840.html#a3988874

[cjolif]

Replying to cjolif:

The ability to use non-discrete values on column chart seem to be non working in some cases. See: ​http://dojo-toolkit.33424.n3.nabble.com/Dojo-Column-chart-why-X-values-are-not-considered-for-plotting-td3988840.html#a3988874

This issue has been entered as #15915