Updating class javafx.beans.binding.When

[Nir Lisker]

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
> 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, 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
>>
>>
>