Bug 543787 - TreeMasterValidationInitiator deletes siblings of the notifier on remove
Summary: TreeMasterValidationInitiator deletes siblings of the notifier on remove
Status: CLOSED FIXED
Alias: None
Product: ECP
Classification: Modeling
Component: Validation (show other bugs)
Version: 1.19.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 1.20.0   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2019-01-24 08:39 EST by Nicole Behlen CLA
Modified: 2019-02-19 09:35 EST (History)
1 user (show)

See Also:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole Behlen CLA 2019-01-24 08:39:26 EST 
When the TreeMasterValidiationInitiator receives the notification of a removed child, the siblings of the notifier from the TreeMasterValdidationInitiator#mapping are removed in the method notifyChange.

This leads to follow up problems, when a new element is added to one of the sibling. The notify of the add is skipped and EMFFormsDMRExpanderDefaultHeuristic#createMissingPathElementIfPossible is not called with the AddCommand (in the write transaction). Unluckily it is called when the new created element is selected in tree and leads to an


java.lang.IllegalStateException: Cannot modify resource set without a write transaction
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.assertWriting(TransactionChangeRecorder.java:349)
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.appendNotification(TransactionChangeRecorder.java:303)
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.processObjectNotification(TransactionChangeRecorder.java:285)
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.notifyChanged(TransactionChangeRecorder.java:241)
	at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:424)
	at org.eclipse.emf.common.notify.impl.NotificationImpl.dispatch(NotificationImpl.java:1027)
	at com.factory.example.base.impl.ConditionImpl.setOperand(ConditionImpl.java:165)
	at com.factory.example.base.impl.ConditionImpl.eSet(ConditionImpl.java:262)
	at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eSet(BasicEObjectImpl.java:1071)
	at org.eclipse.emfforms.internal.core.services.domainexpander.defaultheuristic.EMFFormsDMRExpanderDefaultHeuristic.createMissingPathElementIfPossible(EMFFormsDMRExpanderDefaultHeuristic.java:115)
	at org.eclipse.emfforms.internal.core.services.domainexpander.defaultheuristic.EMFFormsDMRExpanderDefaultHeuristic.prepareDomainObject(EMFFormsDMRExpanderDefaultHeuristic.java:86)
	at org.eclipse.emfforms.internal.core.services.domainexpander.defaultheuristic.EMFFormsDomainExpanderImpl.prepareDomainObject(EMFFormsDomainExpanderImpl.java:102)
	at org.eclipse.emf.ecp.view.internal.context.ViewModelContextImpl.expandAndInitDMR(ViewModelContextImpl.java:300)
	at org.eclipse.emf.ecp.view.internal.context.ViewModelContextImpl.resolveDomainReferences(ViewModelContextImpl.java:289)
	at org.eclipse.emf.ecp.view.internal.context.ViewModelContextImpl.instantiate(ViewModelContextImpl.java:314)
	at org.eclipse.emf.ecp.view.internal.context.ViewModelContextImpl.<init>(ViewModelContextImpl.java:268)
	at org.eclipse.emf.ecp.view.internal.context.ViewModelContextImpl.getChildContext(ViewModelContextImpl.java:889)
	at org.eclipse.emf.ecp.view.internal.context.ViewModelContextImpl.getChildContext(ViewModelContextImpl.java:863)


So far, I have tested, that it works fine when replacing the loop

for (final Object child : childrenTreeContextEntry) {
final ViewModelContext childContext = getExistingOrNewChildContext(treeContextEntry, child);
childContext.removeContextUser(this);
                      childContext.removeContextUser(TreeMasterDetailValidationInitiator.this);
mapping.get(treeContextEntry).remove(child);
}

with just removing the removed value

Object oldValue = notification.getRawNotification().getOldValue();
if (oldValue != null) {
final ViewModelContext childContext = getExistingOrNewChildContext(treeContextEntry, oldValue);
childContext.removeContextUser(this);
                            childContext.removeContextUser(TreeMasterDetailValidationInitiator.this);
mapping.get(treeContextEntry).remove(oldValue);
}

the problem is solved. But I cannot estimate the side effects the change could have, so I will dig deeper into it.

I will try to provide also a demo to reproduce this problem these days.
Comment 1 Nicole Behlen CLA 2019-01-24 09:32:01 EST 
Additionally I noticed, that the validation on tree (decoration) is no longer working correctly, and this might be easy for you to reproduce.

Example: 

- root
 - parent1
 - parent2

Add a child2 to parent2 with validation error (tree shows correctly the decorator)
Select parent1 (tree still shows correctly the decorator con child2

Delete child2 (or simply undo) and add child1 to parent1 with validation error
(tree shows decorator correctly)

Now select parent2 (tree doesn't show any longer the error decorator on child2)

(the suggested code replacement in main description fixes this problem too)
Comment 2 Eugen Neufeld CLA 2019-01-25 04:23:14 EST 
Hi Nicole,
thank you for the report. 

We never tested with the TransactionalEditingDomain.

Could you provide the fix as a gerrit, We can discuss possible side effects then.

Thank you,
Eugen
Comment 3 Eclipse Genie CLA 2019-01-25 04:48:53 EST 
New Gerrit change created: https://git.eclipse.org/r/135756
Comment 4 Nicole Behlen CLA 2019-01-25 04:52:33 EST 
Hi Eugen,

Thx for your quick reaction always. The gerrit change is done.
One remark, I guess the second problem I described, might be also reproducible without TransactionalEditingDomain.

Greets

Nicole