Skip to content

Commit 2ca509a

Browse files
araptearapte
committed
8193800: TreeTableView selection changes on sorting
Reviewed-by: kcr, aghaisas
1 parent a5878e0 commit 2ca509a

File tree

3 files changed

+336
-88
lines changed

3 files changed

+336
-88
lines changed

‎modules/javafx.controls/src/main/java/javafx/scene/control/TreeTablePosition.java

+10
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,16 @@ public TreeTablePosition(@NamedArg("treeTableView") TreeTableView<S> treeTableVi
7777
nonFixedColumnIndex = treeTableView == null || tableColumn == null ? -1 : treeTableView.getVisibleLeafIndex(tableColumn);
7878
}
7979

80+
// Not public API. A Copy-like constructor with a different row.
81+
// It is used for updating the selection when the TreeItems are
82+
// sorted using TreeTableView.sort() or reordered using setAll().
83+
TreeTablePosition(@NamedArg("treeTableView") TreeTablePosition<S, T> pos, @NamedArg("row") int row) {
84+
super(row, pos.getTableColumn());
85+
this.controlRef = new WeakReference<>(pos.getTreeTableView());
86+
this.treeItemRef = new WeakReference<>(pos.getTreeItem());
87+
nonFixedColumnIndex = pos.getColumn();
88+
}
89+
8090

8191

8292
/***************************************************************************

‎modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java

+90-88
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import java.util.Collections;
5757
import java.util.Comparator;
5858
import java.util.HashMap;
59+
import java.util.HashSet;
5960
import java.util.LinkedHashSet;
6061
import java.util.List;
6162
import java.util.Map;
@@ -1804,6 +1805,11 @@ public TreeTableColumn<S,?> getVisibleLeafColumn(int column) {
18041805
return visibleLeafColumns.get(column);
18051806
}
18061807

1808+
private boolean sortingInProgress;
1809+
boolean isSortingInProgress() {
1810+
return sortingInProgress;
1811+
}
1812+
18071813
/**
18081814
* The sort method forces the TreeTableView to re-run its sorting algorithm. More
18091815
* often than not it is not necessary to call this method directly, as it is
@@ -1814,6 +1820,7 @@ public TreeTableColumn<S,?> getVisibleLeafColumn(int column) {
18141820
* something external changes and a sort is required.
18151821
*/
18161822
public void sort() {
1823+
sortingInProgress = true;
18171824
final ObservableList<TreeTableColumn<S,?>> sortOrder = getSortOrder();
18181825

18191826
// update the Comparator property
@@ -1832,6 +1839,7 @@ public void sort() {
18321839
// sortLock = true;
18331840
// TableUtil.handleSortFailure(sortOrder, lastSortEventType, lastSortEventSupportInfo);
18341841
// sortLock = false;
1842+
sortingInProgress = false;
18351843
return;
18361844
}
18371845

@@ -1845,9 +1853,27 @@ public void sort() {
18451853

18461854
// get the sort policy and run it
18471855
Callback<TreeTableView<S>, Boolean> sortPolicy = getSortPolicy();
1848-
if (sortPolicy == null) return;
1856+
if (sortPolicy == null) {
1857+
sortingInProgress = false;
1858+
return;
1859+
}
18491860
Boolean success = sortPolicy.call(this);
18501861

1862+
if (getSortMode() == TreeSortMode.ALL_DESCENDANTS) {
1863+
Set<TreeItem<S>> sortedParents = new HashSet<>();
1864+
for (TreeTablePosition<S,?> selectedPosition : prevState) {
1865+
// This null check is not required ideally.
1866+
// The selectedPosition.getTreeItem() should always return a valid TreeItem.
1867+
// But, it is possible to be null due to JDK-8248217.
1868+
if (selectedPosition.getTreeItem() != null) {
1869+
TreeItem<S> parent = selectedPosition.getTreeItem().getParent();
1870+
while (parent != null && sortedParents.add(parent)) {
1871+
parent.getChildren();
1872+
parent = parent.getParent();
1873+
}
1874+
}
1875+
}
1876+
}
18511877
getSelectionModel().stopAtomic();
18521878

18531879
if (success == null || ! success) {
@@ -1872,7 +1898,6 @@ public void sort() {
18721898
removed.add(prevItem);
18731899
}
18741900
}
1875-
18761901
if (!removed.isEmpty()) {
18771902
// the sort operation effectively permutates the selectedCells list,
18781903
// but we cannot fire a permutation event as we are talking about
@@ -1883,7 +1908,10 @@ public void sort() {
18831908
sm.fireCustomSelectedCellsListChangeEvent(c);
18841909
}
18851910
}
1911+
getSelectionModel().setSelectedIndex(getRow(getSelectionModel().getSelectedItem()));
1912+
getFocusModel().focus(getSelectionModel().getSelectedIndex());
18861913
}
1914+
sortingInProgress = false;
18871915
}
18881916

