changeset 6553:66768fc644fb

RT-35864: do not call updateItem unless the index has changed
author David Grieve<david.grieve@oracle.com>
date Thu, 27 Mar 2014 08:12:42 -0400
parents 755b1d48c070
children fa16857ad91c
files modules/controls/src/main/java/javafx/scene/control/IndexedCell.java modules/controls/src/main/java/javafx/scene/control/ListCell.java modules/controls/src/main/java/javafx/scene/control/TableCell.java modules/controls/src/main/java/javafx/scene/control/TableRow.java modules/controls/src/main/java/javafx/scene/control/TreeCell.java modules/controls/src/main/java/javafx/scene/control/TreeTableCell.java modules/controls/src/main/java/javafx/scene/control/TreeTableRow.java
diffstat 7 files changed, 105 insertions(+), 116 deletions(-) [+]
line wrap: on
line diff
--- a/modules/controls/src/main/java/javafx/scene/control/IndexedCell.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/IndexedCell.java	Thu Mar 27 08:12:42 2014 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -110,16 +110,19 @@
      *         by those implementing new Skins. It is not common
      *         for developers or designers to access this function directly.
      */
-    public void updateIndex(int i) { 
+    public void updateIndex(int i) {
+        final int oldIndex = index.get();
         index.set(i);
-        indexChanged();
+        indexChanged(oldIndex, i);
     }
     
     /** 
      * This method is called whenever the index is changed, regardless of whether
-     * the new index is the same as the old index. 
+     * the new index is the same as the old index.
+     * @param oldIndex
+     * @param newIndex
      */
-    void indexChanged() { 
+    void indexChanged(int oldIndex, int newIndex) {
         // no-op
     }
     
--- a/modules/controls/src/main/java/javafx/scene/control/ListCell.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/ListCell.java	Thu Mar 27 08:12:42 2014 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -151,7 +151,7 @@
      */
     private final ListChangeListener<T> itemsListener = new ListChangeListener<T>() {
         @Override public void onChanged(ListChangeListener.Change<? extends T> c) {
-            updateItem();
+            updateItem(-1);
         }
     };
 
@@ -169,7 +169,7 @@
             if (newValue != null) {
                 newValue.addListener(weakItemsListener);
             }
-            updateItem();
+            updateItem(-1);
         }
     };
 
@@ -285,7 +285,7 @@
                 weakListViewRef = new WeakReference<ListView<T>>(currentListView);
             }
 
-            updateItem();
+            updateItem(-1);
             updateSelection();
             updateFocus();
             requestLayout();
@@ -303,15 +303,11 @@
      *                                                                         *
      **************************************************************************/
 
