changeset 7596:d515c3182f1c

RT-37824 [Charts] Series: NPE when adding data items multiple times Reviewed by: snorthov, jgiles
author Martin Sladecek <martin.sladecek@oracle.com>
date Wed, 30 Jul 2014 10:51:32 +0200
parents fd925446754b
children 562928b80d15
files modules/controls/src/main/java/javafx/scene/chart/AreaChart.java 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/ScatterChart.java modules/controls/src/main/java/javafx/scene/chart/StackedAreaChart.java modules/controls/src/main/java/javafx/scene/chart/XYChart.java
diffstat 6 files changed, 93 insertions(+), 253 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/javafx/scene/chart/AreaChart.java	Wed Jul 30 20:17:40 2014 +1200
+++ b/modules/controls/src/main/java/javafx/scene/chart/AreaChart.java	Wed Jul 30 10:51:32 2014 +0200
@@ -25,10 +25,7 @@
 
 package javafx.scene.chart;
 
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 import javafx.animation.FadeTransition;
 import javafx.animation.Interpolator;
@@ -57,7 +54,6 @@
 import com.sun.javafx.charts.Legend;
 import com.sun.javafx.charts.Legend.LegendItem;
 import com.sun.javafx.css.converters.BooleanConverter;
-import java.util.Collections;
 import javafx.beans.property.BooleanProperty;
 import javafx.css.CssMetaData;
 import javafx.css.Styleable;
