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