8193800: TreeTableView selection changes on sorting

[arapte]

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8193800: TreeTableView selection changes on sorting

Reviewers

• Kevin Rushforth (kcr - Reviewer)
• Ajit Ghaisas (aghaisas - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/244/head:pull/244
$ git checkout pull/244

[bridgekeeper]

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

[mlbridge]

Webrevs

• 06: Full - Incremental (d8c9c92)
• 05: Full - Incremental (bf225d6)
• 04: Full - Incremental (c70178c)
• 03: Full - Incremental (e733a6c)
• 02: Full - Incremental (1561d2d)
• 01: Full - Incremental (44052f3)
• 00: Full (d6376df)

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

@aghaisas can you also review this?

[kevinrushforth]

The algorithm looks correct to me for sorting. How much regression testing have you done for cases where rows or columns are inserted?

I left a few comments, and will complete my testing in parallel.

[arapte]

The algorithm looks correct to me for sorting. How much regression testing have you done for cases where rows or columns are inserted?

I have tested with a small TreeTableView of 15 rows of 3 levels and 3 columns.
Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 columns ?

Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 number of children in each level respectively. The fix works fine till level 3. But can observe issue with level 4 and further. Shall debug this more and update.

[kleopatra]

hmm .. TreeModificationEvent seems to have a different interpretation (than a list change) of permutated: its wasPermutated may return true even on a replace - that's probably why some tests are throwing a IllegalStateException before the fix.

The other way round: do we really want to introduce a singularity in selection handling after replace? The other selectionModels for virtualized controls try to keep the selectedItem/Index only.

[kleopatra]

need a break ;)

Will come back later with an example comparing the behavior of multiple selection state across virtualized controls - preliminary results

Sort:

• ListView/TableView keep selection (corrrectly) on all selectedItems, adjust indices as needed
• TreeView: ??
• TreeTableView: throws IllegalState before this fix, keeps selection (correctly) on all selectedItems, adjusts indices as needed after this fix

Replace with same items in different sequence

• ListView/TableView: keep only selectedItem, adjust selectedIndex as needed
• TreeView: keeps selectedIndices (?)
• TreeTableView: throws IllegalState before this fix, keeps selectedItems, adjusts selectedIndices as needed after this fix

Published an example to play with.

[kevinrushforth]

I have tested with a small TreeTableView of 15 rows of 3 levels and 3 columns.
Would it be enough to test with TreeTableView of ~500 rows of 5 levels, 10 columns ?

Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 number of children in each level respectively. The fix works fine till level 3. But can observe issue with level 4 and further. Shall debug this more and update.

OK, thanks. In addition to making sure that insertion, deletion, and sorting work, are you also checking that the events being sent are as expected?

[kleopatra]

Unrelated to this fix I see two pain points (which were not introduced here :)

A. replaced vs. permutated

These are types of change as decided by the list (aka: model) - treeModificationEvent (aka: view) must follow the lead (vs. spreading its own interpretation). Doing so is a bug, IMO.

B. treeTableView.sort changing internals of the selectionModel

that's a no-no-no-go, always. Instead let the selectionModel handle the permutated state correctly (didn't dig why it was deemed necessary to do so in sort - though it has the side-effect of leaving out selectionModels that are not of the expected type)

[arapte]

Update: I am testing with 7 level of nested rows, with 10, 9, 7, 6, 5, 4, 3 number of children in each level respectively. The fix works fine till level 3. But can observe issue with level 4 and further. Shall debug this more and update.

In the above case, The selection updates correctly but the received change event on SelectedItems is incorrect. As of now it looks like the selection is updated correctly with this fix, but the event generated from sort() method is always incorrect. Looking in to fix the change event....

[arapte]

The other selectionModels for virtualized controls try to keep the selectedItem/Index only.

Keeping the selection after sort/permutation seems more correct from a user's perspective. I did not look into how other controls handle this. Looking at your observation seems like we need to fix this in other controls too, preferably each control in a separate issue :)

