On Tue, 1 Sep 2020 15:17:18 GMT, Leon Linhart 
<github.com+4029915+themrmilchm...@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...@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

Reply via email to