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

8196065: ListChangeListener getRemoved() returns items that were not removed. #478

Closed
wants to merge 14 commits into from

Conversation

@mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Apr 23, 2021

The documentation for ObservableListBase.nextRemove states that a single change always refers to the current state of the list, which likely means that multiple disjoint removed ranges need to be applied in order, otherwise the next change's getFrom doesn't refer to the correct index.

SelectedItemsReadOnlyObservableList doesn't apply removals to itemsRefList, which means that subsequent removals will refer to the wrong index when retrieving the removed elements. This PR fixes the calculation of the current index.


Progress

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

Issue

  • JDK-8196065: ListChangeListener getRemoved() returns items that were not removed.

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 478

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 23, 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 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 23, 2021

@mstr2 mstr2 force-pushed the fixes/JDK-8196065 branch from 45a5afd to aafa0e9 Apr 23, 2021
@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Apr 23, 2021

@mstr2 force-pushed ...

In general we prefer PR authors to not do a force push once a PR is under review. In this case, there is no problem since you did it right after the review was sent, and before anyone would have had time to look at it.

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Apr 23, 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 review from arapte and removed request for aghaisas Apr 26, 2021
@arapte
Copy link
Member

@arapte arapte commented Apr 29, 2021

The fix solves the problem that is described in the bug.
But the similar problem can be observed with different number of items and different selection.

In the test program attached to the bug, make following one line change to add two more items "three" and "four"

final ObservableList<String> listitems = FXCollections.observableArrayList("zero", "one", "two", "three", "four");

Scenarios where the issue still occurs:
Scenario 1:

  1. Click on item one
  2. Press ctrl key and select items two and three
  3. Click on item two or on item one
  4. => Observe that output is incorrect
  5. => Similar issue can be observed with items two, three and four
  6. => But this does not occur with one, two and three

Scenario 2:

  1. Click on item two
  2. Press ctrl key and select items three, four and one in this order
  3. Click on item two or on item one
  4. => Observe that output is incorrect

It looks like getRemoved() is not correct in all cases when first item zero is no involved

Copy link
Member

@arapte arapte left a comment

Provided a couple scenario in previous comment which need to be addressed.

@mstr2 mstr2 marked this pull request as draft May 10, 2021
@openjdk openjdk bot removed the rfr label May 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 11, 2021

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fixes/JDK-8196065
git fetch https://git.openjdk.java.net/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

mstr2 added 2 commits May 11, 2021
# Conflicts:
#	modules/javafx.controls/src/main/java/javafx/scene/control/ControlUtils.java
@openjdk openjdk bot removed the merge-conflict label May 11, 2021
@mstr2 mstr2 marked this pull request as ready for review May 11, 2021
@openjdk openjdk bot added the rfr label May 11, 2021
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented May 11, 2021

I fixed another issue with ControlUtils.buildClearAndSelectChange, which led to incorrect change notifications for selected indices. This PR now fixes

  1. 8196065 ListChangeListener getRemoved() returns items that were not removed.
  2. 8255935 MultipleSelectionModel provides incorrect 'removed' sub-list in change events
  3. The additional scenarios mentioned by @arapte

mstr2 added 2 commits May 17, 2021
# Conflicts:
#	modules/javafx.controls/src/test/java/test/javafx/scene/control/MultipleSelectionModelImplTest.java
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Jun 1, 2021

@kleopatra Do you think this PR is good to go?

@arapte
Copy link
Member

@arapte arapte commented Jun 2, 2021

I tested several scenarios with TreeTableView and ListView. It is behaving as expected.
I still need to review the code changes.

@kleopatra
Copy link
Collaborator

@kleopatra kleopatra commented Jun 2, 2021

@kleopatra Do you think this PR is good to go?

Didn't look close enough to judge that - just interested in the unusual (to put it mildly ;) implementation of selectedItems. Don't intend to go for a review, sry that I didn't make that clear from the start.

Copy link
Member

