On Mon, 15 Jun 2020 19:23:45 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> Issue: >> In TreeTableView, in case of Multiple selection mode, if nested items are >> selected, then TreeTableView does not >> retain/update the selection correctly when the tree items are >> permuted(either by `sort()` or by reordering using >> `setAll()`). Cause: >> >> 1. For permutation, the current implementation uses `TreeModificationEvent` >> to update the selection. >> 2. The indices from these TreeModificationEvents are not reliable. >> 3. It uses the non public `TreeTablePosition` constructor to create >> intermediate `TreeItem` positions, this constructor >> results in another unexpected TreeModificationEvent while one for sorting is >> already being processed. 4. In case of >> sorting, there can be multiple intermediate TreeModificationEvents >> generated, and for each TreeModificationEvent, the >> selection gets updated and results in selection change events being >> generated. 5. Each time a TreeItem is expanded or >> collapsed, the selection must be shifted, but shifting is not necessary in >> case of permutation. All these issues >> combine in wrong update of the selection. Fix: >> >> 1. On each TreeModificationEvent for permutation, for updating the >> selection, use index of TreeItem from the >> TreeTableView but not from the TreeModificationEvent. 2. Added a new non >> public TreeTablePosition constructor, which is >> almost a copy constructor but accepts a different row. 3. In case of >> sorting, send out the set of selection change >> events only once after the sorting is over. 4. In case of setAll, send out >> the set of selection change events same as >> before.(setAll results in only one TreeModificationEvent, which effectively >> results in only one set of selection change >> events). `shiftSelection()` should not be called in case of permutation i.e. >> call `if (shift != 0)` >> Verification: >> The change is very limited to updating of selection of TreeTableView items >> when the TreeItems are permuted, so the >> change should not cause any other failures. Added unit tests which fail >> before and pass after the fix. > > Ambarish Rapte has updated the pull request incrementally with one additional > commit since the last revision: > > Correcting the selection change events generated post sorting The fix looks good now with one suggested cleanup. The tests will catch the bug (including the most recently fixed problem), but I noted a couple of issues with the test inline below. modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java line 1815: > 1814: boolean isSortTreeOfSelectedItems() { > 1815: return sortTreeOfSelectedItems; > 1816: } This method / state attribute isn't really needed. The value is never set to anything other than `true`. I recommend removing it. modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 431: > 430: for (int j = 0; j < FIRST_LEVEL_COUNT - 1; j++) { > 431: TreeItem<String> tj = new TreeItem<>("" + i + j); > 432: tj.setExpanded(true); The tree item strings will not be unique. This won't affect whether the test passes or fails, since `TreeItem` does not override `equals`, but it might be better if the strings were unique (in case there ever was an error it would be easier to understand). modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 472: > 471: countSelectedIndexChangeEvent++; > 472: assertEquals(selectedItemBefore, > treeTableView.getTreeItem(sm.getSelectedIndex())); > 473: }); If this assertion ever fails, I don't think it will cause a test failure, since the event handling code will swallow the exception. The same is true of the other listeners. I don't know if it would be possible to use the UncaughtExceptionHandler as is done in other tests -- see [JDK-8244531](https://bugs.openjdk.java.net/browse/JDK-8244531) -- but that might be worth exploring. Another possibility is to wrap all the listeners in a try/catch and keep a list of `Throwable`s that are caught. ------------- PR: https://git.openjdk.java.net/jfx/pull/244