On Tue, 27 Jul 2021 23:15:10 GMT, Michael Strauß <mstra...@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."
>
>
> ### 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 = "18", forRemoval = true)
> void unbindContent(Object object);
>
>
> ### 4. Replace untyped binding APIs with typed APIs
> The following untyped APIs will be marked for removal in favor of typed
> replacements:
> In `javafx.beans.binding.Bindings`:
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindBidirectional(Object property1, Object property2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContentBidirectional(Object obj1, Object obj2)
>
> @Deprecated(since = "18", forRemoval = true)
> void unbindContent(Object obj1, Object obj2)
>
>
> In `ReadOnlyListProperty`, `ReadOnlySetProperty`, `ReadOnlyMapProperty`:
>
> @Deprecated(since = "18", forRemoval = true)
> 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.~~
Moving to Draft until the discussion surrounding the API issues is resolved.
When they are, this will need a CSR and two reviewers.
-------------
PR: https://git.openjdk.java.net/jfx/pull/590
