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

[Jeanette Winzenburg]

On Fri, 19 Jun 2020 23:04:51 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

>> Marked as reviewed by kcr (Lead).
>
> Pending a second reviewer.
> 
> @aghaisas or @kleopatra can one of you review it?

confirmed test failures before and passing after the fix - impressive :)

While digging a bit (mainly around my [earlier
concerns](https://github.com/openjdk/jfx/pull/244#issuecomment-638702570) - which still hold but could be discussed in
a follow-up issue :) I came up with a NPE thrown by the newly added code block ins sort (the one walking up the parent
hierarchy).

    @Test
    public void testNPEOnSortAfterSetAll() {
        // stand-alone setup
        TreeTableView<String> treeTableView = new TreeTableView<>();
        TreeTableColumn<String, String> col = new TreeTableColumn<String, String>("column");
        col.setSortType(ASCENDING);
        col.setCellValueFactory(param -> new ReadOnlyObjectWrapper<String>(param.getValue().getValue()));
        treeTableView.getColumns().add(col);

        TreeItem<String> root = new TreeItem<String>("root");
        root.setExpanded(true);
        root.getChildren().addAll(
                new TreeItem("Apple"),
                new TreeItem("Orange"),
                new TreeItem("Banana"));

        treeTableView.setRoot(root);
        // add expanded children
        root.getChildren().forEach(child -> {
            child.setExpanded(true);
            String value = child.getValue();
            for (int i = 1; i <= 3; i++) {
                child.getChildren().add(new TreeItem<>(value + i));
            }
        });
        assertEquals("sanity", 13, treeTableView.getExpandedItemCount());
        TreeTableViewSelectionModel<String> selectionModel = treeTableView.getSelectionModel();
        selectionModel.setSelectionMode(SelectionMode.MULTIPLE);
        selectionModel.selectIndices(2, 5, 8, 10);
        assertEquals(10, selectionModel.getSelectedIndex());
        TreeItem<String> lastRootChild = root.getChildren().get(2);
        assertEquals("sanity: selectedItem child of last",
                lastRootChild, selectionModel.getSelectedItem().getParent());
        // replace children of last root child
        List<TreeItem<String>> childrenOfLastRootChild = new ArrayList<>(lastRootChild.getChildren());
        childrenOfLastRootChild.remove(0);
        lastRootChild.getChildren().setAll(childrenOfLastRootChild);
        treeTableView.sort();
    }

(before the fix it throws an IOOB, plus the behavior completely rotten in a visual test)

Did nothing to go to the bottom of this - looks like somehow the state of the selection is (still?) broken after
setAll.

Which might bring us back to the replace != permutation (can't resist to try :)  actually there's no reason to special
case a replace with all same items against a replace with only some same items (doing so introduces a discontinuity in
behavior) - we should either try to keep selected items or not.

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

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