RFR: 8290040: Provide simplified deterministic way to manage listeners [v5]

[John Hendrikx]

On Mon, 26 Sep 2022 19:37:54 GMT, Nir Lisker <nlisker at 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