Bug 355953

Comment 1 Nicolas Guyomar 2011-08-29 04:09:21 EDT

If it can help, the action was still working in 0.1.1.v201106171452

Nicolas

Comment 2 Nicolas Bros 2011-08-30 03:49:20 EDT

Created attachment 202389 [details]
patch

Here is a patch that avoids trying to remove an empty list of rows.

Comment 3 Nicolas Bros 2011-08-30 05:38:41 EDT

Created attachment 202395 [details]
unit test patch

Here's a Junit test.

Comment 4 Gregoire Dupe 2011-09-01 04:54:28 EDT

Comment on attachment 202395 [details]
unit test patch

Please, do not modify or extends existing unit test because:
 - a unit test guaranties the non regression. 
 - unit test source code (and resources) must be independent between to bug fix.

For instance, you should extract the expected common lines in a utility class (org.eclipse.emf.facet.widgets.nattable.tests.internal.TableTestUtils). 

Do not forget to update the root test class (AllUITests).

Please, do not use @SuppressWarnings("null"), unit test must respect the project coding rules.

Comment 5 Gregoire Dupe 2011-09-01 04:55:54 EDT

Comment on attachment 202389 [details]
patch

I disagree with this patch because the trace message will be skipped when rowsToRemove is empty. Furthermore the use of return in the middle of a method may cause maintenance/reading problems.  To my mind, an "if" block surrounding the try/catch and of the last statement would be better.

		if (!rowsToRemove.isEmpty()) {
			try {
				CompoundCommand [...]
			} catch [...]
			}
			this.editingDomain.getCommandStack().execute(compoundCommand);
		}

Comment 6 Nicolas Bros 2011-09-02 10:19:36 EDT

Created attachment 202675 [details]
unit test patch 2

New version of the patch. This patch doesn't seem to have "test.uml". I tried making the patch several times but it wouldn't add it.
So, it must be copied from "Bug352822.uml".

Comment 7 Nicolas Bros 2011-09-02 10:20:28 EDT

Created attachment 202676 [details]
patch

New version of the patch

Comment 8 Gregoire Dupe 2011-09-05 04:32:04 EDT

Comment on attachment 202676 [details]
patch

Ok, but please don't format the lines you didn't modify (in the debug block).

Comment 9 Nicolas Bros 2011-09-05 08:34:36 EDT

Created attachment 202748 [details]
patch

Comment 10 Nicolas Bros 2011-09-05 08:35:16 EDT

Created attachment 202749 [details]
unit test patch

Comment 11 Nicolas Bros 2011-09-05 11:09:48 EDT

Created attachment 202763 [details]
patch for branch 0_1

Comment 12 Nicolas Bros 2011-09-05 11:10:24 EDT

Created attachment 202764 [details]
unit test patch for branch 0_1

Comment 13 Gregoire Dupe 2011-09-05 12:26:18 EDT

Hello Ed,

This bug is cause by a regression caused by the fix of the bug 345730. The bug described here is not critical because a workaround is possible: we can use the table editor in place of the table view. 

But this bug is caused by an invalid EMF command in the table widget implementation and I know that this widget is used by Papyrus.

I think it would be safer to commit this patch before RC3. Do you agree?

Regards,
Gregoire

Comment 14 Nicolas Bros 2011-09-06 04:34:20 EDT

Committed on branch 0_1 in revision 811.

Comment 15 Nicolas Bros 2011-09-06 05:14:01 EDT

Committed on trunk in revision 812.

Comment 16 Gregoire Dupe 2012-05-23 08:53:29 EDT

This bug can be closed.