Bug 345730

Comment 1 Nicolas Guyomar 2011-05-17 04:08:58 EDT

Hi Vincent,

I cannot reproduce this bug, and both unit test covering this scenario are OK.

Could you please update your EMF Facet installation, and tell me if you are able to reproduce it?

Regards,
Nicolas Guyomar

Comment 2 Nicolas Guyomar 2011-05-23 08:39:53 EDT

Created attachment 196325 [details]
Patch for Bug 345730

Hi Vincent,

The solution I found in order to solve this problem is to remove inconsistent rows from the table.

Unfortunately, I was not able to handle "undo" action in the model editor so that it would add the corresponding row to the table. I've tried to add a listener to the editing domain command stack in order to append some kind of refresh command, but I cannot append it to the stack.

Result is that when a model element is destroyed(when the eObject becomes null, or is no longer contained by a resource), I perform a RemoveCommand outside of the command stack (so that only one undo is needed to cancel the element destruction).
If the user wants to see the missing row again, then he has to drop the element in the table.

I hope this would be OK as a solution for this bug.

(a) I, Nicolas Guyomar, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Nicolas Guyomar

Comment 3 Gregoire Dupe 2011-05-23 12:01:36 EDT

Comment on attachment 196325 [details]
Patch for Bug 345730

Hello,

This patch fixes the bug and does not break any non regression tests.
I agree to commit this patch for RC2.

Regards,
Grégoire Dupé

Comment 4 Gregoire Dupe 2011-05-23 12:05:16 EDT

Hello Ed,

Are you agree to commit this patch for RC2 ?

Regards,
Gregoire

Comment 5 Gregoire Dupe 2011-05-24 03:32:07 EDT

Comment on attachment 196325 [details]
Patch for Bug 345730

Here is a contribution from one employee of Mia-Software, targeting future
Indigo release. The company has signed a Member Commiter Agreement. The
contribution does not need a CQ (see bug 322327).

I've committed this contribution.

Committed revision 697.

Comment 6 Gregoire Dupe 2011-05-24 03:32:48 EDT

This bug can be mark as resolved.

Comment 7 Nicolas Guyomar 2011-05-24 03:47:43 EDT

Created attachment 196405 [details]
Patch for Bug 345730 Non regression test

Hi,

Please find attached a non regression test for this scenario.

This test check that in the case:
- "The table editor is opened from a model editor"
The following action:
- "The user delete an element referenced by the table from the model editor"
The expected result is : 
- "The corresponding row in the table has been removed"

(a) I, Nicolas Guyomar, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Nicolas Guyomar

Comment 8 Vincent Lorenzo 2011-06-09 04:52:44 EDT

Hi Nicolas, 
	I'm sorry, but his bug is not fully corrected. In the Ecore case, it's work fine, but in the Papyrus case, I get the following exception. I think you don't use a command to remove the line...
	
	
org.eclipse.swt.SWTException: Failed to execute runnable (java.lang.IllegalStateException: Cannot modify resource set without a write transaction)
	at org.eclipse.swt.SWT.error(SWT.java:4277)
	at org.eclipse.swt.SWT.error(SWT.java:4192)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:137)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4125)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3742)
	at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2696)
	at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2660)
	at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2494)
	at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:674)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:667)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:123)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:344)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
	at java.lang.reflect.Method.invoke(Unknown Source)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:622)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:577)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1410)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1386)
Caused by: java.lang.IllegalStateException: Cannot modify resource set without a write transaction
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.assertWriting(TransactionChangeRecorder.java:348)
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.appendNotification(TransactionChangeRecorder.java:302)
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.processObjectNotification(TransactionChangeRecorder.java:284)
	at org.eclipse.emf.transaction.impl.TransactionChangeRecorder.notifyChanged(TransactionChangeRecorder.java:240)
	at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:380)
	at org.eclipse.emf.ecore.util.EcoreEList.dispatchNotification(EcoreEList.java:255)
	at org.eclipse.emf.common.notify.impl.NotifyingListImpl.remove(NotifyingListImpl.java:719)
	at org.eclipse.emf.edit.command.RemoveCommand.doExecute(RemoveCommand.java:327)
	at org.eclipse.emf.edit.command.AbstractOverrideableCommand.execute(AbstractOverrideableCommand.java:131)
	at org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget.checkTableIntegrity(NatTableWidget.java:3005)
	at org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget.setInput(NatTableWidget.java:412)
	at org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget.access$17(NatTableWidget.java:411)
	at org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget$16$1.run(NatTableWidget.java:2955)
	at org.eclipse.ui.internal.UILockListener.doPendingWork(UILockListener.java:164)
	at org.eclipse.ui.internal.UISynchronizer$3.run(UISynchronizer.java:158)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134)
	... 23 more

