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

Ambarish Rapte arapte at openjdk.java.net
Thu Sep 24 18:36:30 UTC 2020


On Tue, 18 Aug 2020 19:50:55 GMT, Leon Linhart <github.com+4029915+TheMrMilchmann at openjdk.org> wrote:

> Hi, this PR fixes [JDK-8251946](https://bugs.openjdk.java.net/browse/JDK-8251946) computing whether the list was
> actually modified instead of just returning `true`. 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.
> 
> If you want any test coverage for this please let me know.
> 
> I reported the issue a couple of days ago via web formula and waited for the confirmation to open this PR but now I see
> that @kevinrushforth (sorry for pinging) is already assigned to the JBS issue. I'm not too familiar with how you handle
> JBS issues or more specifically whether assigning is used to indicate that the issue is in the assignee's "domain", or
> that the assignee is already working on it. In the latter case, please feel free to close this PR. (My OCA submission
> is still pending anyway.)

Change seems good, suggested minor changes.

modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java line 32:

> 30: import java.util.List;
> 31: import java.util.ListIterator;
> 32: import java.util.Objects;

This import seems unused, please remove.

modules/javafx.base/src/main/java/javafx/collections/ModifiableObservableListBase.java line 97:

> 95:             clear();
> 96:             addAll(col);
> 97:             return true;

I think following code would be more suitable here,
boolean res = super.addAll(c);
return res;
This code is already used in two `addAll()` methods of this class.

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

Changes requested by arapte (Reviewer).

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


More information about the openjfx-dev mailing list