Bug 273450

Comment 1 Szymon Brandys 2009-04-23 11:36:06 EDT

It's an issue with the label. Actually you see two different saveables but labels are wrong. Another issue is that you should not see two saveables, when only one file (A or B) is modified. Anyway data is not lost.

I'll take a look on that.

Comment 2 Szymon Brandys 2009-08-26 17:16:48 EDT

*** Bug 287734 has been marked as a duplicate of this bug. ***

Comment 3 Tomasz Zarna 2010-03-01 10:43:01 EST

*** Bug 304192 has been marked as a duplicate of this bug. ***

Comment 4 Szymon Ptaszkiewicz 2010-03-09 07:11:48 EST

Created attachment 161445 [details]
Patch proposal

This is a proposal of the patch. Few comments in code where added to clarify changes.

Comment 5 Szymon Ptaszkiewicz 2010-03-10 04:43:08 EST

Created attachment 161585 [details]
Patch to version 3.5 proposal

Patch to version 3.5

Comment 6 Tomasz Zarna 2010-05-07 11:06:28 EDT

(In reply to comment #4)

Thanks for the patch Szymon. It's not easy to get your head around this kind of problems, so you did a good job providing a nice patch like this. However, I do have few comments:
* Does SaveablesCompareEditorInput.isSaveNeeded need to be public?
* What about changing 'arg' param in that method to 'saveable'?
* Could you update copyrights where necessary?
* I don't think you actually need lists to keep dirty viewers in CompareEditorInput. AFAIK there's going to be at most 1 dirty viewer on left and right.
* Increasing access in API methods (i.e. ContentMergeViewer.isRightDirty) doesn't break binary compatibility but I'm not sure whether we need to update the @since tag.
* And last but not least: When you change both sides of compare editor, close it and in the dialog select only one of the files, still *both* will be saved. I quickly looked at the code and a fix for that doesn't seem to be trivial. Saving content of compare editor flushes all viewers and both sides of the comparison no matter what has been selected in the dialog.

Comment 7 Szymon Ptaszkiewicz 2010-05-12 09:18:00 EDT

Created attachment 168134 [details]
Patch with tips suggested by Tomasz

Patch with tips suggested by Tomasz. There is still a problem how to deal with dirty flag and fireDirtyState method. For now, after saving only one side, dialog will appear claiming that the other file was changed outside compare editor. 'No' gives the corrent result, however the dialog shouldn't appear at all. Tomasz could you take a look how to improve it?

Comment 8 Tomasz Zarna 2010-05-12 09:34:40 EDT

(In reply to comment #7)

> (...) dialog will appear claiming that the other file was changed 
> outside compare editor. (...)

Have you been able to investigate what event causes the dialog to show up?

Comment 9 Tomasz Zarna 2010-05-12 11:44:55 EDT

(In reply to comment #7)
> There is still a problem how to deal with
> dirty flag and fireDirtyState method.

By that, did you mean the fact that when editing right side of the compare editor and having the file open in a separate editor at the same time, the other editor doesn't get dirty? This is what I have observed playing around with your patch.

Comment 10 Tomasz Zarna 2010-05-13 07:37:36 EDT

(In reply to comment #8)
> > (...) dialog will appear claiming that the other file was changed
> > outside compare editor. (...)
> 
> Have you been able to investigate what event causes the dialog to show up?

Let me answer my own question: when saving the compare editor with the Save action (Ctrl + S) an execution listener[1] added in ContentMergeViewer sets ContentMergeViewer.fIsSaving to true. This trick allows us to ignore a compare input change normally handled in ContentMergeViewer.handleCompareInputChange() where the dialog is produced. We don't set the flag when saving dirty models as the result of closing the editor and that's why the dialog pops up.

[1] org.eclipse.compare.contentmergeviewer.ContentMergeViewer.getExecutionListener()

Comment 11 Tomasz Zarna 2010-05-14 09:36:37 EDT

Created attachment 168536 [details]
Patch v03

Here is a patch that fixes the issue mentioned in comment 10. ContentMergeViewer.handleCompareInputChange() checks now if any of the saveable is being saved, if this is true the dialog is not displayed.

Other things changed in the patch:
* moved IFlushable2 to an internal packed and replaced single flush method with two methods for each side (ie flushLeft and flushRight)
* added ISavingStateProvider interface to access saving state of a saveable 
* removed the execution listener from ContentMergeViewer. The trick is no longer needed as we are now able to consult ISavingStateProvider

What still needs to be done:
* make TextMergeViewerTest tests pass
* more manual testing
* clean comments and javadocs

Comment 12 Tomasz Zarna 2010-05-14 09:36:45 EDT

Created attachment 168537 [details]
mylyn/context/zip

Comment 13 Tomasz Zarna 2010-05-18 07:06:43 EDT

Created attachment 168897 [details]
Patch v04

Fixed failing tests in TextMergeViewerTest class.

TODOs:
* more extensive manual testing
* add/clean comments and javadocs
* add @since tags if necessary
* simplify the code where possible (Szymon)

Given the current shape of the fix and its possible impact I would defer it for 3.7.

Comment 14 Dani Megert 2010-05-18 12:40:00 EDT

>Given the current shape of the fix and its possible impact I would defer it for
>3.7.
I agree.

Comment 15 Szymon Ptaszkiewicz 2010-05-19 04:40:02 EDT

(In reply to comment #13)
> Created an attachment (id=168897) [details]
> Patch v04
> 
> Fixed failing tests in TextMergeViewerTest class.
> 
> TODOs:
> * more extensive manual testing

I did some manual testing and all scenarios that came to my mind are ok with this patch.

Comment 16 Tomasz Zarna 2010-07-28 14:41:19 EDT

I've just started cleaning up the patch and I found one thing that confuses me a bit. In the patch, we change ContentMergeViewer.isRightDirty() and ContentMergeViewer.isLeftDirty() methods access from protected to public. API Tooling claims that at the same time we should increment @since tags, is this expected? Both of the methods are already API, we just make the scope broader. There is no way to mark these methods as available to subclasses but hidden to other clients, isn't it?

Comment 17 Dani Megert 2010-07-29 07:32:54 EDT

>API
>Tooling claims that at the same time we should increment @since tags, is this
>expected? Both of the methods are already API, we just make the scope broader.
Yes, this is expected. The public API gets added in 3.6 hence the tag must be "@since 3.6". You can mention that it was protected before though:
@since 3.6 public, before it was protected

Comment 18 Tomasz Zarna 2010-08-02 11:40:21 EDT

Darin, is there a way to mark a method which has been made public (was protected before) so *only* subclasses can refer to it? In other words, can I use @noreference tag but at the same time keep it visible to subclasses like it has been so far?

This kind of questions make me wonder if the solution in the patch is the best we can do. Of course, I'm aware that compare API is far from perfect and our options are limited here, but still... For instance, we could add two public methods in ContentMergeViewer that would simply wrap the protected ones, and mark them with @noreference tags. However, I'm not saying this would make the code better.

Comment 19 Dani Megert 2010-08-03 02:05:07 EDT

(In reply to comment #18)
> Darin, is there a way to mark a method which has been made public (was
> protected before) so *only* subclasses can refer to it? In other words, can I
> use @noreference tag but at the same time keep it visible to subclasses like it
> has been so far?
No.

>For instance, we could add two public
>methods in ContentMergeViewer that would simply wrap the protected ones, and
>mark them with @noreference tags. However,
Yes, what you can do is to add a new method like this:
	 /**
	 * @since 3.7
	 * @noreference
	 */
	public boolean internalIsRightDirty() {
		return fIsRightDirty;
	 }

Comment 20 Darin Wright 2010-08-03 09:38:05 EDT

Agreed, using @noreference is the best you can do. However, you will still get warnings/errors when using a @noreference method from other bundles (even if they are friends).

Comment 21 Tomasz Zarna 2010-08-04 06:15:29 EDT

Thanks guys. SzymonB, I'll take this one and do a final review of the patch in early 3.7M2.

Comment 22 Tomasz Zarna 2010-08-10 06:21:39 EDT

Created attachment 176219 [details]
Patch v05

Minor changes in the patch:
* added public internalIsRightDirty and internalIsLeftDirty methods to ContentMergeViewer. Both marked with @noreference tag - no new API in ContentMergeViewer
* access qualifier of ContentMergeViewer(TextMergeViewer)#flushLeftSide(Object, IProgressMonitor) and #flushLeftSide set to default (package) - no need to add @noreference tag
* renamed internal ISavingStateProvider to ISavingSaveable
* added javadocs to ISavingSaveable and IFlushable2
* adjusted comments in SaveablesCompareEditorInput and LocalResourceSaveableComparison
* updated copyrights in TextMergeViewerTest

Comment 23 Tomasz Zarna 2010-08-10 06:24:00 EDT

The latest patch has been applied to HEAD. Available in builds >=I20100810-0800.

Comment 24 Dani Megert 2010-08-13 05:51:32 EDT

Verified in N20100812-2000.

Comment 25 Dani Megert 2011-03-24 09:15:43 EDT

The fix is bad: it introduces bug 340666!