changeset 7224:60c28b4342a4

RT-37429: [TreeTable] An ArrayIndexOutOfBoundsException is thrown when calling getAddedSubList on a Change from TreeTableView's selectedItems.
author jgiles
date Tue, 10 Jun 2014 12:56:40 +1200
parents 27adbe346c85
children 7cbfb3021f40
files modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableViewBehaviorBase.java modules/controls/src/main/java/javafx/scene/control/ListView.java modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java modules/controls/src/main/java/javafx/scene/control/TableCell.java modules/controls/src/main/java/javafx/scene/control/TablePosition.java modules/controls/src/main/java/javafx/scene/control/TableView.java modules/controls/src/main/java/javafx/scene/control/TreeTableCell.java modules/controls/src/main/java/javafx/scene/control/TreeTablePosition.java modules/controls/src/main/java/javafx/scene/control/TreeTableView.java modules/controls/src/test/java/javafx/scene/control/TableViewTest.java modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java
diffstat 12 files changed, 469 insertions(+), 99 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableViewBehaviorBase.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/com/sun/javafx/scene/control/behavior/TableViewBehaviorBase.java	Tue Jun 10 12:56:40 2014 +1200
@@ -225,6 +225,10 @@
 
     protected final ListChangeListener<TablePositionBase> selectedCellsListener = c -> {
         while (c.next()) {
+            if (! c.wasAdded()) {
+                continue;
+            }
+
             TableSelectionModel sm = getSelectionModel();
             if (sm == null) return;
 
--- a/modules/controls/src/main/java/javafx/scene/control/ListView.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/ListView.java	Tue Jun 10 12:56:40 2014 +1200
@@ -1337,9 +1337,9 @@
                         } else if (index < getItemCount() && index >= 0) {
                             // Fix for RT-18969: the list had setAll called on it
                             // Use of makeAtomic is a fix for RT-20945
-                            makeAtomic = true;
+                            startAtomic();
                             clearSelection(index);
-                            makeAtomic = false;
+                            stopAtomic();
                             select(index);
                         } else {
                             // Fix for RT-22079
--- a/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/MultipleSelectionModelBase.java	Tue Jun 10 12:56:40 2014 +1200
@@ -160,8 +160,17 @@
      *                                                                     *
      **********************************************************************/
 
-    // Fix for RT-20945
-    boolean makeAtomic = false;
+    // Fix for RT-20945 (and numerous other issues!)
+    private int atomicityCount = 0;
+    boolean isAtomic() {
+        return atomicityCount > 0;
+    }
+    void startAtomic() {
+        atomicityCount++;
+    }
+    void stopAtomic() {
+        atomicityCount = Math.max(0, --atomicityCount);
+    }
 
 
     /***********************************************************************
@@ -301,7 +310,7 @@
         // resulted in the selectedItems and selectedIndices lists never
         // reporting that they were empty.
         // makeAtomic toggle added to resolve RT-32618
-        makeAtomic = true;
+        startAtomic();
 
         // firstly we make a copy of the selection, so that we can send out
         // the correct details in the selection change event
@@ -314,7 +323,7 @@
 
         // and select the new row
         select(row);
-        makeAtomic = false;
+        stopAtomic();
 
         // fire off a single add/remove/replace notification (rather than
         // individual remove and add notifications) - see RT-33324
@@ -348,7 +357,7 @@
         setSelectedIndex(row);
         focus(row);
 
-        if (! makeAtomic) {
+        if (! isAtomic()) {
             int changeIndex = selectedIndicesSeq.indexOf(row);
             selectedIndicesSeq.callObservers(new NonIterableChange.SimpleAddChange<Integer>(changeIndex, changeIndex+1, selectedIndicesSeq));
         }
@@ -612,7 +621,7 @@
     }
 
     @Override public void clearSelection() {
-        if (! makeAtomic) {
+        if (! isAtomic()) {
             setSelectedIndex(-1);
             focus(-1);
         }
@@ -632,7 +641,7 @@
 
             quietClearSelection();
 
-            if (! makeAtomic) {
+            if (! isAtomic()) {
                 selectedIndicesSeq.callObservers(
                         new NonIterableChange.GenericAddRemoveChange<Integer>(0, 0,
                         removed, selectedIndicesSeq));
--- a/modules/controls/src/main/java/javafx/scene/control/TableCell.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TableCell.java	Tue Jun 10 12:56:40 2014 +1200
@@ -119,7 +119,11 @@
      * storeTableView method.
      */
     private ListChangeListener<TablePosition> selectedListener = c -> {
-        updateSelection();
+        while (c.next()) {
+            if (c.wasAdded() || c.wasRemoved()) {
+                updateSelection();
+            }
+        }
     };
 
     // same as above, but for focus
--- a/modules/controls/src/main/java/javafx/scene/control/TablePosition.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TablePosition.java	Tue Jun 10 12:56:40 2014 +1200
@@ -66,7 +66,9 @@
      */
     public TablePosition(@NamedArg("tableView") TableView<S> tableView, @NamedArg("row") int row, @NamedArg("tableColumn") TableColumn<S,T> tableColumn) {
         super(row, tableColumn);
-        this.controlRef = new WeakReference<TableView<S>>(tableView);
+        this.controlRef = new WeakReference<>(tableView);
+        this.itemRef = new WeakReference<>(
+                row >= 0 && row < tableView.getItems().size() ? tableView.getItems().get(row) : null);
     }
     
     
@@ -78,6 +80,7 @@
      **************************************************************************/
 
     private final WeakReference<TableView<S>> controlRef;
+    private final WeakReference<S> itemRef;
 
 
     /***************************************************************************
@@ -109,7 +112,15 @@
         // Forcing the return type to be TableColumn<S,T>, not TableColumnBase<S,T>
         return super.getTableColumn();
     }
-    
+
+    /**
+     * Returns the item that backs the {@link #getRow()} row}, at the point
+     * in time when this TablePosition was created.
+     */
+    final S getItem() {
+        return itemRef == null ? null : itemRef.get();
+    }
+
     /**
      * Returns a string representation of this {@code TablePosition} object.
      * @return a string representation of this {@code TablePosition} object.
--- a/modules/controls/src/main/java/javafx/scene/control/TableView.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TableView.java	Tue Jun 10 12:56:40 2014 +1200
@@ -1423,7 +1423,6 @@
         
         // update the Comparator property
         final Comparator<S> oldComparator = getComparator();
-
         setComparator(sortOrder.isEmpty() ? null : new TableColumnComparator(sortOrder));
 
         // fire the onSort event and check if it is consumed, if
@@ -1441,10 +1440,20 @@
             return;
         }
 
+        final List<TablePosition> prevState = new ArrayList<>(getSelectionModel().getSelectedCells());
+        final int itemCount = prevState.size();
+
+        // we set makeAtomic to true here, so that we don't fire intermediate
+        // sort events - instead we send a single permutation event at the end
+        // of this method.
+        getSelectionModel().startAtomic();
+
         // get the sort policy and run it
         Callback<TableView<S>, Boolean> sortPolicy = getSortPolicy();
         if (sortPolicy == null) return;
         Boolean success = sortPolicy.call(this);
+
+        getSelectionModel().stopAtomic();
         
         if (success == null || ! success) {
             // the sort was a failure. Need to backout if possible
@@ -1452,6 +1461,33 @@
             TableUtil.handleSortFailure(sortOrder, lastSortEventType, lastSortEventSupportInfo);
             setComparator(oldComparator);
             sortLock = false;
+        } else {
+            // sorting was a success, now we possibly fire an event on the
+            // selection model that the items list has 'permutated' to a new ordering
+
+            // FIXME we should support alternative selection model implementations!
+            if (getSelectionModel() instanceof TableViewArrayListSelectionModel) {
+                final TableViewArrayListSelectionModel<S> sm = (TableViewArrayListSelectionModel<S>) getSelectionModel();
+                final ObservableList<TablePosition<S,?>> newState = (ObservableList<TablePosition<S,?>>)(Object)sm.getSelectedCells();
+
+                List<TablePosition<S, ?>> removed = new ArrayList<>();
+                for (int i = 0; i < itemCount; i++) {
+                    TablePosition<S, ?> prevItem = prevState.get(i);
+                    if (!newState.contains(prevItem)) {
+                        removed.add(prevItem);
+                    }
+                }
+
+                if (!removed.isEmpty()) {
+                    // the sort operation effectively permutates the selectedCells list,
+                    // but we cannot fire a permutation event as we are talking about
+                    // TablePosition's changing (which may reside in the same list
+                    // position before and after the sort). Therefore, we need to fire
+                    // a single add/remove event to cover the added and removed positions.
+                    ListChangeListener.Change<TablePosition<S, ?>> c = new NonIterableChange.GenericAddRemoveChange<>(0, itemCount, removed, newState);
+                    sm.handleSelectedCellsListChangeEvent(c);
+                }
+            }
         }
     }
     
@@ -1945,9 +1981,7 @@
                 clearSelection();
             });
 
-            selectedCellsMap = new SelectedCellsMap<>(c -> {
-                handleSelectedCellsListChangeEvent(c);
-            });
+            selectedCellsMap = new SelectedCellsMap<>(c -> handleSelectedCellsListChangeEvent(c));
 
             selectedItems = new ReadOnlyUnbackedObservableList<S>() {
                 @Override public S get(int i) {
@@ -2106,9 +2140,9 @@
                         } else if (index < getItemCount() && index >= 0) {
                             // Fix for RT-18969: the list had setAll called on it
                             // Use of makeAtomic is a fix for RT-20945
-                            makeAtomic = true;
+                            startAtomic();
                             clearSelection(index);
-                            makeAtomic = false;
+                            stopAtomic();
                             select(index);
                         } else {
                             // Fix for RT-22079
@@ -2161,55 +2195,58 @@
                     //   -- detected a sort has happened
                     //   -- Create a permutation lookup map (1)
                     //   -- dump all the selected indices into a list (2)
-                    //   -- clear the selected items / indexes (3)
-                    //   -- create a list containing the new indices (4)
-                    //   -- for each previously-selected index (5)
+                    //   -- create a list containing the new indices (3)
+                    //   -- for each previously-selected index (4)
                     //     -- if index is in the permutation lookup map
                     //       -- add the new index to the new indices list
-                    //   -- Perform batch selection (6)
+                    //   -- Perform batch selection (5)
 
-                    makeAtomic = true;
+                    startAtomic();
 
                     final int oldSelectedIndex = getSelectedIndex();
 
                     // (1)
                     int length = c.getTo() - c.getFrom();
-                    HashMap<Integer, Integer> pMap = new HashMap<Integer, Integer> (length);
+                    HashMap<Integer, Integer> pMap = new HashMap<> (length);
                     for (int i = c.getFrom(); i < c.getTo(); i++) {
                         pMap.put(i, c.getPermutation(i));
                     }
 
                     // (2)
-                    List<TablePosition<S,?>> selectedIndices =
-                            new ArrayList<TablePosition<S,?>>((ObservableList<TablePosition<S,?>>)(Object)getSelectedCells());
-
+                    List<TablePosition<S,?>> selectedIndices = new ArrayList<>((ObservableList<TablePosition<S,?>>)(Object)getSelectedCells());
 
                     // (3)
-                    clearSelection();
+                    List<TablePosition<S,?>> newIndices = new ArrayList<>(selectedIndices.size());
 
                     // (4)
-                    List<TablePosition<S,?>> newIndices = new ArrayList<TablePosition<S,?>>(getSelectedIndices().size());
+                    boolean selectionIndicesChanged = false;
+                    for (int i = 0; i < selectedIndices.size(); i++) {
+                        final TablePosition<S,?> oldIndex = selectedIndices.get(i);
+                        final int oldRow = oldIndex.getRow();
 
-                    // (5)
-                    for (int i = 0; i < selectedIndices.size(); i++) {
-                        TablePosition<S,?> oldIndex = selectedIndices.get(i);
+                        if (pMap.containsKey(oldRow)) {
+                            int newIndex = pMap.get(oldRow);
 
-                        if (pMap.containsKey(oldIndex.getRow())) {
-                            Integer newIndex = pMap.get(oldIndex.getRow());
+                            selectionIndicesChanged = selectionIndicesChanged || newIndex != oldRow;
+
                             newIndices.add(new TablePosition<>(oldIndex.getTableView(), newIndex, oldIndex.getTableColumn()));
                         }
                     }
 
-                    // (6)
-                    quietClearSelection();
-                    makeAtomic = false;
-                    selectedCellsMap.setAll(newIndices);
-                    selectedCellsSeq.callObservers(new NonIterableChange.SimpleAddChange<>(0, newIndices.size(), selectedCellsSeq));
+                    if (selectionIndicesChanged) {
+                        // (5)
+                        quietClearSelection();
+                        stopAtomic();
 
-                    if (oldSelectedIndex >= 0 && oldSelectedIndex < itemCount) {
-                        int newIndex = c.getPermutation(oldSelectedIndex);
-                        setSelectedIndex(newIndex);
-                        focus(newIndex);
+                        selectedCellsMap.setAll(newIndices);
+
+                        if (oldSelectedIndex >= 0 && oldSelectedIndex < itemCount) {
+                            int newIndex = c.getPermutation(oldSelectedIndex);
+                            setSelectedIndex(newIndex);
+                            focus(newIndex);
+                        }
+                    } else {
+                        stopAtomic();
                     }
                 }
             }
@@ -2247,7 +2284,7 @@
             // resulted in the selectedItems and selectedIndices lists never
             // reporting that they were empty.
             // makeAtomic toggle added to resolve RT-32618
-            makeAtomic = true;
+            startAtomic();
 
             // firstly we make a copy of the selection, so that we can send out
             // the correct details in the selection change event
@@ -2259,7 +2296,7 @@
             // and select the new cell
             select(row, column);
 
-            makeAtomic = false;
+            stopAtomic();
 
             // fire off a single add/remove/replace notification (rather than
             // individual remove and add notifications) - see RT-33324
@@ -2280,10 +2317,6 @@
             // if I'm in cell selection mode but the column is null, I don't want
             // to select the whole row instead...
             if (isCellSelectionEnabled() && column == null) return;
-//            
-//            // If I am not in cell selection mode (so I want to select rows only),
-//            // if a column is given, I return
-//            if (! isCellSelectionEnabled() && column != null) return;
 
             TablePosition<S,?> pos = new TablePosition<>(getTableView(), row, column);
             
@@ -2440,7 +2473,7 @@
 
         @Override public void selectRange(int minRow, TableColumnBase<S,?> minColumn,
                                           int maxRow, TableColumnBase<S,?> maxColumn) {
-            makeAtomic = true;
+            startAtomic();
 
             if (getSelectionMode() == SelectionMode.SINGLE) {
                 quietClearSelection();
@@ -2477,7 +2510,7 @@
                     // end copy/paste
                 }
             }
-            makeAtomic = false;
+            stopAtomic();
 
             // fire off events.
             // Note that focus and selection always goes to maxRow, not _maxRow.
@@ -2516,7 +2549,7 @@
         }
 
         @Override public void clearSelection() {
-            if (! makeAtomic) {
+            if (! isAtomic()) {
                 updateSelectedIndex(-1);
                 focus(-1);
             }
@@ -2733,8 +2766,8 @@
             // }
             //
             // A more efficient solution:
-            final List<Integer> newlySelectedRows = new ArrayList<Integer>();
-            final List<Integer> newlyUnselectedRows = new ArrayList<Integer>();
+            final List<Integer> newlySelectedRows = new ArrayList<>();
+            final List<Integer> newlyUnselectedRows = new ArrayList<>();
 
             while (c.next()) {
                 if (c.wasRemoved()) {
@@ -2764,7 +2797,7 @@
             }
             c.reset();
 
-            if (makeAtomic) {
+            if (isAtomic()) {
                 return;
             }
 
@@ -2772,9 +2805,49 @@
             // the observers of the selectedItems, selectedIndices and
             // selectedCells lists.
 
-            // create an on-demand list of the removed objects contained in the
-            // given rows
-            selectedItems.callObservers(new MappingChange<TablePosition<S,?>, S>(c, cellToItemsMap, selectedItems));
+            // here we are considering whether to notify the observers of the
+            // selectedItems list. However, we can't just blindly do that, as
+            // noted below. This is a part of the fix for RT-37429.
+            while (c.next()) {
+                boolean fireChangeEvent;
+                outer: if (c.wasReplaced()) {
+                    // if a replace happened, we need to check to see if the
+                    // change actually impacts on the selected items - it may
+                    // be that the index changed to the new location of the same
+                    // item (i.e. if a sort occurred). Only if the item has changed
+                    // should we fire an event to the observers of the selectedItems
+                    // list
+                    for (int i = 0; i < c.getRemovedSize(); i++) {
+                        TablePosition<S,?> removed = c.getRemoved().get(i);
+                        S removedItem = removed.getItem();
+
+                        boolean matchFound = false;
+                        for (int j = 0; j < c.getAddedSize(); j++) {
+                            TablePosition<S,?> added = c.getAddedSubList().get(j);
+                            S addedItem = added.getItem();
+
+                            if (removedItem.equals(addedItem)) {
+                                matchFound = true;
+                                break;
+                            }
+                        }
+
+                        if (! matchFound) {
+                            fireChangeEvent = true;
+                            break outer;
+                        }
+                    }
+                    fireChangeEvent = false;
+                } else {
+                    fireChangeEvent = true;
+                }
+
+                if (fireChangeEvent) {
+                    // create an on-demand list of the removed objects contained in the
+                    // given rows.
+                    selectedItems.callObservers(new MappingChange<>(c, cellToItemsMap, selectedItems));
+                }
+            }
             c.reset();
 
             // Fix for RT-31577 - the selectedItems list was going to
@@ -2801,11 +2874,11 @@
                 ListChangeListener.Change<Integer> change = createRangeChange(selectedIndicesSeq, newlySelectedRows);
                 selectedIndicesSeq.callObservers(change);
             } else {
-                selectedIndicesSeq.callObservers(new MappingChange<TablePosition<S,?>, Integer>(c, cellToIndicesMap, selectedIndicesSeq));
+                selectedIndicesSeq.callObservers(new MappingChange<>(c, cellToIndicesMap, selectedIndicesSeq));
                 c.reset();
             }
 
-            selectedCellsSeq.callObservers(new MappingChange<TablePosition<S,?>, TablePosition<S,?>>(c, MappingChange.NOOP_MAP, selectedCellsSeq));
+            selectedCellsSeq.callObservers(new MappingChange<>(c, MappingChange.NOOP_MAP, selectedCellsSeq));
             c.reset();
         }
     }
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableCell.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableCell.java	Tue Jun 10 12:56:40 2014 +1200
@@ -116,7 +116,11 @@
      * storeTableView method.
      */
     private ListChangeListener<TreeTablePosition<S,?>> selectedListener = c -> {
-        updateSelection();
+        while (c.next()) {
+            if (c.wasAdded() || c.wasRemoved()) {
+                updateSelection();
+            }
+        }
     };
 
     // same as above, but for focus
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTablePosition.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTablePosition.java	Tue Jun 10 12:56:40 2014 +1200
@@ -66,8 +66,8 @@
      */
     public TreeTablePosition(@NamedArg("treeTableView") TreeTableView<S> treeTableView, @NamedArg("row") int row, @NamedArg("tableColumn") TreeTableColumn<S,T> tableColumn) {
         super(row, tableColumn);
-        this.controlRef = new WeakReference<TreeTableView<S>>(treeTableView);
-        this.treeItemRef = new WeakReference<TreeItem<S>>(treeTableView.getTreeItem(row));
+        this.controlRef = new WeakReference<>(treeTableView);
+        this.treeItemRef = new WeakReference<>(treeTableView.getTreeItem(row));
     }
     
     
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableView.java	Tue Jun 10 12:56:40 2014 +1200
@@ -1659,16 +1659,11 @@
         
         // update the Comparator property
         final Comparator<TreeItem<S>> oldComparator = getComparator();
-        if (sortOrder.isEmpty()) {
-            setComparator(null);
-        } else {
-            Comparator<TreeItem<S>> newComparator = new TableColumnComparatorBase.TreeTableColumnComparator(sortOrder);
-            setComparator(newComparator);
-        }
+        setComparator(sortOrder.isEmpty() ? null : new TableColumnComparatorBase.TreeTableColumnComparator(sortOrder));
         
         // fire the onSort event and check if it is consumed, if
         // so, don't run the sort
-        SortEvent<TreeTableView<S>> sortEvent = new SortEvent<TreeTableView<S>>(TreeTableView.this, TreeTableView.this);
+        SortEvent<TreeTableView<S>> sortEvent = new SortEvent<>(TreeTableView.this, TreeTableView.this);
         fireEvent(sortEvent);
         if (sortEvent.isConsumed()) {
             // if the sort is consumed we could back out the last action (the code
@@ -1681,10 +1676,20 @@
             return;
         }
 
+        final List<TreeTablePosition<S,?>> prevState = new ArrayList<>(getSelectionModel().getSelectedCells());
+        final int itemCount = prevState.size();
+
+        // we set makeAtomic to true here, so that we don't fire intermediate
+        // sort events - instead we send a single permutation event at the end
+        // of this method.
+        getSelectionModel().startAtomic();
+
         // get the sort policy and run it
         Callback<TreeTableView<S>, Boolean> sortPolicy = getSortPolicy();
         if (sortPolicy == null) return;
         Boolean success = sortPolicy.call(this);
+
+        getSelectionModel().stopAtomic();
         
         if (success == null || ! success) {
             // the sort was a failure. Need to backout if possible
@@ -1692,6 +1697,33 @@
             TableUtil.handleSortFailure(sortOrder, lastSortEventType, lastSortEventSupportInfo);
             setComparator(oldComparator);
             sortLock = false;
+        } else {
+            // sorting was a success, now we possibly fire an event on the
+            // selection model that the items list has 'permutated' to a new ordering
+
+            // FIXME we should support alternative selection model implementations!
+            if (getSelectionModel() instanceof TreeTableViewArrayListSelectionModel) {
+                final TreeTableViewArrayListSelectionModel<S> sm = (TreeTableViewArrayListSelectionModel<S>) getSelectionModel();
+                final ObservableList<TreeTablePosition<S, ?>> newState = sm.getSelectedCells();
+
+                List<TreeTablePosition<S, ?>> removed = new ArrayList<>();
+                for (int i = 0; i < itemCount; i++) {
+                    TreeTablePosition<S, ?> prevItem = prevState.get(i);
+                    if (!newState.contains(prevItem)) {
+                        removed.add(prevItem);
+                    }
+                }
+
+                if (!removed.isEmpty()) {
+                    // the sort operation effectively permutates the selectedCells list,
+                    // but we cannot fire a permutation event as we are talking about
+                    // TreeTablePosition's changing (which may reside in the same list
+                    // position before and after the sort). Therefore, we need to fire
+                    // a single add/remove event to cover the added and removed positions.
+                    ListChangeListener.Change<TreeTablePosition<S, ?>> c = new NonIterableChange.GenericAddRemoveChange<>(0, itemCount, removed, newState);
+                    sm.handleSelectedCellsListChangeEvent(c);
+                }
+            }
         }
     }
     
@@ -2206,9 +2238,7 @@
             this.treeTableView.rootProperty().addListener(weakRootPropertyListener);
             updateTreeEventListener(null, treeTableView.getRoot());
 
-            selectedCellsMap = new SelectedCellsMap<>(c -> {
-                handleSelectedCellsListChangeEvent(c);
-            });
+            selectedCellsMap = new SelectedCellsMap<>(c -> handleSelectedCellsListChangeEvent(c));
 
             selectedItems = new ReadOnlyUnbackedObservableList<TreeItem<S>>() {
                 @Override public TreeItem<S> get(int i) {
@@ -2378,7 +2408,7 @@
                         // the items / indices lists get a lot of intermediate
                         // noise. They eventually get the summary event fired
                         // from within shiftSelection, so this is ok.
-                        makeAtomic = true;
+                        startAtomic();
 
                         final int clearIndex = param.getClearIndex();
                         TreeTablePosition<S,?> oldTP = null;
@@ -2400,7 +2430,7 @@
                             selectedCellsMap.add(newTP);
                         }
 
-                        makeAtomic = false;
+                        stopAtomic();
 
                         return null;
                     }
@@ -2475,7 +2505,7 @@
             // resulted in the selectedItems and selectedIndices lists never
             // reporting that they were empty.
             // makeAtomic toggle added to resolve RT-32618
-            makeAtomic = true;
+            startAtomic();
 
             // firstly we make a copy of the selection, so that we can send out
             // the correct details in the selection change event
@@ -2487,7 +2517,7 @@
             // and select the new cell
             select(row, column);
 
-            makeAtomic = false;
+            stopAtomic();
 
             // fire off a single add/remove/replace notification (rather than
             // individual remove and add notifications) - see RT-33324
@@ -2508,24 +2538,17 @@
             // if I'm in cell selection mode but the column is null, I don't want
             // to select the whole row instead...
             if (isCellSelectionEnabled() && column == null) return;
-//            
-//            // If I am not in cell selection mode (so I want to select rows only),
-//            // if a column is given, I return
-//            if (! isCellSelectionEnabled() && column != null) return;
 
             TreeTablePosition<S,?> pos = new TreeTablePosition<>(getTreeTableView(), row, (TreeTableColumn<S,?>)column);
             
             if (getSelectionMode() == SelectionMode.SINGLE) {
                 quietClearSelection();
             }
+
             selectedCellsMap.add(pos);
 
-//            setSelectedIndex(row);
             updateSelectedIndex(row);
-            focus(row, (TreeTableColumn<S,?>)column);
-            
-            int changeIndex = selectedCellsSeq.indexOf(pos);
-            selectedCellsSeq.callObservers(new NonIterableChange.SimpleAddChange<TreeTablePosition<S,?>>(changeIndex, changeIndex+1, selectedCellsSeq));
+            focus(row, (TreeTableColumn<S, ?>) column);
         }
 
         @Override public void select(TreeItem<S> obj) {
@@ -2672,7 +2695,7 @@
 
         @Override public void selectRange(int minRow, TableColumnBase<TreeItem<S>,?> minColumn,
                                           int maxRow, TableColumnBase<TreeItem<S>,?> maxColumn) {
-            makeAtomic = true;
+            startAtomic();
 
             if (getSelectionMode() == SelectionMode.SINGLE) {
                 quietClearSelection();
@@ -2709,7 +2732,7 @@
                     // end copy/paste
                 }
             }
-            makeAtomic = false;
+            stopAtomic();
 
             // fire off events
             // Note that focus and selection always goes to maxRow, not _maxRow.
@@ -2746,7 +2769,7 @@
         }
 
         @Override public void clearSelection() {
-            if (! makeAtomic) {
+            if (! isAtomic()) {
                 updateSelectedIndex(-1);
                 focus(-1);
             }
@@ -2970,8 +2993,8 @@
             // }
             //
             // A more efficient solution:
-            final List<Integer> newlySelectedRows = new ArrayList<Integer>();
-            final List<Integer> newlyUnselectedRows = new ArrayList<Integer>();
+            final List<Integer> newlySelectedRows = new ArrayList<>();
+            final List<Integer> newlyUnselectedRows = new ArrayList<>();
 
             while (c.next()) {
                 if (c.wasRemoved()) {
@@ -3001,7 +3024,7 @@
             }
             c.reset();
 
-            if (makeAtomic) {
+            if (isAtomic()) {
                 return;
             }
 
@@ -3009,9 +3032,49 @@
             // the observers of the selectedItems, selectedIndices and
             // selectedCells lists.
 
-            // create an on-demand list of the removed objects contained in the
-            // given rows
-            selectedItems.callObservers(new MappingChange<TreeTablePosition<S,?>, TreeItem<S>>(c, cellToItemsMap, selectedItems));
+            // here we are considering whether to notify the observers of the
+            // selectedItems list. However, we can't just blindly do that, as
+            // noted below. This is a part of the fix for RT-37429.
+            while (c.next()) {
+                boolean fireChangeEvent;
+                outer: if (c.wasReplaced()) {
+                    // if a replace happened, we need to check to see if the
+                    // change actually impacts on the selected items - it may
+                    // be that the index changed to the new location of the same
+                    // item (i.e. if a sort occurred). Only if the item has changed
+                    // should we fire an event to the observers of the selectedItems
+                    // list
+                    for (int i = 0; i < c.getRemovedSize(); i++) {
+                        TreeTablePosition<S,?> removed = c.getRemoved().get(i);
+                        TreeItem<S> removedTreeItem = removed.getTreeItem();
+
+                        boolean matchFound = false;
+                        for (int j = 0; j < c.getAddedSize(); j++) {
+                            TreeTablePosition<S,?> added = c.getAddedSubList().get(j);
+                            TreeItem<S> addedTreeItem = added.getTreeItem();
+
+                            if (removedTreeItem.equals(addedTreeItem)) {
+                                matchFound = true;
+                                break;
+                            }
+                        }
+
+                        if (! matchFound) {
+                            fireChangeEvent = true;
+                            break outer;
+                        }
+                    }
+                    fireChangeEvent = false;
+                } else {
+                    fireChangeEvent = true;
+                }
+
+                if (fireChangeEvent) {
+                    // create an on-demand list of the removed objects contained in the
+                    // given rows.
+                    selectedItems.callObservers(new MappingChange<>(c, cellToItemsMap, selectedItems));
+                }
+            }
             c.reset();
 
             // Fix for RT-31577 - the selectedItems list was going to
@@ -3038,11 +3101,11 @@
                 ListChangeListener.Change<Integer> change = createRangeChange(selectedIndicesSeq, newlySelectedRows);
                 selectedIndicesSeq.callObservers(change);
             } else {
-                selectedIndicesSeq.callObservers(new MappingChange<TreeTablePosition<S,?>, Integer>(c, cellToIndicesMap, selectedIndicesSeq));
+                selectedIndicesSeq.callObservers(new MappingChange<>(c, cellToIndicesMap, selectedIndicesSeq));
                 c.reset();
             }
 
-            selectedCellsSeq.callObservers(new MappingChange<TreeTablePosition<S,?>, TreeTablePosition<S,?>>(c, MappingChange.NOOP_MAP, selectedCellsSeq));
+            selectedCellsSeq.callObservers(new MappingChange<>(c, MappingChange.NOOP_MAP, selectedCellsSeq));
             c.reset();
         }
     }
--- a/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/test/java/javafx/scene/control/TableViewTest.java	Tue Jun 10 12:56:40 2014 +1200
@@ -3582,4 +3582,93 @@
 
         sl.dispose();
     }
