Updating class javafx.beans.binding.When

[Kevin Rushforth]

This will take me a bit more time to look at than I have right now (and 
Monday is a US holiday), so one quick comment for now:

The refactoring must not alter any public API signatures for exported 
packagers, and must be behavior neutral. So if there are unit tests that 
pass without your fix and fail with your fix, then you will need to 
alter your fix, unless you can show that the tests are testing an 
implementation detail that would not affect an application that just 
uses public API.

-- Kevin


Nir Lisker wrote:
> Let's start with the refactoring then. Before I submit a bug let's 
> check that this plan makes sense. Attached webrev for discussion.
>
> Changes:
> * Main change was to the XxxCondition classes. Instead of having 4 
> combinations of primitives and observables I used the unification 
> approach that NumberConditionBuilder took with wrapping the value in a 
> constant observable. I also replaced these classes with a generic 
> wrappers XxxConditionHolders which hold the binding part of the 
> XxxCondition class.
>
> * I attempted to generify XxxConditionBuilders as well. The attempt is 
> ConditionBuilder and an example implementation was done for boolean 
> only - BooleanConditionBuilder2. I think that it doesn't gain much.
>
> * Added BooleanConstant class in internal API (it was missing for some 
> reason).
>
> * The handling of Numbers in the original class is a bit weird in my 
> eyes. The compile time return types are DoubleBinding if one or both 
> values are double and NumberBinding otherwise [1]. On the other hand, 
> the runtime return types takes special care to return a binding class 
> based on widening conventions and the docs don't mention anything 
> about that. In my change, the runtime type is always DoubleBinding 
> (though I kept a placeholder if-else chain) and that saves some code. 
> The user can always call floatValue() etc. anyway.
>
> Between backwards compatibility and limitations of generics this is 
> the best I could come up with.
>
> Additional notes:
>
> * Running the tests from gradle causes some of the When tests to fail 
> and I don't know why, it's hard to debug. I wrote my own ad-hoc test 
> for one of the failed tests and it passes.
>
> * I noticed that StringConstant extends StringExpression while all the 
> other classes just implement their respective ObservableXxxValue. 
> Don't know why, but I can align these classes to one of those choices.
>
> [1] https://docs.oracle.com/javase/9/docs/api/javafx/beans/binding/When.NumberConditionBuilder.html
>
> On Thu, Feb 15, 2018 at 5:04 AM, Kevin Rushforth 
> <kevin.rushforth at oracle.com <mailto:kevin.rushforth at oracle.com>> wrote:
>
>     Sorry for the delay in responding. I was traveling when this was
>     sent and barely able to keep up with urgent email / tasks.
>
>     Most of what you suggest below seems good. My only concern is
>     whether the "on demand" evaluation will have any side effects.
>     Conceptually, it seems like the right thing to do.
>
>     The refactoring you propose is probably best done as a separate
>     bug fix, so that we don't mix behavioral changes with refactoring,
>     unless you think that the refactoring is intertwined with the fix.
>
>     If you would like to work on a fix, that would be good. It will
>     need to include new unit tests, plus ensuring that the existing
>     unit tests pass.
>
>     Thanks.
>
>     -- Kevin
>
>
>
>     Nir Lisker wrote:
>
>         Hi,
>
>         I was looking at
>         https://bugs.openjdk.java.net/browse/JDK-8089579
>         <https://bugs.openjdk.java.net/browse/JDK-8089579>, which
>         prompted me to have a look at When. There are a few points I
>         would like to
>         address:
>
>         * StringConditionBuilder#otherwise(ObservableStringValue) does
>         not check
>         for null as other condition builders do. This results in a
>         deeper NPE
>         when StringCondition tries to register a listener to the
>         ObservableStringValue.
>
>         * I would like a (re)evaluation on the above bug ticket and
>         thoughts on the
>         proposal of "on demand evaluation" using a Supplier or a
>         similar method.
>         The behavior of the intended implementation would be to
>         evaluate 'then' and
>         'otherwise' whenever their condition is met, and only then.
>
>         * The class can benefit from some small refactoring, such as using
>         Objects.requireNonNull for null checks and some code reuse to
>         reduce the
>         chance of bugs such as the missing null check of
>         StringConditionBuilder.
>
>         * There are a few Javadoc corrections and some clarifications
>         of the
>         current behavior could be beneficial as well.
>
>         I can work on all of the above. How to proceed?
>
>         - Nir
>          
>
>