Nir Lisker wrote:
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 mentioned before, this seems OK long as the refactoring is
behavior-neutral and limited in scope.
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?
What new public API are you envisioning to propose? I would recommend to
file a new Enhancement request, but leave the existing bug open (you can
link it to the new RFE) until there is agreement on any new API, and
until we know it solves the problem specified in the bug report.
-- Kevin
- Nir
On Tue, Feb 20, 2018 at 12:45 PM, Nir Lisker <nlis...@gmail.com
<mailto: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 <mailto: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
<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