-    private int index = -1;
+    /** {@inheritDoc} */
+    @Override void indexChanged(int oldIndex, int newIndex) {
+        super.indexChanged(oldIndex, newIndex);
 
-    /** {@inheritDoc} */
-    @Override void indexChanged() {
-        final int oldIndex = index;
-        super.indexChanged();
-        index = getIndex();
-
-        if (isEditing() && index == oldIndex) {
+        if (isEditing() && newIndex == oldIndex) {
             // no-op
             // Fix for RT-31165 - if we (needlessly) update the index whilst the
             // cell is being edited it will no longer be in an editing state.
@@ -320,7 +316,7 @@
             // will not change to the editing state as a layout of VirtualFlow
             // is immediately invoked, which forces all cells to be updated.
         } else {
-            updateItem();
+            updateItem(oldIndex);
             updateSelection();
             updateFocus();
         }
@@ -435,7 +431,7 @@
      **************************************************************************/
 
     private boolean firstRun = true;
-    private void updateItem() {
+    private void updateItem(int oldIndex) {
         final ListView<T> lv = getListView();
         final List<T> items = lv == null ? null : lv.getItems();
         final int index = getIndex();
@@ -451,11 +447,13 @@
         if (valid) {
             final T newValue = items.get(index);
 
-            // There used to be conditional code here to prevent updateItem from
-            // being called when the value didn't change, but that led us to
-            // issues such as RT-33108, where the value didn't change but the item
-            // we needed to be listening to did. Without calling updateItem we
-            // were breaking things, so once again the conditionals are gone.
+            // RT-34566 - if the index didn't change, then avoid calling updateItem
+            // unless the item has changed.
+            if (oldIndex == index) {
+                if (oldValue != null ? oldValue.equals(newValue) : newValue == null) {
+                    return;
+                }
+            }
             updateItem(newValue, false);
         } else {
             // RT-30484 We need to allow a first run to be special-cased to allow
--- a/modules/controls/src/main/java/javafx/scene/control/TableCell.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TableCell.java	Thu Mar 27 08:12:42 2014 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -301,7 +301,7 @@
         // updateItem normally, when it comes to unit tests we can't have the
         // item change in all circumstances.
         if (! lockItemOnEdit) {
-            updateItem();
+            updateItem(-1);
         }
 
         // it makes sense to get the cell into its editing state before firing
@@ -445,15 +445,16 @@
             tableView.getVisibleLeafColumns().removeListener(weakVisibleLeafColumnsListener);
         }        
     }
-    
-    @Override void indexChanged() {
-        super.indexChanged();
+
+    @Override void indexChanged(int oldIndex, int newIndex) {
+        super.indexChanged(oldIndex, newIndex);
+
         // Ideally we would just use the following two lines of code, rather
         // than the updateItem() call beneath, but if we do this we end up with
         // RT-22428 where all the columns are collapsed.
         // itemDirty = true;
         // requestLayout();
-        updateItem();
+        updateItem(oldIndex);
         updateSelection();
         updateFocus();
     }
@@ -576,7 +577,7 @@
      * changed. You'll note that this is a private function - it is only called
      * when one of the triggers above call it.
      */
-    private void updateItem() {
+    private void updateItem(int oldIndex) {
         if (currentObservableValue != null) {
             currentObservableValue.removeListener(weaktableRowUpdateObserver);
         }
@@ -620,11 +621,13 @@
             currentObservableValue = tableColumn.getCellObservableValue(index);
             final T newValue = currentObservableValue == null ? null : currentObservableValue.getValue();
 
-            // There used to be conditional code here to prevent updateItem from
-            // being called when the value didn't change, but that led us to
-            // issues such as RT-33108, where the value didn't change but the item
-            // we needed to be listening to did. Without calling updateItem we
-            // were breaking things, so once again the conditionals are gone.
+            // RT-34566 - if the index didn't change, then avoid calling updateItem
+            // unless the item has changed.
+            if (oldIndex == index) {
+                if (oldValue != null ? oldValue.equals(newValue) : newValue == null) {
+                    return;
+                }
+            }
             updateItem(newValue, false);
         }
         
@@ -638,7 +641,7 @@
 
     @Override protected void layoutChildren() {
         if (itemDirty) {
-            updateItem();
+            updateItem(-1);
             itemDirty = false;
         }
         super.layoutChildren();
--- a/modules/controls/src/main/java/javafx/scene/control/TableRow.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TableRow.java	Thu Mar 27 08:12:42 2014 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2014, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -226,27 +226,17 @@
      *                                                                         *
      **************************************************************************/
 
-    private int oldIndex = -1;
-    
     /** {@inheritDoc} */
-    @Override void indexChanged() {
-        int newIndex = getIndex();
-        
-        super.indexChanged();
-        
-        // Below we check if the index has changed, but we always call updateItem,
-        // as the value in the given index may have changed.
-        updateItem(newIndex);
-        
-        if (oldIndex == newIndex) return;
-        oldIndex = newIndex;
-        
+    @Override void indexChanged(int oldIndex, int newIndex) {
+        super.indexChanged(oldIndex, newIndex);
+
+        updateItem(oldIndex);
         updateSelection();
         updateFocus();
     }
 
     private boolean isFirstRun = true;
-    private void updateItem(int newIndex) {
+    private void updateItem(int oldIndex) {
         TableView<T> tv = getTableView();
         if (tv == null || tv.getItems() == null) return;
         
@@ -254,6 +244,7 @@
         final int itemCount = items == null ? -1 : items.size();
 
         // Compute whether the index for this cell is for a real item
+        final int newIndex = getIndex();
         boolean valid = newIndex >= 0 && newIndex < itemCount;
 
         final T oldValue = getItem();
@@ -263,11 +254,13 @@
         if (valid) {
             final T newValue = items.get(newIndex);
 
-            // There used to be conditional code here to prevent updateItem from
-            // being called when the value didn't change, but that led us to
-            // issues such as RT-33108, where the value didn't change but the item
-            // we needed to be listening to did. Without calling updateItem we
-            // were breaking things, so once again the conditionals are gone.
+            // RT-34566 - if the index didn't change, then avoid calling updateItem
+            // unless the item has changed.
+            if (oldIndex == newIndex) {
+                if (oldValue != null ? oldValue.equals(newValue) : newValue == null) {
+                    return;
+                }
+            }
             updateItem(newValue, false);
         } else {
             // RT-30484 We need to allow a first run to be special-cased to allow
--- a/modules/controls/src/main/java/javafx/scene/control/TreeCell.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeCell.java	Thu Mar 27 08:12:42 2014 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2014, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -25,9 +25,6 @@
 
 package javafx.scene.control;
 
-import com.sun.javafx.scene.control.skin.VirtualContainerBase;
-import com.sun.javafx.scene.control.skin.VirtualFlow;
-import javafx.collections.FXCollections;
 import javafx.css.PseudoClass;
 import javafx.beans.InvalidationListener;
 import javafx.beans.Observable;
@@ -39,8 +36,6 @@
 import com.sun.javafx.scene.control.skin.TreeCellSkin;
 import javafx.collections.WeakListChangeListener;
 import java.lang.ref.WeakReference;
-import java.util.ArrayList;
-import java.util.List;
 
 import javafx.beans.property.BooleanProperty;
 import javafx.beans.property.ReadOnlyObjectProperty;
@@ -48,7 +43,6 @@
 import javafx.beans.value.ChangeListener;
 import javafx.beans.value.ObservableValue;
 import javafx.beans.value.WeakChangeListener;
-import javafx.scene.Parent;
 import javafx.scene.accessibility.Action;
 import javafx.scene.accessibility.Attribute;
 import javafx.scene.accessibility.Role;
@@ -182,7 +176,7 @@
 
     private final InvalidationListener rootPropertyListener = new InvalidationListener() {
         @Override public void invalidated(Observable observable) {
-            updateItem();
+            updateItem(-1);
         }
     };
     
@@ -322,7 +316,7 @@
                 weakTreeViewRef = new WeakReference<TreeView<T>>(treeView);
             }
 
-            updateItem();
+            updateItem(-1);
             requestLayout();
         }
 
@@ -373,7 +367,7 @@
             return;
         }
 
-        updateItem();
+        updateItem(-1);
         
         // it makes sense to get the cell into its editing state before firing
         // the event to the TreeView below, so that's what we're doing here
@@ -468,17 +462,14 @@
      * Private Implementation                                                  *
      *                                                                         *
      **************************************************************************/
-    
-    private int index = -1;
 
-    @Override void indexChanged() {
-        final int oldIndex = index;
+    /** {@inheritDoc} */
+    @Override void indexChanged(int oldIndex, int newIndex) {
+        super.indexChanged(oldIndex, newIndex);
 
-        index = getIndex();
-        
         // when the cell index changes, this may result in the cell
         // changing state to be selected and/or focused.
-        if (isEditing() && index == oldIndex) {
+        if (isEditing() && newIndex == oldIndex) {
             // no-op
             // Fix for RT-31165 - if we (needlessly) update the index whilst the
             // cell is being edited it will no longer be in an editing state.
@@ -487,18 +478,19 @@
             // will not change to the editing state as a layout of VirtualFlow
             // is immediately invoked, which forces all cells to be updated.
         } else {
-            updateItem();
+            updateItem(oldIndex);
             updateSelection();
             updateFocus();
         }
     }
 
     private boolean isFirstRun = true;
-    private void updateItem() {
+    private void updateItem(int oldIndex) {
         TreeView<T> tv = getTreeView();
         if (tv == null) return;
         
         // Compute whether the index for this cell is for a real item
+        int index = getIndex();
         boolean valid = index >=0 && index < tv.getExpandedItemCount();
         final boolean isEmpty = isEmpty();
         final TreeItem<T> oldTreeItem = getTreeItem();
@@ -509,18 +501,21 @@
             // get the new treeItem that is about to go in to the TreeCell
             TreeItem<T> newTreeItem = tv.getTreeItem(index);
             T newValue = newTreeItem == null ? null : newTreeItem.getValue();
-        
+            T oldValue = oldTreeItem == null ? null : oldTreeItem.getValue();
+
             // For the sake of RT-14279, it is important that the order of these
             // method calls is as shown below. If the order is switched, it is
             // likely that events will be fired where the item is null, even
             // though calling cell.getTreeItem().getValue() returns the value
             // as expected
 
-            // There used to be conditional code here to prevent updateItem from
-            // being called when the value didn't change, but that led us to
-            // issues such as RT-33108, where the value didn't change but the item
-            // we needed to be listening to did. Without calling updateItem we
-            // were breaking things, so once again the conditionals are gone.
+            // RT-34566 - if the index didn't change, then avoid calling updateItem
+            // unless the item has changed.
+            if (oldIndex == index) {
+                if (oldValue != null ? oldValue.equals(newValue) : newValue == null) {
+                    return;
+                }
+            }
             updateTreeItem(newTreeItem);
             updateItem(newValue, false);
         } else {
@@ -540,20 +535,20 @@
 
     private void updateSelection() {
         if (isEmpty()) return;
-        if (index == -1 || getTreeView() == null) return;
+        if (getIndex() == -1 || getTreeView() == null) return;
         if (getTreeView().getSelectionModel() == null) return;
         
-        boolean isSelected = getTreeView().getSelectionModel().isSelected(index);
+        boolean isSelected = getTreeView().getSelectionModel().isSelected(getIndex());
         if (isSelected() == isSelected) return;
         
         updateSelected(isSelected);
     }
 
     private void updateFocus() {
-        if (index == -1 || getTreeView() == null) return;
+        if (getIndex() == -1 || getTreeView() == null) return;
         if (getTreeView().getFocusModel() == null) return;
         
-        setFocused(getTreeView().getFocusModel().isFocused(index));
+        setFocused(getTreeView().getFocusModel().isFocused(getIndex()));
     }
 
     private boolean updateEditingIndex = true;
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableCell.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableCell.java	Thu Mar 27 08:12:42 2014 -0400
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -165,7 +165,7 @@
 
     private final InvalidationListener rootPropertyListener = new InvalidationListener() {
         @Override public void invalidated(Observable observable) {
-            updateItem();
+            updateItem(-1);
         }
     };
     
@@ -309,7 +309,7 @@
         // updateItem normally, when it comes to unit tests we can't have the
         // item change in all circumstances.
         if (! lockItemOnEdit) {
-            updateItem();
+            updateItem(-1);
         }
 
         // it makes sense to get the cell into its editing state before firing
@@ -429,21 +429,11 @@
      *                                                                         *
      **************************************************************************/
 
-    private int index = -1;
+    /** {@inheritDoc} */
+    @Override void indexChanged(int oldIndex, int newIndex) {
+        super.indexChanged(oldIndex, newIndex);
 
-    @Override void indexChanged() {
-        super.indexChanged();
-        // Ideally we would just use the following two lines of code, rather
-        // than the updateItem() call beneath, but if we do this we end up with
-        // RT-22428 where all the columns are collapsed.
-        // itemDirty = true;
-        // requestLayout();
-
-        final int oldIndex = index;
-        super.indexChanged();
-        index = getIndex();
-
-        if (isEditing() && index == oldIndex) {
+        if (isEditing() && newIndex == oldIndex) {
             // no-op
             // Fix for RT-31165 - if we (needlessly) update the index whilst the
             // cell is being edited it will no longer be in an editing state.
@@ -452,7 +442,12 @@
             // will not change to the editing state as a layout of VirtualFlow
             // is immediately invoked, which forces all cells to be updated.
         } else {
-            updateItem();
+            // Ideally we would just use the following two lines of code, rather
+            // than the updateItem() call beneath, but if we do this we end up with
+            // RT-22428 where all the columns are collapsed.
+            // itemDirty = true;
+            // requestLayout();
+            updateItem(oldIndex);
             updateSelection();
             updateFocus();
         }
@@ -577,7 +572,7 @@
      * changed. You'll note that this is a private function - it is only called
      * when one of the triggers above call it.
      */
-    private void updateItem() {
+    private void updateItem(int oldIndex) {
         if (currentObservableValue != null) {
             currentObservableValue.removeListener(weaktableRowUpdateObserver);
         }
@@ -622,11 +617,13 @@
             
             final T newValue = currentObservableValue == null ? null : currentObservableValue.getValue();
 
-            // There used to be conditional code here to prevent updateItem from
-            // being called when the value didn't change, but that led us to
-            // issues such as RT-33108, where the value didn't change but the item
-            // we needed to be listening to did. Without calling updateItem we
-            // were breaking things, so once again the conditionals are gone.
+            // RT-34566 - if the index didn't change, then avoid calling updateItem
+            // unless the item has changed.
+            if (oldIndex == index) {
+                if (oldValue != null ? oldValue.equals(newValue) : newValue == null) {
+                    return;
+                }
+            }
             updateItem(newValue, false);
         }
         
@@ -640,7 +637,7 @@
 
     @Override protected void layoutChildren() {
         if (itemDirty) {
-            updateItem();
+            updateItem(-1);
             itemDirty = false;
         }
         super.layoutChildren();
--- a/modules/controls/src/main/java/javafx/scene/control/TreeTableRow.java	Wed Mar 26 14:40:56 2014 -0700
+++ b/modules/controls/src/main/java/javafx/scene/control/TreeTableRow.java	Thu Mar 27 08:12:42 2014 -0400
@@ -25,9 +25,7 @@
 
 package javafx.scene.control;
 
-import com.sun.javafx.scene.control.skin.VirtualContainerBase;
 import com.sun.javafx.scene.control.skin.VirtualFlow;
-import javafx.collections.FXCollections;
 import javafx.css.PseudoClass;
 import com.sun.javafx.scene.control.skin.TreeTableRowSkin;
 import java.lang.ref.WeakReference;
@@ -285,10 +283,12 @@
      *                                                                         *
      * Public API                                                              *
      *                                                                         *
-     **************************************************************************/
+     *************************************************************************
+     * @param oldIndex
+     * @param newIndex*/
 
     
-    @Override void indexChanged() {
+    @Override void indexChanged(int oldIndex, int newIndex) {
         index = getIndex();
 
         // when the cell index changes, this may result in the cell