@@ -262,10 +258,8 @@
             final int dataListSize = series.getData().size();
             if (itemIndex > 0 && itemIndex < dataSize -1) {
                 animate = true;
-                int index=0; Data<X,Y> d;
-                for (d = series.begin; d != null && index != itemIndex - 1; d=d.next) index++;
-                Data<X,Y> p1 = d;
-                Data<X,Y> p2 = (d.next).next;
+                Data<X,Y> p1 = series.getItem(itemIndex - 1);
+                Data<X,Y> p2 = series.getItem(itemIndex + 1);
                 double x1 = getXAxis().toNumericValue(p1.getXValue());
                 double y1 = getYAxis().toNumericValue(p1.getYValue());
                 double x3 = getXAxis().toNumericValue(p2.getXValue());
@@ -466,7 +460,8 @@
             seriesLine.clear();
             fillPath.clear();
             constructedPath.clear();
-            for (Data<X, Y> item = series.begin; item != null; item = item.next) {
+            for (Iterator<Data<X, Y>> it = getDisplayedDataIterator(series); it.hasNext(); ) {
+                Data<X, Y> item = it.next();
                 double x = getXAxis().getDisplayPosition(item.getCurrentX());
                 double y = getYAxis().getDisplayPosition(
                         getYAxis().toRealValue(getYAxis().toNumericValue(item.getCurrentY()) * seriesYAnimMultiplier.getValue()));
--- a/modules/controls/src/main/java/javafx/scene/chart/BarChart.java	Wed Jul 30 20:17:40 2014 +1200
+++ b/modules/controls/src/main/java/javafx/scene/chart/BarChart.java	Wed Jul 30 10:51:32 2014 +0200
@@ -25,11 +25,7 @@
 
 package javafx.scene.chart;
 
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.ArrayList;
-import java.util.Collections;
+import java.util.*;
 
 import javafx.scene.AccessibleRole;
 import javafx.animation.Animation;
@@ -345,7 +341,8 @@
         int catIndex = 0;
         for (String category : categoryAxis.getCategories()) {
             int index = 0;
-            for (Series<X,Y> series = begin; series != null; series = series.next) {
+            for (Iterator<Series<X, Y>> sit = getDisplayedSeriesIterator(); sit.hasNext(); ) {
+                Series<X, Y> series = sit.next();
                 final Data<X,Y> item = getDataItem(series, index, catIndex, category);
                 if (item != null) {
                     final Node bar = item.getNode();
--- a/modules/controls/src/main/java/javafx/scene/chart/LineChart.java	Wed Jul 30 20:17:40 2014 +1200
+++ b/modules/controls/src/main/java/javafx/scene/chart/LineChart.java	Wed Jul 30 10:51:32 2014 +0200
@@ -266,10 +266,8 @@
             boolean animate = false;
             if (itemIndex > 0 && itemIndex < series.getDataSize()) {
                 animate = true;
-                int index=0; Data<X,Y> d;
-                for (d = series.begin; d != null && index != itemIndex - 1; d=d.next) index++;
-                Data<X,Y> p1 = d;
-                Data<X,Y> p2 = (d.next).next;
+                Data<X,Y> p1 = series.getItem(itemIndex - 1);
+                Data<X,Y> p2 = series.getItem(itemIndex + 1);
                 if (p1 != null && p2 != null) {
                     double x1 = getXAxis().toNumericValue(p1.getXValue());
                     double y1 = getYAxis().toNumericValue(p1.getYValue());
@@ -450,7 +448,8 @@
                 final ObservableList<PathElement> seriesLine = ((Path)series.getNode()).getElements();
                 seriesLine.clear();
                 constructedPath.clear();
-                for (Data<X,Y> item = series.begin; item != null; item = item.next) {
+                for (Iterator<Data<X, Y>> it = getDisplayedDataIterator(series); it.hasNext(); ) {
+                    Data<X, Y> item = it.next();
                     double x = getXAxis().getDisplayPosition(item.getCurrentX());
                     double y = getYAxis().getDisplayPosition(
                             getYAxis().toRealValue(getYAxis().toNumericValue(item.getCurrentY()) * seriesYAnimMultiplier.getValue()));
--- a/modules/controls/src/main/java/javafx/scene/chart/ScatterChart.java	Wed Jul 30 20:17:40 2014 +1200
+++ b/modules/controls/src/main/java/javafx/scene/chart/ScatterChart.java	Wed Jul 30 10:51:32 2014 +0200
@@ -39,6 +39,8 @@
 import com.sun.javafx.charts.Legend;
 import com.sun.javafx.charts.Legend.LegendItem;
 
+import java.util.Iterator;
+
 /**
  * Chart type that plots symbols for the data points in a series.
  * @since JavaFX 2.0
@@ -167,7 +169,8 @@
         // update symbol positions
         for (int seriesIndex=0; seriesIndex < getDataSize(); seriesIndex++) {
             Series<X,Y> series = getData().get(seriesIndex);
-            for (Data<X,Y> item = series.begin; item != null; item = item.next) {
+            for (Iterator<Data<X, Y>> it = getDisplayedDataIterator(series); it.hasNext(); ) {
+                Data<X, Y> item = it.next();
                 double x = getXAxis().getDisplayPosition(item.getCurrentX());
                 double y = getYAxis().getDisplayPosition(item.getCurrentY());
                 if (Double.isNaN(x) || Double.isNaN(y)) {
--- a/modules/controls/src/main/java/javafx/scene/chart/StackedAreaChart.java	Wed Jul 30 20:17:40 2014 +1200
+++ b/modules/controls/src/main/java/javafx/scene/chart/StackedAreaChart.java	Wed Jul 30 10:51:32 2014 +0200
@@ -235,14 +235,9 @@
             if (itemIndex > 0 && itemIndex < series.getDataSize()) {
                 animate = true;
                 int index=0;
-                Data<X,Y> d;
 
-                for (d = series.begin; d != null && index != itemIndex - 1; d = d.next) {
-                    index++;
-                }
-
-                Data<X,Y> p1 = d;
-                Data<X,Y> p2 = d.next.next;
+                Data<X,Y> p1 = series.getItem(itemIndex - 1);
+                Data<X,Y> p2 = series.getItem(itemIndex + 1);
 
                 if (p2 == null) {
                     return;
@@ -537,7 +532,8 @@
             }
             currentSeriesData.clear();
             // now copy actual data of the current series.
-            for(Data<X, Y> item = series.begin; item != null; item = item.next) {
+            for (Iterator<Data<X, Y>> it = getDisplayedDataIterator(series); it.hasNext(); ) {
+                Data<X, Y> item = it.next();
                 DataPointInfo<X, Y> itemInfo = new DataPointInfo<>(item, item.getXValue(),
                         item.getYValue(), PartOf.CURRENT);
                 aggregateData.add(itemInfo);
--- a/modules/controls/src/main/java/javafx/scene/chart/XYChart.java	Wed Jul 30 20:17:40 2014 +1200
+++ b/modules/controls/src/main/java/javafx/scene/chart/XYChart.java	Wed Jul 30 10:51:32 2014 +0200
@@ -28,6 +28,8 @@
 import com.sun.javafx.collections.NonIterableChange;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 
@@ -65,6 +67,8 @@
 import com.sun.javafx.css.converters.BooleanConverter;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
+
 import javafx.css.Styleable;
 import javafx.css.StyleableProperty;
 
@@ -104,68 +108,58 @@
     };
     private final Group plotContent = new Group();
     private final Rectangle plotAreaClip = new Rectangle();
-    /* start pointer of a series linked list. */
-    Series<X,Y> begin = null;
+
+    private final List<Series<X, Y>> displayedSeries = new ArrayList<>();
+
     /** This is called when a series is added or removed from the chart */
     private final ListChangeListener<Series<X,Y>> seriesChanged = c -> {
+        ObservableList<? extends Series<X, Y>> series = c.getList();
         while (c.next()) {
+            // RT-12069, linked list pointers should update when list is permutated.
+            if (c.wasPermutated()) {
+                displayedSeries.sort((o1, o2) -> series.indexOf(o2) - series.indexOf(o1));
+
+            }
+
             if (c.getRemoved().size() > 0) updateLegend();
-            for (Series<X,Y> series : c.getRemoved()) {
-                series.setToRemove = true;
-                series.setChart(null);
-                seriesRemoved(series);
-                int idx = seriesColorMap.remove(series);
+
+            Set<Series<X, Y>> dupCheck = new HashSet<>(displayedSeries);
+            dupCheck.removeAll(c.getRemoved());
+            for (Series<X, Y> d : c.getAddedSubList()) {
+                if (!dupCheck.add(d)) {
+                    throw new IllegalArgumentException("Duplicate series added");
+                }
+            }
+
+            for (Series<X,Y> s : c.getRemoved()) {
+                s.setToRemove = true;
+                s.setChart(null);
+                seriesRemoved(s);
+                int idx = seriesColorMap.remove(s);
                 colorBits.clear(idx);
-//                    seriesDefaultColorIndex --;
             }
+
             for(int i=c.getFrom(); i<c.getTo() && !c.wasPermutated(); i++) {
-                final Series<X,Y> series = c.getList().get(i);
+                final Series<X,Y> s = c.getList().get(i);
                 // add new listener to data
-                series.setChart(XYChart.this);
-                if (series.setToRemove) {
-                    series.setToRemove = false;
-                    series.getChart().seriesBeingRemovedIsAdded(series);
+                s.setChart(XYChart.this);
+                if (s.setToRemove) {
+                    s.setToRemove = false;
+                    s.getChart().seriesBeingRemovedIsAdded(s);
                 }
                 // update linkedList Pointers for series
-                if (XYChart.this.begin == null) {
-                    XYChart.this.begin = getData().get(i);
-                    XYChart.this.begin.next = null;
-                } else {
-                    if (i == 0) {
-                        getData().get(0).next = XYChart.this.begin;
-                        begin = getData().get(0);
-                    } else {
-                        Series<X,Y> ptr = begin;
-                        for (int j = 0; j < i -1 && ptr!=null ; j++) {
-                            ptr = ptr.next;
-                        }
-                        if (ptr != null) {
-                            getData().get(i).next = ptr.next;
-                            ptr.next = getData().get(i);
-                        }
-
-                    }
-                }
+                displayedSeries.add(s);
                 // update default color style class
                 int nextClearBit = colorBits.nextClearBit(0);
                 colorBits.set(nextClearBit, true);
-                series.defaultColorStyleClass = DEFAULT_COLOR+(nextClearBit%8);
-                seriesColorMap.put(series, nextClearBit%8);
+                s.defaultColorStyleClass = DEFAULT_COLOR+(nextClearBit%8);
+                seriesColorMap.put(s, nextClearBit%8);
                 // inform sub-classes of series added
-                seriesAdded(series, i);
+                seriesAdded(s, i);
             }
             if (c.getFrom() < c.getTo()) updateLegend();
             seriesChanged(c);
-            // RT-12069, linked list pointers should update when list is permutated.
-            if (c.wasPermutated() && getData().size() > 0) {
-                XYChart.this.begin = getData().get(0);
-                Series<X,Y> ptr = begin;
-                for(int k = 1; k < getData().size() && ptr != null; k++) {
-                    ptr.next = getData().get(k);
-                    ptr = ptr.next;
-                }
-                ptr.next = null;
-            }
+
         }
         // update axis ranges
         invalidateRange();
@@ -888,12 +882,7 @@
      * @return index of the series in series list
      */
     int getSeriesIndex(Series<X,Y> series) {
-        int itemIndex = 0;
-        for (Series<X,Y> s = XYChart.this.begin; s != null; s = s.next) {
-            if (s == series) break;
-            itemIndex++;
-        }
-        return itemIndex;
+        return displayedSeries.indexOf(series);
     }
 
     /**
@@ -901,11 +890,7 @@
      * @return size of series linked list
      */
     int getSeriesSize() {
-        int count = 0;
-        for (Series<X,Y> d = XYChart.this.begin; d != null; d = d.next) {
-            count++;
-        }
-        return count;
+        return displayedSeries.size();
     }
 
     /**
@@ -916,16 +901,7 @@
      */
     protected final void removeSeriesFromDisplay(Series<X, Y> series) {
         if (series != null) series.setToRemove = false;
-        if (begin == series) {
-            begin = series.next;
-        } else {
-            Series<X,Y> ptr = begin;
-            while(ptr != null && ptr.next != series) {
-                ptr = ptr.next;
-            }
-            if (ptr != null)
-            ptr.next = series.next;
-        }
+        displayedSeries.remove(series);
     }
 
     /**
@@ -936,28 +912,7 @@
      * @return iterator over currently displayed series
      */
     protected final Iterator<Series<X,Y>> getDisplayedSeriesIterator() {
-        return new Iterator<Series<X, Y>>() {
-            private boolean start = true;
-            private Series<X,Y> current = begin;
-            @Override public boolean hasNext() {
-                if (start) {
-                    return current != null;
-                } else {
-                    return current.next != null;
-                }
-            }
-            @Override public Series<X, Y> next() {
-                if (start) {
-                    start = false;
-                } else if (current!=null) {
-                    current = current.next;
-                }
-                return current;
-            }
-            @Override public void remove() {
-                throw new UnsupportedOperationException("We don't support removing items from the displayed series list.");
-            }
-        };
+        return Collections.unmodifiableList(displayedSeries).iterator();
     }
 
     /**
@@ -1040,28 +995,7 @@
      * @return iterator over currently displayed items from this series
      */
     protected final Iterator<Data<X,Y>> getDisplayedDataIterator(final Series<X,Y> series) {
-        return new Iterator<Data<X, Y>>() {
-            private boolean start = true;
-            private Data<X,Y> current = series.begin;
-            @Override public boolean hasNext() {
-                if (start) {
-                    return current != null;
-                } else {
-                    return current.next != null;
-                }
-            }
-            @Override public Data<X, Y> next() {
-                if (start) {
-                    start = false;
-                } else if (current!=null) {
-                    current = current.next;
-                }
-                return current;
-            }
-            @Override public void remove() {
-                throw new UnsupportedOperationException("We don't support removing items from the displayed data list.");
-            }
-        };
+        return Collections.unmodifiableList(series.displayedData).iterator();
     }
 
     /**
@@ -1404,13 +1338,6 @@
         final void setCurrentExtraValue(Object value) { currentExtraValue.setValue(value); }
         final ObjectProperty<Object> currentExtraValueProperty() { return currentExtraValue; }
 
-        /**
-         * Next pointer for the next data item. We maintain a linkedlist of the
-         * data items so even after the data is deleted from the list,
-         * we have a reference to it
-         */
-         protected Data<X,Y> next = null;
-
         // -------------- CONSTRUCTOR -------------------------------------------------
 
         /**
@@ -1472,79 +1399,48 @@
         /** 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
-         * serieses  so even after the series is deleted from the list,
-         * we have a reference to it - needed by BarChart e.g.
-         */
-        Series<X,Y> next = null;
+
+        private List<Data<X, Y>> displayedData = new ArrayList<>();
 
         private final ListChangeListener<Data<X,Y>> dataChangeListener = new ListChangeListener<Data<X, Y>>() {
             @Override public void onChanged(Change<? extends Data<X, Y>> c) {
+                ObservableList<? extends Data<X, Y>> data = c.getList();
                 final XYChart<X, Y> chart = getChart();
                 while (c.next()) {
                     // RT-25187 Probably a sort happened, just reorder the pointers and return.
                     if (c.wasPermutated()) {
-                        Series<X,Y> series = Series.this;
-                        if (series == null || series.getData() == null) return;
-                        Data<X,Y> ptr = begin;
-                        for(int i = 0; i < series.getData().size(); i++) {
-                            Data<X,Y> item = series.getData().get(i);
-                            if (i == 0) {
-                                begin = item;
-                                ptr = begin;
-                                begin.next = null;
-                            } else {
-                                ptr.next = item;
-                                item.next = null;
-                                ptr = item;
-                            }
-                        }
+                        displayedData.sort((o1, o2) -> data.indexOf(o2) - data.indexOf(o1));
                         return;
                     }
+
+                    Set<Data<X, Y>> dupCheck = new HashSet<>(displayedData);
+                    dupCheck.removeAll(c.getRemoved());
+                    for (Data<X, Y> d : c.getAddedSubList()) {
+                        if (!dupCheck.add(d)) {
+                            throw new IllegalArgumentException("Duplicate data added");
+                        }
+                    }
+
                     // update data items reference to series
                     for (Data<X,Y> item : c.getRemoved()) {
                         item.setToRemove = true;
                     }
+
                     if (c.getAddedSize() > 0) {
-                        for (Data<X,Y> itemPtr = begin; itemPtr != null; itemPtr = itemPtr.next) {
+                        for (Data<X, Y> itemPtr : c.getAddedSubList()) {
                             if (itemPtr.setToRemove) {
                                 if (chart != null) chart.dataBeingRemovedIsAdded(itemPtr, Series.this);
                                 itemPtr.setToRemove = false;
                             }
                         }
-                    }
-                    for(int i=c.getFrom(); i<c.getTo(); i++) {
-                        getData().get(i).setSeries(Series.this);
-                        // update linkedList Pointers for data in this series
-                        if (begin == null) {
-                            begin = getData().get(i);
-                            begin.next = null;
+
+                        for (Data<X, Y> d: c.getAddedSubList()) {
+                            d.setSeries(Series.this);
+                        }
+                        if (c.getFrom() == 0) {
+                            displayedData.addAll(0, c.getAddedSubList());
                         } else {
-                            if (i == 0) {
-                                getData().get(0).next = begin;
-                                begin = getData().get(0);
-                            } else {
-                                Data<X,Y> ptr = begin;
-                                for (int j = 0; j < i -1 ; j++) {
-                                    ptr = ptr.next;
-                                }
-                                getData().get(i).next = ptr.next;
-                                ptr.next = getData().get(i);
-                            }
-                        }
-                    }
-                    // check cycle in the data list
-                    // if cycle exists, and the data is not set to be removed,
-                    // eliminate loop and throw exception stating operation not permitted.
-                    // RT-28880 : infinite loop when same data is added to two charts.
-                    Data<X,Y> cycle = checkCycleInList();
-                    if ( cycle != null) {
-                        if (!cycle.setToRemove) {
-                            eliminateLoop(cycle);
-                            throw new IllegalArgumentException(
-                                    "Duplicate data added or same data added to more than one chart ");
+                            displayedData.addAll(displayedData.indexOf(data.get(c.getFrom() - 1)) + 1, c.getAddedSubList());
                         }
                     }
                     // inform chart
@@ -1554,39 +1450,6 @@
             }
         };
 
-        private Data<X,Y> checkCycleInList() {
-            Data<X,Y> slow = null;
-            Data<X,Y> fast = null;
-            slow = fast = begin;
-            while (slow != null && fast != null) {
-                fast = fast.next;
-                if (fast == slow) return slow;
-                if (fast == null) return null;
-                fast = fast.next;
-                if (fast == slow) return fast;
-                slow = slow.next;
-            }
-            return null;
-        }
-
-        private void eliminateLoop(Data<X,Y> cycle) {
-            Data<X,Y> slow = cycle;
-            // Identify the data that is the start of the loop
-            Data<X,Y> fast = begin;
-            //until both the refs are one short of the common element which is the start of the loop
-            while(fast.next != slow.next) {
-                fast = fast.next;
-                slow = slow.next;
-            }
-            Data<X,Y>start = fast.next;
-            //Eliminate loop by setting next pointer of last element to null
-            fast = start;
-            while(fast.next != start) {
-                fast = fast.next;
-            }
-            fast.next = null; //break the loop
-        }
-
         // -------------- PUBLIC PROPERTIES ----------------------------------------
 
         /** Reference to the chart this series belongs to */
@@ -1722,32 +1585,19 @@
          */
         private void removeDataItemRef(Data<X,Y> item) {
             if (item != null) item.setToRemove = false;
-            if (begin == item) {
-                begin = item.next;
-            } else {
-                Data<X,Y> ptr = begin;
-                while(ptr != null && ptr.next != item) {
-                    ptr = ptr.next;
-                }
-                if(ptr != null) ptr.next = item.next;
-            }
+            displayedData.remove(item);
         }
 
         int getItemIndex(Data<X,Y> item) {
-            int itemIndex = 0;
-            for (Data<X,Y> d = begin; d != null; d = d.next) {
-                if (d == item) break;
-                itemIndex++;
-            }
-            return itemIndex;
+            return displayedData.indexOf(item);
+        }
+
+        Data<X, Y> getItem(int i) {
+            return displayedData.get(i);
         }
 
         int getDataSize() {
-            int count = 0;
-            for (Data<X,Y> d = begin; d != null; d = d.next) {
-                count++;
-            }
-            return count;
+            return displayedData.size();
         }
     }