Updating class javafx.beans.binding.When

Nir Lisker nlisker at gmail.com
Tue Mar 13 14:02:43 UTC 2018

Solved the configuration issues, so I'm continuing with this.

I submitted https://bugs.openjdk.java.net/browse/JDK-8199514 for the
refactoring part.

As for https://bugs.openjdk.java.net/browse/JDK-8089579, should it be
closed as Not an Issue and a new issue created for the new API propasal, or
should it be retrofitted?

- Nir

On Tue, Feb 20, 2018 at 12:45 PM, Nir Lisker <nlisker at gmail.com> wrote:

> OK, let's wait with this until I figure out if there's a problem with the
> test suit.
> On Fri, Feb 16, 2018 at 11:46 PM, Kevin Rushforth <
> kevin.rushforth at oracle.com> wrote:
>> 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> 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

More information about the openjfx-dev mailing list