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 <mailto: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
<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
