Bugzilla – Full Text Bug Listing
| Summary: | Implement the new Property-based Databinding-API | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Thomas Schindl <tom.schindl> |
| Component: | Edit | Assignee: | Thomas Schindl <tom.schindl> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | apupier, contact, Ed.Merks, hceylan, Kenn.Hussey, mallo.ovidio, marcelop, nboldt, qualidafial, ronbermejo, tankut, x.maysonnave, yves.yang |
| Version: | 2.5.0 | Flags: | Ed.Merks:
pmc_approved+
|
| Target Milestone: | Galileo | ||
| Hardware: | PC | ||
| OS: | Mac OS X - Carbon (unsup.) | ||
| Whiteboard: | |||
| Bug Depends on: | 194734 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Thomas Schindl
Created attachment 123633 [details]
A first patch
This a 1:1 port from JavaBeans - completely untested only ported to EMF
Created attachment 124701 [details]
Patch including Editing-Properties
added support for the Properties API to editing-project. Currently the EditingObservables don't use the none-editing variants as base classes because there are some calls to super I wasn't sure of now - this is subject to change once we are sure everything works as expected.
Created attachment 125052 [details]
Added support to observe Resource#getContents()
Same as before but with a possibility to observer the resource content
Created attachment 125836 [details]
Databinding patch
* Updated to latest changes
* fixed EMFWritableList to use API instead of internals
Comment on attachment 125836 [details]
Databinding patch
incomplete
Created attachment 125841 [details]
Patch with fixed EMFWritableList
for those interested there's a git repo at http://github.com/tomsontom/emf-databinding-example/tree/master with an example application Created attachment 132310 [details]
Updated to M6 and revised some parts
Created attachment 132579 [details]
Fixed access to an unknown property
* fix for an unknown property
* started implementing support to observe a list-element for use in master-detail
(highly experimental and not finished yet)
CC'ing Matt because he might be interested what I'm doing with his new API here. @Matt the last patch holds the initial bits for the implementation of the List-Value support we chatted about yesterday (I'm not 100% sure about it though and it might be completely wrong). The interesting classes are: - org.eclipse.emf.databinding.IEMFListValueProperty - org.eclipse.emf.databinding.internal.EMFListValueProperty - org.eclipse.emf.databinding.internal.EMFListValuePropertyDecorator (In reply to comment #10) > @Matt the last patch holds the initial bits for the implementation of the > List-Value support we chatted about yesterday (I'm not 100% sure about it > though and it might be completely wrong). IEMFListValueProperty is doing two things: selecting an element according to some criteria, and observing a property of that element. If it were me I'd keep these behaviors in two separate classes for the sake of modularity. The selection / filtering story is being discussed in bug 167436 if you want to take a look. However I just had some thoughts on how we could augment the API to make this really nice with properties: interface IPredicate { public boolean evaluate(Object o); } interface IListProperty { public IValueProperty nth(int index); public IListProperty filter(IPredicate); } With these API you could trivially accomplish something like: IListProperty parents = BeanProperties.value("parents"); IValueProperty age = BeanProperties.value("age"); IPredicate isFemale = new IPredicate() { public boolean evaluate(Object o) { return ((Person)o).getGender() == Gender.FEMALE; } } IValueProperty motherAge = parents.filter(isFemale).nth(0).value(age); It gets more complicated when you start to think about how to automatically add / remove elements from the filtered list as element properties change which are relevant to the filter. Maybe there could be methods on IPredicate which report the properties relevant to the filter, and the filtering observable list would automatically monitor those properties and issue updates as needed. I'm open to suggestions. Perhaps an additional method on IListProperty for convenience: // Equivalent to filter(predicate).nth(0) public IValueProperty select(IPredicate) This sounds good. I think we should wait in EMF until the upstream API is fixed though I really have the need for such a feature before 3.6 :-( I think we should split out this support once more and I'll possibily keep such an implementation local so that we won't break peoples API later on. Created attachment 132670 [details]
Started porting the Bean-Test-Suite
As of now fairly unusable because many parts commented and many tests failing (though I'm quite sure our implementation behaves correct but those are special tests for JavaBean and don't necessarily apply to EList)
*** Bug 274745 has been marked as a duplicate of this bug. *** Created attachment 134185 [details]
Bean Properties port of EMFProperties
Uses similar approach. Code is ported from Bean Properties.
Supports nested properties.
(In reply to comment #16) > Created an attachment (id=134185) [details] > Bean Properties port of EMFProperties > > Uses similar approach. Code is ported from Bean Properties. > > Supports nested properties. > Not sure what that should mean. My Port also supports nested properties (take a look at the FeaturePath-Class which abstracts this a bit) and is besides that a 1:1 port of BeanProperties for pure EMF *and* EMF-Edit. Some remarks: * Why I use FeaturePath in my port is that it is IMHO more natural than a list of features (and even more natural than public static IEObjectValueProperty[] values(EStructuralFeature[]... features)) and it gives us an oportunity to evolve the nested feature support in future (e.g. add support for some String look up, ...) with out adding methods to our EMFProperties factory. * The rest of your patch is more or less the same than mine (but mine is double the size because I also add support for databinding.edit) => There's one small difference that you are returning an ArrayList/HashMap from List/Map-Property whereas I return even null. I'm not sure why the BeanPort does this (Matt?) but it could be because they need introspection because the API doesn't spec this as not null but I'm not even sure an ArrayList the right type in EMF because I'd expect some EList in Ecore for all multi-valued features (Ed?) => You are returning the real type from getElementType() where as we should return the Feature itself because it gives EMF more meta-informations about the object type (See the current observables) * Your patch has better JavaDoc but I'll update them until Ed gives me feedback on the port and what code/API changes he expects from me * Your patch propably has a more accurate implementation of MapProperty (Ed?) What I'd like to see is going my rather big patch getting in and then we could take a look in which areas we can improve with some single smaller patches. (In reply to comment #17) Hello Tom, first of all no offence. I searched bugzilla but although I am a good googler, I couldn't locate this bug. Then I got down and ported the code. Then my bug was marked as duplicate. I asked directions to whether drop working on that or should I go ahead. I removed my patch from my bug, and I was asked to attach to this bug. So basic rationale was not at all, I didn't like your code, nor though it was poor, nor incomplete etc. So my intention is not to step on your toe. [1] Having explained that, - I return the real path, because to reduce the risk I tried to mimic BeanProperties as much to the extend as it is possible. - I return empty list/map beacuse it was so in BeanProperties. As I read it I think thats a safeguard for a null pointer in the nested properties path. ex: take parent.child.grandchild property. If child property returns null, I guess you en up with a NullPointerException. My way, you simply get empty list and other parts like for ex: LabelProviders will display "" instead of doomed iwth a NPE. My two cents, If I offended anyone unintentionally, I am totally OK with my patch being deleted as long as the other one is OK (No offence :) ). [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=274745 Regards, Hasan Ceylan (In reply to comment #18) > (In reply to comment #17) > > > Hello Tom, > > first of all no offence. I searched bugzilla but although I am a good googler, > I couldn't locate this bug. Then I got down and ported the code. I didn't wanted to offend you either i was solely analyzing your code and replying my code also provides nested property support though in a different way. It quite unfortunate that you didn't find the code before investing the time and work into it :-( > > Then my bug was marked as duplicate. I asked directions to whether drop working > on that or should I go ahead. I removed my patch from my bug, and I was asked > to attach to this bug. I see now Ed's comment on the duplicate bug. Did you take a look at my work now that you know it and probably merge in changes you think would make sense? Still I think it would be easier to get in the initial contribution and then file extra bug(s). It simply impossible to some one to compare a 185K and 75K patch against each other. What worries me even more is that we are both not EMF-Committers and hence a IPZilla needs to filed and for 3.5 the deadline this is long over. > > So basic rationale was not at all, I didn't like your code, nor though it was > poor, nor incomplete etc. So my intention is not to step on your toe. [1] > > Having explained that, > > - I return the real path, because to reduce the risk I tried to mimic > BeanProperties as much to the extend as it is possible. You take about the creation of nested-properties right. Let's compare how my API looks like vs yours: Direct BeanProperties (your port): IEObjectValueProperty value(EStructuralFeature feature) IEObjectValueProperty value(EStructuralFeature... features) IEObjectValueProperty[] values(EStructuralFeature... features) public static IEObjectValueProperty[] values(EStructuralFeature[]... features) None-Direct BeanProperties (my port): IEMFValueProperty value(FeaturePath featurePath) IEMFValueProperty value(EStructuralFeature feature) IEMFValueProperty[] values(EStructuralFeature... features) IEMFValueProperty[] values(FeaturePath... featurePaths) There've been things in the direct port that I didn't liked (See my first patch on the bug): * value(EStructuralFeature... features) vs values(EStructuralFeature... features) * values(EStructuralFeature[]... features) doesn't feel good to me (well it's a strange API isn't it?) * value(EStructuralFeature... features) doesn't feel good to me but more looks like I'd like to get multiple elements Maybe we can come up with a declarative Syntax in future to express the feature path in a more declarative manner (maybe the also goes to gether nice filtering feature Matt pointed us to in bug #167436 (an XPath-Syntax would be a cool thing something like 'persons[@gender='male']/addresses[@type='primary']/city/@name')) but this would not mean that we have to add API to our EMFProperties/EMFEditProperties-API but simply add a static builder-method to the FeaturePath in my API (but still the main reason for me is that it feels more fluent). > > - I return empty list/map beacuse it was so in BeanProperties. As I read it I > think thats a safeguard for a null pointer in the nested properties path. ex: > take parent.child.grandchild property. If child property returns null, I guess > you en up with a NullPointerException. My way, you simply get empty list and > other parts like for ex: LabelProviders will display "" instead of doomed iwth > a NPE. > Hm. I'm not sure this can happen in EMF because all multi-valued features by definition return an instance and IIRC it is *illegal* in terms of EMF to silently changes the returned list from invokation to invokation because this leads to subtle problems (e.g. Teneo had this problem in the Ganymede-Release and it took me and Martin 2 full nights to find out what's going wrong). So calling eGet(EStructuralFeature) guarantees that you always get back the same EList else by definition your EObject instance is messed up (Is that correct Ed?). Now we would violate this in Databinding implementation and because feature paths are constructed in EMF from EStructuralFeature I guess it is less of a problem but we need to wait for Matt to comment on it. So to repeat I also didn't wanted to say that your code is bad, ... but highlight some problems, interesting ideas I found in your code, ... . > => There's one small difference that you are returning an ArrayList/HashMap > from List/Map-Property whereas I return even null. I'm not sure why the > BeanPort does this (Matt?) but it could be because they need introspection > because the API doesn't spec this as not null but I'm not even sure an > ArrayList the right type in EMF because I'd expect some EList in Ecore for > all multi-valued features (Ed?) SimpleListProperty.doGetList must return non-null since its result is directly passed to Collections.unmodifiableList. > * values(EStructuralFeature[]... features) doesn't feel good to me (well it's > a strange API isn't it?) BeanProperties.values() is not used to create nested properties, it is a convenience method to create several value properties in one shot. The strings you pass in are all separate property names. This method was introduced specifically to make setting up multi-column viewers simple: ViewerSupport.bind( tableViewer, input, BeanProperties.values( Person.class, new String[] { "firstName", "lastName", "age", "gender", "weight" } ) ); Hi Tom, Good that we are on the same track in offensiveness :) (not a natural English speaker :)) (In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > > > > Hello Tom, > > > > first of all no offence. I searched bugzilla but although I am a good googler, > > I couldn't locate this bug. Then I got down and ported the code. > > I didn't wanted to offend you either i was solely analyzing your code and > replying my code also provides nested property support though in a different > way. It quite unfortunate that you didn't find the code before investing the > time and work into it :-( > We're cool. Yeah, it is indeed unfortunate, I felt somewhat bad after spending 4 hours to port the code on Saturday night :( :) > > > > Then my bug was marked as duplicate. I asked directions to whether drop working > > on that or should I go ahead. I removed my patch from my bug, and I was asked > > to attach to this bug. > > I see now Ed's comment on the duplicate bug. Did you take a look at my work now > that you know it and probably merge in changes you think would make sense? > > Still I think it would be easier to get in the initial contribution and then > file extra bug(s). It simply impossible to some one to compare a 185K and 75K > patch against each other. What worries me even more is that we are both not > EMF-Committers and hence a IPZilla needs to filed and for 3.5 the deadline this > is long over. > > > > > So basic rationale was not at all, I didn't like your code, nor though it was > > poor, nor incomplete etc. So my intention is not to step on your toe. [1] > > > > Having explained that, > > > > - I return the real path, because to reduce the risk I tried to mimic > > BeanProperties as much to the extend as it is possible. > > You take about the creation of nested-properties right. Let's compare how my > API looks like vs yours: > > Direct BeanProperties (your port): > IEObjectValueProperty value(EStructuralFeature feature) > IEObjectValueProperty value(EStructuralFeature... features) > IEObjectValueProperty[] values(EStructuralFeature... features) > public static IEObjectValueProperty[] values(EStructuralFeature[]... > features) > > None-Direct BeanProperties (my port): > IEMFValueProperty value(FeaturePath featurePath) > IEMFValueProperty value(EStructuralFeature feature) > IEMFValueProperty[] values(EStructuralFeature... features) > IEMFValueProperty[] values(FeaturePath... featurePaths) > > There've been things in the direct port that I didn't liked (See my first patch > on the bug): > * value(EStructuralFeature... features) vs values(EStructuralFeature... > features) > * values(EStructuralFeature[]... features) doesn't feel good to me (well it's > a strange API isn't it?) > * value(EStructuralFeature... features) doesn't feel good to me but more > looks like I'd like to get multiple elements > > Maybe we can come up with a declarative Syntax in future to express the feature > path in a more declarative manner (maybe the also goes to gether nice filtering > feature Matt pointed us to in bug #167436 (an XPath-Syntax would be a cool > thing something like > 'persons[@gender='male']/addresses[@type='primary']/city/@name')) but this > would not mean that we have to add API to our > EMFProperties/EMFEditProperties-API but simply add a static builder-method to > the FeaturePath in my API (but still the main reason for me is that it feels > more fluent). > > > > > > - I return empty list/map beacuse it was so in BeanProperties. As I read it I > > think thats a safeguard for a null pointer in the nested properties path. ex: > > take parent.child.grandchild property. If child property returns null, I guess > > you en up with a NullPointerException. My way, you simply get empty list and > > other parts like for ex: LabelProviders will display "" instead of doomed iwth > > a NPE. > > > > Hm. I'm not sure this can happen in EMF because all multi-valued features by > definition return an instance and IIRC it is *illegal* in terms of EMF to Yes your are right. So my assertion no longer stands. Actually it should be a problem with single-valued EReferences. I am not sure I handle this properly nor can be handled properly easily, nor EMFObservables takes care of this already. > silently changes the returned list from invokation to invokation because this > leads to subtle problems (e.g. Teneo had this problem in the Ganymede-Release > and it took me and Martin 2 full nights to find out what's going wrong). So > calling eGet(EStructuralFeature) guarantees that you always get back the same > EList else by definition your EObject instance is messed up (Is that correct > Ed?). > > Now we would violate this in Databinding implementation and because feature > paths are constructed in EMF from EStructuralFeature I guess it is less of a > problem but we need to wait for Matt to comment on it. > > So to repeat I also didn't wanted to say that your code is bad, ... but > highlight some problems, interesting ideas I found in your code, ... . > It is so bad that I have an extremely tight schedule on my current project. So I cannot see the possibility to observe yours and merge the code in about 2 weeks. cheers, Hasan (In reply to comment #20) > > => There's one small difference that you are returning an ArrayList/HashMap > > from List/Map-Property whereas I return even null. I'm not sure why the > > BeanPort does this (Matt?) but it could be because they need introspection > > because the API doesn't spec this as not null but I'm not even sure an > > ArrayList the right type in EMF because I'd expect some EList in Ecore for > > all multi-valued features (Ed?) > > SimpleListProperty.doGetList must return non-null since its result is directly > passed to Collections.unmodifiableList. > Ok then we should JavaDoc it as "must not return null". I'm still not sure it is a good idea to mask the returning of null from an eGet() through the creation of a temporary array because I still believe this is wrong in EMF because one does all the modification on the list itself and EMF-Guarantees that get back the same list for every invokation of eGet() so returning null is sense less. > > * values(EStructuralFeature[]... features) doesn't feel good to me (well it's > > a strange API isn't it?) > > BeanProperties.values() is not used to create nested properties, it is a > convenience method to create several value properties in one shot. The strings > you pass in are all separate property names. This method was introduced > specifically to make setting up multi-column viewers simple: > > ViewerSupport.bind( > tableViewer, > input, > BeanProperties.values( > Person.class, > new String[] { > "firstName", "lastName", "age", "gender", "weight" > } > ) > ); > Well it is not used for that but it does and describes it does: /** * Returns a value property array for the given property names of an * arbitrary bean class. * * @param propertyNames * array of property names. May be nested e.g. "parent.name" * @return a value property array for the given property names of the given * bean class. */ public static IBeanValueProperty[] values(String[] propertyNames) { return values(null, propertyNames); } Which finally ends up in #value(Class,String,Class) and so by default provides nested property access :-) > > Yes your are right. So my assertion no longer stands. Actually it should be a > problem with single-valued EReferences. I am not sure I handle this properly > nor can be handled properly easily, nor EMFObservables takes care of this > already. > Could you explain this in a bit more detail - I'm not 100% sure I got that right. It's perfectly acceptable for a single-valued EStructuralFeature to return null and one can't make up an instance easily but maybe I missunderstood your intention and an example could make this clear. [...] > > It is so bad that I have an extremely tight schedule on my current project. So > I cannot see the possibility to observe yours and merge the code in about 2 > weeks. > Well I think once the first code got in I could take a look at your code and spot where we differ and I can discuss with Ed how to proceed as said I don't think we can do the merge before that. (In reply to comment #22) > Ok then we should JavaDoc it as "must not return null". I'm still not sure it > is a good idea to mask the returning of null from an eGet() through the > creation of a temporary array because I still believe this is wrong in EMF > because one does all the modification on the list itself and EMF-Guarantees > that get back the same list for every invokation of eGet() so returning null is > sense less. Agreed, we should either document the restriction or add some null guard code. Keep in mind that SimpleListProperty#getList() is intended to be used only by the property observables, not by clients. The method is tagged @noreference for that express reason. So the fact that a null return from eGet is masked by SimpleListProperty is not supposed to become a client-facing concern. > > > * values(EStructuralFeature[]... features) doesn't feel good to me (well it's > > > a strange API isn't it?) > > > > BeanProperties.values() is not used to create nested properties, it is a > > convenience method to create several value properties in one shot. The strings > > you pass in are all separate property names. This method was introduced > > specifically to make setting up multi-column viewers simple: > > ... > > Well it is not used for that but it does and describes it does: > ... > Which finally ends up in #value(Class,String,Class) and so by default provides > nested property access :-) Yes, this is true for each individual named property in the string array, and we could improve the javadoc to make this distinction clearer. Here is an example of this scenario in Snippet017TableViewerWithDerivedColumns.java in the DataBinding examples project: ViewerSupport.bind( peopleViewer, viewModel.getPeople(), BeanProperties.values( new String[] { "name", "mother.name", "father.name", "mother.mother.name" } ) ); The above code is roughly equivalent to: ViewerSupport.bind( peopleViewer, viewModel.getPeople(), new IValueProperty[] { BeanProperties.value("name"), BeanProperties.value("mother.name"), BeanProperties.value("father.name"), BeanProperties.value("mother.mother.name"), } ); (In reply to comment #18) > - I return empty list/map beacuse it was so in BeanProperties. As I read it I > think thats a safeguard for a null pointer in the nested properties path. ex: > take parent.child.grandchild property. If child property returns null, I guess > you en up with a NullPointerException. My way, you simply get empty list and > other parts like for ex: LabelProviders will display "" instead of doomed iwth > a NPE. Nested properties should handle nulls gracefully anywhere in the property chain. IListProperty siblingNamesProp = BeanProperties.value("parent").list("children").values("name"); IObservableList siblingNames = siblingNamesProp.observe(node); Null is a valid value for the "parent" property. So null becomes the source object for the "children" property. Take a look at SimpleListProperty.getList(). Since the source object for the "children" property is null, an empty list is automatically used. Now take a look at SimpleValueProperty.getValue(). Since the source object for the "name" property is null, a null value is automatically assumed. So you should not get an NPE just because some property in the chain has a null value. I forgot to say that in my previous example, assume that node.getParent() == null (In reply to comment #23) [...] > Could you explain this in a bit more detail - I'm not 100% sure I got that > right. It's perfectly acceptable for a single-valued EStructuralFeature to > return null and one can't make up an instance easily but maybe I missunderstood > your intention and an example could make this clear. > Person --> Address --> Country --> Name A table with --------------------------------------- |Name Lastname Country | ---------------------------------------- |Ed Merks | |Hasan Ceylan Turkey | ---------------------------------------- But if Ed's address is null, or adsress does not have country set in it, I think we'll get a null pointer as we try to return name of the country for the record in Label provider. > [...] > > > > > It is so bad that I have an extremely tight schedule on my current project. So > > I cannot see the possibility to observe yours and merge the code in about 2 > > weeks. > > > > Well I think once the first code got in I could take a look at your code and > spot where we differ and I can discuss with Ed how to proceed as said I don't > think we can do the merge before that. > agreed... (In reply to comment #25) (In reply to comment #20) I my fear seems to be irrelevant.... Hasan Created attachment 134272 [details]
cleaned up patch
* remove Set-Support because its not making sense in EMF
* integrated Map-Handling from Hasan + attribution clause
* removed experimental stuff with ListValue-Observable
* started fixing JavaDoc
Created attachment 134465 [details]
Updated patch
* more clean up
* completely JavaDoc'ed
Notes from my review of the patch: * EMFEditListProperty.ListVisitorImpl should override handleMove and handleReplace to send more compact commands to the editing domain than the constituent remove and add commands that make them up. * I recommend refactoring EMFObservables to delegate everything to EMFProperties similar to what we did in core.databinding.beans with BeansObservables and BeanProperties. * I also recommend deprecating the public observable classes in this release and removing them in the next one. In core.databinding.beans and jface.databinding we've removed nearly all the observable implementations in favor of the property observable implementations. * I've never used EMF (I know, I know..) but I know we weakened the contract of things like IObservableValue.getValueType() from Class to Object to accomodate EMF using EClass as a type identifier. However EWritableList constructor takes a Class<Type> for the element type--given my limited of EMF I was surprised by the use of Class type here. * EWritableList should add listeners in the protected firstListenerAdded method instead of in the constructor, and should likewise remove listeners in the lastListenerRemoved method. This guards against memory leaks due to unused observables hanging around forever if you forget to dispose them. * I will have to copy your wording for the javadoc of EMFProperties.values(FeaturePath[]) * My name is still plastered everywhere in the javadoc, along with beans-specific javadoc comments and bugzilla bug numbers. (In reply to comment #31) > Notes from my review of the patch: > * EMFEditListProperty.ListVisitorImpl should override handleMove and > handleReplace to send more compact commands to the editing domain than the > constituent remove and add commands that make them up. Ye this would have caused me nights to find out what's wrong > * I recommend refactoring EMFObservables to delegate everything to > EMFProperties similar to what we did in core.databinding.beans with > BeansObservables and BeanProperties. I stayed away from this because are very very late in the 3.5 game and we don't have a test suite to ensure everything is ok > * I also recommend deprecating the public observable classes in this release > and removing them in the next one. In core.databinding.beans and > jface.databinding we've removed nearly all the observable implementations in > favor of the property observable implementations. Yes I had that in some of my patches but removed. But I agree 100%. > * I've never used EMF (I know, I know..) but I know we weakened the contract of > things like IObservableValue.getValueType() from Class to Object to accomodate > EMF using EClass as a type identifier. However EWritableList constructor takes > a Class<Type> for the element type--given my limited of EMF I was surprised by > the use of Class type here. This is a very very special beast and in short there's no feature to observe the root list of an EMF-Object-Tree. > * EWritableList should add listeners in the protected firstListenerAdded method > instead of in the constructor, and should likewise remove listeners in the > lastListenerRemoved method. This guards against memory leaks due to unused > observables hanging around forever if you forget to dispose them. Ok added and come with the next patch > * I will have to copy your wording for the javadoc of > EMFProperties.values(FeaturePath[]) > * My name is still plastered everywhere in the javadoc, along with > beans-specific javadoc comments and bugzilla bug numbers. > Your name is left in by intention because you are the guy to attribute the initial implementation and I'm the one to attribute the port to EMF. The bean specific comments need to get out of course Thanks for your amazing feedback and this wonderful new API - patch to follow in a view minutes Created attachment 134529 [details]
patch addressing fairly all of Matt's comments beside JavaDoc
(In reply to comment #32) > > * I recommend refactoring EMFObservables to delegate everything to > > EMFProperties similar to what we did in core.databinding.beans with > > BeansObservables and BeanProperties. > > I stayed away from this because are very very late in the 3.5 game and we don't > have a test suite to ensure everything is ok Have you looked at the conformance test suite? Just the basic conformance tests is likely to surface a number of bugs. > > * I also recommend deprecating the public observable classes in this release > > and removing them in the next one. In core.databinding.beans and > > jface.databinding we've removed nearly all the observable implementations in > > favor of the property observable implementations. > > Yes I had that in some of my patches but removed. But I agree 100%. So.. does EMF not believe in deprecating provisional API? :) > > * My name is still plastered everywhere in the javadoc, along with > > beans-specific javadoc comments and bugzilla bug numbers. > > > > Your name is left in by intention because you are the guy to attribute the > initial implementation and I'm the one to attribute the port to EMF. The bean > specific comments need to get out of course Okay, so IP/copyright stuff. Given that basically every line of code had to change for the EMF port, I figured that wouldn't be an issue but I guess it still qualifies as a derivative work. I'm fine either way > Thanks for your amazing feedback and this wonderful new API - patch to follow > in a view minutes I'm glad you like it! We scrutinized the heck out of it before going gold (just take a look at the patch history on bug 194734), it really feels like we nailed it. I think the best part is that we haven't had any "wtf was I thinking?!" moments so far. :) So that's always nice. Anything in the new patch you want me to look at? (In reply to comment #34) > (In reply to comment #32) > > > * I recommend refactoring EMFObservables to delegate everything to > > > EMFProperties similar to what we did in core.databinding.beans with > > > BeansObservables and BeanProperties. > > > > I stayed away from this because are very very late in the 3.5 game and we don't > > have a test suite to ensure everything is ok > > Have you looked at the conformance test suite? Just the basic conformance > tests is likely to surface a number of bugs. > Yes I started porting it but didn't finished yet. Do you see any advantages delegating inside EMFObservables? > > > * I also recommend deprecating the public observable classes in this release > > > and removing them in the next one. In core.databinding.beans and > > > jface.databinding we've removed nearly all the observable implementations in > > > favor of the property observable implementations. > > > > Yes I had that in some of my patches but removed. But I agree 100%. > > So.. does EMF not believe in deprecating provisional API? :) It's not that EMF doesn't believe in deprecation I think Ed is even much more open than the rest of Eclipse to change things but the patch is big enough already and I don't want to open another front because it's of high importance for me to get this in for 3.5 else I would have to fork of which is something I reallly reallly want to avoid because then we would have 2 places for EMF-Databinding (one in EMF and one in UFaceKit - it by the way also holds another imlementation of IProperty-API for a very very basic EObject implementation named UBean) > > > > * My name is still plastered everywhere in the javadoc, along with > > > beans-specific javadoc comments and bugzilla bug numbers. > > > > > > > Your name is left in by intention because you are the guy to attribute the > > initial implementation and I'm the one to attribute the port to EMF. The bean > > specific comments need to get out of course > > Okay, so IP/copyright stuff. Given that basically every line of code had to > change for the EMF port, I figured that wouldn't be an issue but I guess it > still qualifies as a derivative work. I'm fine either way > > > Thanks for your amazing feedback and this wonderful new API - patch to follow > > in a view minutes > > I'm glad you like it! We scrutinized the heck out of it before going gold > (just take a look at the patch history on bug 194734), it really feels like we > nailed it. I think the best part is that we haven't had any "wtf was I > thinking?!" moments so far. :) So that's always nice. > > Anything in the new patch you want me to look at? > No I simply implemented your comments. The only thing we need to do is convince you to give EMF a try - believe me you get amazed by the features you get :-) It's really sad that you haven't been at EclipseCon - the EMF/CDO/RCP-Slides I already showed the new Properties-API in action. (In reply to comment #35) > Yes I started porting it but didn't finished yet. Do you see any advantages > delegating inside EMFObservables? The main benefit as I see it is that you don't have two implementations doing the same thing. > It's not that EMF doesn't believe in deprecation I think Ed is even much more > open than the rest of Eclipse to change things but the patch is big enough > already and I don't want to open another front because it's of high importance > for me to get this in for 3.5 else I would have to fork of which is something I > reallly reallly want to avoid because then we would have 2 places for > EMF-Databinding (one in EMF and one in UFaceKit - it by the way also holds > another imlementation of IProperty-API for a very very basic EObject > implementation named UBean) Is it too late even to deprecate the old observable classes? > The only thing we need to do is convince you to give EMF a try - believe me you > get amazed by the features you get :-) It's really sad that you haven't been at > EclipseCon - the EMF/CDO/RCP-Slides I already showed the new Properties-API in > action. Sadly the first and only time I've been able to go to EclipseCon was in '07. It's hard to justify the huge cost to my partners. I'd love to see the slides you mentioned though. Other than that, where would you recommend I look for a good introduction to EMF? Note that all the existing EMF data binding stuff is documented to be provisional so we can most certainly deprecate anything or everything. Perhaps even delete it, though given we're past M7, that would seem not so nice to clients. Documentation like the getting started things on http://www.eclipse.org/modeling/emf/?project=emf will help. There's also the EMF book http://www.informit.com/store/product.aspx?isbn=9780321331885 Ok as a start I'd mark all existing Observables as deprecated (and remove in 3.6). A thing I'm not sure about is whether we should deprecate and remove EMFObservables because e.g. it's not making sense to me currently to use Properties for our Resource-Observable because there's no real resource-root property. I'd probably leave EMFObservables in place. We've left all our *Observables factory classes in place but then again we didn't have a choice--those APIs are non-provisional and several releases old so we couldn't go breaking those. (In reply to comment #38) > it's not making sense to me currently to use > Properties for our Resource-Observable because there's no real resource-root > property. If there's not a true source->property relationship then I would publish it as an observable and not try to shoehorn it into a property. Created attachment 134710 [details]
Final patch for review
* Fixed formatting issues by using EMF-Standards
* Deprecated methods and classes
* Fixed JavaDoc
Notes from review: * You withdrew support for ISetProperty in an earlier patch. What I want to know is whether EMF supports unique lists? If so you could add support back in at some point in the future--it's probably a bit late to add that back in. * The @deprecated comments were copied and pasted, so the word "list" was pasted without correction in: EObjectObservableValue, EObjectObservableMap, EditingDomainEObjectObservableValue, EditingDomainEObjectObservableMap * EMFDataBindingContext.new() says "uses the default realm for *validation*" but new(Realm) says "uses an explicit realm for *synchronization*"--a minor inconsistency in wording that's worth clarifying. * EMFObservables.observeResourceContents could be made into a property. The resource is the source and getContents() is the property. Basically this method should be a convenience for "new ResourceContentsValueProperty().list(EMFProperties.notifyingList())". I have an idea for making a self-aware property for NotifyingList, more farther down. * In EMFUpdateValueStrategy a few constructors say "list" instead of "value." * In all new classes the CVS $Id tags were incorrect--I assume this will be automagically fixed when you check the files in? * EWritableList could be easily converted to a self-list property similar to Properties.selfList(), e.g. EMFProperties.notifyingList(). I can help you implementing this. If you decided to do this you could then deprecate the entire EMFObservables class. * IEMFProperty.getStructuralFeature uses the phrase "property descriptor" which is a holdover from beans. * IEMFEditObservable extends IObserving instead of IEMFObservable--was this intentional? * IEMFEditProperty extends IProperty instead of IEMFProperty, and so must duplicate the getStructuralFeature() method. * EMFEditProperties class description is identical to EMFProperties. This should be enhanced to mention the EditingDomain. * IEMFEditValueProperty class description is identical to IEMFValueProperty. This should be enhanced to mention the EditingDomain. * IEMFEditValueProperty extends IEMFProperty, IValueProperty instead of IEMFValueProperty. Likewise for IEMFEditListProperty and IEMFEditMapProperty. The method names are the same but with Java 1.5 return type covariance this should not be a problem. Created attachment 136365 [details]
Addressing some of Matt's comments
A description which of the suggestions I implemented follows
Matt thank you for you fantasic feedback. Comments below: (In reply to comment #42) > Notes from review: > * You withdrew support for ISetProperty in an earlier patch. What I want to > know is whether EMF supports unique lists? If so you could add support back in > at some point in the future--it's probably a bit late to add that back in. Well it's even simpler because most lists are unique and one has to do extra work to make it none unique (for containment-lists it's even impossible) > * The @deprecated comments were copied and pasted, so the word "list" was > pasted without correction in: EObjectObservableValue, EObjectObservableMap, > EditingDomainEObjectObservableValue, EditingDomainEObjectObservableMap Thanks corrected. > * EMFDataBindingContext.new() says "uses the default realm for *validation*" > but new(Realm) says "uses an explicit realm for *synchronization*"--a minor > inconsistency in wording that's worth clarifying. Thanks corrected. > * EMFObservables.observeResourceContents could be made into a property. The > resource is the source and getContents() is the property. Basically this > method should be a convenience for "new > ResourceContentsValueProperty().list(EMFProperties.notifyingList())". I have > an idea for making a self-aware property for NotifyingList, more farther down. I'll take a look at this in second the latest patch doesn't support for this but I'm taking a look at it. > * In EMFUpdateValueStrategy a few constructors say "list" instead of "value." Thanks corrected. > * In all new classes the CVS $Id tags were incorrect--I assume this will be > automagically fixed when you check the files in? Yes this is corrected by CVS > * EWritableList could be easily converted to a self-list property similar to > Properties.selfList(), e.g. EMFProperties.notifyingList(). I can help you > implementing this. If you decided to do this you could then deprecate the > entire EMFObservables class. Yep next on my list I added the small changes in this patch. > * IEMFProperty.getStructuralFeature uses the phrase "property descriptor" which > is a holdover from beans. Corrected. > * IEMFEditObservable extends IObserving instead of IEMFObservable--was this > intentional? No fixed. > * IEMFEditProperty extends IProperty instead of IEMFProperty, and so must > duplicate the getStructuralFeature() method. Fixed. > * EMFEditProperties class description is identical to EMFProperties. This > should be enhanced to mention the EditingDomain. Fixed. > * IEMFEditValueProperty class description is identical to IEMFValueProperty. > This should be enhanced to mention the EditingDomain. Fixed. > * IEMFEditValueProperty extends IEMFProperty, IValueProperty instead of > IEMFValueProperty. Likewise for IEMFEditListProperty and IEMFEditMapProperty. > The method names are the same but with Java 1.5 return type covariance this > should not be a problem. > Fixed to extend IEMFEditProperty! The reason I'm not extending IEMFValueProperty is that though you are right that the return covariance works but I also inherit methods like -------------8<------------- IEMFMapProperty map(IEMFMapProperty property); -------------8<------------- and even more important for now the set()-method which we are not supporting though I would simply have to copy the stuff from IListProperty because as outlined above. I'll think about this after I took a look on observeResourceContents stuff. >
> and even more important for now the set()-method which we are not supporting
> though I would simply have to copy the stuff from IListProperty because as
> outlined above.
for get this comment - not sure what I was think :-)
Created attachment 136370 [details]
Implemented another suggestion from Matt
IEMFEdit*Property now extends IEMF*Property
Taking ownership and I'll check in the latest patch (today or tomorrow) so that we can work in iterations to complete the stuff. This is all provisional work so I think we are still allowed to bring into the Galileo-Release. Any objections on this? It's getting hard to review these patches--I tried running a diff on two different versions of the patch, but it looks like Eclipse doesn't maintain the order of files when it exports the diffs, so that's a non-starter. As much as I'd like to help out, reviewing each new version is time-consuming. So I for one welcome our new provisional API overlords, and their minions of smaller, more digestible patches. Or something. It's a little late in the cycle, but I do believe our clients are best served by keeping up with the latest property-based data binding design. I'm not sure about deprecating the old classes. They are all still marked provisional, so that seems sufficient. The new ones should be marked in that way as well so it's clear that we may still make arbitrary changes to improve the design. I released the latest patch to CVS with the modification that I removed the deprecation warnings because customers might get confused to get a deprecation warning when using an RC. (In reply to comment #49) > It's a little late in the cycle, but I do believe our clients are best served > by keeping up with the latest property-based data binding design. I'm not sure > about deprecating the old classes. They are all still marked provisional, so > that seems sufficient. The new ones should be marked in that way as well so > it's clear that we may still make arbitrary changes to improve the design. > Why do we need to commit these changes if we anticipate changes to the design? Why not wait until after Galileo or do it in a separate branch? In any case, this issue needs PMC approval before anything can be committed... The changes have broken the build. See: http://modeling.eclipse.org/modeling/emf/emf/downloads/drops/2.5.0/S200905250857/buildlog.txt I suspect this is a result of manually specifying a bundle-version on the org.eclipse.core.databinding.property dependencies. The build actually adds them automatically, so as to always depend on the latest minor version, against which we're building. I imagine it choked on this case where one is already specified. Does anyone want to confirm this before I try removing them and kicking off another build? Yes, please don't hard code ranges. Note that the entire existing base code was and is marked provisional as well as all this new stuff. Clients should have low expectations about the stability of the code base as we evolve it to be something stable hopefully during the next release. This approach helps prepare the path for that better than would waiting, in my opinion. (In reply to comment #52) > The changes have broken the build. See: > http://modeling.eclipse.org/modeling/emf/emf/downloads/drops/2.5.0/S200905250857/buildlog.txt > Does anyone want to confirm this before I try removing them and kicking off > another build? /home/www-data/build/emf/downloads/drops/2.5.0/S200905250857/org.eclipse.emf.releng/builder/all/customTargets.xml:115: Unable to find the version of 'org.eclipse.core.databinding.property1.2.0'. That looks more like a typo in the manifest than anything, but it could be a pattern match failure or some other problem. IIRC, emf.build.SetRequiredBundleVersionRanges was written by Marcelo. See /org.eclipse.emf.releng/org.eclipse.emf.build.ant/tasks/org/eclipse/emf/build/ant/taskdefs/SetRequiredBundleVersionRanges.java (In reply to comment #51) > (In reply to comment #49) > > It's a little late in the cycle, but I do believe our clients are best served > > by keeping up with the latest property-based data binding design. I'm not sure > > about deprecating the old classes. They are all still marked provisional, so > > that seems sufficient. The new ones should be marked in that way as well so > > it's clear that we may still make arbitrary changes to improve the design. > > > > Why do we need to commit these changes if we anticipate changes to the design? > Why not wait until after Galileo or do it in a separate branch? > > In any case, this issue needs PMC approval before anything can be committed... > Sorry for causing confusion but I asked for PMC approval and it was granted by Ed. I didn't knew that I was not allowed to set specific version numbers on my bundles to break the build. We committed the changes because then we can gather feedback from people about this provisional API and hopefully can release a final version in 2.6 else we would need one more provisional cycle. I'm sorry for breaking something and if I did something illegal in my first commit I'm sorry and apologize for it. The classes in questions for deprecation are the ones already available since 2.4.0. Please note that all this is and was provisional. Dave if there's something I should do? Tom, No worries, and no harm done. I don't think you did anything particularly wrong, or at least not something you should have reasonably expected to know about. The way we handle dependency versions is different from most projects. I have removed the offending bundle-versions and now the build is working again: http://modeling.eclipse.org/modeling/emf/downloads/?showAll=1&hlbuild=N200905251202&sortBy=date&project=emf#N200905251202 The expected range is being generated in the build: "[1.2.0,2.0.0)". I've done a quick smoke test and all seems fine. I've kicked off an RC2 build, and I'm planning to promote it as soon as it's done. With PMC approval from Ed, I think we're okay process-wise. Fix available in HEAD: 2.5.0RC2 (S200905251338). Hasan, the Eclipse IP team asked us to file a CQ [https://dev.eclipse.org/ipzilla/show_bug.cgi?id=3368] and because the final patch was holding some lines of code this requires us to get you confirmation that: 1. You authored 100% of the content 2. You have the right to donate the content to Eclipse Hasan can you please confirm the questions I asked in the last comment - else we need to remove those lines of code. Somehow I received the bugzilla mail notification delayed. Anyways, yes I do confirm that: 1. I have authored 100% of the content and copied partially from the eclipse code base in java beans properties API 2. I have the right to donate the content to Eclipse 3. I do donate the code to eclipse. Sorry for the delay and thanks Hasan Thanks Hasan - I'll inform the IP team about your comments. Fix available in HEAD: 2.5.0RC4 (S200906080927). Fix available in HEAD: 2.5.0 (R200906151043). Hi, you mentioned that the equivalent of Properties.selfList() can be implemented: >> * EWritableList could be easily converted to a self-list property similar to >> Properties.selfList(), e.g. EMFProperties.notifyingList(). I can help you >> implementing this. If you decided to do this you could then deprecate the >> entire EMFObservables class. > >Yep next on my list I added the small changes in this patch. Unfortunately, I can't find EMFProperties.selfList() (neither EMFEditProperties.selfList(). Did I miss something? If it is not yet implemented, can you guide me on how to implement it? I suppose that we should provide a SelfEMFEditProperties, should we extend EMFEditproperties or SelfListProperty? or restart from SimpleList because everything need to be modified? Regards, |