Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8189354: Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed #480

Closed
wants to merge 13 commits into from

Conversation

@mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Apr 24, 2021

This PR fixes the implementation of ControlUtils.reducingChange, which incorrectly computed adjacent removed indices, thus resulting in incorrect removal notifications.

Since there were no unit tests for this method, I also added a bunch of tests.

After applying this fix, I can no longer reproduce JDK-8189354 and JDK-8189228.


Progress

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

Issue

  • JDK-8189354: Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/480/head:pull/480
$ git checkout pull/480

Update a local copy of the PR:
$ git checkout pull/480
$ git pull https://git.openjdk.java.net/jfx pull/480/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 480

View PR using the GUI difftool:
$ git pr show -t 480

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/480.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 24, 2021

👋 Welcome back mstr2! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Apr 24, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 24, 2021

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 24, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 24, 2021

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

@kevinrushforth kevinrushforth requested a review from aghaisas Apr 24, 2021
Copy link
Member

@kevinrushforth kevinrushforth left a comment

I'll leave it to Ajit to review (I can be a second reviewer if needed). I noted one part of the change that needs to be reverted.

Also, if you could explain the logic changes a bit, that would be helpful.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 24, 2021

Regarding JDK-8189228, if this fix also fixes that issue, then either that issue should be closed as a duplicate or else that issue should be added to this PR with /isuse add. I'll let @aghaisas decide which (I can see an argument to be made for either approach).

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 24, 2021

Also, if you could explain the logic changes a bit, that would be helpful.

Here's my guess what the original implementation tried to accomplish:

    ...
    while (endPos < removed.size()) {
        if (removed.get(startPos) == removed.get(endPos) - 1) {
            endPos++;
            continue;
        }
        ...
    }

removed is an ascending list of indices that should be removed from selectedIndices. The loop tries to find uninterrupted ascending ranges within that list (for example, [2, 3], [5, 6, 7]). However, the condition removed.get(startPos) == removed.get(endPos) - 1 will always fail after just a single iteration, since startPos is not modified before continue, which means that the loop will never detect any range longer than 2 elements.

The second issue with the original code is at this line:

    selectedIndices._nextRemove(selectedIndices.indexOf(removed.get(startPos)), removed.subList(startPos, endPos));

The first parameter of _nextRemove is the index at which the removal happened, and this index must match the current state of the list.

Let's assume that selectedIndices contains [1, 2, 3, 4, 5, 6, 7, 8], and removed contains [2, 3, 5, 6, 7].
In this case, the first removed range is [2, 3], which happens at index 1. The second removal must account for the already-removed elements: [5, 6, 7] at index 2.

Here's the new implementation (see the comments):

    for (int endPos = 0, max = removed.size(); endPos < max; ++endPos) {

        // This loop increments endPos as long as the next index is adjacent to the previous index
        while (endPos < max - 1 && removed.get(endPos) == removed.get(endPos + 1) - 1) {
            ++endPos;
        }

        // totalRemoved accounts for the elements which have already been removed, so that the
        // index will refer to the state of the list after all previous removal changes have been applied
        selectedIndices._nextRemove(
            selectedIndices.indexOf(removed.get(startPos)) - totalRemoved,
            removed.subList(startPos, endPos + 1));

        totalRemoved += endPos - startPos + 1;
        startPos = endPos + 1;
    }

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 25, 2021

Hmm .. don't quite understand why there is this method: it is (only?) used in tree/TableView treeModificationListeners to fire the appropriate remove changes for selected indices. In that particular case, removed is a sublist of selectedIndices (all selections in the subtree), that is they are already adjacent (in coordinates of selectedIndices). A single nextRemove should be okay, f.i. in TreeTableView

 int firstRemoved = selectedIndices.indexOf(removed.get(0));
 selectedIndices._nextRemove(firstRemoved, removed);
 // orig: not needed, all elements in removed _are_ adjacent (in coordinates of selectedIndices)
 //  ControlUtils.reducingChange(selectedIndices, removed);

On a quick check of the example in the report, this seems to fix JDK-8189354

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 25, 2021

You're right that removing ControlUtils.reducingChange is enough to solve the problem. Maybe the method was created because it might be confusing that, while the indices are often not adjacent, they are (as you correctly say) adjacent with respect to selectedIndices.

I have tested the change with 8189354 and 8189228, and both issues seem to be solved.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 27, 2021

You're right that removing ControlUtils.reducingChange is enough to solve the problem. Maybe the method was created because it might be confusing that, while the indices are often not adjacent, they are (as you correctly say) adjacent with respect to selectedIndices.

I have tested the change with 8189354 and 8189228, and both issues seem to be solved.

yeah, coordinate spaces can be confusing, been there :)

A couple of notes to your test - my personal preferences:

  • they should cover all changes, that is test both Tree- and TreeTableView
  • if possible, have a test that really cover the report: here that would be throwing a AIOOP on expanding again (with a filter to re-select all children)
  • have a test for underlying reason of the reported misbehavior: here the incorrect change
  • testing the change should be complete (in testing its state)

