On Mon, 26 Sep 2022 19:37:54 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add missing new line
>
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java
> line 12:
>
>> 10: private final ObservableValue<Boolean> nonNullCondition;
>> 11:
>> 12: private Subscription subscription;
>
> Isn't it better to use an `EMPTY` subscription, like `FlatMap` does?
I think it would be less good here, since I also need to know whether there
currently is a subscription. Comparing with the empty subscription feels not
very nice, specifically this code:
@Override
protected T computeValue() {
if (isObserved() && isActive()) {
if (subscription.equals(Subscription.EMPTY)) { // was:
if(subscription == null) {
subscription = Subscription.subscribeInvalidations(source,
this::invalidate);
}
}
else {
unsubscribe();
}
return source.getValue();
}
In flatMap this doesn't occur, and we don't care so much whether that specific
subscription is present or not. In flatMap the empty subscription is used
because there always should be a subscription, but in case of a mapping to
`null` there obviously can't be one.
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java
> line 16:
>
>> 14: public ConditionalBinding(ObservableValue<T> source,
>> ObservableValue<Boolean> condition) {
>> 15: this.source = Objects.requireNonNull(source, "source");
>> 16: this.nonNullCondition = Objects.requireNonNull(condition,
>> "condition").orElse(false);
>
> The message should probably match the other classes: "source cannot be null".
Fixed
> modules/javafx.base/src/main/java/com/sun/javafx/binding/ConditionalBinding.java
> line 41:
>
>> 39: protected T computeValue() {
>> 40: if (isObserved() && isActive()) {
>> 41: if(subscription == null) {
>
> Space after `if`.
Fixed (everywhere)
> modules/javafx.base/src/test/java/test/javafx/beans/value/LazyObjectBindingTest.java
> line 65:
>
>> 63:
>> 64: binding.addListener(obs -> {
>> 65: assertEquals("A", binding.get());
>
> Can be converted to an expression.
Fixed
> modules/javafx.base/src/test/java/test/javafx/beans/value/ObservableValueFluentBindingsTest.java
> line 52:
>
>> 50:
>> 51: public class ObservableValueFluentBindingsTest {
>> 52: private int invalidations;
>
> Empty line after class declaration.
Fixed
-------------
PR: https://git.openjdk.org/jfx/pull/830
