RFR: 8268642: Improve property system to facilitate correct usage [v3]

[John Hendrikx]

On Wed, 4 Jan 2023 05:36:21 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> Based on previous discussions, this PR attempts to improve the JavaFX property system by enforcing correct API usage in several cases that are outlined below. It also streamlines the API by deprecating untyped APIs in favor of typed APIs that better express intent.
>> 
>> ### 1. Behavioral changes for regular bindings
>> 
>> var target = new SimpleObjectProperty(bean, "target");
>> var source = new SimpleObjectProperty(bean, "source");
>> target.bind(source);
>> target.bindBidirectional(source);
>> 
>> _Before:_ `RuntimeException` --> "bean.target: A bound value cannot be set."
>> _After:_ `IllegalStateException` --> "bean.target: Bidirectional binding cannot target a bound property."
>> 
>> 
>> var target = new SimpleObjectProperty(bean, "target");
>> var source = new SimpleObjectProperty(bean, "source");
>> var other = new SimpleObjectProperty(bean, "other");
>> source.bind(other);
>> target.bindBidirectional(source);
>> 
>> _Before:_ no error
>> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional binding cannot target a bound property."
>> 
>> 
>> var target = new SimpleObjectProperty(bean, "target");
>> var source = new SimpleObjectProperty(bean, "source");
>> target.bindBidirectional(source);
>> target.bind(source);
>> 
>> _Before:_ no error
>> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a property that is targeted by a bidirectional binding."
>> 
>> 
>> ### 2. Behavioral changes for content bindings
>> 
>> var target = new SimpleListProperty(bean, "target");
>> var source = new SimpleListProperty(bean, "source");
>> target.bindContent(source);
>> target.bindContentBidirectional(source);
>> 
>> _Before:_ no error
>> _After:_ `IllegalStateException` --> "bean.target: Bidirectional content binding cannot target a bound collection."
>> 
>> 
>> var target = new SimpleListProperty(bean, "target");
>> var source = new SimpleListProperty(bean, "source");
>> var other = new SimpleListProperty();
>> source.bindContent(other);
>> target.bindContentBidirectional(source);
>> 
>> _Before:_ no error
>> _After:_ `IllegalArgumentException` --> "bean.source: Bidirectional content binding cannot target a bound collection."
>> 
>> 
>> var target = new SimpleListProperty(bean, "target");
>> var source = new SimpleListProperty(bean, "source");
>> target.bindContentBidirectional(source);
>> target.bindContent(source);
>> 
>> _Before:_ no error
>> _After:_ `IllegalStateException` --> "bean.target: Cannot bind a collection that is targeted by a bidirectional content binding."
>> 
>> 
>> var target = new SimpleListProperty(bean, "target");
>> var source = new SimpleListProperty(bean, "source");
>> target.bindContent(source);
>> target.set(FXCollections.observableArrayList());
>> 
>> _Before:_ no error
>> _After:_ `IllegalStateException` --> "bean.target: Cannot set the value of a content-bound property."
>> 
>> 
>> var target = new SimpleListProperty(bean, "target");
>> var source = new SimpleListProperty(bean, "source");
>> target.bindContent(source);
>> target.add("foo");
>> 
>> _Before_: no error, but binding is broken because the lists are in an inconsistent state
>> _After:_ `RuntimeExeption` via `Thread.UncaughtExceptionHandler` --> "Illegal list modification: Content binding was removed because the lists are out of sync."
>> 
>> 
>> var target = new SimpleListProperty(bean, "target");
>> var source = new SimpleListProperty(bean, "source");
>> target.bindContentBidirectional(source);
>> target.bindContentBidirectional(source);
>> target.add("foo");
>> 
>> _Before_: no error, but `target` contains `[foo, foo, foo, foo]`, while `source` contains `[foo, foo, foo]`
>> _After:_ no error, both lists contain `[foo]` (the second binding replaces the first)
>> 
>> 
>> ### 3. Align un-binding of unidirectional content bindings with regular unidirectional bindings
>> The API of unidirectional content bindings is aligned with regular unidirectional bindings because the semantics are similar. Like `unbind()`, `unbindContent(Object)` should not need an argument because there can only be a single content binding. For this reason, `unbindContent(Object)` will be deprecated in favor of `unbindContent()`:
>> 
>>   void bindContent(ObservableList<E> source);
>> + void unbindContent();
>> + boolean isContentBound();
>> 
>> + @Deprecated(since = "21")
>>   void unbindContent(Object object);
>> 
>> 
>> ### 4. Replace untyped binding APIs with typed APIs
>> The following untyped APIs will be deprecated in favor of typed replacements:
>> In `javafx.beans.binding.Bindings`:
>> 
>> @Deprecated(since = "21")
>> void unbindBidirectional(Object property1, Object property2)
>> 
>> @Deprecated(since = "21")
>> void unbindContentBidirectional(Object obj1, Object obj2)
>> 
>> @Deprecated(since = "21")
>> void unbindContent(Object obj1, Object obj2)
>> 
>> 
>> In `ReadOnlyListProperty`, `ReadOnlySetProperty`, `ReadOnlyMapProperty`:
>> 
>> @Deprecated(since = "21")
>> void unbindContentBidirectional(Object object)
>> 
>> 
>> ~~5. Support un-binding bidirectional conversion bindings with typed API
>> At this moment, `StringProperty` is the only property implementation that offers bidirectional conversion bindings, where the `StringProperty` is bound to a property of another type:~~
>> 
>> <U> void bindBidirectional(Property<U> other, StringConverter<U> converter)
>> 
>> ~~The inherited `Property.unbindBidirectional(Property<String>)` API cannot be used to unbind a conversion binding, because it only accepts arguments of type `Property<String>`.
>> `StringProperty` solves this issue by adding an untyped overload: `unbindBidirectional(Object)`.~~
>> 
>> ~~I propose to relax the definition of `Property.unbindBidirectional`:~~
>> 
>> - void unbindBidirectional(Property<T> other);
>> + void unbindBidirectional(Property<?> other);
>> 
>> ~~This allows property implementations to use the same API to un-bind regular bidirectional bindings, as well as converting bidirectional bindings. The `Property` typing is retained with a minimal impact on existing code (most _usages_ of this API will continue to compile, while classes that override `unbindBidirectional` will need to be changed to match the new API).~~
>> 
>> ~~As a side-effect, this allows the option of easily adding conversion bindings for other property implementations as well.~~
>
> Michael Strauß has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - check for null
>  - javadoc changes

@mstr2 would this PR make it impossible to keep more than 2 properties in sync with each other? Bidirectional bindings previously allowed for this, and I can imagine some use as well when there are a few layers involved. 

I may for example be exposing a model for users to use, which I've bidirectionally bound to internal properties (a Skin might do the same).  The user of that model may then want to do their own bindings to wherever they see fit, which may involve another bidirectional binding.  If adding one would silently replace the one between the exposed model and internal properties, then things break.  I'm pretty sure this would break code I've written in the past (I tend to bidirectionally bind things in custom skins to a control, which in turn may be bound by the user to something)

-------------

PR: https://git.openjdk.org/jfx/pull/590