[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: Updating class javafx.beans.binding.When
From:       Kevin Rushforth <kevin.rushforth () oracle ! com>
Date:       2018-03-13 14:26:18
Message-ID: 5AA7DF8A.4070502 () oracle ! com
[Download RAW message or body]



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 <nlisker@gmail.com 
> <mailto:nlisker@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.rushforth@oracle.com <mailto:kevin.rushforth@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.rushforth@oracle.com
> > <mailto:kevin.rushforth@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
> > 
> > 
> > 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]