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

Dalibor Topic robilad at openjdk.java.net
Fri Sep 4 16:48:19 UTC 2020

On Tue, 1 Sep 2020 00:23:38 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

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

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


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

More information about the openjfx-dev mailing list