On Thu, 20 Aug 2020 12:54:27 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Hi, this PR fixes >> [JDK-8251946](https://bugs.openjdk.java.net/browse/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.) > > @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](https://github.com/openjdk/jfx/tree/master/modules/javafx.base/src/test/java/test/javafx/collections). 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. ------------- PR: https://git.openjdk.java.net/jfx/pull/284
