On Fri, 19 Jun 2020 23:04:51 GMT, Kevin Rushforth <k...@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
