Bug 369382

Comment 1 Vincent Lorenzo 2012-01-23 08:17:02 EST

Done on R6922 on the branch.
Not yet merged with the trunk!

Comment 2 Alain Le Guennec 2012-02-06 06:39:00 EST

I think there is still an issue, as the order is still not quite right here.
I think the pb is in the code for AbstractSynchronizedTableTriggerListener.getAllElementsUsingFillingqueries(), which is using a HashSet to collect all the elements, instead of an ArrayList or any other collection kind that actually does guarantee order.
With a HashSet, the initial order of the elements is corrupted at the very source during collection, making all subsequent processing to respect that corrupted order pointless.
Simply replacing HashSet by ArrayList would probably fix the issue.

Comment 3 Alain Le Guennec 2012-02-06 12:21:22 EST

I confirm that replacing HashSet by ArrayList seems to fix the pb.
I attach the patch below for reference (it's a one-liner):

### Eclipse Workspace Patch 1.0
#P org.eclipse.papyrus.table.common
Index: src/org/eclipse/papyrus/table/common/listener/AbstractSynchronizedTableTriggerListener.java
===================================================================
--- src/org/eclipse/papyrus/table/common/listener/AbstractSynchronizedTableTriggerListener.java	(revision 7131)
+++ src/org/eclipse/papyrus/table/common/listener/AbstractSynchronizedTableTriggerListener.java	(working copy)
@@ -301,7 +301,7 @@
 	 *         a collection with the elements returned by the filling queries
 	 */
 	private Collection<EObject> getAllElementsUsingFillingqueries() {
-		final Collection<EObject> allElements = new HashSet<EObject>();
+		final Collection<EObject> allElements = new ArrayList<EObject>();
 		for(ModelQuery query : this.papyrusTable.getFillingQueries()) {
 			AbstractModelQuery impl = null;
 			try {

Comment 4 Vincent Lorenzo 2012-02-07 04:53:12 EST

Yes but with an ArrayList, you should add a test in order not to add the same element several time. 
I propose you to replace newHashSet byt new LinkedHashSet to preserve order.

Comment 5 Vincent Lorenzo 2012-02-07 04:53:50 EST

Could you test the previous with your example the previous solution?

Comment 6 Alain Le Guennec 2012-02-07 06:05:58 EST

(In reply to comment #4)
> Yes but with an ArrayList, you should add a test in order not to add the same
> element several time. 
> I propose you to replace newHashSet byt new LinkedHashSet to preserve order.

I thought it would be up to the query itself to guarantee uniqueness if necessary, which ought to be possible if there is only one filling query.
Indeed, if there's more than one query, a LinkedHashSet makes sense.

BTW, maybe it could be possible to optimize things a little bit for the (most frequent) case when there's only one filling query:
In that case, you could simply return the result of the (single) query directly (maybe wrapping it in a Collections.unmodifiableCollection to be safe), hence saving the duplication of a potentially big collection. What do you think?

Comment 7 Vincent Lorenzo 2012-02-07 10:02:34 EST

In R7149 : replace the HashSet by a LinkedHashSet

Comment 8 Vincent Lorenzo 2012-02-14 12:03:18 EST

I don't close the bug, because it is not merged on the trunk.

Comment 9 Vincent Lorenzo 2012-11-23 11:37:26 EST

R9464 : merge on branch 0.9.X, and on the trunk 0.10. 

This bug can be marked as colsed fixed.