Comment 9 Gregoire Dupe 2011-06-10 08:48:28 EDT

Comment on attachment 196405 [details]
Patch for Bug 345730 Non regression test

Here is a contribution from one employee of Mia-Software, targeting future
Indigo release. The company has signed a Member Commiter Agreement. The
contribution does not need a CQ (see bug 322327).

I've committed this contribution.

Committed revision 719.

Please, not that I've added the following lines to synchronize the test with the table refresh job :

INatTableWidgetInternal iNatTableWidgetInternal = (INatTableWidgetInternal) natTableWidgetPart.getNatTableWidget();
iNatTableWidgetInternal.waitForResfreshJob();

Comment 10 Gregoire Dupe 2011-06-21 10:11:23 EDT

(In reply to comment #8)
> Hi Nicolas, 
>     I'm sorry, but his bug is not fully corrected. In the Ecore case, it's work
> fine, but in the Papyrus case, I get the following exception. I think you don't
> use a command to remove the line...

Hello Vincent,

The methods NatTableWidget.removeLine(), NatTableWidget.removeLine2() and NatTableWidget.checkTableIntegrity() uses EMF commands. That’s why I’m so sure that the problem comes from EMF Facet.
Please, can you provide a Papyrus independent unit test to reproduce this bug? We need a unit test that only uses EMF. If it is need you can use a test meta-model to help to reproduce the bug with a small amount of data.

Regards,
Gregoire

Comment 11 Vincent Lorenzo 2011-06-30 07:25:35 EDT

Created attachment 198892 [details]
This patch provides tests to reproduce the bug with a TransactionalEditingDomain

2 points : 
  - a new file which tries to reproduce the bug : EcoreUtil.delete(EObject) seems doesn't work... Do you have some idea of this problem?
  - some modification in the SWTBot test, which allows to reproduce the bug
		+ I commented the line 208 //		assertTrue(nattableEditor.isDirty());
because this assertion failed (and I don't understand why there is this assertion).

    (1) I, Vincent LORENZO, wrote 100% of the code I've provided.
    (2) This code contains no cryptography
    (3) I have the right to contribute the code to Eclipse.
    (4) I contribute the content under the EPL.

Comment 12 Gregoire Dupe 2011-07-04 12:30:29 EDT

Created attachment 199062 [details]
Unit test

Hello,
Here is a refactoring of your unit test. You have some troubles whit the delete action because you were executing the test in the UI thread. (Tracing helps to understand the problem.)

Do you agree with this new unit test?

This patch is a refactoring (by myself) of the attachment 198892 [details] provided by Vincent Lorenzo.

(a) I, Gregoire Dupe, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.


Regards,
Gregoire

Comment 13 Gregoire Dupe 2011-07-04 12:37:03 EDT

I assume that the method org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget.checkTableIntegrity() has to removed and replaced by a listener on the command stack.

To place the problem in MVC point of view: the method checkTableIntegrity() is called from the view (org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget.setInput()) and modify the table model. This is not the right pattern. The table model has to be updated by the controller (a set of listeners on the data model).

Comment 14 Vincent Lorenzo 2011-07-05 04:37:34 EDT

(In reply to comment #12)
Hello Grégoire, 
  does this patch work on your computer? The last assert in deleteElement() failed on my computer. I think it's because the deleted rows can't be recreated after the undo, because the DeleteRow command in not done in the command stack in emf.facet.nattable.

Comment 15 Vincent Lorenzo 2011-07-05 04:43:38 EDT

Created attachment 199098 [details]
The same a previous attachment, with some comments in Bug350700Test.

Hello, 
   I added some comments in your JUnit test, but I agree with your test. 
   
the added comments are in the method deleteElement().
   
This patch is a refactoring (by myself) of the attachment 199062 [details] provided by G. Dupé?

1/ I, Vincent Lorenzo, wrote 100% of the code I've provided.
2/ I have the right to contribute the code to Eclipse.
3/ I contribute the content under the EPL.
4/ This contribution contains no Cryptography features.

Thank you for you help to write this test.
Regards, 
Vincent Lorenzo

Comment 16 Gregoire Dupe 2011-07-20 08:25:40 EDT

Created attachment 199986 [details]
Unit tests and fix

Hello,

Here is a patch providing a fix of this bug and a set of unit test.
Some unit tests are improvements of the attachment 199062 [details] (with is itself a  refactoring (by myself) of the attachment 198892 [details] provided by
Vincent Lorenzo.)

(a) I, Gregoire Dupe, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

To solve this bug, I had to do a lot of modifications. The main modification is the ad of triggers to update the table rows and colums, when a model element is deleted. The triggers are in the package: org.eclipse.emf.facet.widgets.nattable.internal.listeners.

I assume that the attachment can be marked as 199098 obsolete.

Regards,
Gregoire Dupe

Comment 17 Gregoire Dupe 2011-07-20 08:29:19 EDT

Created attachment 199987 [details]
Initial version of the plug-in org.eclipse.emf.facet.widgets.nattable.instance.edit

Here is an additional plug-in needed to fix this bug.

This plug-in has be generated using EMF and the an EMF Facet Ecore model (svn+ssh://dev.eclipse.org/svnroot/modeling/org.eclipse.emft.facet/trunk/plugins/org.eclipse.emf.facet.widgets.nattable.instance).

(a) I, Gregoire Dupe, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Gregoire Dupe

Comment 18 Gregoire Dupe 2011-07-22 10:53:26 EDT

Created attachment 200202 [details]
Unit tests

Hello,

Here is a new patch providing a set of unit test. Some unit tests are improvements of the attachment 199062 [details] (with is itself a 
refactoring (by myself) of the attachment 198892 [details] [details] provided by
Vincent Lorenzo.)

(a) I, Gregoire Dupe, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Gregoire Dupe

Comment 19 Gregoire Dupe 2011-07-22 10:55:03 EDT

Created attachment 200203 [details]
Fix

Hello,

Here is a patch providing a fix of this bug.

(a) I, Gregoire Dupe, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Regards,
Gregoire Dupe

Comment 20 Gregoire Dupe 2011-07-22 11:38:48 EDT

Created attachment 200210 [details]
Unit tests

Here is a new patch providing a set of unit test. Some unit tests are
improvements of the attachment 199062 [details] (with is itself a refactoring (by myself) of the attachment 198892 [details] provided by Vincent Lorenzo.)

(a) I, Gregoire Dupe, wrote 100% of the code I've provided.
(b) I have the right to contribute the code to Eclipse.
(c) I contribute the content under the EPL.
(d) This contribution contains no Cryptography features.

Comment 21 Gregoire Dupe 2011-07-22 12:45:10 EDT

Comment on attachment 199987 [details]
Initial version of the plug-in org.eclipse.emf.facet.widgets.nattable.instance.edit

I've committed this new plug-in.

Committed revision 728.

Comment 22 Gregoire Dupe 2011-07-22 13:14:17 EDT

Comment on attachment 200203 [details]
Fix

I've committed this fix.

Committed revision 729.

Comment 23 Gregoire Dupe 2011-07-22 13:21:10 EDT

Comment on attachment 200210 [details]
Unit tests

Hello Patrick,

A part of this contribution has been written by Vincent Lorenzo (CEA employee, attachment 198892 [details]) and the other part by me (EMF Facet committer). You are the only guy who can commit this patch without any CQ.

Please, can you commit this patch?

Please, add an iplog+ on the attachment 198892 [details] (to add Vincent’s contribution in the IP Log) and on this one (to add my contribution in the IP Log), until you will have committed this patch.

Regards,
Gregoire

Comment 24 Gregoire Dupe 2011-07-24 18:24:00 EDT

I've fix the plug-in name, the provider, the version of org.eclipse.emf.facet.widgets.nattable.instance.edit
I've added an about.html file in org.eclipse.emf.facet.widgets.nattable.instance.edit

Committed revision 733.

This should fix, the following unit test errors:
org.eclipse.emf.facet.tests.AllTests$org.eclipse.emf.facet.tests.BundleTest.name
org.eclipse.emf.facet.tests.AllTests$org.eclipse.emf.facet.tests.BundleTest.provider
org.eclipse.emf.facet.tests.AllTests$org.eclipse.emf.facet.tests.BundleTest.version
org.eclipse.emf.facet.tests.AllTests$org.eclipse.emf.facet.tests.BundleTest.about

Comment 25 Gregoire Dupe 2011-07-24 18:38:20 EDT

Two modifications were missing in the last commit:
 - I've add about.html in build.properties.
 - I've changed the version of org.eclipse.emf.facet.widgets.nattable.instance.edit required by org.eclipse.emf.facet.widgets.nattable.

Committed revision 734.

Comment 26 Gregoire Dupe 2011-08-29 07:56:28 EDT

Created attachment 202309 [details]
update of the attachment 200210 [details] (matching the revision 784 of the trunk)

Hello Patrick,

Here is an update version of the attachment 200210 [details]. This new patch can be applied on the head of the trunk (revision 784).

The IP status if this patch is exactly the same than the attachment 200210 [details]

Regards,
Grégoire

Comment 27 Gregoire Dupe 2011-08-29 07:58:13 EDT

Created attachment 202311 [details]
update of the attachment 200210 [details] (matching the revision 784 of the branch 0.1)

Hello Patrick,

Here is an update version of the attachment 200210 [details]. This new patch can be
applied on the head of the branch 0.1(revision 784).

The IP status if this patch is exactly the same than the attachment 200210 [details].

Regards,
Grégoire

Comment 28 Patrick Tessier 2011-08-29 09:49:28 EDT

Comment on attachment 202309 [details]
update of the attachment 200210 [details] (matching the revision 784 of the trunk)

this patch has been applied in the revision r785

Comment 29 Patrick Tessier 2011-08-29 09:50:33 EDT

Comment on attachment 202311 [details]
update of the attachment 200210 [details] (matching the revision 784 of the branch 0.1)

this patch has been applied in the revision r786

Comment 30 Gregoire Dupe 2011-08-29 10:49:16 EDT

Comment on attachment 198892 [details]
This patch provides tests to reproduce the bug with a TransactionalEditingDomain

A part of this contribution has been committed in the revision r785 and r786.

Comment 31 Gregoire Dupe 2011-08-29 13:05:50 EDT

The build of the branch 0.1 run without unit test failure [1].
The build of trunk run without unit test failure [2].

[1] https://hudson.eclipse.org/hudson/job/emffacet-integration/66/
[2] https://hudson.eclipse.org/hudson/job/emffacet-nightly/406/

The bug can then be marked has fixed.

Comment 32 Vincent Lorenzo 2011-09-15 09:32:34 EDT

I confirm that it works fine. This bug can be marked as closed.

Comment 33 Alain Le Guennec 2011-11-16 16:17:36 EST

Apparently, as part of fixing this bug, a dependency from org.eclipse.emf.facet.util.core to JUnit has been added (through class AbstractLogListenerTest.java).
It is a bit strange to have a dependency to the JUnit framework from a core plugin. Such dependencies should only occur in test-dedicated plugins, shouldn't it?
Can this be fixed?

Comment 34 Gregoire Dupe 2012-05-23 06:43:11 EDT

Hello Vincent,

Please, can you close this bug, if your problem has been solved ?

Regards,
Grégoire

Comment 35 Gregoire Dupe 2012-05-23 12:30:34 EDT

Sorry Vincent but this bug has to be closed for the IP log generation.