#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 )
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)
Change History (51)
Changed 10 years ago by
Attachment: | test_anim2d.html added |
---|
Changed 10 years ago by
Attachment: | patchTestAnim2dWithStacked.diff added |
---|
Changed 10 years ago by
Attachment: | DefaultStackedChart.diff added |
---|
Changed 10 years ago by
Attachment: | test_missingPoints_stacked.html added |
---|
comment:2 Changed 10 years ago by
Owner: | changed from Eugene Lazutkin to cjolif |
---|---|
Status: | new → assigned |
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
Attachment: | patchTestMissingPoints.diff added |
---|
Changed 10 years ago by
Attachment: | DefaultStackedChart.2.diff added |
---|
comment:6 Changed 10 years ago by
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 10 years ago by
Description: | modified (diff) |
---|
Changed 10 years ago by
Attachment: | commonStacked.js added |
---|
Changed 10 years ago by
Attachment: | DefaultStackedChart.3.diff added |
---|
Changed 10 years ago by
Attachment: | 15308.patch added |
---|
patch based on moogle with only the minimal refactoring needed
comment:8 Changed 10 years ago by
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 10 years ago by
In last patch I moved stackedstats and stackefValue into new common file. Because we need into columns too (see #15353)
comment:10 Changed 10 years ago by
Ok, I understand. Could you at least use commonStacked ?
I will test
comment:11 Changed 10 years ago by
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 10 years ago by
Milestone: | tbd → 1.8 |
---|
comment:13 Changed 10 years ago by
It's ok for me. I uploaded and test minimal patch with use of commonStacked. thank's
comment:16 Changed 10 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I'm re-opening this, it seems that test_stacked.html is broken following this commit.
Changed 10 years ago by
Attachment: | patchRegression15308.diff added |
---|
comment:18 Changed 10 years ago by
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
Changed 10 years ago by
Attachment: | patchRegression15308.2.diff added |
---|
comment:23 Changed 10 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
comment:24 Changed 10 years ago by
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.
comment:27 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Changed 10 years ago by
Attachment: | patchOnContinuousStacked.diff added |
---|
comment:28 Changed 10 years ago by
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:30 Changed 10 years ago by
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
comment:32 Changed 10 years ago by
Milestone: | 1.8 → 1.8.1 |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
re-opening because of the regression above
comment:33 Changed 10 years ago by
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:36 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:37 follow-up: 38 Changed 10 years ago by
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 Changed 10 years ago by
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
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