8251946: ObservableList.setAll does not conform to specification

[TheMrMilchmann]

Hi, this PR fixes JDK-8251946 computing whether the list was actually modified instead of just returning true. The list was modified if

1. it was not empty (modified by calling #clear()), or if
2. it was modified as result of the #addAll() call.

If you want any test coverage for this please let me know.

I reported the issue a couple of days ago via web formula and waited for the confirmation to open this PR but now I see that @kevinrushforth (sorry for pinging) is already assigned to the JBS issue. I'm not too familiar with how you handle JBS issues or more specifically whether assigning is used to indicate that the issue is in the assignee's "domain", or that the assignee is already working on it. In the latter case, please feel free to close this PR. (My OCA submission is still pending anyway.)

---

Progress

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

Issue

• JDK-8251946: ObservableList.setAll does not conform to specification

Reviewers

• Ambarish Rapte (@arapte - Reviewer)
• Kevin Rushforth (@kevinrushforth - Reviewer)

Download

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

[bridgekeeper]

Hi @TheMrMilchmann, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

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 TheMrMilchmann" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[kevinrushforth]

@TheMrMilchmann I am not actively working on that bug, so you can proceed with this PR. I will review it when it is ready.

Once you have submitted the OCA, please add a comment to this PR with the /signed command (and nothing else in the comment). After your OCA is approved and recorded, the Skara bot will mark it as ready for review.

Please do add a unit test for this. You can find existing collections tests here.

[TheMrMilchmann]

While adding unit tests, I noticed that I missed an important edge-case that has to be considered when computing if a list was modified. The initial implementation assumed that

the list was modified if

1. it was not empty (modified by calling #clear()), or if
2. it was modified as result of the #addAll() call.

However, a non-empty list is not modified either if setAll is called with an equal list. The PR has been updated to take this into account and unit tests have been added. Note that the current implementation is rather complex and could be greatly simplified by making a copy of the list before modification. .i.e

List<E> prev = List.copyOf(this);
clear();
addAll(col);
return !equals(prev);

Please let me know which solution you prefer.

[TheMrMilchmann]

/signed

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[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]

One overall comment while we are waiting for your OCA to be approved.

I don't think the complexity of this proposed fix to setAll is warranted. I would prefer a simpler fix that returns false if both the current list and the new list are empty, and true otherwise. This method is essentially a convenience method that does:

List::clear
List::addAll(...)

Given this, it seems reasonable for setAll to return true if either clear or addAll would modify the list.

If there is a good justification for handling the corner case of calling setAll with the same list of elements in exactly the same order (and I am not sure that there is), then a better approach might be to do the check before actually modifying the list, returning early if the new list and the current list were identical. In that case, the list really would be unmodified and no listeners would be notified.

[robilad]

Unfortunately, I don't see @TheMrMilchmann OCA in the approval queue. Could you please (re)send it to Oracle-ca_us@oracle.com? Thanks!

[TheMrMilchmann]

I don't think the complexity of this proposed fix to setAll is warranted. I would prefer a simpler fix that returns false if both the current list and the new list are empty, and true otherwise.

@kevinrushforth Thanks for your feedback! I have just pushed a commit that greatly simplifies setAll by returning early if both lists are empty and continuing with clear, addAll otherwise.

If there is a good justification for handling the corner case of calling setAll with the same list of elements in exactly the same order (and I am not sure that there is), then a better approach might be to do the check before actually modifying the list, returning early if the new list and the current list were identical.

Fair point. I'm afraid I don't have any justification other than that, in my opinion, this is a violation of the method's contract (which is debatable). I suppose it's best to leave it as-is for this case for now.

---

Unfortunately, I don't see @TheMrMilchmann OCA in the approval queue. Could you please (re)send it to Oracle-ca_us@oracle.com? Thanks!

@robilad That's strange. I have sent a mail on Aug 13 and received no indication that it didn't go through (though neither did I receive an automated confirmation that it arrived - if such a thing is set up). Anyway, I just resent it.

[robilad]

Thanks @TheMrMilchmann and apologies for the delay!

[mlbridge]

Webrevs

• 03: Full - Incremental (5de1a51)
• 02: Full - Incremental (5f260d7)
• 01: Full - Incremental (e885791)
• 00: Full (5f36038)

[arapte]

Change seems good, suggested minor changes.

[kevinrushforth]

The code change looks good. I have one suggestion for the test.

[openjdk]

@TheMrMilchmann 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:

8251946: ObservableList.setAll does not conform to specification

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

• 1c485a3: 8252191: Update to gcc 10.2 on Linux
• 15e52d8: 8253696: WebEngine refuses to load local "file:///" CSS stylesheets when using JDK 15
• 5b42b64: 8252236: TabPane: must keep header of selected tab visible
• 77a183e: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing
• d10f948: 8252811: The list of cells in a VirtualFlow is cleared every time the number of items changes
• 47e67b4: 8253123: Switch FX build to use JDK 15 as boot JDK
• 13ab2cb: 8252446: Screen.getScreens() is empty sometimes
• b2e2000: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec
• 976a763: 8240499: Enforce whitespace checking for additional source files
• d6dee34: 8252547: Correct transformations docs in Node
• ... and 13 more: https://git.openjdk.java.net/jfx/compare/eb9886ae0a961ec69aa3717c8d39f179dc2c620b...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).

[TheMrMilchmann]

/integrate

[openjdk]

@TheMrMilchmann
Your change (at version 5de1a51) is now ready to be sponsored by a Committer.

[nlisker]

/sponsor

[openjdk]

@nlisker @TheMrMilchmann Since your change was applied there have been 23 commits pushed to the master branch:

• 1c485a3: 8252191: Update to gcc 10.2 on Linux
• 15e52d8: 8253696: WebEngine refuses to load local "file:///" CSS stylesheets when using JDK 15
• 5b42b64: 8252236: TabPane: must keep header of selected tab visible
• 77a183e: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing
• d10f948: 8252811: The list of cells in a VirtualFlow is cleared every time the number of items changes
• 47e67b4: 8253123: Switch FX build to use JDK 15 as boot JDK
• 13ab2cb: 8252446: Screen.getScreens() is empty sometimes
• b2e2000: 8252546: Move ObservableValue's equality check and lazy evaluation descriptions to @implSpec
• 976a763: 8240499: Enforce whitespace checking for additional source files
• d6dee34: 8252547: Correct transformations docs in Node
• ... and 13 more: https://git.openjdk.java.net/jfx/compare/eb9886ae0a961ec69aa3717c8d39f179dc2c620b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7857022.

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