Community
Participate
Working Groups
Current implementation of org.eclipse.compare.CompareEditorInput and org.eclipse.compare.contentmergeviewer.ContentMergeViewer blocks further improvements. There is inconsistency in dealing with editing two files in compare editor and dirty states of those files. Currently when saving the compare editor on closing the editor both sides are saved. When we improve it to save only selected side on closing, after saving only one side, dirty flag of the ContentMergeViewer is set to false and the other side will not be saved - we lose changed content of the other side.
(In reply to comment #0) > Currently when saving the compare editor on > closing the editor both sides are saved. When we improve it to save only > selected side on closing, after saving only one side, dirty flag of the > ContentMergeViewer is set to false and the other side will not be saved - we > lose changed content of the other side. This has surfaced while working on bug 273450 (see patch attached to bug 273450, comment 11). It's caused by the update[1] that takes place when viewers are flushed. It resets the dirty flags for both sides, even when only one of them is being saved. So far I can see two possible solutions for that: 1) Make the update respect the dirty flag. 2) Update selectively, refreshing only the part that has been saved. [1] org.eclipse.compare.contentmergeviewer.ContentMergeViewer.updateContent(Object, Object, Object)
Created attachment 192276 [details] Fix setting LocalResourceSaveableComparison dirty. This patch works only when patch attached to bug 340666 is applied. SavablesCompareEditorInput has references to two savables representing left and right side of the comparison. It keeps dirty flag for each of them separately, but both savables are notified about dirty property changed each time any site has changed. This results in false dirty status changes, when both savables are dirty and then: a) one of the savables is saved (the main subject of this bug) b) one of the savables dirty flag is changed to "false", for instance by "undo" (ctrl+Z) This fix makes savables conscious if the "change dirty" notification applies to them or to the other savable. Then SavablesCompareEditorInput sets its dirty state depending on witch savable has changed.
Comment on attachment 192276 [details] Fix setting LocalResourceSaveableComparison dirty. Unfortunately after adding this patch the bug can be still reproduced. But it gets much better if fix for bug 340666 is applied.
(In reply to comment #3) > But it gets much better if fix for bug 340666 is applied. What cases does the fix not cover? Writing them down sounds like a good idea in case we go back to this bug in the future.
It's type-dependent: html, plain text, xml files still get the other site reset; java, properties, manifest files don't. Generally when StructureDiffViewer applies to this compare saving works fine. From short debugging I discovered that while setting up the compare TextFileDocumentProvider.connect is called twice for each site when we deal with StructureDiffViewer and this prevents the right site from resetting later. For other DiffViewers connect is called only once for each site. Each connect/disconnect increases/decreases some counter on file info. The actual reset is made when after saving one site we call SaveablesCompareEditorInput$InternalResourceSaveableComparison.fireInputChange, after saving the left site. It does a pair of disconnect/connect on each site. If a counter on file info goes to 1 this operation resets the file in view.
Moving this bug to 3.8 because it blocks another 3.8 bug.
*** Bug 360429 has been marked as a duplicate of this bug. ***
Created attachment 205988 [details] Fixing refreshing in ContentMergeViewer I found the problem. When the first file is saved CompareInputChange is propagated, ContenMergeViewer refreshes on input change, this resets the second side. ContentMesgeViewer#handleCompareInputChange already had some logic based on dirty state of both sides and isSaving (true if we are in the middle of saving), so I added another condition: if we are in the middle of saving and one of the sides still wasn't saved than we don't refresh, we'll do it when second side is saved. It works for both: when StructureDiffViewer is used and when it's not, before that comparisons with StructureDiffViewer where resistant to this problem, see comment 5.
I suppose I should add some tests.
(In reply to comment #9) > I suppose I should add some tests. Tests are always welcome with open arms :)
Created attachment 206199 [details] Fixing refreshing ContentMergeViewer with test I added tests that verify saving both sides of Compare Editor for various file types. As said in comment 5 the problem didn't occur for some file types so some of the tests don't fail without the fix, but of course we should test them too. I added separate test for each file type I considered as relevant, because if one of them fails we still know status of the others.
Created attachment 206200 [details] mylyn/context/zip
Fixed with 1bbe39d33b1ed5e57003e7e16b483d62633463f2. Available in builds >= N20111123-2000.
