Community
Participate
Working Groups
This is a split-off of bug 300043 and is intended to provide general detail value observables for observable (list|set|map)s which do not rely on the Simple*Property API.
Created attachment 161682 [details] patch with tests That's essentially the last patch from bug 300043 without the reduce API part. Matthew, +1?
Boris, +1?
+1
After having talked to Boris, it seems like it is already too late to commit API for M6 unless it's a critical bugfix. Therefore, it seems like this patch will have to wait. Sorry for not having finished this up a bit earlier...
Created attachment 173246 [details] updated patch for Eclipse 3.7 Updated the patch to release it for Eclipse 3.7 (adapted the versions of the observable/property plugins and changed the @since tags).
Matthew, Boris, +1 for this patch for Eclipse 3.7?
I have a question... when you say "master value" in the following "For every master value in the given map, the provided factory is used to create the corresponding detail observable value", what exactly are you referring to? A map has key, value pairs, so does the resulting map have the same keys as the argument map, and then for each key it contains the result of the factory applied to the master map's value for that key?
(In reply to comment #7) > I have a question... when you say "master value" in the following "For every > master value in the given map, the provided factory is used to create the > corresponding detail observable value", what exactly are you referring to? A map > has key, value pairs, so does the resulting map have the same keys as the > argument map, and then for each key it contains the result of the factory > applied to the master map's value for that key? Yes, that's what I *tried* to express :). However, the JavaDoc is really a bit unclear, mainly because I was not explicitly stating that the key sets are the same in both maps. I've tried to rephrase the JavaDoc of all the three API methods in the MasterDetailObservables class. They are now also similar to their natural property-counterparts, i.e. to the methods IListProperty#values(IValueProperty), ISetProperty#values(IValueProperty), and IMapProperty#values(IValueProperty). I will just post another patch. Would you mind having another look at the JavaDoc? I think it's better now but I'm still not totally happy (maybe, I should just write it down in german or spanish :) ).
Created attachment 173532 [details] attempt at improving the JavaDoc
ping
MasterDetailObservables: a minor spelling correction, I think you meant to say "therefore" rather than "thereby." I don't know if you know any French, but "therefore" is equivalent to "donc," and "thereby" is roughly the same as "par ce moyen." ListDetailValueObservableList: * adaptMasterListDiff: I'd probably rename this to something like handleMasterListChange, to more clearly identify what we're doing here. Sure the method is adapting the diff, but it's also creating and destroying observables and firing a list change on the detail observable list. * we should not register listeners any more than we need to until firstListenerCalled() is invoked, and we should remove all possible listeners when lastListenerRemoved() is invoked. In this case we must listen on the master list in order to keep the detail observables up to date (unless we want to create and destroy observables every time we access the list. yuck). But we can neglect listening on the detail observables until somebody actually registers a listener. * adaptMasterListDiff creates a distinct observable for every item in the list regardless of duplicates. However handleDetailValueChange checks to see if the same observable appears multiple times. If you want to reuse observables, I suggest using an IdentityMap to map from the master list elements to the detail observables over those elements. Take a look at ListSimpleValueObservableList to see how we cache the to see how we're already doing this there. This is all I have time for tonight, I'll try to pick up and review some more in the coming evenings.
MapDetailValueObservableMap: pretty much the same feedback as ListDetailValueObservableList.. * Suggest renaming adaptMasterMapDiff to handleMasterMapChange * Delay registering map change listeners and stale listeners until firstListenerAdded() is invoked, and remove those listeners in lastListenerRemoved() (as well as in disposal) * Suggest using an IdentityMap to map from values in the master map to the detail observables for those values. This minimizes the number of duplicate observables over the same value. Pretty much the same comments for SetDetailValueObservableMap. One more thing I noticed. The javadoc for MasterDetailObservables looks good, but I'm wondering if it would be simpler to rename all three new methods to just "detailValues" to match with the naming convention in properties. Overall this looks good.
Matthew, thanks a lot for your thorough review! I agree with the things you have pointed out so I will change the implementation accordingly. (In reply to comment #11) > If you want to reuse observables, I suggest using an IdentityMap to map from the > master list elements to the detail observables over those elements. Take a look > at ListSimpleValueObservableList to see how we cache the to see how we're > already doing this there. Regarding this point, I've seen that the existing ListSimpleValueObservableList does this caching by maintaining the #knownMasterElements set. However, there's a considerable overhead for maintaining this set when the master list changes (see ListSimpleValueObservableList#masterListener#updateKnownElements()). In order to avoid this, I'm doing the analogous caching in ListDetailValueObservableList by mapping the masters to their detail observables while also maintaining an additional information of how many masters are referencing a single detail observable in case of duplicate elements inside the master list. The reference count is needed in scenarios like the following: masterList.add(master); // create a new detail observable masterList.add(master); // reuse the detail observable from above masterList.remove(master); // must NOT dispose the detail observable which is still reference by 1 master entry masterList.remove(master); // dispose the detail observable which is not referenced anymore Using the master reference count, it is possible to decide when a detail observable is not needed anymore and can be disposed. It's really just to avoid the overhead otherwise needed to maintain a mapping from master elements to detail observables which I think is otherwise unavoidable. I'll attach a new patch which implements this for ListDetailValueObservableList (see ListDetailValueObservableList#masterDetailMap). What do you think of this approach?
Created attachment 179205 [details] patch implementing the caching for ListDetailValueObservableList This patch implements the caching for duplicate master elements on ListDetailValueObservableList using the master reference count as described in the previous comment.
Created attachment 185014 [details] updated patch Matthew, this updated patch includes most of the changes you suggested, including: * the methods on MasterDetailObservables are now all called detailValues() for more consistency with the property API. * cache the detail observables for ListDetailValueObservableList (see also comment 13) * only attach change/stale listeners on the detail values when firstListenerAdded() is called for ListDetailValueObservableList What I have *not* done so far is: * caching of detail observables for MapDetailValueObservableMap * delay registering listeners until firstListenerAdded() is called for MapDetailValueObservableMap The reason why I haven't implemented the last two points for now is that in the MapDetailValueObservableMap implementation, every detail observable has its own listener instance attached (this is not the case in ListDetailValueObservableList). This has the big advantage that when a detail observable changes, the listener can fire the appropriate event on the detail map very quickly since the listener knows for which (unique) key it needs to fire the event. Introducing caching for the detail observables here would make it harder to still propagate change events on detail observables efficiently since then a detail observable might map to multiple keys of the map (the existing MapSimpleValueObservableMap class has a similar problem which was solved by looking up the keys every time a detail value changes => see the MapSimpleValueObservableMap#keysFor() method which, however, requires linear time). Furthermore, the fact that in MapDetailValueObservableMap every detail observable has its own listener instance attached still makes it possible to delay registering the change/stale listeners until firstListenerAdded() is called but it is again more difficult to detach those listeners in lastListenerRemoved() since you would require a mapping from the detail observables to their listener for detaching the listener (right now, the listeners are not referenced anywhere). I personally would probably do no caching of detail observables for MapDetailValueObservableMap since it would lead to some problems and additional complexity as described above. What I'm a bit unsure about is whether the firstListenerAdded()/lastListenerRemoved() methods should be implemented as usual. As described above, implementing firstListenerAdded() is no problem but for lastListenerRemoved() you would have to keep an explicit reference to all the listeners and map them to the detail observables. This is no particular problem but still introduces some more complexity and a larger memory footprint. What do you think about those two points, Matthew?
Matthew, what do you think if we release the current patch to HEAD and I open a separate bug for the firstListenerAdded() stuff on MasterDetailValueObservableMap mentioned in my previous comment? That way, we could also start looking at the bugs blocked by this one (especially bug 300043 would be an interesting one to get in for 3.7).
Created attachment 185795 [details] updated patch to apply to HEAD again It's no change of the actual patch but there was a conflict with the old one on HEAD after the fix of other bugs.
(In reply to comment #16) > Matthew, what do you think if we release the current patch to HEAD and I open a > separate bug for the firstListenerAdded() stuff on > MasterDetailValueObservableMap mentioned in my previous comment? That way, we > could also start looking at the bugs blocked by this one (especially bug 300043 > would be an interesting one to get in for 3.7). That sounds fine. +1 Sorry for the delayed response..
Matthew, thanks a lot for having yet another look at the patch! And no problem at all for the delay!
Boris, +1? From an API point of view, the name of the new methods on MasterDetailObservables have changed since you had last looked at the patch and the JavaDoc was improved.
I'll have a look at this today or tomorrow.
Looks good, +1
Boris, thanks a lot for looking at the patch!
I've created a separate bug for eventually adding the firstListenerAdded/lastListenerRemoved methods to MapDetailValueObservableMap: bug 337662.
Released to HEAD > 20110220
Had forgotten to mark is as FIXED.
