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

Jeanette Winzenburg fastegal at openjdk.java.net
Tue Jun 23 13:11:55 UTC 2020

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

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

        TreeItem<String> root = new TreeItem<String>("root");
                new TreeItem("Apple"),
                new TreeItem("Orange"),
                new TreeItem("Banana"));

        // add expanded children
        root.getChildren().forEach(child -> {
            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.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());

(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

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

More information about the openjfx-dev mailing list