On Thu, 20 Apr 2023 08:28:20 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> These example would probably look like this in the near future:
>> ``` java
>> minimumWidthProperty().addListener(this::requestLayout());
>> minimumWidthProperty().addListener(_ -> this.requestLayout());
>>
>> selectedItem.addListener(getSelectionModel()::select);
>> selectedItem.addListener((_, _, value) -> gelectionModel().select(value));
>>
>> modelStatus.addListener(statusLabel.textProperty()::set);
>> modelStatus.addListener((_, _, value) -> statusLabel.setText(value));
>>
>>
>> You can always ignore the `Observable` parameter, but you can't bring it
>> back once it's gone. I have often used this parameter, for example when my
>> listener handles changes of multiple similar properties. Removing
>> functionality just to make it look a little prettier is not enough
>> justification in my opinion.
>>
>> If a subscription shouldn't be removable by passing the listener to
>> `removeListener`, this can easily be achieved by internally wrapping it into
>> another listener (just what you did, only with the functional interface
>> being different). However, this might not be the best solution, as it
>> doubles the number of listener objects when using the subscription API.
>
> Yeah I'm aware they want to add this, but it doesn't really invalidate my
> point as that syntax is terrible still. Method references are far preferred
> whenever possible.
>
> I stand by my point that using the observable parameter is a rare and quite
> advanced use case, where these API's would be intended to make it easier and
> safer to use method references and lambda's.
>
> When dealing with multiple listeners that require the `Observable`, you are
> likely also dealing with adding and removing that single listener in some
> dynamic fashion. Subscriptions don't work well with this as you'd need to
> track these, unlike `removeListener` which you can just pass the single
> listener reference.
>
> Here's one of the only examples in JavaFX that uses the observable parameter
> (if you know of others, I'd be interested to see those as well):
>
> private final ChangeListener<Boolean> paneFocusListener = new
> ChangeListener<>() {
> @Override public void changed(ObservableValue<? extends Boolean>
> observable, Boolean oldValue, Boolean newValue) {
> if (newValue) {
> final ReadOnlyBooleanProperty focusedProperty =
> (ReadOnlyBooleanProperty) observable;
> final TitledPane tp = (TitledPane)
> focusedProperty.getBean();
> focus(accordion.getPanes().indexOf(tp));
> }
> }
> };
>
> And its management code:
>
> private final ListChangeListener<TitledPane> panesListener = c -> {
> while (c.next()) {
> if (c.wasAdded()) {
> for (final TitledPane tp: c.getAddedSubList()) {
> tp.focusedProperty().addListener(paneFocusListener);
> }
> } else if (c.wasRemoved()) {
> for (final TitledPane tp: c.getRemoved()) {
>
> tp.focusedProperty().removeListener(paneFocusListener);
> }
> }
> }
> };
>
> To do this with `Subscription::unsubscribe` you'd need to track the
> `Subscriptions`... something like:
>
> private final Map<Property<?>, Subscription> subscriptions = new
> HashMap<>();
> private final ListChangeListener<TitledPane> panesListener = c -> {
> while (c.next()) {
> if (c.wasAdded()) {
> for (final TitledPane tp: c.getAddedSubList()) {
> subscriptions.put(tp.focusedProperty(),
> tp.focusedProperty().changes(paneFocusListener));
> ...
I think the three newly added methods are a good choice. I wonder if we can
some up with better names, though. without some verb like "add" or "subscribe"
in the name, the name doesn't really indicate that it is adding a new listener
to the observable.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1069#discussion_r1230071655
