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.rushfo...@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 >> >> >
