Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15308 closed enhancement (fixed)

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

Reported by: Mathevet julien Owned by: cjolif
Priority: undecided Milestone: 1.8.1
Component: Charting Version: 1.7.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by cjolif)

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.

Attachments (13)

test_anim2d.html (3.6 KB) - added by Mathevet julien 7 years ago.
patchTestAnim2dWithStacked.diff (1.3 KB) - added by Mathevet julien 7 years ago.
DefaultStackedChart.diff (23.1 KB) - added by Mathevet julien 7 years ago.
test_missingPoints_stacked.html (4.8 KB) - added by Mathevet julien 7 years ago.
patchTestMissingPoints.diff (3.1 KB) - added by Mathevet julien 7 years ago.
DefaultStackedChart.2.diff (24.2 KB) - added by Mathevet julien 7 years ago.
commonStacked.js (1.6 KB) - added by Mathevet julien 7 years ago.
DefaultStackedChart.3.diff (23.1 KB) - added by Mathevet julien 7 years ago.
15308.patch (19.5 KB) - added by cjolif 7 years ago.
patch based on moogle with only the minimal refactoring needed
15308.diff (17.5 KB) - added by Mathevet julien 7 years ago.
Use commonStacked
patchRegression15308.diff (2.4 KB) - added by Mathevet julien 7 years ago.
patchRegression15308.2.diff (3.2 KB) - added by Mathevet julien 7 years ago.
patchOnContinuousStacked.diff (1.1 KB) - added by Mathevet julien 7 years ago.

Download all attachments as: .zip

Change History (51)

Changed 7 years ago by Mathevet julien

Attachment: test_anim2d.html added

Changed 7 years ago by Mathevet julien

Changed 7 years ago by Mathevet julien

Attachment: DefaultStackedChart.diff added

Changed 7 years ago by Mathevet julien

comment:1 Changed 7 years ago by 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

Last edited 7 years ago by Mathevet julien (previous) (diff)

comment:2 Changed 7 years ago by cjolif

Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned
Summary: [patch][CLA] Refactorinf to apply fixes done into default to Stacked chart[patch][CLA] Refactoring to apply fixes done into default to Stacked chart

comment:3 Changed 7 years ago by 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

comment:4 Changed 7 years ago by 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.

comment:5 Changed 7 years ago by 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.

Changed 7 years ago by Mathevet julien

Attachment: patchTestMissingPoints.diff added

Changed 7 years ago by Mathevet julien

Attachment: DefaultStackedChart.2.diff added

comment:6 Changed 7 years ago by Mathevet julien

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

comment:7 Changed 7 years ago by cjolif

Description: modified (diff)

Changed 7 years ago by Mathevet julien

Attachment: commonStacked.js added

Changed 7 years ago by Mathevet julien

Attachment: DefaultStackedChart.3.diff added

Changed 7 years ago by cjolif

Attachment: 15308.patch added

patch based on moogle with only the minimal refactoring needed

comment:8 Changed 7 years ago by 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.

comment:9 Changed 7 years ago by Mathevet julien

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

comment:10 Changed 7 years ago by Mathevet julien

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

I will test

comment:11 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by cjolif

Milestone: tbd1.8

Changed 7 years ago by Mathevet julien

Attachment: 15308.diff added

Use commonStacked

comment:13 Changed 7 years ago by Mathevet julien

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

comment:14 Changed 7 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [28565]:

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

comment:15 Changed 7 years ago by cjolif

In [28566]:

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

comment:16 Changed 7 years ago by cjolif

Resolution: fixed
Status: closedreopened

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

comment:17 Changed 7 years ago by cjolif

In [28588]:

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

Changed 7 years ago by Mathevet julien

Attachment: patchRegression15308.diff added

comment:18 Changed 7 years ago by 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

comment:19 Changed 7 years ago by Mathevet julien

And sorry about regression....

Changed 7 years ago by Mathevet julien

Attachment: patchRegression15308.2.diff added

comment:20 Changed 7 years ago by Mathevet julien

Second patch fix anim2d also and includes previous too.

comment:21 Changed 7 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

In [28589]:

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

comment:22 Changed 7 years ago by cjolif

In [28590]:

refs #15308. , -> ;

comment:23 Changed 7 years ago by Eugene Lazutkin

Resolution: fixed
Status: closedreopened

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.

Last edited 7 years ago by Eugene Lazutkin (previous) (diff)

comment:24 Changed 7 years ago by 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.

Last edited 7 years ago by cjolif (previous) (diff)

comment:25 Changed 7 years ago by cjolif

In [28604]:

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

comment:26 Changed 7 years ago by Mathevet julien

thk's I was busy. Sorry about these

comment:27 Changed 7 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

Changed 7 years ago by Mathevet julien

comment:28 Changed 7 years ago by 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

comment:29 Changed 7 years ago by cjolif

In [28739]:

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

comment:31 Changed 7 years ago by cjolif

In [29545]:

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

comment:32 Changed 7 years ago by cjolif

Milestone: 1.81.8.1
Resolution: fixed
Status: closedreopened

re-opening because of the regression above

comment:33 Changed 7 years ago by 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.

comment:34 Changed 7 years ago by 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).

comment:35 Changed 7 years ago by cjolif

In [29547]:

refs #15308. Backport fix from trunk to 1.8

comment:36 Changed 7 years ago by cjolif

Resolution: fixed
Status: reopenedclosed

comment:37 Changed 7 years ago by 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

comment:38 in reply to:  37 Changed 7 years ago by 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

Note: See TracTickets for help on using tickets.