Bug 467025

Comment 1 Alexander Nyßen 2015-09-22 05:29:09 EDT

We could also consider to use JavaFX properties throughout (as these are UI independent).

Comment 2 Alexander Nyßen 2015-12-18 10:42:14 EST

In either case, we should remove the own observable implementation from GEF4 Common, as the listeners are currently only used to get notified about collection changes inside GEF4 Graph.

Comment 3 Alexander Nyßen 2015-12-19 10:56:53 EST

I pushed the following changes to origin/master:

- Introduced ListProperty and MapProperty to properties package, which implement IPropertyChangeNotifier.
- Introduced a specific KeyedPropertyChangeEvent (as a complement to IndexedPropertyChangeEvent) to notify about non-bulk changes of a map property. An IndexedPropertyChangeEvent is accordingly used for non-bulk changes of a ListProperty.
- Changed Graph, Node, and Edge to use ListProperty and MapProperty instead of observable collections.
- Removed observable collections and related proprietary observers.

While the map and list property implementations are internally used, they are not exposed by Graph, Node, and Edge yet. Therefore, we consistently rely on PropertyChange notifications throughout the non JavaFX-related parts of the API, not exposing any observable properties.

Comment 4 Alexander Nyßen 2015-12-20 11:50:13 EST

Resolving as fixed in 3.11.0 M5.

Comment 5 Alexander Nyßen 2015-12-20 16:12:13 EST

As we make use of JavaFX observable properties within GEF4 FX, we could also provide observable properties (based on GEF4 Common MapProperty and ListProperty), so listeners could directly be registered at respective properties rather than the complete bean. We could still leave all beans implementing IPropertyChangeNotifier as well, so that a PropertyChangeListener could thereby be registered at all the bean's properties simultaneously. In order to make use of observable properties within MVC, we would have to provide additional observable properties for Google's SetMultimap, which could be realized based on a ForwardingSetMultimap.

Comment 6 Alexander Nyßen 2015-12-20 16:14:29 EST

(In reply to Alexander Nyßen from comment #5)
> As we make use of JavaFX observable properties within GEF4 FX, we could also
> provide observable properties (based on GEF4 Common MapProperty and
> ListProperty), so listeners could directly be registered at respective
> properties rather than the complete bean. We could still leave all beans
> implementing IPropertyChangeNotifier as well, so that a
> PropertyChangeListener could thereby be registered at all the bean's
> properties simultaneously. In order to make use of observable properties
> within MVC, we would have to provide additional observable properties for
> Google's SetMultimap, which could be realized based on a
> ForwardingSetMultimap.

We should also improve the current test cases for MapProperty and ListProperty, so it covers all basic cases properly.

Comment 7 Alexander Nyßen 2015-12-21 10:07:32 EST

At least where JavaFX code is directly involved (e.g. within GEF4 FX), we could related on javafx properties alone. We currently do so within org.eclipse.gef4.fx, but not within org.eclipse.gef4.fx.swt.

Comment 8 Alexander Nyßen 2015-12-21 11:00:32 EST

Changed that all GEF4 FX modules use JavaFX properties directly.

Comment 9 Alexander Nyßen 2015-12-21 13:49:45 EST

The following state is consistent in a sense that:

- directly JavaFX-related code (GEF4 FX, FX.SWT, FX.JFace) uses JavaFX properties
- non JavaFX-related code (GEF4 Graph, GEF4 Layout, GEF4 MVC) uses property change notifications
- indirectly JavaFX-related code (GEF4 MVC.FX, GEF4 Zest.FX) uses property change notifications, and accesses JavaFX properties only where visuals are involved.

As we have adopted JavaSE-1.7 BREE throughout all our modules, and as JavaFX properties are not UI-related, we should think of adopting them throughout.

There are two prerequisites to achieve this. First, we will have to provide implementations for properties based on SetMultimap and Multiset (which are used in MVC). Further we would need to add support for delaying notifications (which is also needed in MVC).

I have opened bug #484774 to keep track of adopting JavaFX properties. Resolving this as fixed in 3.11.0 M5.