Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 362649

Summary:

Root model elements can't be deleted in local projects

Product:

[Modeling] EMFStore

Reporter:

Nikolay Kasyanov <corrmage>

Component:

Common

Assignee:

Maximilian Koegel <mkoegel>

Status:

RESOLVED FIXED

QA Contact:

Severity:

critical

Priority:

CC:

eclipse, emueller, mkoegel

Version:

unspecified

Target Milestone:

---

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
fixes #362649	eclipse: iplog+

Description Nikolay Kasyanov

2011-11-02 06:16:33 EDT

Build Identifier: 20110916-0149

Root elements cannot be deleted from projects. Actually, they deletes, but appears again after eclipse relaunch (i.e., no actual changes written to storage).

I've made some investigation and found that in case of deleting root model element state persister doesn't save resource, because dirtyResources is empty after deleting. May be it is because of condition in file /org.eclipse.emf.emfstore.common.model/src/org/eclipse/emf/emfstore/common/model/util/EObjectChangeNotifier.java on line 181.

I.e. if notifier is project, project change observers doesn't notified about element removal, and state persister then doesn't add project's resource to dirtyResources.

reproducible on svn, git & current update site versions.

Reproducible: Always

Steps to Reproduce:
1. Create project
2. Create an model element in project
3. Delete this element
4. Relaunch Eclipse
5. Element still exists in project

Comment 1 Nikolay Kasyanov

2011-11-02 06:19:12 EDT

Yes, removing condition part !(notifier instanceof Project) solved problem, but I don't know if it's safe to do that.

Comment pls!

Comment 2 Maximilian Koegel

2011-11-02 12:56:27 EDT

Thanks for pinning down this bug! I think the change you suggested is potentially dangerous since receivers might not expect notifications of Project. Maybe they should be aware of that, but for the time being I would suggest the following change. I had no time to test it, I am on the EclipseCon currenly, could you please try? Also it would be great if you could supply a test case. A potential starting point for a test is CompositeOperationTest.abortSmallComposite() in client.test.

Index: src/org/eclipse/emf/emfstore/client/model/impl/StatePersister.java
===================================================================
--- src/org/eclipse/emf/emfstore/client/model/impl/StatePersister.java	(revision 12442)
+++ src/org/eclipse/emf/emfstore/client/model/impl/StatePersister.java	(working copy)
@@ -5,6 +5,7 @@
 
 import org.eclipse.emf.common.command.Command;
 import org.eclipse.emf.common.notify.Notification;
+import org.eclipse.emf.common.notify.impl.AdapterImpl;
 import org.eclipse.emf.common.util.BasicEMap;
 import org.eclipse.emf.common.util.TreeIterator;
 import org.eclipse.emf.common.util.URI;
@@ -27,7 +28,7 @@
 import org.eclipse.emf.emfstore.common.model.util.ModelUtil;
 import org.eclipse.emf.emfstore.common.model.util.ProjectChangeObserver;
 
-public class StatePersister implements CommandObserver, ProjectChangeObserver {
+public class StatePersister extends AdapterImpl implements CommandObserver, ProjectChangeObserver {
 
 	private IdEObjectCollection collection;
 
Index: src/org/eclipse/emf/emfstore/client/model/impl/ProjectSpaceImpl.java
===================================================================
--- src/org/eclipse/emf/emfstore/client/model/impl/ProjectSpaceImpl.java	(revision 12442)
+++ src/org/eclipse/emf/emfstore/client/model/impl/ProjectSpaceImpl.java	(working copy)
@@ -1682,6 +1682,8 @@
 		// TODO: initialization order important
 		this.getProject().addProjectChangeObserver(this.operationRecorder);
 		this.getProject().addProjectChangeObserver(statePersister);
+		// FIXME: BUGFIX move me into StatePersister
+		this.getProject().eAdapters().add(this.statePersister);
 
 		if (project instanceof ProjectImpl) {
 			((ProjectImpl) this.getProject()).setUndetachable(operationRecorder);


(In reply to comment #1)
> Yes, removing condition part !(notifier instanceof Project) solved problem, but
> I don't know if it's safe to do that.
> 
> Comment pls!

Comment 3 Nikolay Kasyanov

2011-11-03 16:23:35 EDT

Hi!
Thanks for quick reply.

Tried patch, it doesn't work. I've got your point, but notification doesn't passed to notify method of StatePersister.

Comment 4 Maximilian Koegel

2011-11-09 14:37:37 EST

We found a fix and it seems to work, we will do a code review tomorrow and then check it in. I will let you know.

Comment 5 Max Hohenegger

2011-11-14 08:50:56 EST

Created attachment 206943 [details]
fixes #362649

Comment 6 Max Hohenegger

2011-11-14 08:54:05 EST

The patch attached should solve this bug.
As Maximilian pointed out, some receivers do not expect notifications from projects, thus, additionally to removing the check in EObjectChangeNotifier, we added a new filter to the default FilterStack. NotifiableIdEObjectCollectionFilter makes sure that notifications from Projects only come through if that is the desired behavior.
Another instanceof-check for project is required in the StatePersisters to prevent it from trying to get an ID from a project and trying to set it in the resource. Otherwise StatePersister will throw a RuntimeException.

Comment 7 Edgar Mueller

2011-11-14 09:05:42 EST

The proposed patch is OK and has been applied to the master.

Comment 8 Nikolay Kasyanov

2011-11-14 10:48:26 EST

great news!