Bugzilla – Full Text Bug Listing
| Summary: | [EditorMgmt] AbstractTextEditor#isSaveOnCloseNeeded not called | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | adrian <stori> |
| Component: | UI | Assignee: | Nick Edgar <n.a.edgar> |
| Status: | VERIFIED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | ahunter.eclipse, celek, daniel_megert, hudsonr, kai-uwe_maetzel, Matthew_Hatem, n.a.edgar, peddle, phil.hunt, steven.wasleski |
| Version: | 2.0.2 | ||
| Target Milestone: | 3.1 M5 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
|
Description
adrian
This is correct but it is not a Platform Text bug but a Platform UI bug: EditorManager.savePart should do this. As a workaround you could overwrite doSave() in your editor. committed for 3.0 Any news :) No news, but will consider for 3.1 M5. We added support for this for views in 3.0.1 (if they implement ISaveablePart), and should fix up editors too. No news, but will consider for 3.1 M5. We added support for this for views in 3.0.1 (if they implement ISaveablePart), and should fix up editors too. *** Bug 60528 has been marked as a duplicate of this bug. *** It turns out that the change to not call isSaveOnCloseNeeded() was a deliberate change back in 2.0. See bug 2714 and bug 24496. At that time, isSaveOnCloseNeeded was apparently interpreted as "will closing this editor lose changes", which, in the case of a shared document model like that in Text, is false when more than one editor is open on the same document. The user's expectation is that if they close an editor that has changes, they'll get prompted to save even if the same document is open in another window (the scenario in bug 2714 really was confusing). By changing the workbench to only check isDirty(), ignoring isSaveOnCloseNeeded(), we essentially hardwired that policy into the workbench, rather than leaving the decision to save on close up to the editor itself. While we believe that the current behaviour matches the recommended best practice, there are apparently advanced scenarios where the editor should be allowed to override this policy. Kai and Dani, would you be willing to go down the other path suggested in bug 2714 and delete the AbstractTextEditor.isSaveOnCloseNeeded() method, thereby getting the default behaviour in WorkbenchPart? The spec for isSaveOnCloseNeeded() states simply: "Returns whether the contents of this part should be saved when the part is closed." The default implementation in WorkbenchPart simply forwards to isDirty(). Despite this, I believe the workbench should not assume this and should have the following logic: on editor close, if editor.isSaveOnCloseNeeded() if editor.isDirty() prompt to save (Yes, No, Cancel) if cancel, cancel the close else if yes, call doSave() to save the changes close the editor else if no close the editor without saving the changes Moving to Text for comment. See also bug 76768 comment 9. Nick, we removed the method (builds > 20050210). Please let us know if we have to react to bug 76768 comment 9. Randy, do you know if the change to start calling isSaveOnCloseNeeded would affect any GEF editors? With the change to Text (thanks Dani), I'm inclined to go ahead and put the call to isSaveOnCloseNeeded back in, so the scenarios here will not require the change described in bug 76768 comment 9. I think it's too risky to have it replace the call to isDirty() as originally intended, so the behaviour will be as sketched above in comment 7. There is still some risk of breakage, but any parts that just ignored isSaveOnCloseNeeded() will work fine. We don't override that method in our base editor so it is equivalent to isDirty (). IMO, the dirty indicator "*" should always result in a prompt on close. If the editor makes inconsequential model changes, such as changing the zoom level, the document position, expand/collapsed state, etc., then the dirty indicator must not appear. This is how MS Office behaves. So I would say the fix is to change the isDirty() behavior for this editor. If the user closes a dirty editor and is not prompted, you can expect future bug reports. Yes, people who want to override isSaveOnCloseNeeded() should be aware that this may be confusing to the end user (see bug 2714). Editor implementors will need to decide whether having a different policy for prompt on close will have more pros than cons. But I think that editors should be able to override the policy, as long as the default policy in the workbench stays the same, and meets the typical user expectation. Fix and tests released to head. Affected types: Workbench WorkbenchPage IViewPartTest IEditorPartTests MockEditorPart SaveableMockViewPart Verified in I20040215-2300. |