Community
Participate
Working Groups
The destroy(MSLEditingDomain, EObject, int) method will throw an NPE if the EObject is part of a substitution group reference which means that it uses a FeatureMap in EMF. The problem is on line 220. container.eSet(reference, null); The code block is if (reference.isMany()) ((Collection) container.eGet(reference)).remove(eObject); else container.eSet(reference, null); This needs to be set to the following: if (reference.isMany()) ((Collection) container.eGet(reference)).remove(eObject); else container.eUnset(reference); The eUnset will do the correct action when using FeatureMaps. Passing a null value into a FeatureMap eSet(...) is a bad thing. :)
I'm looking for a test case for the fix to the bug. Is it possible for you to provide an example attached to this bugzilla?
Part of the problem here is that I forgot to mention which class has the problem. :) The problem is with org.eclipse.gmf.runtime.emf.core.util.EObjectUtil#destroy(...). This method is similar to the ECoreUtil.remove(EObject) method. public static void remove(EObject eObject) { EObject container = ((InternalEObject)eObject).eInternalContainer(); if (container != null) { EReference feature = eObject.eContainmentFeature(); if (feature.isMany()) { ((EList)container.eGet(feature)).remove(eObject); } else { container.eUnset(feature); } .... Note that when you are intending to remove an EObject from its container you should use eUnset for a single valued reference and not eSet(feature, null). The second doesn't work when the feature is affiliated with a substitution group. It looks like the EObjectUtil#destroy(...) method copied some of the ECoreUtil.remove(...) code but it has not been updated. A test case could be created but it requires the creation of an EMF model from XSD where a type has a reference to a global element using FeatureMaps. Then define a second model (XSD) that extends the first and defines a substitution for the global element. Then create an instance of the original object with a reference instance to the substitution type. Then use EcoreUtil.destroy(...) to destroy it. This is a lot of work for a simple fix that looks like a miss copy or a change from the original copy code.
Why are we not using EcoreUtil.remove()?
That was actually going to be my next question. :-)
(In reply to comment #3) > Why are we not using EcoreUtil.remove()? EObjectUtil.destroy() works with the MSLEditingDomain and can be undone while EcoreUtil.remove() does not and is a one way trip.
(In reply to comment #3) > Why are we not using EcoreUtil.remove()? > I'm looking into making that call instead of doing everything ourselves and not being up-to-date with EMF.
(In reply to comment #5) > (In reply to comment #3) > > Why are we not using EcoreUtil.remove()? > > EObjectUtil.destroy() works with the MSLEditingDomain and can be undone while > EcoreUtil.remove() does not and is a one way trip. > This is not true. We can undo a remove as well as a destroy.
Daniel, I tried to reproduce the problem as you had said but my XSD knowledge is a little weak. I was wondering if you could provide me with some XSD's (or a single XSD) that I could use to generate EMF code and a test case. I just want to be sure that I'm actually fixing the problem before I deliver any changes.
(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > Why are we not using EcoreUtil.remove()? > > > > EObjectUtil.destroy() works with the MSLEditingDomain and can be undone while > > EcoreUtil.remove() does not and is a one way trip. > > > This is not true. We can undo a remove as well as a destroy. Could you enlighten me: when I use destroy(), an undo is inserted in the Edit menu. When I use remove(), nothing is. When I look at the destroy() code, it invokes the delete against the resource's edit domain. When I look at the remove()'s code, it seems to just do a mechanical removal of an item from an array without regard to any editing domains. What am I missing?
(In reply to comment #9) > (In reply to comment #7) > > (In reply to comment #5) > > > (In reply to comment #3) > > > > Why are we not using EcoreUtil.remove()? > > > > > > EObjectUtil.destroy() works with the MSLEditingDomain and can be undone while > > > EcoreUtil.remove() does not and is a one way trip. > > > > > This is not true. We can undo a remove as well as a destroy. > > Could you enlighten me: when I use destroy(), an undo is inserted in the Edit > menu. When I use remove(), nothing is. When I look at the destroy() code, it > invokes the delete against the resource's edit domain. When I look at the > remove()'s code, it seems to just do a mechanical removal of an item from an > array without regard to any editing domains. What am I missing? > The way that MSL works is based on a content adapter of sorts. It listens to every event coming from all EObjects that belongs to its domain (or ever did belong to the domain). It is able to undo/redo based on the history of events that it receives. This is the value added to base EMF where you normally need to construct commands and provide undo/redo logic or you need to create a change recorder and activate it for the session that you wish to undo/redo.
> The way that MSL works is based on a content adapter of sorts. It listens to > every event coming from all EObjects that belongs to its domain (or ever did > belong to the domain). It is able to undo/redo based on the history of events > that it receives. > This is the value added to base EMF where you normally need to construct > commands and provide undo/redo logic or you need to create a change recorder > and activate it for the session that you wish to undo/redo. So EObjectUtil.destroy() uses an edit domain, but EcoreUtil.remove() depends on MSL to be listening to EMF model changes and constructing undo/redo commands in the background? I've seen this mechinism before and think it's pretty cool-- unfortunately right now in my editor destroy() works for undo and remove() doesn't--for what ever reason. So it would be really nice if destroy() had this bug fixed...
(In reply to comment #11) > > The way that MSL works is based on a content adapter of sorts. It listens to > > every event coming from all EObjects that belongs to its domain (or ever did > > belong to the domain). It is able to undo/redo based on the history of events > > that it receives. > > This is the value added to base EMF where you normally need to construct > > commands and provide undo/redo logic or you need to create a change recorder > > and activate it for the session that you wish to undo/redo. > > So EObjectUtil.destroy() uses an edit domain, but EcoreUtil.remove() depends on > MSL to be listening to EMF model changes and constructing undo/redo commands in > the background? I've seen this mechinism before and think it's pretty cool-- > unfortunately right now in my editor destroy() works for undo and remove() > doesn't--for what ever reason. So it would be really nice if destroy() had this > bug fixed... > In both cases, MSL is listening to everything that happens to the EObjects. It can undo/redo in either case. It's just that destroy is an operation that EMF doesn't directly support so we provide a utility to perform the destroy. It could be that there's a bug in the editor that you are using if you're not seeing an undo/redo for the remove case.
Regardless of whether we should use EcoreUtil.remove(...) or EObjectUtil.destroy(...) the fact still remains that your EObjectUtil.destroy(...) API does not work if the EObject passed has references that are tied to FeatureMaps because internally it is using EObject.eSet(EStructuralFeature, null) and not EObject.eUnset(EStructuralFeature) [similar to the EObject.remove(...) method)].
Created attachment 36011 [details] Patch to delegate to EcoreUtil.remove()
Bugzilla 130401 is fixed so EcoreUtil.remove() will handle feature map cases properly. With the attached patch, we will be calling EcoreUtil.remove() instead of removing ourselves. Hopefully this will fix the problem.
reviewed and applied patch
[target cleanup] 1.0 M6 was the original target milestone for this bug
[GMF Restructure] Bug 319140 : product GMF and component Runtime EMF was the original product and component for this bug
