Community
Participate
Working Groups
I've realized that the current (2.4M4) implementation of the ItemProviderAdapter.hasChildren() method is simple but not very efficient. [Ed] I can imagine that for lazy loading situations asking for the actual children is less than optimal. The design does ensure that no matter how folks specialize getChildren, the hasChildren will be correct and won't require parallel changes. Especially in cases where the underlying object knows the sizes of all containment features but needs do expensive network requests to fill the collections with proper values. Currently I inject an own ItemProviderAdapter base class into the hierarchy of the generated item providers which avoids to access the children themselves: public class CDOItemProviderAdapter extends ItemProviderAdapter { public CDOItemProviderAdapter(AdapterFactory adapterFactory) { super(adapterFactory); } @Override @SuppressWarnings("deprecation") public boolean hasChildren(Object object) { Collection<? extends EStructuralFeature> anyChildrenFeatures = getChildrenFeatures(object); if (anyChildrenFeatures.isEmpty()) { anyChildrenFeatures = getChildrenReferences(object); } EObject eObject = (EObject)object; for (EStructuralFeature feature : anyChildrenFeatures) { if (feature.isMany()) { List<?> children = (List<?>)eObject.eGet(feature); if (!children.isEmpty()) { return true; } } else { if (eObject.eIsSet(feature)) { return true; } } } return false; } } This method seems to work pretty well for me. Do you see any obstacles with it? [Ed] That seems like a good approach. I could imagine though that the default value of a feature might be displayed as a child... Would it be possible to change the EMF code accordingly? [Ed] Possible, but you could imagine that breaking clients. Perhaps we could provide the implementation of the method but leave it to clients to configure something to actually use it. You'd likely still want your own root class for that... If so, be aware that the first five lines of my method only replicate the function of the private getAnyChildrenFeatures() method!
Created attachment 85756 [details] Support lazyHasChildren I changed your code slightly to use eGet rather than eIsSet. Not sure if that has a bad performance impact for you but it's the only way to properly match what getChildren actually does. Note that in Ecore itself we have features like getEType and getEGenericType where only one of the two will return eIsSet as true so that we don't serialize them both. This is an example where the eIsSet test would produce bad results.
That looks good. Do you think it's possible that you add a genmodel property "Lazy Has Children"?
That seems like a reasonable idea. It would sure make it more useful. :-P
Created attachment 85837 [details] Patches for specializing the generator. I think the patch includes changes from another bug too (https://bugs.eclipse.org/bugs/show_bug.cgi?id=211270). I wasn't sure where the flag should be. It could be on GenModel, GenPackage, or GenClass. The later makes sense but would require specifying this property perhaps hundreds of times. So I put it on GenPackage. I'll need Dave's review to decide if that's really the best way...
Just reviewed the patch and I do have a few comments... 1. There's no need for the first 5 lines of lazyHasChildren(). As Eike mentioned, that's already done by getAnyChildrenFeatures(), which can be called directly from the for expression. Eike's derived class couldn't do this because it's private. 2. I'm not crazy about the name lazyHasChildren(). What would you think about introducing a two-argument form of hasChildren() instead? public boolean hasChildren(Object object) { return hasChildren(object, false); } protected boolean hasChildren(Object object, boolean optimized) { if (!optimized) return !getChildren(object).isEmpty(); EObject eObject = (EObject)object; ... } My objection to the name isn't terribly strong, but somehow "lazy" doesn't sound right to me. I'd say its optimized to take advantage of lazy loading when available, but it's not lazy in and of itself. So, actually exposing that as the name in a public method (and making it not start with a verb in the process) seems a bit ugly to me. 3. Adding the new attribute to GenPackage (incidentally, I'd suggest optimizedHasChildren over lazyHasChildren for the same reason as above) doesn't seem consistent with what we've done in the past: in this situation, where things properly belong at the class or feature level but would normally be the same throughout a model, we've put it on the GenModel. I'm pretty sure we've reserved the GenPackage for things related directly to the package or package-level artifacts. I started down the road of changing ItemProviderAdapter and moving/renaming the GenModel attribute, but for some reason I found that my attribute was being added to GenModel.ecore *before* testsPluginID and testsPluginVariables. I'll attach my changed cat file. Ed, would you mind taking a closer look to see what's happening? You've been busy while I was away and I'd like to try to catch up... ;)
Created attachment 86018 [details] Problematic GenModel model
Created attachment 86115 [details] Updates as suggested I haven't really tested it well.
Created attachment 86136 [details] Added ItemProviderAdpater Changes Hi Ed, Looks good to me, but your patch was missing the changes to ItemProviderAdapter that go with the code generator changes. Since I had already made that change in my workspace (with Javadoc even <gin>), I tested your changes and dropped that in the patch. I also tweaked the description of the new GenModel property slightly. I left the other changes in your patch (i.e. bug 210778) in there. I think this is ready to go, if you're happy with it.
The changes are committed to CVS for 2.4.
Fixed in I200801090807.
Move to verified as per bug 206558.
