Community
Participate
Working Groups
Hi, I am working on comparing XML files using a custom compare editor. I have encountered numerous problems regarding the Compare API. I had issues with ContentMergeViewer, for example. I couldn't use the createCenter method to draw lines between my elements. TextMergeViewer inherits from ContentMergeViewer and uses package visibility to extend those methods. I will provide a patch very soon to correct the API. As this is my first contribution, I will appreciate any help reviewing my code ! I will be as less intrusive as possible.
Created attachment 43086 [details] First patch First patch on ContentMergeViewer and TextMergeViewer. Replaces some package methods to protected visibility to ensure extensibility.
Created attachment 43092 [details] Invasive patch on CompareEditorInput Additionaly to the previous patch, this one contains AbstractCompareEditorInput. It is a new superclass for CompareEditorInput. CompareUI and CompareUIPlugin use AbstractCompareEditorInput for lauching the dialog box. Graphical methods like createContents are set abstract. My goal is to enable custom editor creation by extending AbstractCompareEditorIn put, to use other graphical objects other than CompareViewerSwitchingPane (TabFolder for example).
I'm not sure why you made this dependant on bug 120240. If anything it would be the other way around. However, we were able to work with the current viewer superclass so we closed by 120240.
(In reply to comment #3) I found your bug interesting because I faced the same difficulties. We were in the same case, trying to extend ContentMergeViewer and CompareEditorInput. I used the last CVS version to write my patches. I don't see more extensibility in ContentMergeviewer's last version. That's why I was a bit surprised to see your bug marked as resolved. Are any changes to this class considered ? Is this written somewhere ?
I marked that bug as resolved because I was able to use the ContentMergeeViewer to create an example model-based comparison without modifying it. That is not to say that we shouldn't investigate changes to the API. The problem with my case was that it was only an example and only a real scenario is justification for adding API. I think the point you make in your original comment is valid. If TestMergeViewer overrides methods in ContentMergeViewer, then these should probably be made API or an alternate means to extend the behavior should be provided.
Thank you for your quick answer. I reviewed my first patch and foudn a couple of functions I wasn't implementing, yet TextMergeViewer was extending using package visibility. I am sorry if some people have already reviewed my first patch - I think this one is complete. I propose it for review by the compare team.
Created attachment 43347 [details] First patch - completed with all extended TextMergeViewers functions' visibility set to protected
Adding Obeo developers as they will need to be updated on the bug status, since they are going to work on the EMF Compare Project.
Is this "EMF Compare Project" an Eclipse project? I haven't heard of it before and it is not mentioned on the on the Eclipse Modeling of EMF websites.
We are currently working on such a project to be proposed as part of EMF. See this post on the EMf newsgroup for more information, and please do not hesitate to contact me or Cedric for more details.
I have started to look at these patches and here are my comments: 1) I have added a protected handleSetFocus to ContentMergeViewer to allow subclasses to handle the setFocus request. 2) I don't see any reason to make the propertyChange method API as subclasses can register their own listeners. 3) I agree that it would be nice for the ContentMergeViewer to provide as API all the facilities used by TextMergeViewer. However, the 3 center related changes you proposed are just the tip of the iceburg. Are you sure that the 3 propsed changes are all you would need or are you just speculating (i.e. do you have a working prototype built on top of these changes or are you waiting for the API to be provided before you start)? 4) In an earlier patch you proposed the addition of an AbstractCompareEditorInput. Unfortunately, your patch would break binary compatibility. To prevent this, we would need to duplicate all the openIn methods on CompareUI. I would prefer not to do this. Can yu give me more details about why you need to generalize CompareEdiorInput? In it just for the DiffViewer (i.e. structure input) pane that you need an alternate for the CompareViewerSwitchingPane? Have have released to fix menetioned in point 1 to HEAD and await further input before addressing the other points.
The EMF Compare Utility is using a modified ContentMergeViewer where all default methods have been replaced by protected methods. So, yes, I use this in a prototype. On #2 : I have to check that in detail, it seems that a weird call was made durig the initialization of the panels. The property manager was there used to register something, I'd need to check that in detail. On #4 : I did the same as ContentMergeViewer in the end, ie I changed default methods to protected methods. I need to reimplement the CompareEditorInput class because I wanted to introduce an other behaviour on the top pane. For now, the top pane represents the structure of the compared files, and you zoom on a difference when you double-click on one of its elements. In the EMF Compare Utility case, I represent the two models structures, and the top pane is reserved to show the differences between them. The double-click listener which was implemented needed to be changed. I can file a new patch with the last state-of-the-art classes. They haven't been touched for a while, so I think they are quite stable.
Yes, it would be good if you provided the latest patch so we know we are providing all the API that is required. As for the CompareEditorInput, that is not something the EMF compare would access to. To put it another way, there are two parties involved in the Compare: the provider of the content (e.g. a repository provider like CVS) and the providing of the compare viewers (e.g. EMF Compare). As such, the EMF compare would really only be able to control the later funtionality while only the providers of the content would have any influence over the CompareEditorInput used. If you have particular workflow requirements that you would like to see integrated into the CompareEditorInput, I would suggest you log a separate enhancement request that describes your screnario. We can then investigate adding support for it to the existing CompareEditorInput class.
I have released changes to ContentMergeViewer that surface all the funtionality that is required by TextMergeViewer as API (including the propertChange listener). Please has a look at the code once M3 ships (or before) to ensure that it meets your needs.
