Search code examples
javajavafxproperty-binding

ReadOnlyBooleanWrapper: incorrect behaviour when used with Bindings.or


I encountered a very strange behavior when binding two ReadOnlyBooleanWrapper objects using a Binding.or. Namely, the result is not correct (still false) when I do b1.set(true) on the first argument.

Here's a simple unit test (that fails):

@Test
public void testReadOnlyBooleanWrapper() {
    // Fails!!!
    testOr(new ReadOnlyBooleanWrapper(false), new ReadOnlyBooleanWrapper(false));
}

public void testOr(BooleanProperty b1, BooleanProperty b2) {
    BooleanExpression or = b1.or(b2);

    assertEquals(or.get(), b1.get() || b2.get());
    b1.set(true);
    assertEquals(or.get(), b1.get() || b2.get());
    b1.set(false);
    assertEquals(or.get(), b1.get() || b2.get());
}

Note that the same test with SimpleBooleanProperties works fine:

@Test
public void testSimpleBooleanProperty() {
    // Passes
    testOr(new SimpleBooleanProperty(false), new SimpleBooleanProperty(false));
}

I'm probably missing some point here and misusing the properties, because I can't imagine such a bug in the implementation! :)

Thanks for any ideas how to explain it!

UPDATE:

Your answers and comments have put me on the right track :) I don't have the solution yet, but I've noticed that if I bind the or to ReadOnlyProperties (supposed to be exposed) and not to the wrappers themselves, then the test passes:

@Test
public void testOrReadOnly() {
    ReadOnlyBooleanWrapper b1 = new ReadOnlyBooleanWrapper(false);
    ReadOnlyBooleanWrapper b2 = new ReadOnlyBooleanWrapper(false);

    BooleanExpression or = b1.getReadOnlyProperty().or(b2.getReadOnlyProperty());

    System.out.println("Condition " + or.get());
    assertEquals(or.get(), b1.get() || b2.get());
    b1.set(true);
    System.out.println("Condition " + or.get());
    assertEquals(or.get(), b1.get() || b2.get());
    b1.set(false);
    System.out.println("Condition " + or.get());
    assertEquals(or.get(), b1.get() || b2.get());
}

Which makes me think that the synchronization between the internal read- and write-able property and the read-only one, is somehow broken (?)

From the Docs about ReadOnlyBooleanWrapper:

This class provides a convenient class to define read-only properties. It creates two properties that are synchronized. One property is read-only and can be passed to external users. The other property is read- and writable and should be used internally only.


Solution

  • This is a bug (imo).

    What is happening is that the implementation of the or binding implements a shortcircuit for the "or" (this is in Bindings.java):

        @Override
        public void invalidated(Observable observable) {
            final BooleanOrBinding binding = ref.get();
            if (binding == null) {
                observable.removeListener(this);
            } else {
                // short circuit invalidation. This BooleanBinding becomes
                // only invalid if the first operator changes or the
                // first parameter is false.
                if ((binding.op1.equals(observable) || (binding.isValid() && !binding.op1.get()))) {
                    binding.invalidate();
                }
            }
        }
    

    The listener implementing this is added to each of the operands in the or: however, ReadOnlyBooleanWrapper delegates the listener to its ReadOnlyBooleanProperty (from ReadOnlyBooleanWrapper.java):

    @Override
    public void addListener(InvalidationListener listener) {
        getReadOnlyProperty().addListener(listener);
    }
    

    Thus in the shortcircuit implementation, binding.op1.equals(observable) is false (because one is the ReadOnlyBooleanWrapper, and the other is its ReadOnlyBooleanProperty). So the logic here is (incorrectly) assuming the second operator has changed. Since the first operator has the value true, the conclusion of the implementation is that the evaluation of the or cannot have changed, so no invalidation is fired. Hence your or doesn't get re-evaluated.

    The workaround, as you have discovered, is to use the wrapped ReadOnlyBooleanProperty in computing the binding. I recommend you report this bug.