Community
Participate
Working Groups
In order to address bug 193321, we need a compare editor input for a single file that supports editing on both sides. Team provides a SaveableCompareEditorInput that supports editing on the left. We either need to modify the class to support editing on both sides or create a new class (which could possibly share a super class with SaveableCompareEditorInput ) that provides this funtionality.
The first things you will want to do is create an object contribution in Team that overrides the Compare with Each Other that is provided by Compare (since the SaveableCompareEditorInput is provided by Team/UI). Have a look at the Compare with Local History action in the org.eclipse.team.ui plugin.xml to see how this is done for the local history action.
I've created a wiki page that describes a bit about SaveableCompareEditorInput and some other topics related to CompareEditorInputs. http://wiki.eclipse.org/index.php/Compare
Created attachment 71868 [details] Patch at initial stage I've overridden the Compare w/ Each Other action from Compare with an action from Team/UI. I called it org.eclipse.team.internal.ui.actions.CompareAction. The new action creates a compare editor using org.eclipse.team.internal.ui.synchronize.TwoSidesSaveableCompareEditorInput, which is a new class I introduced. It extends SaveableCompareEditorInput and supports comparing and editing on both sides of the editor. It's intended to work only for selection of 2 local files. I indicated all methods copied from other classes, so you can review the code quicker :). Stars mean that a method was slightly modified. The code is not final, especially all the "doSomethingWithLeft/Right" methods. However, I belive it's better to show it to you at this moment than wait :) I would be glad to hear any comments from you Michael.
Created attachment 71869 [details] mylar/context/zip
This is a good start. Here are some comments: - There's a copy/past error in TwoSidesSaveableCompareEditorInput#getRightResource() where is says "if (left instancof..." - You have getRightLocalElement() and getLeftLocalElement() that cast the right and left to LocalResourceTypeElement but they are only used by a method that then casts to IResourceProvider. You also have two copies of getLeftResource and getRightResource: one with a parameter and one without. - The save behavior and dirty flag don't work properly but this is because you haven't cretaed a second savable for the input. The biggest issue that remains is treating both files a separate Saveables. The SaveableCompareEditorInput is really tailored to only have a singel saveable (for now, think of a Saveable is a special wrapper around a file). For this enhancement, we need to treat each side as a Saveable (i.e. the input will need to have two saveables: one for the left and one for the right). You are going to need to generalize the SaveableCompareEditorInput so that it supports two saveables as well as one. A few places that are hardcoded to accept only one saveable: - InternalResourceSaveableComparison inner class - registerContextMenu/handleMenuAboutToShow - isDirty - findContentViewer - getActiveSaveables - contentsCreated/handleDispose I'll leave it up to you to decide whether you can generalize the current class or whether you need to create a common superclass (e.g. AbstractSaveableCompareEditorInput). Also keep in mind that any changes you make must be backwards compatible since SaveableCompareEditorInput is API.
Created attachment 72494 [details] Patch v2 Here is the patch addressing your comments Michael. Once again, I've started with subclassing SaveableCompareEditorInput, but this is just for now. When done, I will decide to one of your suggestions from the previous comment. At this moment, I'm working on making the inputChanged event to be fired only once, after both sides are saved.
One more thing, I didn't touch registerContextMenu/handleMenuAboutToShow, so please don't look for any changes in this area. I hope they are not obligatory at this stage.
The latest patch is going in the right direction. Here are some comments: - In the contentsCreated method, you assign two listeners to the same instance variable. This may cause memory to be leaked in the dispose. - In your createLeftSaveable/createRightSaveable, I think the elements could be null so you would need to accommodate this. - As you mentioned before, there is a warning prompt when saving both sides that we need to fix. I had a quick look and one problem is that the LocalResourceSaveableComparison#performSave method (for one side) calls CompareEditorInput#flushViewers which triggers the save on the other side. I would suggest we leave this issue to the end. After you address the first two issues above, I would go ahead with the refactoring you mentioned (i.e. common super class) and we can then go from there.
Created attachment 73935 [details] Patch v3 Here is a patch addressing your comments and issues mentioned above. The biggest change is the common super class (named AbstractSaveableCompareInput). I made both SaveableCompareEditorInput (which is still abstract) and TwoSidesSavebaleCompareEditorInput to extend the new abstract class. I pulled up as many methods as possible, some of them had it's access modifier changed. In order to make the number of inherited methods bigger I changed signatures of: 1) getFileElement method : first parameter ICompareInput -> ITypedElement 2) handleMenuAboutToShow method : added ITypedElement parameter 3) constructor of InternalResourceSaveableComparison : ITypedElement parameter added The last thing is an answer to your comment: > In your createLeftSaveable/createRightSaveable, I think the elements could be null so you would need to accommodate this. The null element is handled the same way as in SaveableCompareEditorInput (i.e. even when an element is null it's passed anyway). If you still think that the elements need an extra check I will add it.
I've released your patch to branch bug_193324. I changes some of the method you made protected to package private since it's not clear that clients need to access them (especially for the instance variables since I like to keep those private if at all possible). I like to be conservative when providing API. I think what we need to do now is develop a manual test plan and put it with the others in the CVS/Test plug-in. We can then use this to drive the final tweaking of the patch. If you can think of some automated tests, that woudl be good too.
Created attachment 74552 [details] Manual test proposition This is a proposition of the manul test for Compare with each other action. The patch is created based on the HEAD version of the project.
I've committed the manual tests to HEAD.
Created attachment 75050 [details] Automated test draft This is a very rough, first version of JUnit tests for the bug. I decided to show it do you guys, so can comment on it. It would be great to know if the piece of code I wrote is worth anything. Please, feel free to comment, I'm opened to any criticism.
Created attachment 77123 [details] The second part of the manual tests The patch is created based on the HEAD version of the project. According the the first part of the manual tests I should add an option to use the Common Ancestor. Basing on the previous patches and comments do you think it's the right moment?
I've released the update to the manual tests to HEAD. I have not released the automated tests yet. In answer to your question, yes, I think it would be good to add the ancestor selection logic so we can test both the 2-way and three-way cases. I think it would be good to have some automated tests for the three-way case as well.
Postponing to 3.5.
Created attachment 118372 [details] Patch v4 Added 3-way comparison (selecting an ancestor using the same looking dialog as for ResourceCompareEditorInput). Further pulling up methods from both Saveable* and Saveables* inputs to AbstractSaveable* class. There is still an issue with registering a saveable which has been already added to the SaveableList (it's a warning but it shouldn't happen). This fix improves the experience while working with the patch for bug 250633.
Created attachment 118373 [details] mylyn/context/zip
Created attachment 118899 [details] Patch v05
Created attachment 118928 [details] Patch v06 Szymon's patch with some minor changes: getTooltip/getTitle methods, comments.
Created attachment 118929 [details] mylyn/context/zip
Released to HEAD but logged bug 256805 and bug 256806. Thanks for helping me with the patch Szymon.
In Eclipse Version: 3.7.0 Build id: I20110613-1736 I could not save changes in the right and left panel compareto editor, if I edit two files (after selecting in compareto->each other dialog) CHANGING BOTH. If I change only one, this is successfull, but if I am changing both and the try to save (by clicking the multiple or the single save button) changes are ALWAYS LOST on the RIGHT side (focusing on the right does not change this). Hint: I could not find the class org/eclipse/team/internal/ui/synchronize/TwoSidesSaveableCompareEditorInput.java mentioned above in org.eclipse.team.ui_3.6.100.I20110525-0800.jar...??
(In reply to comment #23) > I could not save changes in the right and left panel compareto editor, if I edit > two files (after selecting in compareto->each other dialog) CHANGING BOTH. If I > change only one, this is successfull, but if I am changing both and the try to > save (by clicking the multiple or the single save button) changes are ALWAYS > LOST on the RIGHT side (focusing on the right does not change this). Good catch, entered bug 360429 for the issue. > Hint: I could not find the class > org/eclipse/team/internal/ui/synchronize/TwoSidesSaveableCompareEditorInput.java > mentioned above in org.eclipse.team.ui_3.6.100.I20110525-0800.jar...?? Not sure what happened to said class, I guess it has been renamed to SaveablesCompareEditorInput. This is the place where I would start looking for the culprit.
