RFR: 8193800: TreeTableView selection changes on sorting [v6]

[Kevin Rushforth]

On Wed, 24 Jun 2020 11:02:53 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:
> 
>   review comment corrections, adding new tests

The changes to the fix itself look fine. I have a question and minor comment on the test changes.

As for the questions raised by @kleopatra as long as you plan to address them in the follow-up issue(s), that seems
fine, since you aren't regressing any behavior.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 400:

> 399:
> 400:     @Ignore("JDK-8248217")
> 401:     @Test public void testSelectionUpdatesCorrectlyAfterRemovingSelectedItem() {

Question: did you mean to point to the umbrella Task or is there a more specific bug that you could point to (I think
referring to the Task is fine if you aren't sure). This applies to all of the newly added `@Ignore`d tests.

modules/javafx.controls/src/test/java/test/javafx/scene/control/TreeTableViewTest.java line 443:

> 442:     @Test public void testSelectionUpdatesCorrectlyAfterChildRemoveOneAndSetAll() {
> 443:         TreeTableColumn<String, String> col = setupForPermutationTest();
> 444:         TreeItem<String> parentTreeItem = ((TreeItem<String>)sm.getSelectedItem()).getParent();

Minor: `col` is unused.

-------------

PR: https://git.openjdk.java.net/jfx/pull/244