[Laszlo-dev] For Review: Change 20070808-jcrowley-4 Summary: Move new charting and graphing from sandbox into components
J Crowley
jcrowley at laszlosystems.com
Tue Sep 25 14:16:07 PDT 2007
Excellent -- thanks for the very thorough review. I'll check this in
after fixing a few of the smaller issues, and I'll file a bunch of
bugs based on the larger ones.
Thanks again.
On Sep 25, 2007, at 11:19 AM, Philip Romanik wrote:
> Approved. You should check this in. I have a bunch of comments
> though that you should look at. Open new jira bugs to capture the
> bigger ones. The new charting package is 50x better than the old
> one and is much faster!
>
>
>
> QA Comments:
>
> - After zooming a portion of a chart, the scale isn't always right.
> The legend doesn't match the data. For example, look at
> barchart_example_02.lzx
>
> - Selecting a region only seems to work if I start in the upper-
> left corner and drag to lower-right. If I do it the other 3 ways,
> the selection is ignored.
>
> - dhtml: Text artifacts on many of the labels. Example, see
> barchart_example_01
>
> - Text labels in pie charts overlap each other when viewing more
> than one chart per page. This is fine for now but you might want an
> enhancement where the labels are shown outside the pie.
>
> -linechart_example_02 dhtml: After selecting a region to zoom, the
> light/dark overlay bars don't fill the chart.
>
> -piechart_example_01 The piece pieces don't pop out when dragging
> over their labels.
>
> -piechart_example_05 The legend for the cost chart is too difficult
> to read (when viewed 4/page). In dhtml this legend is not shown at
> all.
>
> - I don't think any of the demos show a drag-able chart, only zoom-
> able charts.
>
>
>
> General code comments:
>
> - This file needs a merge: /WEB-INF/lps/misc/lzx-
> autoincludes.properties
>
> - I haven't seen much error detection. Are mandatory items checked
> to make sure they are present before trying to access them?
>
> - There are many special cases handled in the /shared classes. I
> wonder if there is a way to remove these cases out of these
> classes. One option is to use subclassing to add this
> functionality. Another way is to add it to the chart-specific
> classes you already have (linechart.lzx, barchart.lzx, piechart.lzx).
>
> - Do a visual diff or barchartbacking.lzx and linechartbacking.lzx.
> You will see a fair amount of overlap between the two. Perhaps some
> additional functionality can be pushed into the parent class For
> example, line charts and bar charts both have a dataclip view
>
>
> Specific Comments:
>
> - legend.lzx has some long methods (200 lines) that can be split up.
>
> - basechart.lzx (valueCheck)
>
> if(this.minimum >= this.maximum){
> this.setAttribute('minimum', 0);
> this.setAttribute('maximum', 100);
> Debug.warn("Attribute 'minimum' must be smaller
> than attribute 'maximum'.");
> }
>
> minimum and maximum can be set by the user so changing them to
> [0,100] isn't right. You are better off swapping the minimum and
> maximum values. If minimum=maximum you can generate a warning.
>
>
> if(this.altminimum >= this.altmaximum){
> this.setAttribute('minimum', 0);
> this.setAttribute('maximum', 10);
> Debug.warn("Attribute 'altminimum' must be smaller
> than attribute 'altmaximum'.");
> }
>
> This looks like a copy/paste mistake since you are checking
> altminimum, but setting minimum. Also, see the above comment.
>
> - basechart.lzx findData() doesn't return a value in all cases (do
> what findLegend() does). You might want to print a debugging
> message in findData if the chartdata element is missing (or modify
> the places where findData is called).
>
> - basechart.lzx findData() and findLegend(). These return the index
> of the subnode that contains the information. Why not just return
> the node itself.
>
> - basechart.lzx adjustToData(). Store 'subnodes[dnode].subviews[j]'
> in a variable for performance and readability.
>
> - barchartbacking.lzx defines a class called chartbacking. The same
> class is defined inside linechartbacking.lzx.
>
> - barchartbacking.lzx. There is a lot of duplication in the file.
> For example, there are many places where the code to draw a line is
> duplicated. This should be made a method and moved into the base
> class (like you did with renderRect).
>
> XXX.globalAlpha = 1;
> XXX.strokeStyle = parent.htick;
> XXX.lineWidth = parent.htickwidth;
> XXX.beginPath();
> XXX.moveTo(maxoffset + ((i * parent.hgridspacing) *
> parent.altscaler), this.height);
> XXX.lineTo(maxoffset + ((i * parent.hgridspacing) *
> parent.altscaler), this.height + parent.vticklength);
> XXX.stroke();
>
> - chartzoomer.lzx
> This comment is in the code but zooming appears to work in drawview:
> Due to a bug with drawview, this doesn't currently work in DHTML.
>
>
> - label.lzx
> Lines like these need a comment to explain what they are doing.
> Do you really need to directly access the sprite from the charting
> code?
> if(this.sprite.width != 0 && this.sprite.width != 4){
> ...
> }
>
> - Here's an example of some fragile code that exists in some of the
> files:
>
> var barlink = parent.classroot.parent.dataclip.datapane
> ['bar'+this.labelset+this.labelnumber];
>
> Any redesign or enhancement down the road can break this.
>
>
> - barchart.lzx
> The methods buildBarsVert() and buildBarsHoriz() are pretty
> similar, yet are different methods that don't share any code.
>
>
>
>
>> Change 20070808-jcrowley-4 by jcrowley at doctormanhattan.mshome.net on
>> 2007-08-08 17:08:55 EDT
>> in /Users/jcrowley/src/svn/openlaszlo/legals-cc
>> for http://svn.openlaszlo.org/openlaszlo/branches/legals
>>
>> Summary: Move new charting and graphing from sandbox into components
>>
>> New Features: New charting and graphing package.
>>
>> Bugs Fixed: LPP-4399
>>
>> Technical Reviewer: max
>> QA Reviewer: pbr
>> Doc Reviewer: jsundman
>>
>> Documentation: Run ant doc to generate the documentation for each
>> individual class.
>>
>> Release Notes: Autoinclude has been updated to include this new
>> version of charting instead of the older one, which has
>> been rendered de facto deprecated by this new version.
>> To use the older charting package, one must manually
>> include the files in /lps/components/deprecated/charts/.
>>
>> Details: This is all the new charting stuff. Much easier to use,
>> much more pleasing to the eye, far more concise to
>> implement, and generally far more sensible, speedy, etc.
>>
>> There are still a few things left to do (get legend
>> working properly in DHTML and play around with it a
>> little... add some more customization for a few more
>> things (individual label coloring, perhaps), write a
>> tooltip class, etc), and I'm sure there are going to be
>> a few bugs here and there with this new stuff, but I'm
>> confident this is worlds better than what we had.
>>
>> There are probably also a few better or more concise
>> ways of doing certain things. I'm open to suggestions.
>> Any additional improvements or features can be easily
>> implemented.
>>
>> Zooming doesn't currently work in DHTML because of an
>> issue with drawview. I'm going to work out a test case
>> and file a bug about it.
>>
>> Anyway, for the time being, I'm sending this out for
>> review, just to get it integrated, and will have some
>> additional improvements sent out in the future.
>>
>> Tests: Run the stuff in /test/charting/, which will demonstrate
>> progressive chart examples from simple to more complex.
>> I'll probably add several others with different kinds
>> of alternatives for all the different ways one can
>> customize visual appearance.
>>
>> (Note: I've never done an svn move before, and this is
>> kind of an enormous changeset, so I'm hoping everything
>> ends up in its right place. Let me know if anything's
>> particularly fishy.)
>>
>> Files:
>> A test/charting
>> A test/charting/barchart_data_02.xml
>> A test/charting/barchart_example_03.lzx
>> A test/charting/piechart_example_01.lzx
>> A test/charting/piechart_data_01.xml
>> A test/charting/piechart_example_02.lzx
>> A test/charting/piechart_example_03.lzx
>> A test/charting/piechart_example_04.lzx
>> A test/charting/piechart_example_05.lzx
>> A test/charting/pics
>> A test/charting/pics/meer.jpg
>> A test/charting/pics/gable.jpg
>> A test/charting/linechart_example_01.lzx
>> A test/charting/linechart_data_01.xml
>> A test/charting/linechart_example_02.lzx
>> A test/charting/linechart_example_03.lzx
>> A test/charting/barchart_example_01.lzx
>> A test/charting/barchart_example_02.lzx
>> A test/deprecated
>> A + test/deprecated/charts
>> R test/charts
>> R test/charts/columnchart
>> R test/charts/columnchart/images
>> D test/charts/columnchart/images/master.gif
>> D test/charts/columnchart/images/background.gif
>> D test/charts/columnchart/test_column_01.lzx
>> R test/charts/columnchart/data
>> D test/charts/columnchart/data/simple-redsox-data.xml
>> D test/charts/columnchart/test_column_02.lzx
>> D test/charts/columnchart/test_column_03.lzx
>> D test/charts/columnchart/test_column_04.lzx
>> D test/charts/columnchart/test_column_05.lzx
>> D test/charts/columnchart/chartstyle.lzx
>> R test/charts/piechart
>> R test/charts/piechart/images
>> D test/charts/piechart/images/master.gif
>> D test/charts/piechart/images/background.gif
>> D test/charts/piechart/test_pie_01.lzx
>> R test/charts/piechart/data
>> D test/charts/piechart/data/pie-data2.xml
>> D test/charts/piechart/test_pie_02.lzx
>> D test/charts/piechart/test_pie_03.lzx
>> D test/charts/piechart/test_pie_04.lzx
>> D test/charts/piechart/chartstyle.lzx
>> R test/charts/images
>> D test/charts/images/master.gif
>> D test/charts/images/background.gif
>> D test/charts/images/column_icon.jpg
>> D test/charts/images/pie_icon.jpg
>> D test/charts/images/opensource-550x475.gif
>> D test/charts/images/OpenLaszloLogo.jpg
>> D test/charts/images/LaszloLogo2.gif
>> D test/charts/images/bar_icon.jpg
>> D test/charts/images/line_icon.jpg
>> R test/charts/barchart
>> D test/charts/barchart/test_bar_01.lzx
>> D test/charts/barchart/test_bar_02.lzx
>> D test/charts/barchart/test_bar_03.lzx
>> D test/charts/barchart/test_bar_04.lzx
>> D test/charts/barchart/test_bar_05.lzx
>> D test/charts/barchart/test_bar_06.lzx
>> R test/charts/barchart/images
>> D test/charts/barchart/images/master.gif
>> D test/charts/barchart/images/background.gif
>> D test/charts/barchart/images/opensource-550x475.gif
>> R test/charts/barchart/data
>> D test/charts/barchart/data/simple-redsox-data.xml
>> D test/charts/barchart/chartstyle.lzx
>> R test/charts/data
>> D test/charts/data/columnchart-data-example1.xml
>> D test/charts/data/ebay.xml
>> D test/charts/data/columnchart-data-example2.xml
>> D test/charts/data/ebay1.xml
>> D test/charts/data/ebay1000.xml
>> D test/charts/data/simple-redsox-data.xml
>> D test/charts/data/pie-data.xml
>> D test/charts/data/pie-data2.xml
>> D test/charts/data/simple.xml
>> D test/charts/data/redsox-data.xml
>> D test/charts/data/redsox-data1.xml
>> D test/charts/data/redsox-data2.xml
>> R test/charts/linechart
>> D test/charts/linechart/test_line_01.lzx
>> D test/charts/linechart/test_line_02.lzx
>> D test/charts/linechart/test_line_03.lzx
>> R test/charts/linechart/images
>> D test/charts/linechart/images/master.gif
>> D test/charts/linechart/images/background.gif
>> D test/charts/linechart/images/pie_icon.jpg
>> D test/charts/linechart/images/OpenLaszloLogo.jpg
>> D test/charts/linechart/test_line_04.lzx
>> D test/charts/linechart/test_line_05.lzx
>> D test/charts/linechart/test_line_06.lzx
>> D test/charts/linechart/test_line_07.lzx
>> R test/charts/linechart/data
>> D test/charts/linechart/data/ebay.xml
>> D test/charts/linechart/data/testdata.xml
>> D test/charts/linechart/data/redsox-data.xml
>> D test/charts/linechart/partialchartstyle.lzx
>> D test/charts/linechart/chartstyle.lzx
>> D test/charts/index.html
>> M WEB-INF/lps/misc/lzx-autoincludes.properties
>> A docs/src/reference/resources/barchart_data_02.xml
>> A docs/src/reference/resources/linechart_data_01.xml
>> A docs/src/reference/resources/piechart_data_01.xml
>> A lps/components/deprecated
>> A lps/components/deprecated/charts
>> A + lps/components/deprecated/charts/columnchart
>> A + lps/components/deprecated/charts/piechart
>> A + lps/components/deprecated/charts/library.lzx
>> A + lps/components/deprecated/charts/styles
>> A + lps/components/deprecated/charts/addon
>> A + lps/components/deprecated/charts/barchart
>> A + lps/components/deprecated/charts/common
>> A + lps/components/deprecated/charts/docs
>> A + lps/components/deprecated/charts/linechart
>> D lps/components/charts/columnchart
>> D lps/components/charts/columnchart/columnchartplotarea.lzx
>> D lps/components/charts/columnchart/columnchart.lzx
>> D lps/components/charts/piechart
>> D lps/components/charts/piechart/piepiece.lzx
>> D lps/components/charts/piechart/piechartplotarea.lzx
>> D lps/components/charts/piechart/piechart.lzx
>> R lps/components/charts/library.lzx
>> A lps/components/charts/barchart.lzx
>> D lps/components/charts/styles
>> D lps/components/charts/styles/defaultchartstyle.lzx
>> D lps/components/charts/styles/strokestyle.lzx
>> D lps/components/charts/styles/defaultchartstyle.xml
>> D lps/components/charts/styles/styleparser.lzx
>> D lps/components/charts/styles/chartstyle.lzx
>> D lps/components/charts/addon
>> D lps/components/charts/addon/library.lzx
>> D lps/components/charts/addon/slider
>> D lps/components/charts/addon/slider/images
>> D lps/components/charts/addon/slider/images/spin_down_r.swf
>> D lps/components/charts/addon/slider/images/
>> slider_background.swf
>> D lps/components/charts/addon/slider/images/slider_d.swf
>> D lps/components/charts/addon/slider/images/slider_back_rc.swf
>> D lps/components/charts/addon/slider/images/spin_up_n.swf
>> D lps/components/charts/addon/slider/images/spin_box_left.swf
>> D lps/components/charts/addon/slider/images/slider_hairline.swf
>> D lps/components/charts/addon/slider/images/slider_l.swf
>> D lps/components/charts/addon/slider/images/spin_up_r.swf
>> D lps/components/charts/addon/slider/images/slider_m.swf
>> D lps/components/charts/addon/slider/images/spin_box_middle.swf
>> D lps/components/charts/addon/slider/images/slider_back.swf
>> D lps/components/charts/addon/slider/images/spin_down_d.swf
>> D lps/components/charts/addon/slider/images/slider_r.swf
>> D lps/components/charts/addon/slider/images/slider_arrow_n.swf
>> D lps/components/charts/addon/slider/images/slider_arrow_r.swf
>> D lps/components/charts/addon/slider/images/spin_down_n.swf
>> D lps/components/charts/addon/slider/images/slider_back_m.swf
>> D lps/components/charts/addon/slider/images/slider_arrow_s.swf
>> D lps/components/charts/addon/slider/images/autoPng
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_hairline.png
>> D lps/components/charts/addon/slider/images/autoPng/slider_l.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_up_r.png
>> D lps/components/charts/addon/slider/images/autoPng/slider_m.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_box_middle.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_back.png
>> D lps/components/charts/addon/slider/images/autoPng/slider_r.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_down_d.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_arrow_n.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_down_n.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_arrow_r.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_arrow_s.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_back_m.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_up_d.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_back_lc.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_background.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_down_r.png
>> D lps/components/charts/addon/slider/images/autoPng/slider_d.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> slider_back_rc.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_up_n.png
>> D lps/components/charts/addon/slider/images/autoPng/
>> spin_box_left.png
>> D lps/components/charts/addon/slider/images/slider_back_lc.swf
>> D lps/components/charts/addon/slider/images/spin_up_d.swf
>> D lps/components/charts/addon/slider/slider.lzx
>> D lps/components/charts/addon/zoomarea.lzx
>> A lps/components/charts/linechart.lzx
>> D lps/components/charts/barchart
>> D lps/components/charts/barchart/barchart.lzx
>> D lps/components/charts/barchart/barchartplotarea.lzx
>> A lps/components/charts/shared
>> A lps/components/charts/shared/piepiece.lzx
>> A lps/components/charts/shared/dataseries.lzx
>> A lps/components/charts/shared/library.lzx
>> A lps/components/charts/shared/barchartbacking.lzx
>> A lps/components/charts/shared/linechartbacking.lzx
>> A lps/components/charts/shared/basechartbacking.lzx
>> A lps/components/charts/shared/basechart.lzx
>> A lps/components/charts/shared/wholepie.lzx
>> A lps/components/charts/shared/chartzoomer.lzx
>> A lps/components/charts/shared/databar.lzx
>> A lps/components/charts/shared/legend.lzx
>> A lps/components/charts/shared/label.lzx
>> D lps/components/charts/common
>> D lps/components/charts/common/axis.lzx
>> D lps/components/charts/common/library.lzx
>> D lps/components/charts/common/tickmarklabel.lzx
>> D lps/components/charts/common/datapoints.lzx
>> D lps/components/charts/common/datalabel.lzx
>> D lps/components/charts/common/valuepoints.lzx
>> D lps/components/charts/common/datamarker.lzx
>> D lps/components/charts/common/horizontalaxis.lzx
>> D lps/components/charts/common/label.lzx
>> D lps/components/charts/common/valueregion.lzx
>> D lps/components/charts/common/viewspoolmanager.lzx
>> D lps/components/charts/common/dataseries.lzx
>> D lps/components/charts/common/valueline.lzx
>> D lps/components/charts/common/datatip.lzx
>> D lps/components/charts/common/rectangularchart.lzx
>> D lps/components/charts/common/legend.lzx
>> D lps/components/charts/common/databar.lzx
>> D lps/components/charts/common/verticalaxis.lzx
>> D lps/components/charts/common/virtualdrawview.lzx
>> D lps/components/charts/common/chart.lzx
>> D lps/components/charts/docs
>> D lps/components/charts/docs/charts
>> D lps/components/charts/docs/charts/columnchart
>> D lps/components/charts/docs/charts/columnchart/columnchart.html
>> D lps/components/charts/docs/charts/columnchart/styles.css
>> D lps/components/charts/docs/charts/piechart
>> D lps/components/charts/docs/charts/piechart/piechart.html
>> D lps/components/charts/docs/charts/piechart/
>> piechartplotarea.html
>> D lps/components/charts/docs/charts/piechart/styles.css
>> D lps/components/charts/docs/charts/styles
>> D lps/components/charts/docs/charts/styles/chartstyle.html
>> D lps/components/charts/docs/charts/styles/axisstyle.html
>> D lps/components/charts/docs/charts/styles/datastylelist.html
>> D lps/components/charts/docs/charts/styles/datastyle.html
>> D lps/components/charts/docs/charts/styles/basestyle.html
>> D lps/components/charts/docs/charts/styles/chartbgstyle.html
>> D lps/components/charts/docs/charts/styles/plotstyle.html
>> D lps/components/charts/docs/charts/styles/labelstyle.html
>> D lps/components/charts/docs/charts/styles/valueregionstyle.html
>> D lps/components/charts/docs/charts/styles/regionstyle.html
>> D lps/components/charts/docs/charts/styles/valuelinestyle.html
>> D lps/components/charts/docs/charts/styles/styles.css
>> D lps/components/charts/docs/charts/styles/valuepointstyle.html
>> D lps/components/charts/docs/charts/styles/linestyle.html
>> D lps/components/charts/docs/charts/styles/pointstyle.html
>> D lps/components/charts/docs/charts/styles/tickstyle.html
>> D lps/components/charts/docs/charts/addon
>> D lps/components/charts/docs/charts/addon/zoomarea.html
>> D lps/components/charts/docs/charts/addon/styles.css
>> D lps/components/charts/docs/charts/addon/basezoomarea.html
>> D lps/components/charts/docs/charts/barchart
>> D lps/components/charts/docs/charts/barchart/barchart.html
>> D lps/components/charts/docs/charts/barchart/styles.css
>> D lps/components/charts/docs/charts/common
>> D lps/components/charts/docs/charts/common/horizontalaxis.html
>> D lps/components/charts/docs/charts/common/label.html
>> D lps/components/charts/docs/charts/common/valueregion.html
>> D lps/components/charts/docs/charts/common/dataseries.html
>> D lps/components/charts/docs/charts/common/valueline.html
>> D lps/components/charts/docs/charts/common/datatip.html
>> D lps/components/charts/docs/charts/common/datacolumn.html
>> D lps/components/charts/docs/charts/common/rectangularchart.html
>> D lps/components/charts/docs/charts/common/legend.html
>> D lps/components/charts/docs/charts/common/verticalaxis.html
>> D lps/components/charts/docs/charts/common/chart.html
>> D lps/components/charts/docs/charts/common/axis.html
>> D lps/components/charts/docs/charts/common/styles.css
>> D lps/components/charts/docs/charts/common/datalabel.html
>> D lps/components/charts/docs/charts/common/valuepoints.html
>> D lps/components/charts/docs/charts/common/datamarker.html
>> D lps/components/charts/docs/charts/linechart
>> D lps/components/charts/docs/charts/linechart/styles.css
>> D lps/components/charts/docs/charts/linechart/linechart.html
>> D lps/components/charts/docs/index.html
>> D lps/components/charts/docs/index.css
>> D lps/components/charts/linechart
>> D lps/components/charts/linechart/linechart.lzx
>> D lps/components/charts/linechart/linechartplotarea.lzx
>> A lps/components/charts/piechart.lzx
>>
>> Changeset: http://svn.openlaszlo.org/openlaszlo/patches/20070808-
>> jcrowley-4.tar
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.openlaszlo.org/pipermail/laszlo-dev/attachments/20070925/ae3ad04d/attachment-0001.html
More information about the Laszlo-dev
mailing list