Bugzilla – Full Text Bug Listing
|
Description
Thomas Schindl
Tom, I'm not sure I'm following. What would this provide? Ok. Currently an Object you want to bind has to implement: - addPropertyChangeListener - removePropertyChangeListener Now we have 2 model Objects A and B. Normally you create a base Object C which provides the PropertyChangeSupport but now let's say those two model object are already derived from a BaseObject you don't have access to and because Java does support multiple inheritance you have to implement PropertyChangeSupport twice (ugly :-). The solution to this is that you create C and let A and B delegate calls to add/remove to this class but you still have to implement the methods in every class. If you now take the IAdaptable-concept you instantiate your delegation class and return C when ever your model object has to adapt you return this object. It sounds like you're trying to fix issues with the bean spec and not data binding. Since we're not a library for creating bean API my initial feeling is that this isn't an appropriate change to be included in data binding. Boris? (In reply to comment #3) > It sounds like you're trying to fix issues with the bean spec and not data > binding. Since we're not a library for creating bean API my initial feeling is > that this isn't an appropriate change to be included in data binding. Boris? > Also this would make your bean only look like a bean in data binding and technically it wouldn't be a bean. (In reply to comment #3) > ... > that this isn't an appropriate change to be included in data binding. Boris? Yes, same here - I don't think we should compensate for shortcomings of the current Javabeans spec, unless we are supporting a coding pattern that is of widespread use. Btw, because of these shortcomings, I don't think that we want any of the Platform plug-ins to have a dependency on org.eclipse.core.databinding.beans. So what you both suggest implementing all those methods in objects and delegating to my Object C? I would suggest writing a helper class for this that does not live in org.eclipse.core.databinding.beans, something like: TomObservables.observeValue(...) I would do one of the following: 1. extend from a common bean superclass that provides the API 2. look into creating/finding a wizard that would generate a bean creating the necessary API 3. or not use beans at all It's an old spec with a lot of shortcomings. Closing as WONTFIX. Reopening based on a discussion in the newsgroup.
Tim H wrote:
> If someone could confirm that there is absolutely no way I can listen to
> changes on a bean without adding PropertyChangeSupport to all my pojo's
> then I'll go ahead and go down that road. But I'm still curious to what
> the actual point of PojoObservables are.
We could try to open PojoObservables so that users can plug in their own strategy for listening to changes.
An adaptation of the Property<S,V> class from the beans-binding project (on dev.java.net) might be a good solution to this problem. Something like this:
interface IProperty {
void addChangeListener(Object source, IChangeListener listener);
void removeChangeListener(Object source, IChangeListener);
}
interface IValueProperty extends IProperty {
void addValueChangeListener(Object source, IValueChangeListener listener);
void removeValueChangeListener(Object source, IValueChangeListener listener);
Object getValue(Object source);
void setValue(Object source, Object value);
}
abstract class ValueProperty implements IValueProperty {
// superclass implements add/remove listener methods, calls the
// following methods when first listener is added or last listener
// removed, respectively:
protected abstract void addListenerTo(Object source)
protected abstract void removeListenerFrom(Object source)
// concrete subclasses call this method when a value property changes on a source object
protected void fireValueChange(Object source, ValueDiff diff)
}
Then we could introduce a PropertyObservableValue class to handle properties generically:
class PropertyObservableValue implements IObservableValue {
PropertyObservableValue(Object source, IProperty property) { ... }
Object getValue() {
return property.getValue(source);
}
void setValue(Object value) {
property.setValue(source, value);
}
}
BeansObservables, PojoObservables, and possibly EMFObservables could be retrofitted to use this pattern. This has the potential to eliminate a lot of duplicate code in the project as well.
This same pattern would apply for observable lists, sets, and maps too.
Boris, I have some example code to demonstrate generic property observables--do you mind if I take the wheel on this bug? Created attachment 106529 [details]
Prototype
This is just a basic prototype to demonstrate how we might introduce generic properties. Note that the properties have the addChangeListener / addValueChangeListener dichotomy as the observables do. However since PropertyObservableValue only be registers a value change listener on the property, I don't think the generic change listeners are necessary.
(In reply to comment #12) > Boris, I have some example code to demonstrate generic property observables--do > you mind if I take the wheel on this bug? Sure, no problem! (In reply to comment #13) > ... Note that the properties have the addChangeListener / > addValueChangeListener dichotomy as the observables do. However since > PropertyObservableValue only be registers a value change listener on the > property, I don't think the generic change listeners are necessary. Agreed. Unless there was a way to have a ComputedValue depend on a property directly (not through a proper observable). This would have to be supported by ObservableTracker as a special case though. Taking ownership (In reply to comment #15) > Taking ownership > I'm not following the thread here. So what would the client code for this look like? //this bean doesn't implement any property change stuff Employee empBean = new Employee(); ObservableValue val = PropertyObservables.observeValue(empBean,"firstName"); ?? (In reply to comment #16) > I'm not following the thread here. So what would the client code for this look > like? > > //this bean doesn't implement any property change stuff > Employee empBean = new Employee(); > > ObservableValue val = PropertyObservables.observeValue(empBean,"firstName"); Here is the expected usage as of the current prototype patch: Employee empBean = new Employee(); // First define the property IValueProperty firstNameProperty = BeanProperties.valueProperty(Employee.class, "firstName"); // Then create an observable which observes that property on a specific source object. IObservableValue firstNameObservable = PropertyObservables.observeValue(empBean, property); Of course you can already do this in a more direct manner using BeansObservables. However by abstract the property access / mutation / listener registration concepts into a separate strategy, we are freeing folks to use their own property patterns, provided they supply their own property implementations. This is much easier than writing a compliant observable implementations. In the long term, I could see us retrofitting all of our SWT and Viewers observables to use properties instead of direct observable implementations, in a manner that is completely transparent to the client. I suspect we may see a significant decrease in code size and project complexity because of it. Created attachment 107000 [details]
getRealizedElements prototype with demo of problem explained earlier
To see the problem I described earlier, apply this patch and run Snippet013TableViewerEditing.java. Around line 228 I've modified the patch so there are two ways to set up the label provider: using getKnownElements() or using getRealizedElements(). With the JavaBeanObservableMap bug covered the labels now come up blank. This is because ObservableMapLabelProvider listens for changed entries, but not for added or removed entries, thus the labels are initially blank and stay that way even after the elements are added to the realized elements set. However changing any of the names in a cell editor will cause the changed value to show up in the viewer.
A similar, but more complicated fix would need to be applied for ObservableCollectionTreeContentProvider and friends as well.
Created attachment 107001 [details]
mylyn/context/zip
Oops, wrong bug! Disregard comments 18 and 19.. Created attachment 107041 [details]
Progress patch (untested)
I've removed the generic change listeners per earlier discussion. Still need to develop property sets, lists, and maps.
Also we should investigate implementing observable maps using an observable set as the key set, similar to how BeansObservables.observeMaps(IObservableSet domain, String propertyName) works. Presumably we could make this easy for anyone to use by abstracting the pattern out so that a client need only supply an IValueProperty for determining (and observing) the value associated with each key.
Created attachment 107042 [details]
mylyn/context/zip
As a thought to this discussion, a more library-intrusive approach is to provide adapters (or other mechanisms) at the property change listener level as an external interface, but an adapter (not necessarily an Eclipse platform adapter) mechanism in the binding context. When I was goofing around with a data binding library, I added an adapter mechanism that when the equivalent of context.addBinding(Object lhs_potential_observable_endpoint, Object rhs_potential_observable_endpoint,...) is called, each potential observable is either an observable directly or a context-level adapter mechanism (this could be the eclipse platform adapter mechanism) is checked to adapt the endpoint to an observable type. The JCP binding also has something similar and is much more type safety as indicated below but slightly different than the specific property API described. I know my comment loosens up type safety but opens a variety of ways to hook objects as observables. For example, it provides a way to remove the need for some of the static classes, if its okay to loosen type safety for some programmers. The adapter mechanism can be layered on the more safe binding context class and give a more user friendly choice to some who want it but also retains the current underlying API and type safety in the way it is implemented today for those who want that. (In reply to comment #23) > When I was goofing around with a data binding library, I added an adapter > mechanism that when the equivalent of context.addBinding(Object > lhs_potential_observable_endpoint, Object rhs_potential_observable_endpoint,...) > is called, each potential observable is either an observable directly or a > context-level adapter mechanism (this could be the eclipse platform adapter > mechanism) is checked to adapt the endpoint to an observable type. The JCP > binding also has something similar and is much more type safety as indicated > below but slightly different than the specific property API described. I'm having difficulty visualizing how this would work. Could you post some sample code, along with a synopsis of what adapter magic is being performed by the library? > I know my comment loosens up type safety but opens a variety of ways to hook > objects as observables. For example, it provides a way to remove the need for > some of the static classes, if its okay to loosen type safety for some > programmers. The adapter mechanism can be layered on the more safe binding > context class and give a more user friendly choice to some who want it but also > retains the current underlying API and type safety in the way it is implemented > today for those who want that. If I understand you correctly, you're talking about being able to pass in any object to the binding method and have the library automatically convert it to an observable. This is how the databinding project originally worked. We called them uberfactories and they quickly became unweildy. The problem is that it's not always obvious what aspect of the object you want to observe. The main problem, as Dave Orme observed, was that because anybody could register new observable factories, anybody could likewise override the default adapter, thereby completely changing the outcome of existing code. I was not a member of the project at the time, but this is my understanding of the reasons for abandoning uberfactories. In the wake of these problems, the project migrated to using factories in an explicit manner rather than asking the library to figure it out for us. Thus if I want to observe the enablement of a SWT widget, I call SWTObservables.observeEnabled(widget). The code is more verbose but there is no ambiguity about what the code is supposed to do. I believe you are correct and understand that this was the case to a certain degree--there may have been other reasons for tossing it as well.
I think however, that in line with the thinking behind providing property change listening support of the type initially outlined in this bug (and to some of your other comments as well on the SWT library), that this flexibility is an envelope around the generic PCL support.
Given the code you sketched out, one can make the argument that this mechanism is actually a generic endpoint management system that is semantically similar to the factories you mention. Your interface provides one level of indirection between an object and the ability to observe it and a factory provides a level of indirection between an object and the ability to make it into an eclipse databinding observable. I actually added both mechanisms as well as adapters for creating endpoints to a hack data binding library to try them out. As you would expect, you can bind across a wide range of objects with simpler client code then using static factories. As for the adapter logic, think of it as equivalent to having a configurable factory living in the context to create PropertyObservableValues automatically when the user specifies an endpoint object (either source or target) that is not an observable.
In your case it is managed by the user explicitly (or used to create the binding library with less code) and for factories it is managed by the context and employed behind the scenes. Personally, I believe that the factory approach can be made pragmatically manageable with the benefit that client code is simplified while still managing ambiguity.
Here's the interface I had. I think regardless of what you do, it can all be layered on the eclipse binding library fairly easily. I saw that PojoObservables posting and was thinking back to all of this stuff as well.
Just some more thoughts as you work through your details.
/**
* A binding endoint manager to manage endpoints and their properties as well as
* manage listeners for changes on endpoints. The binding library allows you to
* specify endpoints directly for binding and this manager class allows you to
* abstract the protocol for acting on those endpoints. You can configure the
* factory that a binding manager users to create endpoints and in concert you
* should provide a {@code IEndpointManager} to manage those endpoint types. A
* single endpoint manager is shared across multiple instances of
* IBindingManager implementations from the same IBindingManagerFactory so be
* careful about creating any state in concrete implementations.
* </p>
*
* <p>
* The interface is independent of whether the bean's properties are bound or
* unbound. Of course, if the bean's properties are unbound, then the value
* change listeners will not add much value.
* </p>
* <p>
* This interface aggregates different methods that are needed to create an
* abstract protocol to manipulate endpoints. This class makes the data binding
* library relatively independent of any specific endpoint implementation such
* as found in the JSR or SWT data binding implementations and allows us to
* create an infrastructure that can bridge the adapters and endpoints in those
* libraries. It also allows client's to specify a generic way that values or
* set/get from an object and hence abstracts the notion of a string-specified
* property name.
* </p>
*/
public interface IEndpointManager {
boolean canHandle(Class<?> endpointObjectClass);
Object getValue(IBinding binding, Object endpoint);
void setValue(IBinding binding, Object endpoint, Object value);
Object getObjectFromEndpoint(Object endpoint);
Object createChangeListener(IBinding binding, BindingDirection direction);
void addValueChangeListener(IBinding binding, Object endpoint,
Object listener);
void removeValueChangeListener(IBinding binding, Object endpoint,
Object listener);
Object getTypeHint(Object endpoint, boolean blindLook);
}
/**
* An interface that adapts one type of an object that the user specifies with
* an object that can act as an endpoint. The adaption should return an endpoint
* object that matches the protocol known to a {@code IEndpointManager} object.
* A single instance of an implementation is shared across all binding managers
* from a factory if set explicitly in the factory or across all binding
* managers if picked up in the {@code BindingHelper} class. This adapter
* mechanism help bridge between libraries with standard objects that are not
* easy to use with binding libraries and the data binding framework. The
* adapter mechanism is different than the {@code IBeanBindingEndpointFactory}
* which also produces endpoints but the {@code IBeanBindingEndpointFactory}
* produces a default bean binding endpoint object.
*
* <p>
* Endpoints that are bound to directly are not adapted first and hence only
* when the user specifies an object and a property name is the adapter
* mechanism used.
*
* <p>
* A single instance of the adapted endpoint may be shared across adapted
* properties, that is, an adapter may adapt more than one property and hence be
* reused inside a binding manager.
*
* <p>
* This is very similar to the {@code BeanAdapterFactory} and
* {@code BeanAdapterProvider} found in the beans binding JSR 175 and the
* interface is being made intentionally similar over time.
*
*/
public interface IBindingAdapterFactory {
String getName();
String getDescription();
Object adapt(IBindingManager manager, Object o, PropertyPath property);
boolean canAdapt(IBindingManager manager, Object o, PropertyPath property);
Class<?> getAdapterClass(IBindingManager manager, Class<?> type);
}
Created attachment 111164 [details]
Updated patch with value, set, list, and map properties implemented
BeansObservables and PojoObservables have been retrofitted (as proof of concept) to use BeanProperties or PojoProperties, respectively, in conjunction with PropertyObservables. For example, the following:
IObservableValue personName = BeansObservables.observeValue(person, "name");
can also be written as:
IObservableValue personName = PropertyObservables.observeValue(person,
BeanProperties.valueProperty(person.getClass(), "name"));
The property can be defined once and reused across multiple observables.
Created attachment 111165 [details]
mylyn/context/zip
Created attachment 111335 [details]
Added MapPropertyObservableMap
MapPropertyObservableMap may be a good candidate to replace CompositeMap in most cases. Basically you provide an IObservableMap and an IValueProperty, and you get an observable map with the same keys, but each value is the property value of the values in the original map.
i.e., given an observable map for Person->Address mappings, and a value property for the Address.City property, you can construct a map of Person->City mappings via the Person.Address property.
For my next trick, I plan to write a derived observable list along the same lines: provide an IObservableList and an IValueProperty and you get an IObservableList where each element corresponds to the property value of the element from the original list.
Created attachment 111336 [details]
mylyn/context/zip
Planned for 3.5M2 Created attachment 111448 [details]
Added experimental MasterDetailProperties
No implementation, just stub methods exploring the APIs and combinations of properties that might be possible.
Right now the collection typed properties each have a pair of methods for access to the backing collection:
IListProperty {
List getList(Object source)
void setList(Object source, List list)
}
(likewise methods for ISetProperty and IValueProperty)
One shortcoming of this approach is that with nested properties, each nested property has to make its own copy of the collection / map and make modifications on that copy, then put it back on the master property. This is not very efficient. I'm considering changing the IListProperty, ISetProperty and IMapProperty to mirror the List, Set and Map interfaces from the collections framework, except that each method will have the property source object prepended to the argument list:
e.g.
Set {
boolean add(Object o)
int size()
void clear()
}
ISetProperty {
boolean add(Object source, Object o)
int size(Object source)
void clear(Object source)
}
I'm looking for feedback on which API folks would find preferable.
Created attachment 111449 [details]
mylyn/context/zip
Created attachment 111640 [details] Progress patch Revised the IListProperty, ISetProperty and IMapProperty interfaces as described in comment #31. If needed, we could provide abstract base classes to make set, list and map properties easier to implement. I expect this will come later after the initial patch has been released. MasterDetailProperties are implemented but not tested. I think I've covered all the main property combinations: IValueProperty of IValueProperty -> IValueProperty (ValuePropertyDetailValue) IListProperty of IValueProperty -> IListProperty (ValuePropertyDetailList) ISetProperty of IValueProperty -> ISetProperty (ValuePropertyDetailSet) IMapProperty of IValueProperty -> IMapProperty (ValuePropertyDetailMap) IValueProperty of IListProperty -> IListProperty (ListPropertyDetailValueList) IValueProperty of ISetProperty -> IMapProperty (SetPropertyDetailValueMap) IValueProperty of IMapProperty -> IMapProperty (MapPropertyDetailValueMap) Let me know if I missed any. Fixed bean property classes to compensate for the (very odd) condition provided for in the bean spec, where a PropertyChangeEvent having oldValue == null && newValue == null indicates that an unknown change occured. The fix was achieved by caching property values for all property sources being listened to. I'm still planning to do ListValuePropertyDetailObservableList, a detail list where the elements are the property values of their corresponding elements in the master list. As of this patch there are compiler errors because I am still changing the tests so that they test the objects from BeansObservables.observe* instead of the old JavaBeanObservable* classes. Created attachment 111641 [details]
mylyn/context/zip
Created attachment 111717 [details]
Progress patch
All tests have been adapted for properties. Only a few tests are failing, due to BeanObservable*Decorator classes not decorating the observables properly. This will be easier to address once 237718 is fixed.
Created attachment 111719 [details]
mylyn/context/zip
Just to demonstrate what can be done with the latest patch..
Given the following class (with getters and setters implied for each field):
class Person {
Person mother;
Person father;
List<Person> children;
String name;
}
A number of relationships can be observed very easily:
IValueProperty fatherProperty = BeanProperties.valueProperty(Person.class, "father");
IValueProperty motherProperty = BeanProperies.valueProperty(Person.class, "mother");
IValueProperty nameProperty = BeanProperties.valueProperty(Person.class, "name");
IListProperty childrenProperty = BeanProperties.listProperty(Person.class, "children");
IObservableValue fatherObservable = PropertyObservables.observeValue(person, fatherProperty);
IObservableValue paternalGrandFatherObservable = MasterDetailObservables.detailValue(
fatherObservable, PropertyObservables.valueFactory(fatherProperty), Person.class);
Properties can be combined in master-detail fashion to observe more complex relationship without having to create observables at each step:
IValueProperty paternalGrandFatherProperty = MasterDetailProperties.detailValue(fatherProperty, fatherProperty);
IObservableValue paternalGrandFatherObservable = PropertyObservables.observeValue(person, paternalGrandFatherProperty);
IListProperty childrenNamesProperty = MasterDetailProperties.detailList(childrenProperty, nameProperty);
IObservableList childrenNamesObservable = PropertyObservables.observeList(person, childrenNamesProperty);
(In reply to comment #10) > Reopening based on a discussion in the newsgroup. > > Tim H wrote: > > If someone could confirm that there is absolutely no way I can listen to > > changes on a bean without adding PropertyChangeSupport to all my pojo's > > then I'll go ahead and go down that road. But I'm still curious to what > > the actual point of PojoObservables are. > > We could try to open PojoObservables so that users can plug in their own > strategy for listening to changes. I have two main objectives with this bug: 1) Identify all the spots in DataBinding where observables are observing a property of another object (beans, pojos, SWTObservables, ViewersObservables) as opposed to just holding a value (WritableValue, ConstantObservableValue) and extract the property-oriented code to specialized property classes. I expect to eliminate a lot of duplicate code here, especially in SWT and Viewers observables. 2) Make it really easy for folks to write their own properties. Coupled with bug 237718 (decorating observables) this should make the task of implementing a custom set of observables very straightforward. Created attachment 111845 [details]
Progress patch (Swapping out SWTObservables to properties)
Created attachment 111846 [details]
mylyn/context/zip
Created attachment 111897 [details]
Progress patch
Converted several more SWT observables to use properties instead. This is going to take time..
Extracted common code from BeanObservable(Value|Set|List|Map) to BasicObservable(Value|Set|List|Map), for properties where all changes go through a single setter-style method.
Created attachment 111898 [details]
mylyn/context/zip
Created attachment 111993 [details]
Previous patch without the changes that were moved to 246626
Created attachment 111994 [details]
mylyn/context/zip
Created attachment 112161 [details]
Finished implementing ListValuePropertyObservableList
Created attachment 112162 [details]
mylyn/context/zip
Created attachment 112268 [details]
Added javadoc to all new public API
Created attachment 112269 [details]
mylyn/context/zip
Created attachment 112360 [details]
Finished converting SWTObservables to properties
Created attachment 112361 [details]
mylyn/context/zip
Created attachment 112370 [details]
Refactoring SWT properties, ported some other patches to properties
The SWT properties were refactored to reduce duplication, compiled code size.
Ported Tom Schindl's patch adding support for Control.location, Control.size, Control.bounds and Control.focused properties
Ported Chris's (zx) patch adding support for StyledText.text property
Created attachment 112371 [details]
mylyn/context/zip
Created attachment 112471 [details]
Finished porting ViewersObservables to properties
There are now several new classes in the org.eclipse.jface.databinding.viewers package giving access to the new properties, similar to what was added in the SWT package. As such, the factory methods in ViewersObservables are now essentially shortcuts that delegate to PropertyObservables with the appropriate property. For example:
IObservableList selection = ViewersObservables.observeMultiSelection(viewer);
is roughly equivalent to :
IObservableList selection = PropertyObservables.observeList(viewer, SelectionProviderProperties.multipleSelection());
One thing I'd like to improve on is the value caching situation. Many properties have to do their property caching in order to build diffs for change events. This is being reimplemented everywhere and I'd like to find a way to factor out the common parts.
Now that everything is ported to properties I really have no excuse to put off writing unit tests any longer. :)
Created attachment 112472 [details]
mylyn/context/zip
I see two compile errors in the latest patch. Matthew, can you check that you are set up correctly with the CDC Foundation 1.0 and 1.1 profiles? The method valueOf(String) in the type Integer is not applicable for the arguments (int) - ListPropertyDetailValueList.java line 130 The method valueOf(String) in the type Integer is not applicable for the arguments (int) - ListValuePropertyObservableList.java line 104 Created attachment 112606 [details] Fixed bug mentioned in comment #55 Created attachment 112607 [details]
mylyn/context/zip
Created attachment 112629 [details]
Removed portions of patch that were moved to bugs 247367 and 247394
Created attachment 112630 [details]
mylyn/context/zip
Retargeting for M3 Created attachment 113149 [details]
Backup patch
I'm planning to split this into core, beans, swt and viewers patches. This bug will be for the core portion, and the other portions will be handled in separate bugs.
Created attachment 113150 [details]
mylyn/context/zip
Created attachment 113165 [details]
Patch with just core IProperty API
Created attachment 113166 [details]
mylyn/context/zip
Created attachment 114858 [details]
Experimenting with stateless property API
Modified the listener API for properties, to allow property objects to remain stateless.
/** Marker interface */
interface IPropertyChangeListener {}
interface IProperty {
public void addPropertyChangeListener(Object source, IPropertyChangeListener listener);
public void removePropertyChangeListener(Object source, IPropertyChangeListener listener);
}
interface IValueProperty extends IProperty {
public IPropertyChangeListener adaptListener(IValuePropertyChangeListener listener);
}
Usage:
IValueProperty property = BeanProperties.value(Bean.class, "value");
IValuePropertyChangeListener valueListener = new IValuePropertyChangeListener() {
public void handleValueChanged(ValuePropertyChangeEvent e) {
// handle change
}
}
IPropertyChangeListener listener = property.adaptListener(valueListener);
property.addPropertyChangeListener(bean, listener);
There is also a shortcut:
IPropertyChangeListener listener = property.addValueChangeListener(source, valueListener);
Benefits to the stateless implementation:
* Eliminating caching of property state significantly decreased code size
* Implementing custom properties (by clients) is much easier because you don't need to cache state. Previously this was a big headache.
Drawbacks:
* Property mutator methods e.g. IValueProperty.setValue() no longer fire a property change implicitly. There may be a property change fired by the property source, but there is no guarantee.
* Client code is more verbose. In practice, if users are just creating / combining properties and making observables out of them, then this won't matter much.
* Master-detail properties must be stateful so we can unregister listeners on the detail property source when the master property changes value.
* Could not remove caching in Bean properties, because the bean spec allows for newValue == null && oldValue == null (meaning an unknown change) in a PropertyChangeEvent.
* Proliferation of listeners and listener support objects - reused properties can no longer reuse listener support objects, because listener supports must be moved to the listener adapters. This is roughly equivalent to the number of listeners being used now though.
* Because properties do not predictably fire change events, use of master-detail APIs may be severely broken
Any help overcoming these drawbacks would be appreciated.
Created attachment 114859 [details]
mylyn/context/zip
Created attachment 115196 [details]
Update
Added first tests (still needs a lot more, but it's a start)
I've moved each group of property classes into their own package according to value/set/list/map categories, similar to the package structure for observables.
The Basic(Value|Set|List|Map)Property classes have been renamed to Simple(Value|Set|List|Map)Property and now have built in property caching. With these changes, introducing custom properties should be fairly trivial. For example, to implement a list property you would:
Subclass SimpleListProperty
Implement getList(Object), updateList(Object source, List list, ListDiff diff) and getElementType()
If the source class has a listener API for the property, extend addListenerTo(Object) and removeListenerFrom(Object) to register a listener. The listener must call firePropertyChange(Object, ListDiff) when a change is observed.
Created attachment 115197 [details]
mylyn/context/zip
Created attachment 115284 [details]
Update
Updated the copyright comments on all new files. I still need to go through any new files that were derived from existing files and copy the contribution comments over.
I'm experimenting in Simple(Value|Set|List|Map)Property with a behavior where a call to getValue(source) / getSet(source) / etc on a source object that the property is listening to, will compare the cached property against the live property and fire a change if they are different. This has a few side effects:
1. If a source object has no listener API, a change to the property will be fired as soon as it is recognized by the property, e.g.
Viewer viewer = ... // there is no listener API for viewer.input property
IValueProperty property = ViewerProperties.input();
class Counter implements IValuePropertyChangeListener {
private int count = 0;
public void handleValuePropertyChange(ValuePropertyChangeEvent event) { count++ }
}
Counter counter = new Counter();
property.addValueChangeListener(viewer, counter);
Object input = new Object();
viewer.setInput(input);
assertEquals(0, counter);
// Because the property is "listening" to the viewer, it compares
// the live value against the last known value and fires a value
// change because they are different.
assertEquals(input, property.getValue(viewer));
assertEquals(1, counter);
2. Because a listened-to property compares the last known and live objects on every retrieval, this may have serious performance implications, especially with collections.
*** Bug 248173 has been marked as a duplicate of this bug. *** *** Bug 248171 has been marked as a duplicate of this bug. *** Created attachment 115458 [details]
Update
Merged with HEAD, added getKeyType() and getValueType() to IMapProperty to mirror the new methods in IObservableMap
Created attachment 115459 [details]
mylyn/context/zip
Matt just because I saw it is this implementation compatible to the one proposed by JSR 295? I'm just looking at JSR 295 and JSR 303 in terms of UFacekit to gather ideas and other things. The IValueProperty interface is a subset of the operations in org.jdesktop.beansbinding.Property. So it would be trivial to adapt a beans-binding Property to an IValueProperty, but the reverse might be more difficult. beans-binding does not appear to support collection-typed properties. They do have observable list and observable map APIs but they are not property based. Created attachment 117500 [details]
Patch
Merged previous patch with HEAD
Created attachment 117501 [details]
mylyn/context/zip
Created attachment 117990 [details]
Patch (stateless)
Latest patch modified to stateless
Created attachment 117991 [details]
mylyn/context/zip
A new idea is to remove the add/remove listener from the interfaces and replace it with createObservable():
interface IValueProperty {
public Object getValue(Object source);
public void setValue(Object source, Object value);
public IObservableValue createObservable(Realm realm, Object source);
}
A few abstract base classes would be provided so that implementers can implement properties with add/remove listener semantics:
class SimpleValueProperty implements IValueProperty {
/** Adapt the listener to a "native" listener for the property source. */
public abstract IPropertyListener adaptListener(IValuePropertyChangeListener listener)
public abstract void addPropertyListener(Object source, IPropertyListener listener);
public abstract void removePropertyListener(Object source, IPropertyListener listener);
public IObservableValue createObservable(Realm realm, Object source) {
// SimplePropertyObservableValue will employ the above methods to
// register / unregister listeners as needed
return new SimplePropertyObservableValue(realm, source, this);
}
}
A few of the goals I had in mind that led to this design:
* Make property objects stateless
* Move all state previously in properties to the observables where it belongs
* Trivialize the creation of chained properties. With listener API you have to walk down the chain and ensure that listeners are added and removed as properties change all along the chain. With createObservable() we are leveraging existing master-detail observable infrastructure and listeners are managed at every link.
* Account for corner cases and property sources that do not fire change event, or do not fire them reliably. For instance, some SWT widgets only fire events when the change happens through user interaction, but do not fire events when those changes are initiated programmatically. These cases can be accommodated for in the observable much more easily, and without duplicating the same code in every property class.
I hope this makes sense. If anybody sees holes in this design please point them out. (preferably before I spend a lot of time updating the patch!)
Created attachment 119231 [details] Patch (stateless properties) The properties are now fully stateless, with no need for implementers to cache property values in listeners. Boris, as far as I can tell this patch comprises all the goals we discussed, namely: * Recenter the API around observables. Each property interface (IValueProperty, ISetProperty, IListProperty, IMapProperty) have methods to observe the value, set, list or map on a given realm, and which produce the expected observable type. * Stateless properties with no caching required at the implementer. This was accomplished by moving all state into observables where it belongs * Facilitate property chaining. i.e. Person -> Address -> Street is observed by streetProperty.observeDetailValue(addressProperty.observeDetailValue(personProperty.observeValue(realm, model))). With some work we can easily use these to make a fluent API for observing nested properties (bug 195222) * Make custom property implementations extremely simple. Most people will get what they need by extending Simple(Set|List|Map)Property or ValueProperty and implementing the few remaining methods. Created attachment 119232 [details]
mylyn/context/zip
This looks interesting. From an EMF point of view, I imagine that specifying an EStructuralFeature would generally be sufficient to specify which property is to be observed. I don't see anything on the surface that would be difficult to implement. I suppose it would be good if I applied these patches and tried producing the necessary EMF changes to support the new patterns. Do you have a *.psf file for the plugins I'd need in the workspace to apply the patches? Created attachment 119274 [details]
project set
Here you go Ed
Ed, if you've kept your observable implementations private or if you're free to replace them, try retrofitting EMFObservables to use properties behind the scenes. In the patch you'll see I've already done this with BeansObservables, PojoObservables, SWTObservables and ViewersObservables. I'm interested to see if retrofitting EMF for properties will shrink the compiled size of the jar. I suggest using these base classes when implementing EMF properties: ValueProperty, SimpleListProperty, SimpleSetProperty, SimpleMapProperty The Simple* classes have a doSet<List|Set|Map>() method that accept both the complete collection and a diff describing the change. The diff is provided so that collection properties without setter methods can apply the change incrementally using the diff. Moving to M5 I have looked at org.eclipse.core.databinding.property, org.eclipse.core.databinding.property.list, and org.eclipse.core.databinding.property.value (not at the implementation yet). So far, I like it but of course there are some questions and comments: @noimplement tags should explain to clients which abstract class(es) to subclass instead of implementing the interface directly. IListProperty.getList: Are there any restrictions on the returned list? Is it a copy, or the "live" list that changes state over time? For how long can it be held on to? Is it ok for clients to change the returned list? How do I add a listener to an IListProperty or an IValueProperty? Should I create observables instead of adding listeners directly? Why is there API for a listener (IValuePropertyChangeListener) but no addValuePropertyChangeListener? IProperty: This interface feels weird because it looks like it is of no value to clients because of the @noreference methods. "Nothing to see here, move along"? Property: needs @noextend tag? IValueProperty.observeDetailValues(IObservableMap master): Should say what the resulting map's keys and values are. I assume keys are the master's keys? (In reply to comment #87) > @noimplement tags should explain to clients which abstract class(es) to > subclass instead of implementing the interface directly. No problem, I'll put that in the next patch. > IListProperty.getList: Are there any restrictions on the returned list? Is it a > copy, or the "live" list that changes state over time? For how long can it be > held on to? Is it ok for clients to change the returned list? This is whatever list is returned by the property source itself. The returned list might be live and should not be modified. Whether it is live depends on whether the property uses defensive copy. It may be prudent to wrap the list in Collections.unmodifiableList() before returning it. > How do I add a listener to an IListProperty or an IValueProperty? Should I > create observables instead of adding listeners directly? Why is there API for a > listener (IValuePropertyChangeListener) but no addValuePropertyChangeListener? The preferred usage is to create an observable and listen to that. The accessor, mutator and listener methods in IProperty and friends are intended to be used only by the observables they create. Unless you are implementing a custom property, only the methods that create observables are intended for general consumption. The protocol is for registering listeners is : 1) adapt an IValuePropertyChangeListener into an IPropertyListener. IProperty has a method adaptListener(IPropertyChangeListener) which returns an IPropertyListener. The IPropertyListener instance for a particular property will implement the appropriate "native" listener interface for the expected type of source object, and parlays events to the IValuePropertyChangeListener. (I considered naming it INativePropertyListener instead of IPropertyListener originally, I don't remember now why I changed it) 2) Register the native listener on the property source using addListener(Object source, IPropertyListener listener). The expectation is that the property implementation will typecast the source and listener objects and call the "native" addXyzListener method. 3) Unregister the native listener later using removeListener(Object source, IPropertyListener listener). Since only the observe* methods are intended for general use, it may make sense to remove all other methods from the interfaces, and make them abstract methods in the abstract base classes instead. > IProperty: This interface feels weird because it looks like it is of no value > to clients because of the @noreference methods. "Nothing to see here, move > along"? That was supposed to be @noimplement. > Property: needs @noextend tag? Yes. Although if we move methods around as discussed above this may become moot, since there will be no methods left in IProperty. > IValueProperty.observeDetailValues(IObservableMap master): Should say what the > resulting map's keys and values are. I assume keys are the master's keys? Correct, I'll try to make that clearer in the documentation. ---- Based on your input and my own thoughts I think the next evolution should be: Remove all but the observe* methods from the interfaces. These methods will be pushed into abstract base classes and will be protected. This includes the methods in IProperty. Have the 4 interfaces represented as abstract base classes: ValueProperty implements IValueProperty SetProperty implements ISetProperty ListProperty implements IListProperty MapProperty implements IMapProperty Only those methods which can be defined in terms of another method (e.g. IValueProperty.observeDetailValue can delegate to observeValue) will be implemented in these classes. All other methods will be abstract. Next have 4 base classes which are intended as the starting point for user-created properties: SimpleValueProperty extends ValueProperty SimpleSetProperty extends SetProperty SimpleListProperty extends ListProperty SimpleMapProperty extends MapProperty All the methods removed from the interfaces will be put in these classes instead, as protected abstract methods. The reason for this dichotomy is that the methods in Simple*Property are not really applicable for chained properties. So chained properties will extend ValueProperty for example, whereas most other properties will extend SimpleValueProperty. (In reply to comment #88) > > IListProperty.getList: Are there any restrictions on the returned list? Is it a > > copy, or the "live" list that changes state over time? For how long can it be > > held on to? Is it ok for clients to change the returned list? > > This is whatever list is returned by the property source itself. The returned > list might be live and should not be modified. Whether it is live depends on > whether the property uses defensive copy. It may be prudent to wrap the list > in Collections.unmodifiableList() before returning it. You should put this information in the Javadoc. > > How do I add a listener to an IListProperty or an IValueProperty? Should I > > create observables instead of adding listeners directly? Why is there API for a > > listener (IValuePropertyChangeListener) but no addValuePropertyChangeListener? > > The preferred usage is to create an observable and listen to that. The > accessor, mutator and listener methods in IProperty and friends are intended to > be used only by the observables they create. Unless you are implementing a > custom property, only the methods that create observables are intended for > general consumption. So could we remove IValuePropertyChangeListener, ValuePropertyChangeEvent, etc. from the public API? > ---- > > Based on your input and my own thoughts I think the next evolution should be: > > ... Makes sense. Btw, it seems to me that we are converging now, not just evolving. (In reply to comment #89) > > The preferred usage is to create an observable and listen to that. The > > accessor, mutator and listener methods in IProperty and friends are intended to > > be used only by the observables they create. Unless you are implementing a > > custom property, only the methods that create observables are intended for > > general consumption. > > So could we remove IValuePropertyChangeListener, ValuePropertyChangeEvent, etc. > from the public API? These interfaces are used by Simple<Value|List|Set|Map>Property to receive change events from the source object. Subclasses of SimpleValueProperty will need access to IValuePropertyChangeListener and ValuePropertyChangeEvent in order to adapt the listener to a native listener. So they have to remain public. By the way, what do you think of renaming IPropertyListener to INativePropertyListener? Created attachment 120541 [details]
Patch
Includes all of the changes we discussed in the last 4 comments as best as I can see
Created attachment 120542 [details]
mylyn/context/zip
Created attachment 120651 [details]
Patch
Merge with bug fixes in HEAD, add toString() implementation to all properties for debugging purposes
Created attachment 120652 [details]
mylyn/context/zip
Created attachment 120653 [details]
Patch
Experimenting with "fluent" API to make property chaining and observable creation simple and concise.
Example:
IValueProperty name = BeanProperties.value(Person.class, "name");
IValueProperty mother = BeanProperties.value(Person.class, "mother");
IValueProperty father = BeanProperties.value(Person.class, "father");
IObservableValue maternalGrandfathersName = mother.chain(father).chain(name).observeValue(person);
I've also copied Snippet017 to create Snippet025, and replaced all use of observable factories with properties just to show how natural it is to use. Notice especially that by defining common properties once and reusing them the code gets smaller and easier to read.
Created attachment 120654 [details]
mylyn/context/zip
A comparison of using nested properties with CompositeMap vs. IValueProperty to set up a table viewer:
// CompositeMap
IObservableMap nameMap = BeansObservables.observeMap(knownElements, Person.class, "name");
IObservableMap motherMap = BeansObservables.observeMap( knownElements, Person.class, "mother");
IObservableMap fatherMap = BeansObservables.observeMap( knownElements, Person.class, "father");
IObservableMap grandmotherMap = new CompositeMap(motherMap, BeansObservables.setToMapFactory(Person.class, "mother"));
IObservableMap[] columnMaps = new IObservableMap[] {
nameMap,
new CompositeMap(motherMap, BeansObservables.setToMapFactory(Person.class, "name")),
new CompositeMap(fatherMap, BeansObservables.setToMapFactory(Person.class, "name")),
new CompositeMap(grandmotherMap, BeansObservables.setToMapFactory(Person.class, "name")) };
// Properties
IValueProperty name = BeanProperties.value(Person.class, "name");
IValueProperty mother = BeanProperties.value(Person.class, "mother");
IValueProperty father = BeanProperties.value(Person.class, "father");
IObservableMap[] columnMaps = new IObservableMap[] {
name.observeDetailValues(knownElements),
mother.chain(name).observeDetailValues(knownElements),
father.chain(name).observeDetailValues(knownElements),
mother.chain(mother).chain(name).observeDetailValues(knownElements) };
To anyone interested in giving properties a spin, checkout the branch "mhall_bug194734_properties" and start by running Snippet025 I still haven't had the time to look at supporting this in EMF. I think it will be a great fit though from what little I understand... Reminder to self: Simple<Value|List|Set|Map>Property.set<Value|List|Set|Map> should return a boolean indicating whether the property was changed. This should prevent firing bogus changes in the property observable for example whenever attempting to make sure on a null source object, for example. Created attachment 120914 [details]
Update
Removed all methods from SimpleListProperty, SimpleSetProperty and SimpleMapProperty that echo the corresponding collections interfaces. Turns out the effort was being duplicated in both SimpleListProperty and SimpleListPropertyObservableValue--so I moved everything to the latter.
Created attachment 120915 [details]
mylyn/context/zip
Created attachment 120989 [details]
Update
Move SimpleValueProperty.getValueType() implementation from SimpleValueProperty back to implementing classes
Move SimpleListProperty.getElementType() implementation from SimpleListProperty back to implementing classes
Move SimpleSetProperty.getElementType() implementation from SimpleSetProperty back to implementing classes
Move SimpleMapProperty.getElementType() implementation from SimpleMapProperty back to implementing classes
Removed all methods which mirror the Set, List and Map interfaces. SimpleListProperty now has only two methods:
protected List getList(Object source);
protected boolean setList(Object source, List list, ListDiff diff);
setList() receives both a List and a ListDiff so that the specific property can apply the change wholesale with the list if the source object allows it, or can apply the change incrementally by walking the diff.
(After repurposing the properties API as merely a convenience to creating observables, the observables were duplicating most of the work happening in the corresponding property, so it didn't make sense to keep that extra behavior in the properties).
Created attachment 120990 [details]
mylyn/context/zip
Applied latest patch to mhall_bug194734_properties branch Created attachment 121020 [details]
Update
Refactored Delegating<Value|List|Set|Map>Property out of Generic*Property.
Renamed Generic*Property to Anonymous*Property
Created attachment 121021 [details]
mylyn/context/zip
Forgot to mention, I also added Snippet026AnonymousBeanProperties.java in the last patch. This snippet demonstrates a tree viewer with elements of different types and no common ancestor class (ContactGroup, Contact), and different subsets of properties (i.e. both have a "name" property but only Contact has a "status" property). Whoops, the previous 3 comments were intended for bug 247997 After further work on bug 247997 (anonymous bean properties) it seems I need to refine the Simple*Property classes a little further: For example, in SimpleListProperty, the method: protected boolean setList(Object source, List list, ListDiff diff) should be: protected ListDiff setList(Object source, List list, ListDiff diff) This will ensure that if the full diff cannot be applied to property on the source object, we will at least know what changes _did_ take place. Created attachment 121141 [details] Abandoned experiment Saving current state of experiment outlined in comment #110. Maybe I'll find a way to make this work in the future. The concern that brought this up is that some properties may restrict which items may be added, or modify them in some way during addition. Without some way of giving a result from the setter methods, an observable could blindly fire change event with the wrong diff. However I don't want every single property implementation to have to handle this case. Back to the drawing board... Created attachment 121142 [details]
mylyn/context/zip
Results from discussion with Boris on IRC: * Having a separate factory class for every type of SWT and viewer properties makes the binary footprint larger than it needs to be. Now that we have a delegating property implementation (bug 247997) we can implement our properties in switchboard style like SWTObservables and ViewersObservables e.g. TextProperties.text(int) StyledTextProperties.text(int) could be combined into WidgetProperties.text(int). Using DelegatingValueProperty we can determine the correct delegate property to use at runtime. * Not exactly a propos, but there was a discussion about the names of our static factory classes. For example there is Bean_s_Observables (plural) vs Pojo__Observables (singular). There is also Viewer_s_Observables (plural, class name) vs SWT__Observables (plural, project name). My personal feeling is that the classes should use singular names and that they should refer to the type of class they operate on (widgets) rather than the project name (SWT). In an effort to clean up the library we may want to consider renaming some of these static factory classes to make everything consistent. Backward compatibility would be maintained by moving the old classes to a compatibility plugin. Created attachment 122228 [details]
Merged all widget properties into WidgetProperties
Some tests are failing--this is because I've moved the responsibility of wrapping observables in ISWTObservable* implementors into the properties themselves.
Still need to combine all the viewer properties into ViewerProperties
Created attachment 122229 [details]
mylyn/context/zip
(In reply to comment #114) > Created an attachment (id=122228) > Merged all widget properties into WidgetProperties I should say, merged all widget property _factory methods_ into WidgetProperties Created attachment 122234 [details]
Update
Merged all viewer property factory methods into ViewerProperties class. Fixed failing tests.
Created attachment 122235 [details]
mylyn/context/zip
Created attachment 122288 [details]
Update
Some minor cleanup, making the javadoc more consistent
Created attachment 122289 [details]
mylyn/context/zip
Committed latest attached patch to mhall_bug194734_properties branch Similar to the work committed earlier on SWTObservables and ViewersObservables, I've committed changes to BeansObservables and PojoObservables so that all four factory classes delegate entirely to properties. Thus there should be no observable (ha ha) difference in behavior when creating observables through WidgetProperties vs SWTObservables. Created attachment 122609 [details]
Update
Per review with Boris:
* Added IDiff marker interface to ValueDiff, ListDiff, SetDiff and MapDiff
* Added public final IDiff diff field to PropertyChangeEvent. If non-null, consuming observables will use this diff instead of computing it manually. This will preserve performance for clients like EMF where list properties are already creating list diffs.
* Removed Property.getPreferredRealm(). Properties that want to provide a default or preferred realm can override the observe(Object) method as is now done in the widget and viewer properties.
* Removed the Property class. With getPreferredRealm() gone this class serves no useful purpose at this time.
* The observable factory methods lacking a realm argument, e.g. IValueProperty.valueFactory(), now return a factory that delegates to the corresponding observe(Object) method, e.g. IValueProperty.observe(Object).
Created attachment 122610 [details]
mylyn/context/zip
Committed previous attachment to mhall_bug194734_properties branch > 20090114 Created attachment 122613 [details]
Possible further refinement
Update the cached state incrementally if a diff is provided in a property change event
Created attachment 122614 [details]
mylyn/context/zip
Created attachment 122775 [details]
package.html for org.eclipse.core.databinding.property package
Boris, can you review the wording and point out anything that's not clear?
Created attachment 122776 [details]
mylyn/context/zip
The wording is good; the only thing I would add is pointers to the main API classes for observable implementers (SimpleValueProperty etc). Committed package.html with some comments about Simple*Property, as well as a little info on Delegating*Property. released addition changes to mhall_bug194734_properties branch: Ensure that inner observables created by nested properties get disposed when the outer observable is disposed (since these are not visible to clients). Created attachment 122813 [details]
Make methods public in Simple*Property and Delegating*Property
Some arguments for making these methods public:
* With protected methods, all clients of these classes must live in the same package rather than in an internal package where they belong
* No ability for clients to provide their own observables, although why anybody would want to do this is beyond me :)
* Methods must be public if we ever decide to extract an interface from these classes.
* Once the properties API is published we will not be able to make these methods public.
Created attachment 122814 [details]
mylyn/context/zip
Thanks for doing this work. You're making it harder to complete the databinding chapter ;) (In reply to comment #135) > You're making it harder to complete the databinding chapter ;) ..but _easier_ to use databinding! ;) At least, that's the plan. Created attachment 122852 [details]
Possible change
Proposed changes in this patch (submitted to be reviewed by Boris):
* Renamed IPropertyChangeListener to ISimplePropertyListener, and PropertyChangeEvent to SimplePropertyEvent, since these are only used with the Simple*Property classes.
* Added ISimpleProperty, ISimpleValueProperty, ISimpleListProperty, ISimpleSetProperty and ISimpleMapProperty interfaces and added those methods which were needed to have all internal classes use these interfaces instead of direct references to SimpleValueProperty for example.
* Made Delegating*Property.getDelegate() method public to facilitate
* The previous two items made it possible to move all property observable implementations to an internal package.
Another possibility is to pull up the get* and set* methods from ISimple<Value|List|Set|Map>Property up to ISimpleProperty (which would require making the method names and arguments more generic and thus lose some type safety) in an effort to keep the class count down.
Created attachment 122853 [details]
mylyn/context/zip
Created attachment 122864 [details]
Another variation
In this patch the get/set methods of the ISimple<Value|List|Set|Map>Property interfaces have been renamed / refactored so they could be pulled up and unified in ISimpleProperty
Boris, do you have an opinion on whether we should adopt either this patch or the previous one?
Created attachment 122865 [details]
mylyn/context/zip
Committed only these changes from the patch described in comment 137: * Renamed IPropertyChangeListener to ISimplePropertyListener, and PropertyChangeEvent to SimplePropertyEvent, since these are only used with the Simple*Property classes. * Moved all property observable implementations to an internal package. * Made methods in SimpleValueProperty, SimpleListProperty, SimpleSetProperty and SimpleMapProperty public to make them visible to internal observable implementations. * Made the Delegating*Property.getDelegate() methods public to make them visible to internal observable implementations. Created attachment 123137 [details]
Final patch
Created attachment 123138 [details]
mylyn/context/zip
LEEROY JENKINS!!! Released final patch to HEAD > 20090120 Verified in I20090129-0100 by running Snippet026AnonymousBeanProperties and by running test suite |