Bug 342511

Comment 1 Ed Willink 2011-04-12 03:44:49 EDT

Also:

DelegateResourceAdapter.unloadDelegates() creates adapters if they do not exist.

DelegateResourceAdapter.notifyChanged misses the RESOURCE__IS_LOADED set false event because RESOURCE__CONTENTS clear occurs first so there is nothing to unload.

Comment 2 Ed Willink 2011-04-12 04:51:53 EDT

I think the root of the trouble is multiple delegate behaviours per package (a rather dubious flexibility; let the application that needs forked delegates introduce a forking one - any way too late to sort that).

Therefore DelegateEPackageAdapter needs a two level cacahe.

Map<DelegatedBehavior, Map<String (uri), DelegateDomain>>

in place of

	protected Map<String, DelegateDomain> delegateDomainMap = null;


There might be a bit more symmetry if this cache was partitioned

as DelegateEPackageAdapter maps DelegatedBehavior to DelegateDomain.Factory
and OCLDelegateDomainFactory maps URI to DelegateDomain

The other issues are just tidying up.

Comment 3 Axel Uhl 2011-04-12 08:15:51 EDT

(In reply to comment #0)

> a) Why is an OCLDelegateDomain per-package, when apart from toString() all
> functionality is actually for the package's ResourceSet?

Creation of delegate domains is always per package, as controlled by the DelegateDomain.Factory interface. Caching of delegate factories is also by package, likely controlled by the delegate URIs attached to the respective package. The whole way DelegateEPackageAdapter seems to be designed, the delegate domain life cycle is attached to the EPackage granule rather than a ResourceSet.

You could of course try to play with OCLDelegateDomainFactory t oconstruct a new OCLDelegateDomain only if for the EPackage's resource set there isn't one already. The check would require a new cache.

> b) Why does DelegateEPackageAdapter claim to cache its DelegateDomain when it
> doesn't?

The comments seem to be a bit off. DelegateEPackageAdapter's javadoc talks about "DelegateEClassifierAdapter". Both, DelegateEClassifierAdapter and DelegateEPackageAdapter talk about "extends ..." but they both extend AdapterImpl. Regarding their caching behavior, at least DelegateEPackageAdapter.initializeDelegatedeBehavior stores all results of createDelegateDomain(URI) in delegateDomainMap. It is rather AbstractOCLDelegateFactory calling createDelegateDomain which caches the result locally. I wonder why AbstractOCLDelegateFactory.getDelegateDomain(EPackage) is using createDelegateDomain instead of getDelegateDomain. This would use the DelegateEPackageAdapter cache.

> c) Why does DelegateEPackageAdapter.delegateDomainMap claim to be initializable
> by the non-existent delegate_domain extension point?

Sounds like a stale comment.

> d) Why does AbstractOCLDelegateFactory.getDelegateDomain(EPackage) cache an
> EPackage dependent delegateDomain, but return the cache for any EPackage?

This would get sorted out by my above comment, using DelegateEPackageAdapter.getGetDelegateDomain instead of createDelegateDomain, and doing so in all cases, not doing any local caching, relying on the DelegateEPackageAdapter cache.

> e) DelegateEPackageAdapter class Javadoc refers to DelegateEClassifierAdapter.

See above.

What also bothers me about the delegate domain life cycle is how the OCL instance is constructed. In all cases that I can see (constructor of OCLDelegateDomain), the OCL instance is constructed using an EcoreEnvironmentFactory object (unless someone dared to assign a more specific factory to EcoreEnvironmentFactory.INSTANCE which seems possible but with unclear timing / initialization ordering and hence not reliable). Of course it's always possible to use a different delegate URI to point to a different, more specific delegate domain which uses, e.g., EcoreEnvironmentFactoryWithHiddenOpposites instead. Would you think that the desire to use a different environment factory justifies the introduction of a different URI with a different set of delegate classes?

Comment 4 Ed Willink 2011-04-12 12:30:10 EDT

(In reply to comment #3)

> > b) Why does DelegateEPackageAdapter claim to cache its DelegateDomain when it
> > doesn't?
> 
> I wonder why AbstractOCLDelegateFactory.getDelegateDomain(EPackage) is
> using createDelegateDomain instead of getDelegateDomain. This would use the
> DelegateEPackageAdapter cache.

Will you investigate an enhancement here or shall I?

> 
> What also bothers me about the delegate domain life cycle is how the OCL
> instance is constructed.

The API is very unsatisfactory, it was all presented as a suitable for OCL fait accompli just before M6 and no opportunity for any feedback from the OCL end.

Particularly awkward is the lack of a 'compile' hook to allow the DElegateDomain to get proactively created by the delegate domain installation and so do other things too, like auto-compute the classifier list.

----

Philosophically delegates are a model extension that are neutral to any particular usage scenario, and consequently it is not appropriate to pass information to the delegate domains; it is possible via a ResourceSet adapter but it creates usage asymmetries.

In the case of opposites, the first usage of an EPackage could analyze the EPackage to see whether opposites are in use and select the default environment factory accordingly.

Or, there should be yet another magic model EAnnotation that requests opposite support. You can't change Ecore annotations, but you could add an EPackage annotation

    <eAnnotations source="http://www.eclipse.org/emf/2002/Ecore/OCL">
      <details key="EnvironmentFactoryClass" value="org.eclipse.ocl.ecore.opposites.EcoreEnvironmentFactoryWithHiddenOpposites"/>
    </eAnnotations>

Comment 5 Ed Willink 2011-04-12 14:48:28 EDT

Created attachment 193084 [details]
Uses the cache

(In reply to comment #3)
> I wonder why AbstractOCLDelegateFactory.getDelegateDomain(EPackage) is
> using createDelegateDomain instead of getDelegateDomain. This would use the
> DelegateEPackageAdapter cache.

Well spotted. Works beautifully.

Comment 6 Axel Uhl 2011-04-12 17:46:19 EDT

(In reply to comment #5)
> Created attachment 193084 [details]
> Uses the cache
> 
> (In reply to comment #3)
> > I wonder why AbstractOCLDelegateFactory.getDelegateDomain(EPackage) is
> > using createDelegateDomain instead of getDelegateDomain. This would use the
> > DelegateEPackageAdapter cache.
> 
> Well spotted. Works beautifully.

+1

Is the delegateDomain field still required, or would it work equally well just always fetching it from the package adapter for the specific ePackage passed to getDelegateDomain(EPackage)? It would ensure that delegate domain, adapter and package always co-vary.

Comment 7 Ed Willink 2011-04-13 02:29:51 EDT

(In reply to comment #6)
> Is the delegateDomain field still required, or would it work equally well just
> always fetching it from the package adapter for the specific ePackage passed to
> getDelegateDomain(EPackage)? It would ensure that delegate domain, adapter and
> package always co-vary.

Probably not in practice, but yes for API compatibility. delegateDomain is protected and used by unused derived AbstractOCLDelegateFactory constructors.

It's a cache of small benefit at small cost. OCLValidationDelegateFactory.getValidationDelegate has a smelly overriding assignment.

I'll see if I can at least provide some deprecations based on a simplification for the pivot variant.

Comment 8 Ed Willink 2011-05-05 15:12:44 EDT

Shall we just commit this? The 'deprecations' are a cosmetic red herring.

Comment 9 Axel Uhl 2011-05-06 13:36:10 EDT

Sure. Again, +1

Comment 10 Ed Willink 2011-05-07 01:48:06 EDT

Committed to HEAD for RC1.

Comment 11 Ed Willink 2011-05-27 03:13:26 EDT

Closing