I have seen you using the MockListObserver in an earlier version of this PR - which is perfect for the last bullet, we should use it always in asserting the correctness of listChange notification :) The only very minor drawback is that it requires to update the Eclipse classpath to add-exports for the base testing package (not a big deal).

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 27, 2021

Regarding JDK-8189228, if this fix also fixes that issue, then either that issue should be closed as a duplicate or else that issue should be added to this PR with /isuse add. I'll let @aghaisas decide which (I can see an argument to be made for either approach).

my 2 cents: if we can prove (by tests) that it's another expression of the exact same underlying reason, we should add that issue here - doing so makes the relation more "visible" than closing as duplicate.

@kevinrushforth kevinrushforth requested review from arapte and removed request for aghaisas Apr 27, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 27, 2021

I reread the description of JDK-8189228 and it describes multiple problems. Are all of them addressed by this PR? If so, then both JBS issues should be closed as part of this PR. One more option for doing so would be to close JDK-8189354 as a duplicate of JDK-8189228, since the AIOOBE is one of the multiple problems described in the latter bug.

If only some of the issues described in JDK-8189228 are fixed by this PR (meaning this PR doesn't really address the larger problem described in JDK-8189228), then that issue should remain open, and a comment added to JDK-8189228 that the fix for JDK-8189354 addresses some of the problems.

@arapte has agreed to be the primary reviewer for this, and can make the call as to which way to proceed regarding those two JBS bugs.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 27, 2021

I reread the description of JDK-8189228 and it describes multiple problems. Are all of them addressed by this PR?

This JBS issue lists the following issues:

  1. The SelectedItems list does not get Remove change notifications in many cases
  2. IndexOutOfBoundsException is thrown while changing the selection in some cases
  3. If the collapse widget is toggled for the parent item of selected items, that parent item becomes selected. However, if none of the child items were selected, toggling the collapse/expand widget does not change the selection. But, if a parent of the the parent is also selected while child nodes are selected toggling the collapse widget doesn't select the item, and the parent item remains selected.

Regarding 1:
I think this is an artifact of the test program attached to the JBS issue; it is not a bug in JavaFX. The test program uses a if {...} else if {...} else if {...} ... branch to query the change, which is not a correct implementation and misses remove notifications that are part of replace notifications.

Regarding 2:
This PR fixes that.

Regarding 3:
The observation is partly correct: if a tree branch that contains selected items is collapsed, and the primary selection is within that branch, then the collapsed branch remains selected. However, if the primary selection is not within the collapsed branch, it does not remain selected.
TreeView:1390 specifically tests whether the primary selection is within or outside of the collapsed branch. This seems very intentional, so I'm wondering what the expected behavior should be.
However, since this is clearly no unintentional behavior, I would suggest to create a new ticket if the specification should be changed to always keep the collapsed tree branch selected.

So, to summarize: I think both JSB issues refer to the same bug; JDK-8189228 does not describe a larger problem.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 27, 2021

A couple of notes to your test - my personal preferences:

  • they should cover all changes, that is test both Tree- and TreeTableView
  • if possible, have a test that really cover the report: here that would be throwing a AIOOP on expanding again (with a filter to re-select all children)
  • have a test for underlying reason of the reported misbehavior: here the incorrect change
  • testing the change should be complete (in testing its state)

I have seen you using the MockListObserver in an earlier version of this PR - which is perfect for the last bullet, we should use it always in asserting the correctness of listChange notification :) The only very minor drawback is that it requires to update the Eclipse classpath to add-exports for the base testing package (not a big deal).

I think you're right that the same test could also be added for TreeTableView, and that the test should probably cover the entirety of the change notification. However, my preference is to not create a test that duplicates all aspects of the specific program that was attached to the JBS issue. I think it's more meaningful to test the most minimal setup that can reproduce the underlying issue; adding bells and whistles (as was done in the JBS code sample) feels like adding unnecessary noise to the test.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 27, 2021

The only very minor drawback is that it requires to update the Eclipse classpath to add-exports for the base testing package (not a big deal).

Can you share where specifically this needs to be added?

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Apr 28, 2021

There is one more issue mentioned (2nd comment by OP): toggling selection throws an NPE - that's still the case, both with and without this PR.

*hach .. that's fixed by the most recent change in MultipleSelectionModelBase (wasn't aware you did change sooo much ;)

The boundaries of this fix are getting a bit too blurry (as is the other bug report JDK-8189228 ;) for my taste.

Options either:

  • keep this focused on the IOOB error
  • find/open an new issue about the incorrect clearSelection impl in MultipleSelectionModelBase

or:

  • explicityl widen the scope of this to include the clearSelection bug

My preference would be the first.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Apr 28, 2021

Hmm .. doesn't seem to be throwing since fx9 as of Ajit's first comment, in fact can't reproduce it with current fx16dev, neither with nor without this PR