@arapte arapte left a comment

Fix looks good to me. Provided few minor comments.

@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Jun 11, 2021

Done.

arapte
arapte approved these changes Jun 14, 2021
Copy link
Member

@arapte arapte left a comment

Looks good to me.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Looks good. I have one minor comment on the test.

arapte
arapte approved these changes Jun 27, 2021
@@ -105,6 +106,7 @@ static void requestFocusOnControlOnlyIfCurrentFocusOwnerIsChild(Control c) {
}

@Override public int getRemovedSize() {
checkState();
Copy link
Member

@Maran23 Maran23 Jun 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed here? Just wondering because it was not there before.

Copy link
Collaborator Author

@mstr2 mstr2 Jun 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the check, it would be possible to inspect the size without calling next() first.

Copy link
Member

@Maran23 Maran23 Jun 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. But I want to understand why this was not there before.

Is this needed, because if you didn't called next(), there is no change yet to retrieve the removed size?

Copy link
Collaborator Author

@mstr2 mstr2 Jun 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed according to the documentation of ListChangeListener.Change:

Important: It's necessary to call next() method before calling any other method of Change. The same applies after calling reset(). The only methods that works at any time is getList().

Also, it intuitively makes sense: a Change can consist of any number of sub-changes, so you need to select a sub-change first by calling next() before any of the other methods can be meaningfully called.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 27, 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:

8196065: ListChangeListener getRemoved() returns items that were not removed.

Reviewed-by: arapte, kcr

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 35 new commits pushed to the master branch:

  • 78179be: 8267554: Support loading stylesheets from data-URIs
  • 3fd4c97: 8234920: Add SpotLight to the selection of 3D light types
  • 063bfe8: 8264137: Suppress deprecation and removal warnings of internal methods
  • 8e11b94: 8268915: WebKit build fails with Xcode 12.5
  • 45786ac: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
  • 04493e5: 8165214: ListView.EditEvent.getIndex() does not return the correct index
  • 13cffba: 8269026: PasswordField doesn't render bullet character on Android
  • 171e484: 8267551: Support loading images from inline data-URIs
  • 98138c8: 8268219: hlsprogressbuffer should provide PTS after GStreamer update
  • c81a722: 8264139: Suppress removal warnings for Security Manager methods
  • ... and 25 more: https://git.openjdk.java.net/jfx/compare/9c97d9b21232a67a10debdc8dc3b10c419780f7a...master

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 (@arapte, @kevinrushforth) 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 Jun 27, 2021
@mstr2
Copy link
Collaborator Author

@mstr2 mstr2 commented Jun 27, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jun 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 27, 2021

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

@arapte
Copy link
Member

@arapte arapte commented Jun 28, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

Going to push as commit c4cc998.
Since your change was applied there have been 35 commits pushed to the master branch:

  • 78179be: 8267554: Support loading stylesheets from data-URIs
  • 3fd4c97: 8234920: Add SpotLight to the selection of 3D light types
  • 063bfe8: 8264137: Suppress deprecation and removal warnings of internal methods
  • 8e11b94: 8268915: WebKit build fails with Xcode 12.5
  • 45786ac: 8252238: TableView: Editable (pseudo-editable) cells should respect the row editability
  • 04493e5: 8165214: ListView.EditEvent.getIndex() does not return the correct index
  • 13cffba: 8269026: PasswordField doesn't render bullet character on Android
  • 171e484: 8267551: Support loading images from inline data-URIs
  • 98138c8: 8268219: hlsprogressbuffer should provide PTS after GStreamer update
  • c81a722: 8264139: Suppress removal warnings for Security Manager methods
  • ... and 25 more: https://git.openjdk.java.net/jfx/compare/9c97d9b21232a67a10debdc8dc3b10c419780f7a...master

Your commit was automatically rebased without conflicts.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 28, 2021

@arapte @mstr2 Pushed as commit c4cc998.

💡 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
5 participants