changeset 5605:2ac7d6a188dd

RT-32033 NPE in charts when data is modified. RT_26955 Can't replace the data in a barchart reviewed by Jonathan.
author psomashe
date Thu, 31 Oct 2013 13:41:00 -0700
parents b9b072992e27
children 68f799d4b93c
files modules/controls/src/main/java/javafx/scene/chart/BarChart.java modules/controls/src/main/java/javafx/scene/chart/LineChart.java modules/controls/src/main/java/javafx/scene/chart/XYChart.java modules/controls/src/test/java/javafx/scene/chart/BarChartTest.java
diffstat 4 files changed, 172 insertions(+), 51 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/javafx/scene/chart/BarChart.java	Thu Oct 31 16:15:59 2013 -0400
+++ b/modules/controls/src/main/java/javafx/scene/chart/BarChart.java	Thu Oct 31 13:41:00 2013 -0700
@@ -69,15 +69,16 @@
     private Map<Series, Map<String, Data<X,Y>>> seriesCategoryMap = 
                                 new HashMap<Series, Map<String, Data<X,Y>>>();
     private Legend legend = new Legend();
-    private boolean seriesRemove = false;
     private final Orientation orientation;
     private CategoryAxis categoryAxis;
     private ValueAxis valueAxis;
     private Timeline dataRemoveTimeline;
-    private Data<X,Y> dataItemBeingRemoved = null;
-    private Series<X,Y> seriesOfDataRemoved = null;
     private double bottomPos  = 0;
     private static String NEGATIVE_STYLE = "negative";
+    private ParallelTransition pt;
+    // For storing data values in case removed and added immediately.
+    private Map<Data<X,Y>, Double> XYValueMap = 
+                                new HashMap<Data<X,Y>, Double>();
     // -------------- PUBLIC PROPERTIES ----------------------------------------
 
     /** The gap to leave between bars in the same category */
@@ -217,15 +218,6 @@
         categoryMap.put(category, item);
         Node bar = createBar(series, getData().indexOf(series), item, itemIndex);
         if (shouldAnimate()) {
-            if (dataRemoveTimeline != null && dataRemoveTimeline.getStatus().equals(Animation.Status.RUNNING)) {
-                if (dataItemBeingRemoved != null && dataItemBeingRemoved == item) {
-                    dataRemoveTimeline.stop();
-                    getPlotChildren().remove(bar);
-                    removeDataItemFromDisplay(seriesOfDataRemoved, item);
-                    dataItemBeingRemoved = null;
-                    seriesOfDataRemoved = null;
-                }
-            }
             animateDataAdd(item, bar);
         } else {
             getPlotChildren().add(bar);
@@ -235,24 +227,17 @@
     @Override protected void dataItemRemoved(final Data<X,Y> item, final Series<X,Y> series) {
         final Node bar = item.getNode();
         if (shouldAnimate()) {
+            XYValueMap.clear();
             dataRemoveTimeline = createDataRemoveTimeline(item, bar, series);
-            dataItemBeingRemoved = item;
-            seriesOfDataRemoved = series;
             dataRemoveTimeline.setOnFinished(new EventHandler<ActionEvent>() {
                 public void handle(ActionEvent event) {
                     item.setSeries(null);
-                    getPlotChildren().remove(bar);
                     removeDataItemFromDisplay(series, item);
-                    dataItemBeingRemoved = null;
-                    updateMap(series, item);
                 }
             });
             dataRemoveTimeline.play();
         } else {
-            item.setSeries(null);
-            getPlotChildren().remove(bar);
-            removeDataItemFromDisplay(series, item);
-            updateMap(series, item);
+            processDataRemove(series, item);
         }
     }
 
@@ -277,7 +262,7 @@
              item.getNode().getStyleClass().remove(NEGATIVE_STYLE);
          }
     }