+
+    @Test public void test_rt_37429() {
+        // get the current exception handler before replacing with our own,
+        // as ListListenerHelp intercepts the exception otherwise
+        final Thread.UncaughtExceptionHandler exceptionHandler = Thread.currentThread().getUncaughtExceptionHandler();
+        Thread.currentThread().setUncaughtExceptionHandler((t, e) -> fail("We don't expect any exceptions in this test!"));
+
+        // table columns - 1 column; name
+        TableColumn<String, String> nameColumn = new TableColumn<>("name");
+        nameColumn.setCellValueFactory(param -> new ReadOnlyObjectWrapper(param.getValue()));
+        nameColumn.setPrefWidth(200);
+
+        // table
+        TableView<String> table = new TableView<>();
+        table.setItems(FXCollections.observableArrayList("one", "two", "three", "four", "five"));
+        table.getColumns().addAll(nameColumn);
+
+        table.getSelectionModel().getSelectedItems().addListener((ListChangeListener<String>) c -> {
+            while (c.next()) {
+                if(c.wasRemoved()) {
+                    // The removed list of items must be iterated or the AIOOBE will
+                    // not be thrown when getAddedSubList is called.
+                    c.getRemoved().forEach(item -> {});
+                }
+
+                if (c.wasAdded()) {
+                    c.getAddedSubList();
+                }
+            }
+        });
+
+        StageLoader sl = new StageLoader(table);
+
+        table.getSelectionModel().select(0);
+        table.getSortOrder().add(nameColumn);
+
+        sl.dispose();
+
+        // reset the exception handler
+        Thread.currentThread().setUncaughtExceptionHandler(exceptionHandler);
+    }
+
+    private int rt_37429_items_change_count = 0;
+    private int rt_37429_cells_change_count = 0;
+    @Test public void test_rt_37429_sortEventsShouldNotFireExtraChangeEvents() {
+        // table columns - 1 column; name
+        TableColumn<String, String> nameColumn = new TableColumn<>("name");
+        nameColumn.setCellValueFactory(param -> new ReadOnlyObjectWrapper(param.getValue()));
+        nameColumn.setPrefWidth(200);
+
+        // table
+        TableView<String> table = new TableView<>();
+        table.setItems(FXCollections.observableArrayList("a", "c", "b"));
+        table.getColumns().addAll(nameColumn);
+
+        table.getSelectionModel().getSelectedItems().addListener((ListChangeListener<String>) c -> {
+            while (c.next()) {
+                rt_37429_items_change_count++;
+            }
+        });
+        table.getSelectionModel().getSelectedCells().addListener((ListChangeListener<TablePosition>) c -> {
+            while (c.next()) {
+                rt_37429_cells_change_count++;
+            }
+        });
+
+        StageLoader sl = new StageLoader(table);
+
+        assertEquals(0, rt_37429_items_change_count);
+        assertEquals(0, rt_37429_cells_change_count);
+
+        table.getSelectionModel().select(0);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(1, rt_37429_cells_change_count);
+
+        table.getSortOrder().add(nameColumn);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(1, rt_37429_cells_change_count);
+
+        nameColumn.setSortType(TableColumn.SortType.DESCENDING);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(2, rt_37429_cells_change_count);
+
+        nameColumn.setSortType(TableColumn.SortType.ASCENDING);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(3, rt_37429_cells_change_count);
+
+        sl.dispose();
+    }
 }
