Bug 351593

Comment 1 Laurent Goubet 2011-07-19 09:17:30 EDT

Created attachment 199899 [details]
example models

Thanks Vincent, we've been able to reproduce this with the attached models. The fix for this issue is a little tricky though (or should I say, the scope of this fix is bigger than just UML), and we'll need to create the unit tests for both the generic implementation and the UML use case.

For the record, here is the generic use case that reflects this UML issue and needs to be fixed along :

---------------
With difference D1 : adding an element to the model
and difference D2 : referencing this added element

 - If I apply D1, then D2 stays to be applied manually by the user : the model is not corrupt.
 - If I apply D2, then D1 has to be applied automatically in order not to corrupt the model.
 - If I undo D1, then D2 has to be undone automatically as it would otherwise reference a removed element.
 - If I undo D2, D1 stays to be undone or applied manually.

This use case is already handled in a suboptimal way and could use the same "generic" fix as the UML use case.
---------------

Comment 2 Mikaël Barbero 2011-09-08 10:43:34 EDT

Created attachment 203002 [details]
the generic patch

Comment 3 Mikaël Barbero 2011-09-08 10:45:01 EDT

Created attachment 203003 [details]
the uml2 patch (bigger 'cause of unit tests updates)

Comment 4 Mikaël Barbero 2011-09-08 10:45:37 EDT

Please find attached 2 patches repectively on generic and UML2 parts of EMFCompare. 

These patches add a double sided reference on DiffElement to modelize the requirement to merge the target before the current one.

It also adds a bunch of unit tests and updates to existing ones, for generics and UML2 parts (still few failures, due to previously reported bugs, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=355897 && https://bugs.eclipse.org/bugs/show_bug.cgi?id=355876)

Mika

Comment 5 Mikaël Barbero 2011-09-12 04:39:57 EDT

Created attachment 203134 [details]
the new icons not added in previous patch

Comment 6 Laurent Goubet 2011-09-12 05:05:33 EDT

Thanks Mikael, these two patches and their icons have been pushed on master, they'll be available in the next 1.3.0 builds.

Comment 7 Vincent Lorenzo 2011-09-21 03:45:35 EDT

I reopen the bug, because I always have the problem. I work with EMF Compare	1.3.0.v20110912-0936.
I tested with the example (attachment 199899 [details]). 
I compared the 2 files : so there is 2 changes (the file with new elements is to the right)
	- Standard profile has been unapplied on <Model> myModel
	- Stereotype <Stereotype> Metaclass has been been unapplied on <Class> myClass
	
When I select the Stereotype in Structural Difference and I click on "Copy current change from right to left" : 
	1/ The stereotype is not applied on the left model
	2/ The profile application doesn't come with the stereotype application

Comment 8 Cedric Notot 2011-09-26 06:03:26 EDT

This bug is related to 357916.

It was fixed but the corresponding patch has not been integrated yet on eclipse trunk.

Please find this attached patch here:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=357916

Comment 9 Laurent Goubet 2011-09-27 10:30:48 EDT

The linked bug has now been fixed on master. Could you confirm whether it truly fixes the issue?

Comment 10 Vincent Lorenzo 2011-09-28 10:27:48 EDT

I Laurent, 
  I'm sorry, but the bug is always here. 
  I tested with your new build 1.3.0.v20110926-1257 and downloading source code on your git http://git.eclipse.org/c/emfcompare/org.eclipse.emf.compare.git

Best regards, 
/Vincent

Comment 11 Laurent Goubet 2011-09-28 11:06:46 EDT

Vincent,

Playing a little bit more with your sample models, seems like you're right : we did fix the issue with some changes and directions, but we're missing others. Namely, if we merge the application of a stereotype, we properly merge the requires profile application. However in the other direction, if we undo a stereotype "un-"application, then we do not properly undo the "un-"application of the profile.

Cedric, could you check why merges in this direction are not handled?

Comment 12 Cedric Notot 2011-09-29 08:53:27 EDT

I have just fixed the issue.
The direction was not completely and correctly managed for this specific use case in UML2. I have also added the related tests.

Comment 13 Cedric Notot 2011-09-29 08:55:35 EDT

Created attachment 204286 [details]
Fix

Comment 14 Cedric Notot 2011-09-29 08:57:11 EDT

You can find the path (Fix) in attachments.

Comment 15 Laurent Goubet 2011-09-29 09:53:17 EDT

Cedric,

This last patch does fix part of the remaining issues... but not all. merging the "profile unapplied" should in turn merge the "stereotype unapplied" (if I remove the profile, the stereotype should be removed). It is not the case with this patch.

Comment 16 Laurent Goubet 2011-09-29 10:56:00 EDT

My mistake, it is the UI that does not refresh itself when we unapply a stereotype, yet the model is properly updated.

Vincent, a nightly build just finished, it can be installed through the update site https://hudson.eclipse.org/hudson/view/Modeling/job/emf-compare-master/lastSuccessfulBuild/artifact/EMF.Compare.p2.repository/ . Could you confirm whether this fixes the issue for you?

Comment 17 Vincent Lorenzo 2011-09-30 09:23:25 EDT

Created attachment 204377 [details]
a new example with a bug

Hi Laurent, 
 This new version works fine with the very simple example. 
 Unfortunately, I found another example(see attachment), in which the StereotypeApplication merge doesn't worked correctly! (maybe I should create a new bug, because in this case the ProfileApplication is correctly merged...)

Steps to reproduce : 
	1/ Compare the 2 uml files
	2/ Try to merge the Model MyNiceModel
		-> The Stereotype Application is not correctly merged



 Best Regards, 
 /Vincent Lorenzo

Comment 18 Cedric Notot 2011-10-05 06:04:28 EDT

I analyzed the problem and I have just found a solution to fix it.

The stereotype applications was not in the UML2 match scope. These ones are located on the root of the model and reference the UML Elements on which it is required to apply a stereotype.

So, when you tried to copy an UML element which contains sub elements "decorated" by a stereotype application, these applications were not copied because there was no related difference. And, the EMF Comapre core is able to copy any "out-going" references but it can not guess to copy other things. This is a specific UML use case.

I added the stereotype applications in the match scope and I created an difference extension for each UML model element change. This extension will own a dependency to potential stereotype application change extensions (and it will hide them) which are related to embedded UML elements. So, when you will merge (copy or delete) this model element change extension, the concerned application stereotype changes will be merged too.

I have to launch UML2 JUnit tests to check if there are regressions or not, and update these automatic tests (data set). Then, I will contribute the patch to fix it.

Comment 19 Cedric Notot 2011-10-05 13:04:34 EDT

Created attachment 204612 [details]
Patch to fix the issue on the merge of a model element change with embedded stereotypes

Please, find in attachments the patch to fix the issue about merging an UML model element change on an UML element which contains a stereotype application at least on its children.

The patch also contains an update of the non regression JUnit tests.

Comment 20 Cedric Notot 2011-10-05 13:07:33 EDT

Created attachment 204613 [details]
Patch to fix the issue on the merge of a model element change with embedded stereotypes (part 2)

The patch is too big. So, I cut it into 2 zip files.

Comment 21 Cedric Notot 2011-10-06 11:42:42 EDT

Created attachment 204680 [details]
Missing icons

2 icons are missing in the patch.
Find them in attachments.

Comment 22 Cedric Brun 2011-10-07 12:23:51 EDT

Latest patches have been integrated in the trunk.

Comment 23 Cedric Notot 2011-10-13 11:39:38 EDT

Created attachment 205141 [details]
Fix for regression

Patch in attachments to fix a regression about DiffGroup creations and a oversight about checkstyle constraints.

Comment 24 Cedric Brun 2011-10-13 11:47:19 EDT

commited and pushed in 34878a6a2667acd2cdd1ae9b915da0a4b147d2bb

Comment 25 Laurent Goubet 2013-01-17 08:31:57 EST

batch-closing a bunch of "RESOLVED" bugs.