[kleopatra]

The other selectionModels for virtualized controls try to keep the selectedItem/Index only.

Keeping the selection after sort/permutation seems more correct from a user's perspective. I did not look into how other controls handle this. Looking at your observation seems like we need to fix this in other controls too, preferably each control in a separate issue :)

well, I don't think so (except for tree which is doing the-wrong-thingy-always): they do keep all state on sort, but not on replace-in-different-sequence - which is correct, I think: it's the semantic of the change notification that must rule, not some assumption of the view.

If a replace-in-different-sequence is required to be interpretated as a permutation in a particular use-case, the place to implement it would be a custom list that checks for that corner case and sends a notification of type wasPermutated (instead of a wasReplaced).

[ebresie]

Hi @ebresie, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user ebresie for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

• I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

[arapte]

In the above case, The selection updates correctly but the received change event on SelectedItems is incorrect. As of now it looks like the selection is updated correctly with this fix, but the event generated from sort() method is always incorrect. Looking in to fix the change event....

Please take a look at the updated changes. The GenericAddRemoveChange event generated from TreeTableView.sort() method does not result in correct selection change events. @kevinrushforth , Thanks for the guidance on this in offline discussion. The solution is to sort the tree of selected items upfront before generating the GenericAddRemoveChange event. Rest of the TreeItems can be left to be sorted lazily whenever accessed.

[kevinrushforth]

The fix looks good now with one suggested cleanup.

The tests will catch the bug (including the most recently fixed problem), but I noted a couple of issues with the test inline below.

[kevinrushforth]

Pending a second reviewer.

@aghaisas or @kleopatra can one of you review it?

[kleopatra]

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

While digging a bit (mainly around my earlier concerns - 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.

[arapte]

While digging a bit (mainly around my earlier concerns - 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.

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.
Please take a look.

[kevinrushforth]

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.

[kleopatra]

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.

agreed - plus it's not only not regressing, but considerably improving the behavior :)

[openjdk]

@arapte This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8193800: TreeTableView selection changes on sorting

Reviewed-by: kcr, aghaisas

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 14 commits pushed to the master branch:

• a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
• 8440b64: 8208169: can not print selected pages of web page
• 1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
• 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
• fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
• bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
• b200891: 8244824: TableView : Incorrect German translation
• b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
• afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
• 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
• ... and 4 more: https://git.openjdk.java.net/jfx/compare/853ac78ad5099ebda00dc401b3ead67cafa59aa8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate a5878e055f65506eb639c48d3c513915f118b478.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[arapte]

/integrate

[openjdk]

@arapte The following commits have been pushed to master since your change was applied:

• a5878e0: 8214699: Node.getPseudoClassStates must return the same instance on every call
• 8440b64: 8208169: can not print selected pages of web page
• 1727dfa: 8247163: JFXPanel throws exception on click when no Scene is set
• 54e2507: 8247360: Add missing license file for Microsoft DirectShow Samples
• fb962ac: 8244418: MenuBar: IOOB exception on requestFocus on empty bar
• bf2e972: 8246348: Crash in libpango on Ubuntu 20.04 with some unicode chars
• b200891: 8244824: TableView : Incorrect German translation
• b2b46eb: 8242892: SpinnerValueFactory has an implicit no-arg constructor
• afa805f: 8245575: Show the ContextMenu of input controls with long press gesture on iOS
• 5304266: 8245635: GlassPasteboard::getUTFs fails on iOS
• ba501ef: 8246357: Allow static build of webkit library on linux
• a02e09d: 8246195: ListViewSkin/Behavior: misbehavior on switching skin
• 9749982: 8246204: No 3D support for newer Intel graphics drivers on Linux
• 6bd0e22: 8239095: Upgrade libFFI to the latest 3.3 version

Your commit was automatically rebased without conflicts.

Pushed as commit 2ca509a.