[Rev 03] RFR: 8193800: TreeTableView selection changes on sorting

Kevin Rushforth kcr at openjdk.java.net
Mon Jun 15 23:55:26 UTC 2020

On Mon, 15 Jun 2020 19:23:45 GMT, Ambarish Rapte <arapte at 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

More information about the openjfx-dev mailing list