-    
+
     @Override protected void seriesAdded(Series<X,Y> series, int seriesIndex) {
         // handle any data already in series
         // create entry in the map
@@ -311,22 +296,21 @@
         updateDefaultColorIndex(series);
         // remove all symbol nodes
         if (shouldAnimate()) {
-            ParallelTransition pt = new ParallelTransition();
+            pt = new ParallelTransition();
             pt.setOnFinished(new EventHandler<ActionEvent>() {
                 public void handle(ActionEvent event) {
                     removeSeriesFromDisplay(series);
                 }
             });
+            
+            boolean lastSeries = (getSeriesSize() > 1) ? false : true;
+            XYValueMap.clear();
             for (final Data<X,Y> d : series.getData()) {
                 final Node bar = d.getNode();
-                seriesRemove = true;
                 // Animate series deletion
-                if (getSeriesSize() > 1) {
-                    for (int j=0; j< series.getData().size(); j++) {
-                        Data<X,Y> item = series.getData().get(j);
-                        Timeline t = createDataRemoveTimeline(item, bar, series);
+                if (!lastSeries) {
+                        Timeline t = createDataRemoveTimeline(d, bar, series);
                         pt.getChildren().add(t);
-                    }
                 } else {
                     // fade out last series
                     FadeTransition ft = new FadeTransition(Duration.millis(700),bar);
@@ -334,8 +318,7 @@
                     ft.setToValue(0);
                     ft.setOnFinished(new EventHandler<ActionEvent>() {
                          @Override public void handle(ActionEvent actionEvent) {
-                            getPlotChildren().remove(bar);
-                            updateMap(series, d);
+                            processDataRemove(series, d);
                          }
                     });
                     pt.getChildren().add(ft);
@@ -434,6 +417,13 @@
         }
         if (seriesCategoryMap.isEmpty() && categoryAxis.isAutoRanging()) categoryAxis.getCategories().clear();
     }
+    
+    private void processDataRemove(final Series<X,Y> series, final Data<X,Y> item) {
+        Node bar = item.getNode();
+        getPlotChildren().remove(bar);
+        updateMap(series, item);
+    }
+     
     private void animateDataAdd(Data<X,Y> item, Node bar) {
         double barVal;
         if (orientation == Orientation.VERTICAL) {
@@ -471,24 +461,29 @@
         Timeline t = new Timeline();
         if (orientation == Orientation.VERTICAL) {
 //            item.setYValue(getYAxis().toRealValue(getYAxis().getZeroPosition()));
+            
+            // save data values in case the same data item gets added immediately.
+            XYValueMap.put(item, ((Number)item.getYValue()).doubleValue());
             item.setYValue(getYAxis().toRealValue(bottomPos));
             t.getKeyFrames().addAll(new KeyFrame(Duration.ZERO,
                                     new KeyValue(item.currentYProperty(), item.getCurrentY())),
                                     new KeyFrame(Duration.millis(700), new EventHandler<ActionEvent>() {
                                     @Override public void handle(ActionEvent actionEvent) {
-                                        getPlotChildren().remove(bar);
-                                        updateMap(series, item);
+                                        processDataRemove(series, item);
+                                        XYValueMap.clear();
                                     }
                                 },
                                 new KeyValue(item.currentYProperty(), item.getYValue(),
                                 Interpolator.EASE_BOTH) ));
         } else {
+            // save data values in case the same data item gets added immediately.
+             XYValueMap.put(item, ((Number)item.getXValue()).doubleValue());
             item.setXValue(getXAxis().toRealValue(getXAxis().getZeroPosition()));
             t.getKeyFrames().addAll(new KeyFrame(Duration.ZERO, new KeyValue(item.currentXProperty(), item.getCurrentX())),
                 new KeyFrame(Duration.millis(700), new EventHandler<ActionEvent>() {
                         @Override public void handle(ActionEvent actionEvent) {
-                            getPlotChildren().remove(bar);
-                            updateMap(series, item);
+                            processDataRemove(series, item);
+                            XYValueMap.clear();
                         }
                     },
                     new KeyValue(item.currentXProperty(), item.getXValue(),
@@ -497,6 +492,54 @@
         return t;
     }
     
+    @Override void dataBeingRemovedIsAdded(Data item, Series series) {
+        if (dataRemoveTimeline != null) {
+            dataRemoveTimeline.setOnFinished(null);
+            dataRemoveTimeline.stop();
+        }
+        processDataRemove(series, item);
+        item.setSeries(null);
+        removeDataItemFromDisplay(series, item);
+        restoreDataValues(item);
+        XYValueMap.clear();
+    }
+    
+    private void restoreDataValues(Data item) {
+        Double value = XYValueMap.get(item);
+        if (value != null) {
+            // Restoring original X/Y values 
+            if (orientation.equals(Orientation.VERTICAL)) {
+                item.setYValue(value);
+                item.setCurrentY(value);
+            } else {
+                item.setXValue(value);
+                item.setCurrentX(value);
+
+            }
+        }
+    }
+    @Override void seriesBeingRemovedIsAdded(Series<X,Y> series) {
+        boolean lastSeries = (pt.getChildren().size() == 1) ? true : false;
+        if (pt!= null) {
+            if (!pt.getChildren().isEmpty()) {
+                for (Animation a : pt.getChildren()) {
+                    a.setOnFinished(null);
+                }
+            }
+            for (Data item : series.getData()) {
+                processDataRemove(series, item);
+                if (!lastSeries) {
+                    restoreDataValues(item);
+                }
+            }
+            XYValueMap.clear();
+            pt.setOnFinished(null);
+            pt.getChildren().clear();
+            pt.stop();
+            removeSeriesFromDisplay(series);
+        }
+    }
+     
     private void updateDefaultColorIndex(final Series<X,Y> series) {
         int clearIndex = seriesColorMap.get(series);
         colorBits.clear(clearIndex);
@@ -524,7 +567,7 @@
         Map<String, Data<X,Y>> catmap = seriesCategoryMap.get(series);
         return (catmap != null) ? catmap.get(category) : null;
     }
-
+    
     // -------------- STYLESHEET HANDLING ------------------------------------------------------------------------------
 
     /**
--- a/modules/controls/src/main/java/javafx/scene/chart/LineChart.java	Thu Oct 31 16:15:59 2013 -0400
+++ b/modules/controls/src/main/java/javafx/scene/chart/LineChart.java	Thu Oct 31 13:41:00 2013 -0700
@@ -77,7 +77,10 @@
     private Timeline dataRemoveTimeline;
     private Series<X,Y> seriesOfDataRemoved = null;
     private Data<X,Y> dataItemBeingRemoved = null;
-
+    private FadeTransition fadeSymbolTransition = null;
+    private Map<Data<X,Y>, Double> XYValueMap = 
+                                new HashMap<Data<X,Y>, Double>();
+    private Timeline seriesRemoveTimeline = null;
     // -------------- PUBLIC PROPERTIES ----------------------------------------
 
     /** When true, CSS styleable symbols are created for any data items that don't have a symbol node specified. */
@@ -230,6 +233,7 @@
         // remove item from sorted list
         int itemIndex = series.getItemIndex(item);
         if (shouldAnimate()) {
+            XYValueMap.clear();
             boolean animate = false;
             if (itemIndex > 0 && itemIndex < series.getDataSize()) {
                 animate = true;
@@ -277,16 +281,16 @@
                 // fade out symbol
                 if (symbol != null) {
                     symbol.setOpacity(0);
-                    FadeTransition ft = new FadeTransition(Duration.millis(500),symbol);
-                    ft.setToValue(0);
-                    ft.setOnFinished(new EventHandler<ActionEvent>() {
+                    fadeSymbolTransition = new FadeTransition(Duration.millis(500),symbol);
+                    fadeSymbolTransition.setToValue(0);
+                    fadeSymbolTransition.setOnFinished(new EventHandler<ActionEvent>() {
                         @Override public void handle(ActionEvent actionEvent) {
                             item.setSeries(null);
                             getPlotChildren().remove(symbol);
                             removeDataItemFromDisplay(series, item);
                         }
                     });
-                    ft.play();
+                    fadeSymbolTransition.play();
                 }
             }
             if (animate) {
@@ -395,8 +399,8 @@
                 startValues[j]   = new KeyValue(nodes.get(j).opacityProperty(),1);
                 endValues[j]       = new KeyValue(nodes.get(j).opacityProperty(),0);
             }
-            Timeline tl = new Timeline();
-            tl.getKeyFrames().addAll(
+            seriesRemoveTimeline = new Timeline();
+            seriesRemoveTimeline.getKeyFrames().addAll(
                 new KeyFrame(Duration.ZERO,startValues),
                 new KeyFrame(Duration.millis(900), new EventHandler<ActionEvent>() {
                     @Override public void handle(ActionEvent actionEvent) {
@@ -405,7 +409,7 @@
                     }
                 },endValues)
             );
-            tl.play();
+            seriesRemoveTimeline.play();
         } else {
             getPlotChildren().remove(series.getNode());
             for (Data d:series.getData()) getPlotChildren().remove(d.getNode());
@@ -442,10 +446,46 @@
             }
         }
     }
+    /** @inheritDoc */
+    @Override void dataBeingRemovedIsAdded(Data item, Series series) {
+        if (fadeSymbolTransition != null) {
+            fadeSymbolTransition.setOnFinished(null);
+            fadeSymbolTransition.stop();
+        }
+        if (dataRemoveTimeline != null) {
+            dataRemoveTimeline.setOnFinished(null);
+            dataRemoveTimeline.stop();
+        }
+        final Node symbol = item.getNode();
+        if (symbol != null) getPlotChildren().remove(symbol);
+        
+        item.setSeries(null);
+        removeDataItemFromDisplay(series, item);
+        
+        // restore values to item
+        Double value = XYValueMap.get(item);
+        if (value != null) {
+            item.setYValue(value);
+            item.setCurrentY(value);
+        }
+        XYValueMap.clear();
+    }
+    /** @inheritDoc */
+    @Override void seriesBeingRemovedIsAdded(Series<X,Y> series) {
+        if (seriesRemoveTimeline != null) {
+            seriesRemoveTimeline.setOnFinished(null);
+            seriesRemoveTimeline.stop();
+            getPlotChildren().remove(series.getNode());
+            for (Data d:series.getData()) getPlotChildren().remove(d.getNode());
+            removeSeriesFromDisplay(series);
+        }
+    }
 
     private Timeline createDataRemoveTimeline(final Data<X,Y> item, final Node symbol, final Series<X,Y> series) {
         Timeline t = new Timeline();
-
+        // save data values in case the same data item gets added immediately.
+        XYValueMap.put(item, ((Number)item.getYValue()).doubleValue());
+            
         t.getKeyFrames().addAll(new KeyFrame(Duration.ZERO, new KeyValue(item.currentYProperty(),
                 item.getCurrentY()), new KeyValue(item.currentXProperty(),
                 item.getCurrentX())),
@@ -453,6 +493,7 @@
                     @Override public void handle(ActionEvent actionEvent) {
                         if (symbol != null) getPlotChildren().remove(symbol);
                         removeDataItemFromDisplay(series, item);
+                        XYValueMap.clear();
                     }
                 },
                 new KeyValue(item.currentYProperty(),
--- a/modules/controls/src/main/java/javafx/scene/chart/XYChart.java	Thu Oct 31 16:15:59 2013 -0400
+++ b/modules/controls/src/main/java/javafx/scene/chart/XYChart.java	Thu Oct 31 13:41:00 2013 -0700
@@ -112,6 +112,7 @@
             while (c.next()) {
                 if (c.getRemoved().size() > 0) updateLegend();
                 for (Series<X,Y> series : c.getRemoved()) {
+                    series.setToRemove = true;
                     series.setChart(null);
                     seriesRemoved(series);
 //                    seriesDefaultColorIndex --;
@@ -120,6 +121,10 @@
                     final Series<X,Y> series = c.getList().get(i);
                     // add new listener to data
                     series.setChart(XYChart.this);
+                    if (series.setToRemove) {
+                        series.setToRemove = false;
+                        series.getChart().seriesBeingRemovedIsAdded(series);
+                    }
                     // update linkedList Pointers for series
                     if (XYChart.this.begin == null) {
                         XYChart.this.begin = getData().get(i);
@@ -555,6 +560,19 @@
     protected void updateLegend(){}
 
     /**
+     * This method is called when there is an attempt to add series that was  
+     * set to be removed, and the removal might not have completed. 
+     * @param series 
+     */
+    void seriesBeingRemovedIsAdded(Series<X,Y> series) {}
+    
+    /**
+     * This method is called when there is an attempt to add a Data item that was
+     * set to be removed, and the removal might not have completed.
+     * @param data 
+     */
+    void dataBeingRemovedIsAdded(Data<X,Y> item, Series<X,Y> series) {}
+    /**
      * Called when a data item has been added to a series. This is where implementations of XYChart can create/add new
      * nodes to getPlotChildren to represent this data item. They also may animate that data add with a fade in or
      * similar if animated = true.
@@ -906,6 +924,7 @@
      * @param series The series to remove
      */
     protected final void removeSeriesFromDisplay(Series<X, Y> series) {
+        if (series != null) series.setToRemove = false;
         if (begin == series) {
             begin = series.next;
         } else {
@@ -1447,7 +1466,7 @@
 
         /** the style class for default color for this series */
         String defaultColorStyleClass;
-
+        boolean setToRemove = false;
         Data<X,Y> begin = null; // start pointer of a data linked list.
         /*
          * Next pointer for the next series. We maintain a linkedlist of the
@@ -1485,7 +1504,8 @@
                     if (c.getAddedSize() > 0) {
                         for (Data<X,Y> itemPtr = begin; itemPtr != null; itemPtr = itemPtr.next) {
                             if (itemPtr.setToRemove) {
-                                removeDataItemRef(itemPtr);
+                                getChart().dataBeingRemovedIsAdded(itemPtr, Series.this);
+                                itemPtr.setToRemove = false;
                             }
                         }
                     }
@@ -1696,6 +1716,7 @@
          * when data is deleted.
          */
         private void removeDataItemRef(Data<X,Y> item) {
+            if (item != null) item.setToRemove = false;
             if (begin == item) {
                 begin = item.next;
             } else {
@@ -1724,5 +1745,5 @@
             return count;
         }
     }
-
+    
 }
--- a/modules/controls/src/test/java/javafx/scene/chart/BarChartTest.java	Thu Oct 31 16:15:59 2013 -0400
+++ b/modules/controls/src/test/java/javafx/scene/chart/BarChartTest.java	Thu Oct 31 13:41:00 2013 -0700
@@ -30,11 +30,9 @@
 import org.junit.Test;
 import static org.junit.Assert.assertEquals;
 import javafx.collections.*;
-import com.sun.javafx.pgstub.StubToolkit;
-import com.sun.javafx.tk.Toolkit;
 
 import javafx.scene.Node;
-import javafx.scene.Scene;
+import javafx.scene.chart.XYChart.Series;
 import javafx.scene.layout.StackPane;
 
 /**
@@ -114,4 +112,22 @@
         assertEquals("0", xAxis.getCategories().get(0));
     }
     
+    @Test
+    public void testRemoveAndAddSameSeriesBeforeAnimationCompletes() {
+        startApp();
+        assertEquals(2, bc.getData().size());
+        // remove and add the same series.
+        bc.getData().add(bc.getData().remove(0));
+        pulse();
+        assertEquals(2, bc.getData().size());
+    }
+    
+    @Test
+    public void testRemoveAndAddSameDataBeforeAnimationCompletes() {
+        startApp();
+        Series s = bc.getData().get(0);
+        assertEquals(3, s.getDataSize());
+        s.getData().add(s.getData().remove(0));
+        assertEquals(3, s.getDataSize());
+    }
 }