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
