On Sat, 3 Apr 2021 15:20:41 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
> The internal BidirectionalBinding class implements bidirectional bindings for
> JavaFX properties. The design intent of this class is to provide
> specializations for primitive value types to prevent boxing conversions (cf.
> specializations of the Property class with a similar design intent).
>
> However, the primitive BidirectionalBinding implementations do not meet the
> design goal of preventing boxing conversions, because they implement
> ChangeListener.
>
> ChangeListener is a generic SAM interface, which makes it impossibe to invoke
> an implementation of ChangeListener::changed with a primitive value (i.e. any
> primitive value will be auto-boxed).
>
> The boxing conversion happens, as with all ChangeListeners, at the invocation
> site (for example, in ExpressionHelper). Since the boxing conversion has
> already happened by the time any of the BidirectionalBinding implementations
> is invoked, there's no point in using primitive specializations of
> BidirectionalBinding after the fact.
>
> This issue can be solved by having BidirectionalBinding implement
> InvalidationListener instead, which by itself does not incur a boxing
> conversion. Because bidirectional bindings are eagerly evaluated, the
> observable behavior remains the same.
>
> I've filed a bug report with the same title.
The fix looks good, but I noted two places where I think you need to initialize
`oldValue`.
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 585:
> 583: private TypedGenericBidirectionalBinding(Property<T> property1,
> Property<T> property2) {
> 584: super(property1, property2);
> 585: propertyRef1 = new WeakReference<>(property1);
Don't you need to initialize `oldValue` here?
oldValue = property1.get();
modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
line 657:
> 655: private TypedNumberBidirectionalBinding(Property<T> property1,
> Property<Number> property2) {
> 656: super(property1, property2);
> 657: propertyRef1 = new WeakReference<>(property1);
Same comment as above. Shouldn't `oldValue` be set?
-------------
PR: https://git.openjdk.java.net/jfx/pull/454