--- a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewMouseInputTest.java	Tue Jun 10 12:56:40 2014 +1200
@@ -530,7 +530,7 @@
         final int items = 8;
         root.getChildren().clear();
         root.setExpanded(true);
-        for (int i = 0; i < items; i++) {
+        for (int i = 1; i < items; i++) {
             root.getChildren().add(new TreeItem<>("Row " + i));
         }
         tableView.setRoot(root);
--- a/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java	Mon Jun 09 16:45:35 2014 -0700
+++ b/modules/controls/src/test/java/javafx/scene/control/TreeTableViewTest.java	Tue Jun 10 12:56:40 2014 +1200
@@ -2343,7 +2343,7 @@
 
         // now test to ensure that CCC is still the only selected item, but now
         // located in index 1 (as the first child of the root)
-        assertTrue(sm.isSelected(1));
+        assertTrue(debug(), sm.isSelected(1));
         assertEquals(1, sm.getSelectedIndex());
         assertEquals(1, sm.getSelectedIndices().size());
         assertTrue(sm.getSelectedIndices().contains(1));
@@ -3606,4 +3606,117 @@
 
         sl.dispose();
     }
+
+    @Test public void test_rt_37429() {
+        // get the current exception handler before replacing with our own,
+        // as ListListenerHelp intercepts the exception otherwise
+        final Thread.UncaughtExceptionHandler exceptionHandler = Thread.currentThread().getUncaughtExceptionHandler();
+        Thread.currentThread().setUncaughtExceptionHandler((t, e) -> {
+            e.printStackTrace();
+            fail("We don't expect any exceptions in this test!");
+        });
+
+        // table items - 3 items, 2nd item has 2 children
+        TreeItem<String> root = new TreeItem<>();
+
+        TreeItem<String> two = new TreeItem<>("two");
+        two.getChildren().add(new TreeItem<>("childOne"));
+        two.getChildren().add(new TreeItem<>("childTwo"));
+        two.setExpanded(true);
+
+        root.getChildren().add(new TreeItem<>("one"));
+        root.getChildren().add(two);
+        root.getChildren().add(new TreeItem<>("three"));
+
+        // table columns - 1 column; name
+        TreeTableColumn<String, String> nameColumn = new TreeTableColumn<>("name");
+        nameColumn.setCellValueFactory(param -> new ReadOnlyObjectWrapper(param.getValue().getValue()));
+        nameColumn.setPrefWidth(200);
+
+        // table
+        TreeTableView<String> table = new TreeTableView<>();
+        table.setShowRoot(false);
+        table.setRoot(root);
+        table.getColumns().addAll(nameColumn);
+
+        table.getSelectionModel().getSelectedItems().addListener((ListChangeListener<TreeItem<String>>) c -> {
+            while (c.next()) {
+                if(c.wasRemoved()) {
+                    // The removed list of items must be iterated or the AIOOBE will
+                    // not be thrown when getAddedSubList is called.
+                    c.getRemoved().forEach(item -> {});
+                }
+
+                if (c.wasAdded()) {
+                    c.getAddedSubList();
+                }
+            }
+        });
+
+        StageLoader sl = new StageLoader(table);
+
+        table.getSelectionModel().select(0);
+        table.getSortOrder().add(nameColumn);
+
+        sl.dispose();
+
+        // reset the exception handler
+        Thread.currentThread().setUncaughtExceptionHandler(exceptionHandler);
+    }
+
+    private int rt_37429_items_change_count = 0;
+    private int rt_37429_cells_change_count = 0;
+    @Test public void test_rt_37429_sortEventsShouldNotFireExtraChangeEvents() {
+        // table items - 3 items, 2nd item has 2 children
+        TreeItem<String> root = new TreeItem<>();
+
+        root.getChildren().add(new TreeItem<>("a"));
+        root.getChildren().add(new TreeItem<>("c"));
+        root.getChildren().add(new TreeItem<>("b"));
+
+        // table columns - 1 column; name
+        TreeTableColumn<String, String> nameColumn = new TreeTableColumn<>("name");
+        nameColumn.setCellValueFactory(param -> new ReadOnlyObjectWrapper(param.getValue().getValue()));
+        nameColumn.setPrefWidth(200);
+
+        // table
+        TreeTableView<String> table = new TreeTableView<>();
+        table.setShowRoot(false);
+        table.setRoot(root);
+        table.getColumns().addAll(nameColumn);
+
+        table.getSelectionModel().getSelectedItems().addListener((ListChangeListener<TreeItem<String>>) c -> {
+            while (c.next()) {
+                rt_37429_items_change_count++;
+            }
+        });
+        table.getSelectionModel().getSelectedCells().addListener((ListChangeListener<TreeTablePosition<String, ?>>) c -> {
+            while (c.next()) {
+                rt_37429_cells_change_count++;
+            }
+        });
+
+        StageLoader sl = new StageLoader(table);
+
+        assertEquals(0, rt_37429_items_change_count);
+        assertEquals(0, rt_37429_cells_change_count);
+
+        table.getSelectionModel().select(0);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(1, rt_37429_cells_change_count);
+
+        table.getSortOrder().add(nameColumn);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(1, rt_37429_cells_change_count);
+
+        nameColumn.setSortType(TreeTableColumn.SortType.DESCENDING);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(2, rt_37429_cells_change_count);
+
+        nameColumn.setSortType(TreeTableColumn.SortType.ASCENDING);
+        assertEquals(1, rt_37429_items_change_count);
+        assertEquals(3, rt_37429_cells_change_count);
+
+        sl.dispose();
+    }
 }