Community
Participate
Working Groups
In order to reproduce this bug, I use the example : Open Ecore Tabular Editor Step to reproduce : 1/Use the action Open Ecore Tabular Editor on a EPackage 2/Do Undo : the facets columns facet disappears, but they are still loaded! 3/Do Undo : something is done, because the scrollbars blink 4/You can still do 8 Undo, before the Undo Action will be disabled 5/You need to do 10 Redo to get the "initial" state 2/I think there is 2 problems here : - The facets columns are destroyed, but the facets are still applied - Undo Action should be disabled just after the creation of the table, or it could be only one Undo, to empty the table.
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
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
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
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 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 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.
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.
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.
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 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.
(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.
Unit tests are missing to close this bug. => P1
This bug has to be closed for the IP log.
