8268642: Improve property system to facilitate correct usage

[mstr2]

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.~~

---

Progress

• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue
• [ ] Change must be properly reviewed

Integration blocker

 ⚠️ The change requires a CSR request to be approved.

Issue

• JDK-8268642: Improve property system to facilitate correct usage

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/590/head:pull/590
$ git checkout pull/590

Update a local copy of the PR:
$ git checkout pull/590
$ git pull https://git.openjdk.java.net/jfx pull/590/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 590

View PR using the GUI difftool:
$ git pr show -t 590

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/590.diff

[bridgekeeper[bot]]

:wave: Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[kevinrushforth]

Moving to Draft until the discussion surrounding the API issues is resolved. When they are, this will need a CSR and two reviewers.

/csr /reviewers 2

[openjdk[bot]]

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mstr2 this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /issue command in a comment in this pull request.

[openjdk[bot]]

@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[mlbridge[bot]]

Webrevs

• 02: Full - Incremental (2aa560b8)
• 01: Full (86280bea)
• 00: Full (244b76f2)

[kevinrushforth]

3. Align un-binding of unidirectional content bindings with regular unidirectional bindings

Same comment as for item 4. We need to finish the discussion of whether and how to do this.

[hjohn]

@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)

[mstr2]

@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.

Sure, this PR doesn't remove any functionality that currently exists. The point of this PR is to explicitly disallow scenarios that cannot meaningfully work at all.

[openjdk[bot]]

@mstr2 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout feature/property-enhancements
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[bridgekeeper[bot]]

@mstr2 This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@mstr2 This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.