RFR: 8251946: ObservableList.setAll does not conform to specification

[Kevin Rushforth]

On Fri, 21 Aug 2020 14:27:15 GMT, Leon Linhart <github.com+4029915+TheMrMilchmann at openjdk.org> wrote:

>> @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.

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.

-------------

PR: https://git.openjdk.java.net/jfx/pull/284