Community
Participate
Working Groups
I found a case where the result of hashCode and equals for AbstractTextEditor#TextEditorSaveable can change. If a compare editor and a text editor are open and have equal saveables, closing the text editor will cause its document to go away. This will change the result of the equals method and cause the workbench to prompt with the wrong message and then throw an assert exception.
Created attachment 56472 [details] Patch with potential solution I have attached a patch with a potential solution. It records the document the first time it is obtained. This fix requires that the document be available the first time it is obtained and I don;t know if this is true all the time (it was in my tests). It also assumes that the document will not change while the editor is opened. If the document of the editor can change, then the editor would need to fire save lifecycle events to remove this saveable and add a new one for the new document.
This may lead to lost data. Can this be fixed for the next I build?
>This may lead to lost data. Can this be fixed for the next I build? Do you have steps? I wasn't able to reproduce it. I think I fixed the problem but since I don't have steps it is hard to be sure.
> Do you have steps? I wasn't able to reproduce it. I don't, Michael?
The steps are: 1) Open editor A on file X 2) Open editor B on file X (both editor must use file buffers) 3) Dirty one of the editors (both should appear dirty) 4) Close editor A. Since this was the first one open, the saveable for this editor was used in the reference count hashmap of the workbench. 5) Close editor B. The resulting prompt should indicate that this is the last occurance of the buffer but I was prompted indicating that the buffer was open elsewhere. If you like, I can verify your fix since I need to retest the fix for bug 167607 anyway.
OK, my fix doesn't address this case but I don't think it is a good idea to keep a reference to the savable of a closed editor. The problem with this is that objects , e.g. the closed editor and editor input, are still referenced even though they should be gone. This is bad. Currently my savable is an inner class and hence references the editor. Even if I would change that I would need to have a reference to the editor in order to perform the save. Since I already send out listener.handleLifecycleEvent(new SaveablesLifecycleEvent(this, SaveablesLifecycleEvent.POST_CLOSE, getSaveables(), false)); I would expect that this makes sure that my savables are no longer used i.e. the framework should replace it with another savable that's still in use. If that's too complicated then maybe the savable should get a getKey() method which could then be used instead of the savable itself. For now I released a workaround but I think we need a better solution that doesn't hold on to objects that got closed. What's you take on this one Boris?
Boris, the same problem happens to the compare editor's savable. We need a solution that doesn't hold on to savables of closed editors.
BTW, in comment #1 it was stated "It also assumes that the document will not change while the editor is opened." I'm pretty sure this is not always true for all editors in Eclipse in general, and am pretty sure it is not part of the "Editor API contract" -- so to now require it, would be a subtle API change. Dani, do you have an opinion or insights on this "changing document" issue? Is this even relevant anymore? Or is the attached patch obsolete?
>BTW, in comment #1 it was stated "It also assumes that the document will not >change while the editor is opened." That was a wrong comment: it can and in fact does change, e.g. if the editor is reused. This is handled in the doSetInput(...) method were the life-cycle listener is notified and a new savable created. > Or is the attached patch obsolete? The patch is rather a workaround and I released something along that line until the real problem (see comment 6) is fixed).
Created attachment 56645 [details] Patch to decouple saveable from editor One solution that Boris suggested to me was to decouple the saveable form the editor. This should work since the editors must react to saves made from other sources anyway. The patch I have attached decouples the saveable from the text editor. One requirement was to use the same saveable from the compare editor and this is possible due to the reason I stated above (i.e. internally I use a saveable that handles all the compare stuff but I present the shared document saveable to the workbench). For the hashCode and equals method, the editor input is used. There is, however, one outstanding issue. The saveable also needs to ensure that the document provider used by two sveables would be the same. This is problematic because document providers can be wrapped and I couldn't find API to access the wrapped provider (so, I think API would need to be added to document providers to make this work). Thoughts?
Document providers are not just wrapped but can be completely different and still use the same file buffer/document.
I didn't look exactly how the UI code is implemented but I would expect (and that is important for compatibility reasons) that when I execute save on editor X it will trigger EditorPart.doSave(...) and documentProvider.saveDocument(...). While the editor (e.g. Java editor) can react to the changes made this way it may loose important functionality/checks that are done in editor.save or its version of the document provider, e.g. organize import on save. What about decoupling the savable that does the work form the object that is used to identify whether two savables are equal?
In regards to the document provider wrapping, what I think would be required is API on a document provider that would be able to perform the proper test (i.e. API that asks the question "would you return the same file buffer as this other document provider"). In regards to the non-deterministic save behavior depending on the target of the save command, another approach would be to incorporate save participants at the file buffer level. Or, to put it another way, if I have said that I want to format Java code on save, wouldn't I want this to work if I saved from a compare editor as well?
>if I have said that I want to >format Java code on save, wouldn't I want this to work if I saved from a >compare editor as well? I think this is another discussion: you would like to have *all* Java editor functionality and hence also the organize import on save. However, if I open a *.java file in the simple text editor I (i.e. me) would not want these side-effects.
Sounds reasonable. A possible solution then would be for the Java editor to subclass the SynchronizedDocumentSaveable and extends it to perform the additional behavior. Having such a subclass would then potentially make that behavior available to the Java compare viewer as well.
Sorry but I don't like the document provider based solution. What speaks against the current solution but with a method to return an object used as comparable? This object can be independent of the editor, the editor input and also the document provider. There would be no tweaks needed by Platform UI to make sure not to hold on to a disposed object any longer. Personally even in the compare editor (e.g. while committing) I would probably not want to do anything else than a pure save.
The issue I see is that what you are saying is that the two Saveables we are talking about are not really equal. Based on what you said, if I close a dirty Java editor on a file that is open in another editor, if the prompt were to truly reflect the situation it would read something like "This file is opened in another editor so if you choose not to save here, you will not loose changes but you will also not get the behavior related to the Java editor such as format on save". Obviously, this is an exaggeration to make a point but, I'm concerned that users who see the current prompt would assume there was no difference between choosing No and then saving it elsewhere or choosing Yes. As for your suggestion of have API that returns a comparable, what's the difference between that and writing a Saveable that null's it reference to the editor when the editor is closed but keeps the hashCode and equals consistent? I guess it would make the separation more explicit but that's about it as far as I can see.
>As for your suggestion of have API that returns a comparable, what's the >difference between that and writing a Saveable that null's it reference to the >editor when the editor is closed but keeps the hashCode and equals consistent? The object would not be needed if Platform UI would correctly remove its reference to the saveable that the client marked as being closed. If that can't be done then the new object/key is cleaner and makes debugging and leak hunting easier i.e. if you close the editor its savable is gone too.
>"This file is opened >in another editor so if you choose not to save here, you will not loose changes >but you will also not get the behavior related to the Java editor such as >format on save". This is not only true for the Java Editor but also the Compare Editor i.e. if you close the Compare Editor you will loose the ability to compare the dirty file with another one and updating the current file with from the other one in that compare editor. Maybe the dialog should indeed point the user to this. I think changing the implementation to only treat similar editors with same model as being equal would be too restricting.
What's the state of this for M5?
(In reply to comment #16) > Sorry but I don't like the document provider based solution. What speaks > against the current solution but with a method to return an object used as > comparable? This object can be independent of the editor, the editor input and > also the document provider. There would be no tweaks needed by Platform UI to > make sure not to hold on to a disposed object any longer. Sorry, but I don't understand how this would work. To me, it all makes sense when there is one saveable that is shared by the Java editor and the Compare editor. I'm confused as to what you mean by a comparable object, could you sketch the API you have in mind? In the case of Save All, what do you expect to happen if the same file is open in both editors? One will be saved before the other, so depending on the order you will get format on save or not.
I should add that ISaveablesSource and Saveable are both @since 3.2, not 3.3, so any change would have to be binary upwards compatible. As for the dialog, I'm not convinced that we need additional information in there.
After releasing a fix similar to comment 1 (see my comment 9) I am no longer able to reproduce the problem indicated in comment 5. I think the main problem was caused because I was sending wrong life-cycle events (see bug 170184) and that is now fixed. Boris, I think my suggested solution is not needed. The reason why the savables list held on to my editor (the source) was caused by bug 170184. >In the case of Save All, what do you expect to happen if the same file is open >in both editors? That is indeed a problem and maybe we need to back off treating Java Editor (CompilationUnitDocumentProvider) and Text editor (TextFileDocumentProvider) the same.
Closing (or was there anything left to do, Dani?)
Not what this PR regards. Just a hint to Michael: I enhanced my code with a method to disconnect the editor when the editor closes. This breaks the link from the savable which is still used as key in the SavableList ref count map. This way many objects (like actions) can be freed. Of course the question whether we should treat Java and Text editor's document provider remains. For now I'd say we leave it as is since we may "only" loose the post-save operations which is not lost work.
