Re: [Laszlo-dev] For Review: Change 20070808-jcrowley-4 Summary: Move new charting and graphing from sandbox into components

[J Crowley]

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 [EMAIL PROTECTED] 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