18891917
/**
@@ -2540,68 +2568,40 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
25402568
shift += -count + 1;
25412569
startRow++;
25422570
} else if (e.wasPermutated()) {
2543-
// General approach:
2544-
// -- detected a sort has happened
2545-
// -- Create a permutation lookup map (1)
2546-
// -- dump all the selected indices into a list (2)
2547-
// -- create a list containing the new indices (3)
2548-
// -- for each previously-selected index (4)
2549-
// -- if index is in the permutation lookup map
2550-
// -- add the new index to the new indices list
2551-
// -- Perform batch selection (5)
2552-
2553-
startAtomic();
2554-
2555-
final int offset = startRow + 1;
2556-
2557-
// (1)
2558-
int length = e.getTo() - e.getFrom();
2559-
HashMap<Integer, Integer> pMap = new HashMap<> (length);
2560-
for (int i = e.getFrom(); i < e.getTo(); i++) {
2561-
pMap.put(i, e.getChange().getPermutation(i));
2562-
}
2563-
2564-
// (2)
2565-
List<TreeTablePosition<S,?>> selectedIndices = new ArrayList<>(getSelectedCells());
2566-
2567-
// (3)
2568-
List<TreeTablePosition<S,?>> newIndices = new ArrayList<>(selectedIndices.size());
2571+
// Approach:
2572+
// Get the current selection.
2573+
// Create a new selection with updated index(row).
2574+
// Update the current selection with new selection.
2575+
// If sorting is in progress then one Selection change event will be sent from
2576+
// TreeTableView.sort() method, and should not be sent from here.
2577+
// else, in case otherwise, the selection change events would be generated.
2578+
// Do not call shiftSelection() in case of permutation change(when shift == 0).
2579+
2580+
List<TreeTablePosition<S, ?>> currentSelection = new ArrayList<>(selectedCellsMap.getSelectedCells());
2581+
List<TreeTablePosition<S, ?>> updatedSelection = new ArrayList<>();
25692582

2570-
// (4)
25712583
boolean selectionIndicesChanged = false;
2572-
for (int i = 0; i < selectedIndices.size(); i++) {
2573-
final TreeTablePosition<S,?> oldIndex = selectedIndices.get(i);
2574-
final int oldRow = oldIndex.getRow() - offset;
2575-
2576-
if (pMap.containsKey(oldRow)) {
2577-
int newIndex = pMap.get(oldRow) + offset;
2578-
2579-
selectionIndicesChanged = selectionIndicesChanged || newIndex != oldRow;
2580-
2581-
newIndices.add(new TreeTablePosition<>(oldIndex.getTreeTableView(), newIndex, oldIndex.getTableColumn()));
2582-
}
2583-
2584-
// check if the root element of this event was selected, so that we can retain it
2585-
if (oldIndex.getRow() == startRow) {
2586-
newIndices.add(new TreeTablePosition<>(oldIndex.getTreeTableView(), oldIndex.getRow(), oldIndex.getTableColumn()));
2584+
for (TreeTablePosition<S, ?> selectedCell : currentSelection) {
2585+
int newRow = treeTableView.getRow(selectedCell.getTreeItem());
2586+
if (selectedCell.getRow() != newRow) {
2587+
selectionIndicesChanged = true;
25872588
}
2589+
updatedSelection.add(new TreeTablePosition<>(selectedCell, newRow));
25882590
}
2589-
25902591
if (selectionIndicesChanged) {
2591-
// (5)
2592-
quietClearSelection();
2593-
stopAtomic();
2594-
2595-
selectedCellsMap.setAll(newIndices);
2596-
2597-
final int offsetOldIndex = oldSelectedIndex - offset;
2598-
if (offsetOldIndex >= 0 && offsetOldIndex < getItemCount()) {
2599-
int newIndex = e.getChange().getPermutation(offsetOldIndex);
2600-
setSelectedIndex(newIndex + offset);
2601-
focus(newIndex + offset);
2592+
if (treeTableView.isSortingInProgress()) {
2593+
startAtomic();
2594+
selectedCellsMap.setAll(updatedSelection);
2595+
stopAtomic();
2596+
} else {
2597+
startAtomic();
2598+
quietClearSelection();
2599+
stopAtomic();
2600+
selectedCellsMap.setAll(updatedSelection);
2601+
int selectedIndex = treeTableView.getRow(getSelectedItem());
2602+
setSelectedIndex(selectedIndex);
2603+
focus(selectedIndex);
26022604
}
2603-
} else {
2604-
stopAtomic();
26052605
}
26062606
} else if (e.wasAdded()) {
26072607
// shuffle selection by the number of added items
@@ -2663,42 +2663,44 @@ private void updateTreeEventListener(TreeItem<S> oldRoot, TreeItem<S> newRoot) {
26632663
}
26642664
} while (e.getChange() != null && e.getChange().next());
26652665

2666-
shiftSelection(startRow, shift, new Callback<ShiftParams, Void>() {
2667-
@Override public Void call(ShiftParams param) {
2668-
2669-
// we make the shifts atomic, as otherwise listeners to
2670-
// the items / indices lists get a lot of intermediate
2671-
// noise. They eventually get the summary event fired
2672-
// from within shiftSelection, so this is ok.
2673-
startAtomic();
2674-
2675-
final int clearIndex = param.getClearIndex();
2676-
final int setIndex = param.getSetIndex();
2677-
TreeTablePosition<S,?> oldTP = null;
2678-
if (clearIndex > -1) {
2679-
for (int i = 0; i < selectedCellsMap.size(); i++) {
2680-
TreeTablePosition<S,?> tp = selectedCellsMap.get(i);
2681-
if (tp.getRow() == clearIndex) {
2682-
oldTP = tp;
2683-
selectedCellsMap.remove(tp);
2684-
} else if (tp.getRow() == setIndex && !param.isSelected()) {
2685-
selectedCellsMap.remove(tp);
2666+
if (shift != 0) {
2667+
shiftSelection(startRow, shift, new Callback<ShiftParams, Void>() {
2668+
@Override public Void call(ShiftParams param) {
2669+
2670+
// we make the shifts atomic, as otherwise listeners to
2671+
// the items / indices lists get a lot of intermediate
2672+
// noise. They eventually get the summary event fired
2673+
// from within shiftSelection, so this is ok.
2674+
startAtomic();
2675+
2676+
final int clearIndex = param.getClearIndex();
2677+
final int setIndex = param.getSetIndex();
2678+
TreeTablePosition<S,?> oldTP = null;
2679+
if (clearIndex > -1) {
2680+
for (int i = 0; i < selectedCellsMap.size(); i++) {
2681+
TreeTablePosition<S,?> tp = selectedCellsMap.get(i);
2682+
if (tp.getRow() == clearIndex) {
2683+
oldTP = tp;
2684+
selectedCellsMap.remove(tp);
2685+
} else if (tp.getRow() == setIndex && !param.isSelected()) {
2686+
selectedCellsMap.remove(tp);
2687+
}
26862688
}
26872689
}
2688-
}
26892690

2690-
if (oldTP != null && param.isSelected()) {
2691-
TreeTablePosition<S,?> newTP = new TreeTablePosition<>(
2692-
treeTableView, param.getSetIndex(), oldTP.getTableColumn());
2691+
if (oldTP != null && param.isSelected()) {
2692+
TreeTablePosition<S,?> newTP = new TreeTablePosition<>(
2693+
treeTableView, param.getSetIndex(), oldTP.getTableColumn());
26932694

2694-
selectedCellsMap.add(newTP);
2695-
}
2695+
selectedCellsMap.add(newTP);
2696+
}
26962697

2697-
stopAtomic();
2698+
stopAtomic();
26982699

2699-
return null;
2700-
}
2701-
});
2700+
return null;
2701+
}
2702+
});
2703+
}
27022704
}
27032705
};
27042706

0 commit comments

Comments
 (0)