Skip to content

Commit

Permalink
8187145: (Tree)TableView with null selectionModel: throws NPE on sorting
Browse files Browse the repository at this point in the history 
Reviewed-by: aghaisas, kcr
  • Loading branch information
Andy Goryachev committed Dec 15, 2022
1 parent adfc022 commit 58376eb
Show file tree
Hide file tree
Showing 15 changed files with 457 additions and 91 deletions.
24 changes: 17 additions & 7 deletions ...javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/CellBehaviorBase.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2022, 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
Expand Down Expand Up @@ -271,15 +271,21 @@ protected void doSelect(final double x, final double y, final MouseButton button

protected void simpleSelect(MouseButton button, int clickCount, boolean shortcutDown) {
final int index = getIndex();
MultipleSelectionModel<?> sm = getSelectionModel();
boolean isAlreadySelected = sm.isSelected(index);
boolean isAlreadySelected;

if (isAlreadySelected && shortcutDown) {
sm.clearSelection(index);
getFocusModel().focus(index);
MultipleSelectionModel<?> sm = getSelectionModel();
if (sm == null) {
isAlreadySelected = false;
} else {
sm.clearAndSelect(index);
isAlreadySelected = sm.isSelected(index);

if (isAlreadySelected && shortcutDown) {
sm.clearSelection(index);
getFocusModel().focus(index);
isAlreadySelected = false;
} else {
sm.clearAndSelect(index);
}
}

handleClicks(button, clickCount, isAlreadySelected);
Expand All @@ -300,6 +306,10 @@ protected void handleClicks(MouseButton button, int clickCount, boolean isAlread
}

void selectRows(int focusedIndex, int index) {
if (getSelectionModel() == null) {
return;
}

final boolean asc = focusedIndex < index;

// and then determine all row and columns which must be selected
Expand Down
25 changes: 16 additions & 9 deletions ...x.controls/src/main/java/com/sun/javafx/scene/control/behavior/TableCellBehaviorBase.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2022, 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
Expand Down Expand Up @@ -204,23 +204,30 @@ protected void doSelect(final double x, final double y, final MouseButton button
@Override
protected void simpleSelect(MouseButton button, int clickCount, boolean shortcutDown) {
final TableSelectionModel<S> sm = getSelectionModel();
final int row = getNode().getIndex();
final TableColumnBase<S,T> column = getTableColumn();
boolean isAlreadySelected = sm.isSelected(row, column);
boolean isAlreadySelected;

if (isAlreadySelected && shortcutDown) {
sm.clearSelection(row, column);
getFocusModel().focus(row, (TC) column);
if (sm == null) {
isAlreadySelected = false;
} else {
// we check if cell selection is enabled to fix RT-33897
sm.clearAndSelect(row, column);
final int row = getNode().getIndex();
final TableColumnBase<S,T> column = getTableColumn();
isAlreadySelected = sm.isSelected(row, column);

if (isAlreadySelected && shortcutDown) {
sm.clearSelection(row, column);
getFocusModel().focus(row, (TC) column);
isAlreadySelected = false;
} else {
// we check if cell selection is enabled to fix RT-33897
sm.clearAndSelect(row, column);
}
}

handleClicks(button, clickCount, isAlreadySelected);
}

private int getColumn() {
// this method will not be called if selection model is null
if (getSelectionModel().isCellSelectionEnabled()) {
TableColumnBase<S,T> tc = getTableColumn();
return getVisibleLeafIndex(tc);
Expand Down
8 changes: 7 additions & 1 deletion ...avafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TableViewBehavior.java
View file
Expand Up @@ -27,6 +27,7 @@

import javafx.beans.value.ChangeListener;
import javafx.beans.value.WeakChangeListener;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.scene.control.TableColumn;
import javafx.scene.control.TableColumnBase;
Expand Down Expand Up @@ -118,7 +119,12 @@ public void dispose() {

/** {@inheritDoc} */
@Override protected ObservableList<TablePosition> getSelectedCells() {
return getNode().getSelectionModel().getSelectedCells();
TableViewSelectionModel<T> sm = getNode().getSelectionModel();
if (sm == null) {
return FXCollections.emptyObservableList();
} else {
return sm.getSelectedCells();
}
}

/** {@inheritDoc} */
Expand Down
17 changes: 13 additions & 4 deletions ...x.controls/src/main/java/com/sun/javafx/scene/control/behavior/TreeTableViewBehavior.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2022, 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
Expand Down Expand Up @@ -29,6 +29,7 @@

import javafx.beans.value.ChangeListener;
import javafx.beans.value.WeakChangeListener;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;
import javafx.scene.control.TableColumnBase;
import javafx.scene.control.TableFocusModel;
Expand All @@ -38,6 +39,7 @@
import javafx.scene.control.TreeTableColumn;
import javafx.scene.control.TreeTablePosition;
import javafx.scene.control.TreeTableView;
import javafx.scene.control.TreeTableView.TreeTableViewSelectionModel;
import com.sun.javafx.scene.control.inputmap.InputMap;
import javafx.util.Callback;

Expand Down Expand Up @@ -121,7 +123,12 @@ public TreeTableViewBehavior(TreeTableView<T> control) {

/** {@inheritDoc} */
@Override protected ObservableList<TreeTablePosition<T,?>> getSelectedCells() {
return getNode().getSelectionModel().getSelectedCells();
TreeTableViewSelectionModel<T> sm = getNode().getSelectionModel();
if (sm == null) {
return FXCollections.observableArrayList();
} else {
return sm.getSelectedCells();
}
}

/** {@inheritDoc} */
Expand Down Expand Up @@ -187,7 +194,8 @@ public TreeTableViewBehavior(TreeTableView<T> control) {
* on whether we are in row or cell selection.
*/
private void rightArrowPressed() {
if (getNode().getSelectionModel().isCellSelectionEnabled()) {
TreeTableViewSelectionModel<T> sm = getNode().getSelectionModel();
if ((sm != null) && sm.isCellSelectionEnabled()) {
if (isRTL()) {
selectLeftCell();
} else {
Expand All @@ -199,7 +207,8 @@ private void rightArrowPressed() {
}

private void leftArrowPressed() {
if (getNode().getSelectionModel().isCellSelectionEnabled()) {
TreeTableViewSelectionModel<T> sm = getNode().getSelectionModel();
if ((sm != null) && sm.isCellSelectionEnabled()) {
if (isRTL()) {
selectRightCell();
} else {
Expand Down
29 changes: 19 additions & 10 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TableView.java
View file
Expand Up @@ -995,6 +995,7 @@ public final Node getPlaceholder() {
@Override protected void invalidated() {

if (oldValue != null) {
oldValue.clearSelection();
oldValue.cellSelectionEnabledProperty().removeListener(weakCellSelectionModelInvalidationListener);

if (oldValue instanceof TableViewArrayListSelectionModel) {
Expand Down Expand Up @@ -1567,20 +1568,26 @@ public void sort() {
return;
}

final List<TablePosition> prevState = new ArrayList<>(getSelectionModel().getSelectedCells());
final int itemCount = prevState.size();
TableViewSelectionModel<S> selectionModel = getSelectionModel();
final List<TablePosition> prevState = selectionModel == null ?
null :
new ArrayList<>(selectionModel.getSelectedCells());

// 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();
if (selectionModel != null) {
selectionModel.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 (selectionModel != null) {
selectionModel.stopAtomic();
}

if (success == null || ! success) {
// the sort was a failure. Need to backout if possible
Expand All @@ -1593,15 +1600,16 @@ public void sort() {
// 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();
if (selectionModel instanceof TableViewArrayListSelectionModel) {
final TableViewArrayListSelectionModel<S> sm = (TableViewArrayListSelectionModel<S>)selectionModel;
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 (prevState != null) {
for (TablePosition<S, ?> prevItem : prevState) {
if (!newState.contains(prevItem)) {
removed.add(prevItem);
}
}
}

Expand All @@ -1611,6 +1619,7 @@ public void sort() {
// 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.
int itemCount = prevState == null ? 0 : prevState.size();
ListChangeListener.Change<TablePosition<S, ?>> c = new NonIterableChange.GenericAddRemoveChange<>(0, itemCount, removed, newState);
sm.fireCustomSelectedCellsListChangeEvent(c);
}
Expand Down
3 changes: 3 additions & 0 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableRow.java
View file
Expand Up @@ -442,6 +442,9 @@ private void updateSelection() {

TreeTableViewSelectionModel<T> sm = getTreeTableView().getSelectionModel();
if (sm == null) {
if (isSelected()) {
updateSelected(false);
}
return;
}

Expand Down
70 changes: 46 additions & 24 deletions modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java
View file
Expand Up @@ -1068,6 +1068,7 @@ public final ObjectProperty<TreeTableViewSelectionModel<S>> selectionModelProper
// need to listen to the cellSelectionEnabledProperty
// in order to set pseudo-class state
if (oldValue != null) {
oldValue.clearSelection();
oldValue.cellSelectionEnabledProperty().removeListener(weakCellSelectionModelInvalidationListener);

if (oldValue instanceof TreeTableViewArrayListSelectionModel) {
Expand All @@ -1077,7 +1078,12 @@ public final ObjectProperty<TreeTableViewSelectionModel<S>> selectionModelProper

oldValue = get();

if (oldValue != null) {
if (oldValue == null) {
// show no focused rows with a null selection model
if (getFocusModel() != null) {
getFocusModel().setFocusedIndex(-1);
}
} else {
oldValue.cellSelectionEnabledProperty().addListener(weakCellSelectionModelInvalidationListener);
// fake invalidation to ensure updated pseudo-class states
weakCellSelectionModelInvalidationListener.invalidated(oldValue.cellSelectionEnabledProperty());
Expand Down Expand Up @@ -1847,13 +1853,17 @@ public void sort() {
return;
}

final List<TreeTablePosition<S,?>> prevState = new ArrayList<>(getSelectionModel().getSelectedCells());
final int itemCount = prevState.size();
TreeTableViewSelectionModel<S> selectionModel = getSelectionModel();
final List<TreeTablePosition<S,?>> prevState = (selectionModel == null) ?
null :
new ArrayList<>(selectionModel.getSelectedCells());

// 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();
if (selectionModel != null) {
selectionModel.startAtomic();
}

// get the sort policy and run it
Callback<TreeTableView<S>, Boolean> sortPolicy = getSortPolicy();
Expand All @@ -1863,22 +1873,27 @@ public void sort() {
}
Boolean success = sortPolicy.call(this);

if (getSortMode() == TreeSortMode.ALL_DESCENDANTS) {
Set<TreeItem<S>> sortedParents = new HashSet<>();
for (TreeTablePosition<S,?> selectedPosition : prevState) {
// This null check is not required ideally.
// The selectedPosition.getTreeItem() should always return a valid TreeItem.
// But, it is possible to be null due to JDK-8248217.
if (selectedPosition.getTreeItem() != null) {
TreeItem<S> parent = selectedPosition.getTreeItem().getParent();
while (parent != null && sortedParents.add(parent)) {
parent.getChildren();
parent = parent.getParent();
if (prevState != null) {
if (getSortMode() == TreeSortMode.ALL_DESCENDANTS) {
Set<TreeItem<S>> sortedParents = new HashSet<>();
for (TreeTablePosition<S,?> selectedPosition : prevState) {
// This null check is not required ideally.
// The selectedPosition.getTreeItem() should always return a valid TreeItem.
// But, it is possible to be null due to JDK-8248217.
if (selectedPosition.getTreeItem() != null) {
TreeItem<S> parent = selectedPosition.getTreeItem().getParent();
while (parent != null && sortedParents.add(parent)) {
parent.getChildren();
parent = parent.getParent();
}
}
}
}
}
getSelectionModel().stopAtomic();

if (selectionModel != null) {
selectionModel.stopAtomic();
}

if (success == null || ! success) {
// the sort was a failure. Need to backout if possible
Expand All @@ -1891,29 +1906,36 @@ public void sort() {
// 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();
if (selectionModel instanceof TreeTableViewArrayListSelectionModel) {
final TreeTableViewArrayListSelectionModel<S> sm = (TreeTableViewArrayListSelectionModel<S>)selectionModel;
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 (prevState != null) {
for (TreeTablePosition<S, ?> prevItem: prevState) {
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.
int itemCount = prevState == null ? 0 : prevState.size();
ListChangeListener.Change<TreeTablePosition<S, ?>> c = new NonIterableChange.GenericAddRemoveChange<>(0, itemCount, removed, newState);
sm.fireCustomSelectedCellsListChangeEvent(c);
}
}
getSelectionModel().setSelectedIndex(getRow(getSelectionModel().getSelectedItem()));
getFocusModel().focus(getSelectionModel().getSelectedIndex());

if (selectionModel != null) {
selectionModel.setSelectedIndex(getRow(selectionModel.getSelectedItem()));
}

getFocusModel().focus(selectionModel == null ? -1 : selectionModel.getSelectedIndex());
}
sortingInProgress = false;
}
Expand Down

0 comments on commit 58376eb

Please sign in to comment.