Bug 344921

Comment 1 Nicolas Guyomar 2011-05-20 04:14:15 EDT

Created attachment 196187 [details]
Patch for Bug 344921

Hi,

In bug Bug 344925 we made sure that every table actions was executed in a single command. It fixes the blinking

In this patch, I changed the way the table was initialized, and added the facetColumn creation along with the common columns creation.

The dirty mode of the EMF Facet table editor has been modified to check whether we are working with a non serialized resource (to enable the save action), and once serialized, to use the editingDomain.

This correction introduced a regression (infinite undo/redo loop) because there were still adapters on column/row which performed unnecessary commands.

Last but not least, I changed the way the table resource is created. In fact while saving, we used to create a new resource with the user new Uri, add the table instance to its content, and then serialize this resource.
This way of serializing was creating a new resource while the tableInstance had already one, and moreover we were losing all the listeners/adapters/trackingModeOption that we previously set on the tableInstance.

In this patch, I only set the new Uri to the tableInstance containing resource, and serialize it.

(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 2 Gregoire Dupe 2011-05-20 10:44:10 EDT

Hello,

This bug is not blocking or critical. In conformance with the ramp down policy
[1], the commit of its fixing will then be delayed to version 0.1.1 (Indigo
SR1).

[1] http://wiki.eclipse.org/Modeling_Project_Ramp_Down_Policy

Regards,
Gregoire Dupe

Comment 3 Nicolas Guyomar 2011-05-25 05:39:00 EDT

Created attachment 196523 [details]
Patch for Bug 344921

Hi,

I forgot to include plug-in org.eclipse.emf.facet.widgets.nattable.workbench in my previous patch.

Please find attached an updated version of the patch.

(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 4 Nicolas Guyomar 2011-05-25 07:48:23 EDT

Created attachment 196543 [details]
Patch for Bug 344921 Non regression test

Hi,

Please find attached a non regression test 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 5 Gregoire Dupe 2011-06-10 13:16:16 EDT

Comment on attachment 196523 [details]
Patch for Bug 344921

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 720.

Comment 6 Gregoire Dupe 2011-06-10 13:17:41 EDT

Comment on attachment 196543 [details]
Patch for Bug 344921 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 720.

I’ve removed in useless try-catch before to commit the patch.

Comment 7 Gregoire Dupe 2011-06-10 13:31:48 EDT

I think that the try-catch in the method org.eclipse.emf.facet.widgets.nattable.NatTableWidgetUtils.createColumns(TableInstance) is useless. Here is to my mind a better to write this method. 
Index: NatTableWidgetUtils.java
===================================================================
--- NatTableWidgetUtils.java	(revision 720)
+++ NatTableWidgetUtils.java	(working copy)
@@ -285,15 +285,12 @@
 						0,
 						"An exception has occured while retrieving structural features of the eObjects", //$NON-NLS-1$
 						new Exception());
-				try {
-					throw new CoreException(globalStatus);
-				} catch (CoreException e) {
-					Logger.logError(e, Activator.getDefault());
-					MessageDialog.openError(Display.getCurrent().getActiveShell(),
-							"Failed to load facets", //$NON-NLS-1$
-							"Some facets failed to load. See error to have more details."); //$NON-NLS-1$
-					// TODO Those string have to externalized
-				}
+				CoreException e = new CoreException(globalStatus);
+				Logger.logError(e, Activator.getDefault());
+				MessageDialog.openError(Display.getCurrent().getActiveShell(),
+						"Failed to load facets", //$NON-NLS-1$
+						"Some facets failed to load. See error to have more details."); //$NON-NLS-1$
+				// TODO Those string have to externalized
 			}
 			for (EAttribute eAttribute : eAttributes) {
 				FacetAttributeColumn facetAttributeColumn = TableinstanceFactory.eINSTANCE


It would be great to check this behavior (the user has to be notified if something is wrong with a facet) with a unit test.

I’ll wait to answer to this question before to mark this bug as fixed.

Comment 8 Gregoire Dupe 2011-06-10 13:36:26 EDT

The method org.eclipse.emf.facet.widgets.nattable.workbench.internal.editor.NatTableEditor.isDirty() uses the internal class org.eclipse.emf.facet.widgets.nattable.internal.NatTableWidget.
This is not a good practice because integrators will not be able to implement them own editor properly. We then have to find an architecture fix. This has also to be solved before to mark this bug as fixed.

Comment 9 Gregoire Dupe 2011-06-20 07:38:16 EDT

Created attachment 198251 [details]
patch to fix the architecture problem described in comment #8

Here is a patch to fix the architecture problem described in comment #8.

(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 10 Gregoire Dupe 2011-06-20 08:17:26 EDT

Comment on attachment 198251 [details]
patch to fix the architecture problem described in comment #8

To be able to deal with events, the class NatTableWidget needs a TableInstance contained in a resource. I’m not so sure that the class NatTableWidget has to instantiate a temporary resource. The NatTableWidget may have to require a TableInstance contained in a resource.
But if we change that this will be an API break, which is not possible in this version. I’ve then committed this patch.

I’ve open the bug 349797 to add the method NatTableWidget.usesTmpResource() in the API. The question of the use of a temporary resource has to solved in the bug 349797.

I’ve then committed this patch.

Committed revision 724.

Comment 11 Gregoire Dupe 2011-06-21 12:41:07 EDT

(In reply to comment #7)
> I think that the try-catch in the method
> org.eclipse.emf.facet.widgets.nattable.NatTableWidgetUtils.createColumns(TableInstance)
> is useless. 

I've fixed that.

> It would be great to check this behavior (the user has to be notified if
> something is wrong with a facet) with a unit test.

Before to mark this bug as fixed, we have to provide this unit test.

Comment 12 Gregoire Dupe 2011-07-20 10:52:13 EDT

Unit tests are missing to close this bug. => P1

Comment 13 Gregoire Dupe 2012-05-23 12:24:20 EDT

This bug has to be closed for the IP log.