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

[Dalibor Topic]

On Tue, 1 Sep 2020 15:17:18 GMT, Leon Linhart <github.com+4029915+TheMrMilchmann at openjdk.org> wrote:

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

Thanks @TheMrMilchmann and apologies for the delay!

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

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