Bug 486294

Comment 1 Alexander Nyßen 2016-01-21 13:56:53 EST

(In reply to Matthias Wienand from comment #0)
> The JavaFX properties API has a few shortcomings, i.e. not all property
> implementations support the following features:
> 
> - Iterable atomic changes: Per method call, only one change event should be
> fired. This change event should be iterable to allow processing of atomic
> changes, one by one. (Currently only implemented for Lists.)

That is, each change to an observable collection should result in a single notification of attached listeners (invalidation, as well as collection-specific listener). Currently only ObservableList constructs such changes (while the comprised changes of such a composite atomic change are not minimal, i.e. a setAll() will lead to a "clear-change" and an "add-all-change" comprised in a single change object), while other collection do not support it.

> - Re-entrance safety: When iterating over a change object, that change
> object should not be altered by the outside, i.e. no more changes should be
> added to that change object. (Currently only implemented for Set, Map,
> Multiset, and SetMultimap.)

This refers to https://bugs.openjdk.java.net/browse/JDK-8092504, which prevents that listeners manipulate the source list from within the change notification.

> - Old value support: It should be possible to examine the value of a
> property before a change took place, i.e. when multiple changes are combined
> in one change event, it should still be possible to retrieve the old value
> of the property. (Currently not supported.)

This is quite easy for all collections except ObservableList, where computing the old value is really hard. If we do not need to replace the list implementation we could solve this by providing a utility class that allows to compute the old value from a given change.

> 
> Additionally, some issues of JavaFX properties should be addressed:
> 
> - https://bugs.openjdk.java.net/browse/JDK-8136465
> - https://bugs.openjdk.java.net/browse/JDK-8089557

These are already addressed by the replacement classes we already provide.

Comment 2 Alexander Nyßen 2016-01-21 14:40:10 EST

In addition we could well use an atomic move operation for list, which would lead to an  atomic change. It could be used to improve the current code within AbstractVisualPart#reorderChild().

Comment 3 Alexander Nyßen 2016-01-22 09:21:54 EST

Pushed the following changes to origin/master:

- Enhanced MultisetChangeListener so that atomic changes to a Multiset can be notified in a single (iterable) change, similar as supported by ListChangeListener. Adjusted tests and usages to comply to the changed contract.

- Renamed getSource() in MultisetChangeListener to getMultiset() and to getSetMultimap() within SetMultimapChangeListener to be better aligned with JavaFX change objects.

Comment 4 Alexander Nyßen 2016-01-22 11:45:35 EST

Another thing we should clean up is that in almost all cases we expose both, an observable collection as well as a wrapping property. That IMHO makes no sense, we want clients to register themselves at the property. As such, we should turn those collections into non-overvable ones and only offer the properties to register observable collection changes.

Comment 5 Alexander Nyßen 2016-01-25 16:05:29 EST

I pushed the following changes to origin/master:

- Enhanced SetMultimapChangeListener so that atomic changes to a SetMultimap can be notified in a single (iterable) change (providing the previous value), similar as supported by ListChangeListener. Adjusted tests and usages to comply to the changed contract.

In addition, Matthias added CollectionUtils.getPreviousContents(), which offers support to compute the previous value based on a list change.

Comment 6 Alexander Nyßen 2016-01-26 03:15:12 EST

With respect to re-entrace safety/immutable changes: This is only the case for ObservableList, all other implementations are currently safe. Also, List seems to be the only case where exceptions, which happen within the listener notification are caught and forwarded to the uncaught-exception-handler. IMHO we should try to homogenize this as well.

Comment 7 Alexander Nyßen 2016-02-17 12:56:34 EST

(In reply to Alexander Nyßen from comment #6)
> With respect to re-entrace safety/immutable changes: This is only the case
> for ObservableList, all other implementations are currently safe. Also, List
> seems to be the only case where exceptions, which happen within the listener
> notification are caught and forwarded to the uncaught-exception-handler.
> IMHO we should try to homogenize this as well.

This is not completely correct. The exception handling seems to be the same for set and map, thus its only the mutability of changes that is special for list.

Comment 8 Alexander Nyßen 2016-02-19 11:30:25 EST

I pushed the following changes to origin/master:

-Added ObservableListWrapperEx to fix that the JavaFX default implementation (ObservableListWrapper) does not make change objects immutable and that exceptions, which occur during listener notification, are captured.
- Added tests for ObservableListWrapperEx to test conformance with JavaFX ObservableList behavior.
- Reduced visibility of collection wrappers and provided utility methods within CollectionUtils as a replacement (similar to FXCollections).
- Refactored test classes for ObservableMultiset and ObservableSetMultimap to be more concise.

Despite the exception handling within set and map (and the fact that they do not fire atomic changes), the behavior of the observable collections is now comparable.

Comment 9 Alexander Nyßen 2016-02-20 08:54:17 EST

Pushed the following changes to origin/master:

- Changed implementation of ListChangeListenerHelper, MultisetChangeListenerHelper, and SetMultimapChangeListenerHelper, so that exceptions during listener notification are captured and forwarded to the registered uncaught exception handler (so our observable collection behave like provided by JavaFX). As this does not hold for change listener notifications within properties (here, JavaFX does not capture exceptions) did not add this behavior to our properties either.
- As we do not want to have exceptions being silently captured within the framework, added an uncaught exception listener registration to the constructor of AbstractDomain, so at least within GEF4 MVC this does not lead to exceptions being silently captured.
- Adjusted ObservableListTests to check for correct exception handler. Adjusted ContentSynchronizationTests to no longer use an own uncaught exception handler in the test code.

Adopting map and set to also report atomic changes would require to extend the JavaFX provided observable collection interfaces. The same would hold for list, or we would have to expose ObservabelListWrapperEx. I think we will live with this inconsistency for now (as JavaFX does so as well). Exception handling is now safely done as well. As such, resolving this as fixed in 4.0.0 M6.