Bugzilla – Full Text Bug Listing
| Summary: | [EditorMgmt] Closing compare editor asks to save editor - Saveable should implement IAdaptable | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Martin Aeschlimann <martinae> | ||||||
| Component: | Compare | Assignee: | Michael Valenta <Michael.Valenta> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | bokowski, daniel_megert, Michael.Valenta, snorthov | ||||||
| Version: | 3.2 | ||||||||
| Target Milestone: | 3.3 M5 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Bug Depends on: | 131068 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Martin Aeschlimann
The Compare editor is just behaving like any other editor. That is, if I open a Java editor and a Text editor, dirty one and then close one, I get prompted to save the editor. Moving to Platform/Text for comment. The problem regarding textual editors (i.e. inheriting from AbstractTextEditor) is covered by bug 131068 and planned to be resolved for 3.3 via ISavablesSource. Since the compare and the textual editors won't use the same savables the problem will persist. A solution might be that text makes its savable class public but I didn't yet look to close at that ISavablesSource to tell whether that will work out. Michael and I discussed this today and we found a common ground which allows us to test our two Saveables for equality. To achieve this we need the Saveables to be adaptable (i.e. inherit from IAdaptable). Just discussed this idea with Boris and hence moving to him to implement this ;-) Saveable now implements IAdaptable. This is in I20061219-0800. However, when implementing equals() and hashCode() based on this, keep the following in mind (copied from Saveable.equals()): * Two * saveables should be equal if their dirty state is shared, and saving one * will save the other. If two saveables are equal, their names, tooltips, * and images should be the same because only one of them will be shown when * prompting the user to save. Reopening to do the remaining work. Fixed the TextEditorSavable to use the editor input in equals: ... editorInput=(IEditorInput)((Saveable)obj).getAdapter(IEditorInput.class); return fEditorInput.equals(editorInput); Availabel in builds > N20061219-0010 (excl. this weeks I-build). Moving to Compare for the last bit. I am a little worried that just using the editor input might be unsafe. Depending on whether the other Saveable uses the same underlying buffer or not is what should be used in equals. Good catch. Fixed by using the document and returning the shared document in the adapter (IDocument.class). Created attachment 56409 [details]
Patch that implements equals and hashCode on Compare's Saveable
I've attempted to implement this in Compare but I am getting some bad behavior from the Workbench. I can easily get into a state where I have a single dirty Compare editor open but when I close it, I get prompted saying that the model is referenced else where. Through debugging, I determined that the reference counts are not being decremented properly but I haven't been able to pinpoint the cause. This may come from my implementation of hashCode which is technically not correct because it returns a different values when it doesn't have access to the document. Regardless of what the problem is, implementing this has made me think that using equals to make two different Saveables equal is a mistake. I think we need to come up with another way to do this. One possibility would be to have a method of Saveable for doing the test. We could then have a convention at the editor level that subclasses of Saveable should adapt the Saveable being compared to the underlying document. Thoughts? If we do this I guess we have to make it backwards compatible since other now make use of the equals/hashCode approach. The problem is indeed my implementation of equals/hashCode. The problem is that, at the time the saveable is created (and used by the workbench), I don't know whether the merge viewer will use the file buffer or not. This means that I should delay the creation of the Saveable until I know whether the viewer uses the buffer or not (or change the saveable from one that doesn't to one that does once I know the buffer will be used). This may be complicated since I am going through 3 or 4 levels of indirection but I'll have a look. Dani, is there any possibility that the document for the text editor will ever change? For example, if the file changes in the file system, does the editor obtain another document? If so, this may cause problems since the hashCode of the TextEditorSaveable uses the document and hashCodes should never change. Created attachment 56473 [details] Updated patch This updated patch now works since it implements hashCode and equals in a non-changing way. However, I found a bug in the TextEditorSaveable which causes the wrong prompt to appear so I don;t want to release this until the bug is fixed. The bug number is bug 169707. Fix released to HEAD. Should work for both the model-based and non-model-based Synchronizations. Verified using I20070206-0010 |