Community
Participate
Working Groups
Build Identifier: M20090917-0800 The way to observe properties (e.g. to refresh cell labels in a TableViewer) of a regular property (a distinct element) is trivial, but doing the same for a collection of complex types is awkward, and requires the application developer to write a significant amount of code using "obscure" classes. This is one way to observe properties of a distinct instance: IObservableMap father = BeansObservables.observeMap(elements, "father.name"); I would like to observe children more or less the same way: IObservableMap children = BeansObservables.observeMap(elements, "children.name"); 'children' would fire events whenever a child was added or removed, or whenever the name of a child was modified. (will attempt to attach a screenshot to illustrate) For bonus points :-) In both cases, it would also be nice if I could specify a collection of properties to observe, and have the method return an aggregated map of some sort. This would allow me to compile all necessary observables in one single map. Although the typical use of the returned map might be to pass it as an argument to a label provider (which typcally accepts arrays of maps, e.g. 'org.eclipse.jface.databinding.viewers.ObservableMapLabelProvider.ObservableMapLabelProvider(IObservableMap[])'), returning a single, composite map would make it easier to use in contexts where an array is not accepted. One example could be to return a map from a factory passed to 'org.eclipse.core.databinding.observable.map.CompositeMap.CompositeMap(IObservableMap, IObservableFactory)'. Hence, I would also like to have the following methods: IObservableMap father = BeansObservables.observeMap(elements, "father.firstName", "father.lastName"); IObservableMap children = BeansObservables.observeMap(elements, "children.firstName", "children.lastName"); An extended version of the snippet org.eclipse.jface.examples.databinding.snippets.Snippet025TableViewerWithPropertyDerivedColumns could be used to illustrate the problem and the solution. Right now, it only holds columns with distinct entities. Adding a column with "children" would illustrate the issue. Reproducible: Always Steps to Reproduce: 1. Open snippet org.eclipse.jface.examples.databinding.snippets.Snippet025TableViewerWithPropertyDerivedColumns 2. Add the property "Person.children" and let it return a Set or a List 3. Add a column to represent the new property, and have it updated whenever the collection of children changes, or any of the children has its name changed. 4. For the "bonus point issue": modify Person to have "name" derived from "firstName" and "lastName", and have all columns updated when either changes.
Created attachment 156495 [details] Screenshot of modified snippet025 Screenshot of modified snippet, now showing "children column": org.eclipse.jface.examples.databinding.snippets.Snippet025TableViewerWithPropertyDerivedColumns Modified snippet solves part of the problem (using a ComputedObservableMap), but does not provide the simple interface requested.
Per our e-mail conversations (which I'll have to dig up and paste here soon) the intended approach is to implement a "fold" or "reduce" type API on IListProperty, ISetProperty and possibly IMapProperty. The proposed example code from that conversation was: IObservableSet knownElements = ... // e.g. from ObservableListContentProvider IValueProperty childrenNamesProp = BeanProperties.list("children").values("name").reduce(ListReducers.join(", ")); IObservableMap labelMap = childrenNamesProp.observeDetail(knownElements);
As already stated, I think the proposed solution is satisfactory from my "consumer" perspective - this would make it much easier to deal with the situation described above.
Created attachment 157682 [details] draft patch as a playground As already mentioned in our e-mail conversation, I think that having generic default implementations for the following methods of the IValueProperty interface would allow for a relatively simple implementation of the reduce API: * IObservableList observeDetail(IObservableList master) * IObservableMap observeDetail(IObservableSet master) * IObservableMap observeDetail(IObservableMap master) I think that it would be nice anyway to have default implementations of those methods. In this patch, I'm providing a draft implementation of the first two methods. The actual implementation is in ListDetailValueObservableList and SetDetailValueObservableMap, respectively, where the first class is from bug #175737. It's really just a draft implementation for now but it should work so far. I have also added a method "IValueProperty IListProperty#reduce()" which does an identity-reduction by returning the list property's list as a simple value. This method is used in Boris' modified snippet in order to illustrate its application. The patch is really just to have something to play around with for now.
Matthew, do you think that the approach sketched in the attached patch is reasonable or do you see a simpler solution? The point I'm a bit unsure about is that the three mentioned methods to be implemented on IValueProperty already have a default implementation but only for the SimpleValueProperty class (see ListSimpleValueObservableList & Co.). What I'm proposing is a more general implementation of those methods which is not restricted to the Simple*Property API. If we do something like this, we could even move it to a different bug since it's not directly related to the reduce API stuff.
Created attachment 158232 [details] updated patch with better ListDetailValueObservableList implementation The implementation of ListDetailValueObservableList is now a little bit more serious :).
(In reply to comment #6) > Created an attachment (id=158232) [details] > updated patch with better ListDetailValueObservableList implementation > > The implementation of ListDetailValueObservableList is now a little bit more > serious :). I need to think about this one. I'm hesitant to introduce a lot of functionality that effectively duplicates ListSimpleValueObservableList. That class was hard enough to write and we still haven't worked all the bugs out. Another approach that might work is to introduce a value property based on an IObservableFactory: class Properties { public static IValueProperty valueFactory(IObservableFactory) public static IValuePRoperty valueFactory(IObservableFactory) } This could be put to use as follows: IObservableList masterList = ... IObservableFactory valueFactory = ... IValueProperty valueProp = Properties .valueFactory(valueFactory) .value(Properties.observableValue(valueType)); IObservableList detailList = valueProp.observeDetail(masterList); We would still have to write an observable list that appropriately handled creation and disposal of IObservableValues, but it may be possible to subclass e.g. ListSimpleValueObservableList and change a few key things to accomplish this elegantly. What do you think? Would this satisfy your use case for using IObservableFactory?
(In reply to comment #7) > This could be put to use as follows: > > IObservableList masterList = ... > IObservableFactory valueFactory = ... > IValueProperty valueProp = Properties > .valueFactory(valueFactory) > .value(Properties.observableValue(valueType)); > IObservableList detailList = valueProp.observeDetail(masterList); > > We would still have to write an observable list that appropriately handled > creation and disposal of IObservableValues, but it may be possible to subclass > e.g. ListSimpleValueObservableList and change a few key things to accomplish > this elegantly. > > What do you think? Would this satisfy your use case for using > IObservableFactory? I'm not sure whether the observable list we need to implement when taking this approach would be really different from the one provided in the patch which also works in terms of an IObservableFactory? However, I'm also not happy with having duplicated code so we could indeed try to use subclassing in order to reduce the amount of duplicated code. Another possibility would be to drop the existing classes in favor of the new ones which are more general and, thus, can substitute the existing ones. The only drawback here is that the classes based on the Simple*Property API might behave a bit better in terms of required memory (I'm not sure how big the difference actually is).
Ovidio, let's move the IObservableFactory part of the discussion to another bug, and keep this one limited to just the reducer API.
Actually Ovidio I take that back. As I'm looking into this, it's apparent we're going to need the IObservableFactory stuff to make this bug a reality. Take the example of a label map where for a Node element, we want the list of sibling names: IValueProperty parent = BeanProperties.value("parent"); IListProperty children = BeanProperties.list("children"); IValueProperty name = BeanProperty.value("name"); IListReducer join = ListReducers.join(", "); IValueProperty siblingNames = parent.list(children).values(name).reduce(join); IObservableSet knownElements = ... IObservableMap labelMap = siblingNames.observeDetail(knownElements); But when you look at the expansion it's a little less cut and dry: IObservableSet<knownElements> + IValueProperty<parent> = IObservableMap<element parents> IObservableMap<elementParents> + IListProperty<children> = ??? So what we have to do is follow the property chain until it reduced to a value property again: IListProperty<children> + IValueProperty<name> + IListReducer<join> = IValueProperty<children names joined> At this point we can add: IMapProperty<element parents> + IValueProperty<children names joined> = IMapProperty<element sibling names joined> In order to make that leap, we need a ListDetailValueObservableList implementation that uses IObservableFactory<IObservableValue> to get each child element. It would also have to properly manage the lifecycle of every IObservableValue it created.
Ovidio: ListDetailValueObservableList master list listener could use ListDiffVisitor to capture move() events in the master list, and save time by just moving the observable we already have to the new index, rather than disposing it and creating it again.
Created attachment 160266 [details] Add IListReducer, IListProperty.reduce implementations to Ovidio's patch
(In reply to comment #12) > Created an attachment (id=160266) > Add IListReducer, IListProperty.reduce implementations to Ovidio's patch Looks really nice! BTW, did you have anything special in mind for exposing the IListReducer#observeDetail() method? It might also be handy to have some kind of identity-reduction on ListReducers where a list is reduced to itself (as a value).
(In reply to comment #13) > Looks really nice! BTW, did you have anything special in mind for exposing the > IListReducer#observeDetail() method? Yes. Without this method, the only way to use a reducer is by chaining it to an IListProperty. But I wanted to make it possible to reducer any IObservableList (e.g. a ComputedList). > It might also be handy to have some kind of identity-reduction on ListReducers > where a list is reduced to itself (as a value). How about ListReducers.self(), with an implied value type of List.class?
(In reply to comment #14) > (In reply to comment #13) > > Looks really nice! BTW, did you have anything special in mind for exposing the > > IListReducer#observeDetail() method? > > Yes. Without this method, the only way to use a reducer is by chaining it to an > IListProperty. But I wanted to make it possible to reducer any IObservableList > (e.g. a ComputedList). That's a good idea but I was wondering whether it might be better to keep this method out of the IListReducer interface by making it e.g. a static method in the Observables class, something like Observables#observeReducedList(IObservableList master, IListReducer reducer)? Just because I think that there seems to be no real reason for customizing the functionality of this method by overriding the ListReducer#observeDetail() and this way we would also keep the reducer interface a bit more lightweight. We might also want to move the reducer classes from the property-plugin to the observable-plugin in case they are not directly tied to the properties API but should also be applicable to plain observables. > > It might also be handy to have some kind of identity-reduction on ListReducers > > where a list is reduced to itself (as a value). > > How about ListReducers.self(), with an implied value type of List.class? Sounds good!
(In reply to comment #10) > Actually Ovidio I take that back. As I'm looking into this, it's apparent we're > going to need the IObservableFactory stuff to make this bug a reality. I do also not see any other workaround. In that case, I would suggest that we try to also implement the missing MapDetailValueObservableMap?
(In reply to comment #16) > I do also not see any other workaround. In that case, I would suggest that we > try to also implement the missing MapDetailValueObservableMap? You wanna take a stab at that? :)
(In reply to comment #17) > (In reply to comment #16) > > I do also not see any other workaround. In that case, I would suggest that we > > try to also implement the missing MapDetailValueObservableMap? > > You wanna take a stab at that? :) May I? Please, please, please... :)
Created attachment 161317 [details] added MapDetailValueObservableMap and some tests Here's a first version (to be tested) of the MapDetailValueObservableMap. I have also added some tests for the ListDetailValueObservableList and will also do so for it's set and map counterparts. Matthew, maybe it would make sense to separate this part from the actual reduce API. Shall I create a new bug for the detail observables?
While cleaning up the code and writing some further tests I've noticed that there are still some things which are not working correctly on the (Set|Map)DetailValueObservableMap so I need to go through this code again. Matthew, I'm not sure whether the intention is to get any of this API for 3.6 since the API freeze is very close? I'm a bit busy this week but I will try to get as much running as possible...
Created attachment 161641 [details] added some more tests This now includes many more tests and some minor fixes. After all, it was not as bad as I thought. I'll try to finish this up this evening or so.
I've just filed a new bug for the detail observables in order to separate that part from the reduce API: bug 305367
Created attachment 189359 [details] removed the changes released as part of bug 305367 from the patch removed the changes released as part of bug 305367 from the patch
Matthew, I don't think I will have time in the following days to look at this so unless you have time to give it a try, I guess it's too late to get this done for 3.7.
any news here? is the current code in some repository?
