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