You're right, somehow I was under the impression it did throw. Maybe I could use this PR to fix the IOOB bug, and use the other JBS issue to fix the NPE bug (which is mentioned in there).

Copy link
Member

@arapte arapte left a comment

The fix looks good to me. Performed a regressive testing on TreeTableView and TreeView. Though there is an existing issue that SelectionModel.getSelectedItems() and SelectionModel.getSelectedIndices() are incorrect when some intermediate change events are received.
Also, I agree with Jeanette on that the two issues in discussion seem different. The AIOOB Exception is not reproducible with TreeView.
But I found another NPE with TreeView which gets fixed by this PR.
Steps:

  1. Run the test program attached to JDK-8189228
  2. Select/Click any child of root for instance 4 Item 1
  3. Ctrl click the same item to deselect it
    -> Observe the NPE
    This gets fixed by this PR.

The issues that are currently listed in the bug JDK-8189228 are not reproducible, so it can be closed as cannot reproduce
or
May be we should update the description of JDK-8189228 to include this NPE and add that bug to this PR ?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 3, 2021

I'm a bit confused about the various issues.

The issues that are currently listed in the bug JDK-8189228 are not reproducible, so it can be closed as cannot reproduce

If any are reproducible before this fix, then closing as "cannot reproduce" is incorrect. Either close as a duplicate, or list that issue in this PR.

But... I thought based on the earlier discussion that one or more NPEs in TreeView multi-selection remain even after this fix, which focuses on fixing IOOBE. Is this not the case?

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented May 3, 2021

I've removed the clearSelection fix, which can be addressed in another PR. What remains is a fix for the underlying issue that the list of removed selected indices is incorrect when a tree branch is collapsed.

The IOOB exception is an artifact of the elaborate test setup in 8189354. I propose to change the title of the JBS issue to List of removed selected indices is incorrect when a tree branch is collapsed and have this PR be the fix for that.

@arapte
Copy link
Member

@arapte arapte commented May 4, 2021

If any are reproducible before this fix, then closing as "cannot reproduce" is incorrect.

I was suggesting to close JDK-8189228 because the three issues that are currently listed in JDK-8189228 are not reproducible with or without this PR.
We can file a new bug for the NPE that I noticed with TreeView.

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented May 4, 2021

I was suggesting to close JDK-8189228 because the three issues that are currently listed in JDK-8189228 are not reproducible with or without this PR.
We can file a new bug for the NPE that I noticed with TreeView.

Good idea. Can you close JDK-8189228 as "Cannot reproduce", and then file the new NPE issue?

@arapte
Copy link
Member

@arapte arapte commented May 5, 2021

Good idea. Can you close JDK-8189228 as "Cannot reproduce", and then file the new NPE issue?

I have closed JDK-8189228 and reported a new one for NPE JDK-8266539

@arapte
Copy link
Member

@arapte arapte commented May 5, 2021

The IOOB exception is an artifact of the elaborate test setup in 8189354. I propose to change the title of the JBS issue to List of removed selected indices is incorrect when a tree branch is collapsed and have this PR be the fix for that.

True, we need to change the summary of this issue.
I would suggest a little different rewording like, Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed.
I will update the JBS then you can update the title of this PR

@arapte
Copy link
Member

@arapte arapte commented May 5, 2021

We have couple more similar issues reported

  1. JDK-8255935 : MultipleSelectionModel provides incorrect 'removed' sub-list in change events
  2. JDK-8196065 : ListChangeListener getRemoved() returns items that were not removed.

@mstr2 mstr2 changed the title 8189354: ArrayIndexOutOfBoundsException when listening to selection changes on TreeTableView 8189354: Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed May 7, 2021
@arapte
Copy link
Member

@arapte arapte commented May 10, 2021

We have couple more similar issues reported

  1. JDK-8255935 : MultipleSelectionModel provides incorrect 'removed' sub-list in change events
  2. JDK-8196065 : ListChangeListener getRemoved() returns items that were not removed.

Both these above issues do not get fixed by this PR.

arapte
arapte approved these changes May 10, 2021
Copy link
Member

@arapte arapte left a comment

looks good to me.

Copy link
Collaborator

@kleopatra kleopatra left a comment

Looks good :)

Verified tests failing/passing before/after fix, verified example in bug report working correctly

@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8189354: Change.getRemoved() list contains incorrect selected items when a TreeItem is collapsed

Reviewed-by: arapte, fastegal

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 31 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte, @kleopatra) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 10, 2021
@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented May 10, 2021

fyi: filed JDK-8266811 for the eclipse classpath issue introduced with this (by using MockListObserver) - will fix when this is integrated :)

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented May 10, 2021

/integrate

@openjdk openjdk bot added the sponsor label May 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

@mstr2
Your change (at version f052411) is now ready to be sponsored by a Committer.

@arapte
Copy link
Member

@arapte arapte commented May 10, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

@arapte @mstr2 Since your change was applied there have been 31 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit eb913cb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants