Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Tue, 23 Jun 2020 13:09:44 GMT, Jeanette Winzenburg wrote: > 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 :) Yes, We need to address these issues. How should we treat permutation vs replace, which may involve answering few more small questions. Like, What should be the selected item when we remove the current selected item? What should the contents of an event be in such cases? Regarding the scenario in your test, In `TreeTableView`, when `setAll()` is called by providing the exact same items in a different order, then it is considered as permutation. and a `wasPermutated` `TreeModificationEvent` gets generated. But if even a single `TreeItem` is removed and `setAll(` with the remaining n-1 TreeItems`)` is called, then it is considered as adding new items. (May be it should be generate as `wasRemoved`). And similar issue can be observed when a `TreeItem` is added. In the test that you provided, one `TreeItem` is removed before doing `setAll()`. So this results in a `wasAdded` `TreeModificationEvent `and does not enter the `wasPermutated` code block. This fix only changes the `wasPermutated` event. So the behavior is still buggy more or less similar to before this change. There are multiple such issues with updating the selection. I have grouped few already reported issues under [JDK-8248217](https://bugs.openjdk.java.net/browse/JDK-8248217). > I came up with a NPE thrown by the newly added code block ins sort (the one > walking up the parent hierarchy). I have added a null check for now, with a reasoning comment. We will have to remove that check once we fix the other issues. Thanks for the providing the test :). code really helps to understand. I have added this scenario to test along with few others. The tests are ignored using [JDK-8248217](https://bugs.openjdk.java.net/browse/JDK-8248217). Please take a look. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Tue, 23 Jun 2020 10:02:17 GMT, Ajit Ghaisas wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove the un-required flag > > modules/javafx.controls/src/main/java/javafx/scene/control/TreeTablePosition.java > line 79: > >> 78: } >> 79: >> 80: // Copy-like constructor with a different row. > > Suggest to add > // Not public API > before this description. > > (Similar to the constructor above this newly added constructor) Corrected in the next commit. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Tue, 23 Jun 2020 12:32:22 GMT, Kevin Rushforth wrote: >> modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java >> line 1856: >> >>> 1855: Callback, Boolean> sortPolicy = >>> getSortPolicy(); >>> 1856: if (sortPolicy == null) return; >>> 1857: Boolean success = sortPolicy.call(this); >> >> Do we need to set sortingInProgress to false before returning from here? > > Yes, I think so. Good catch. Corrected in the next commit. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Fri, 19 Jun 2020 23:04:51 GMT, Kevin Rushforth 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 treeTableView = new TreeTableView<>();
TreeTableColumn col = new TreeTableColumn("column");
col.setSortType(ASCENDING);
col.setCellValueFactory(param -> new
ReadOnlyObjectWrapper(param.getValue().getValue()));
treeTableView.getColumns().add(col);
TreeItem root = new TreeItem("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 selectionModel =
treeTableView.getSelectionModel();
selectionModel.setSelectionMode(SelectionMode.MULTIPLE);
selectionModel.selectIndices(2, 5, 8, 10);
assertEquals(10, selectionModel.getSelectedIndex());
TreeItem lastRootChild = root.getChildren().get(2);
assertEquals("sanity: selectedItem child of last",
lastRootChild, selectionModel.getSelectedItem().getParent());
// replace children of last root child
List> 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
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Tue, 23 Jun 2020 10:07:37 GMT, Ajit Ghaisas wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove the un-required flag > > modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java > line 1856: > >> 1855: Callback, Boolean> sortPolicy = >> getSortPolicy(); >> 1856: if (sortPolicy == null) return; >> 1857: Boolean success = sortPolicy.call(this); > > Do we need to set sortingInProgress to false before returning from here? Yes, I think so. Good catch. - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Wed, 17 Jun 2020 11:38:45 GMT, Ambarish Rapte 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: > > Remove the un-required flag modules/javafx.controls/src/main/java/javafx/scene/control/TreeTablePosition.java line 79: > 78: } > 79: > 80: // Copy-like constructor with a different row. Suggest to add // Not public API before this description. (Similar to the constructor above this newly added constructor) modules/javafx.controls/src/main/java/javafx/scene/control/TreeTableView.java line 1856: > 1855: Callback, Boolean> sortPolicy = > getSortPolicy(); > 1856: if (sortPolicy == null) return; > 1857: Boolean success = sortPolicy.call(this); Do we need to set sortingInProgress to false before returning from here? - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Wed, 17 Jun 2020 12:28:41 GMT, Kevin Rushforth wrote: >> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove the un-required flag > > Marked as reviewed by kcr (Lead). Pending a second reviewer. @aghaisas or @kleopatra can one of you review it? - PR: https://git.openjdk.java.net/jfx/pull/244
Re: RFR: 8193800: TreeTableView selection changes on sorting [v5]
On Wed, 17 Jun 2020 11:38:45 GMT, Ambarish Rapte 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: > > Remove the un-required flag Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/244
