Community
Participate
Working Groups
Our RCP applications need the ability to provide an application defined custom dialog when prompting the user to save. This is specific to ViewParts that implement the ISaveablePart interface. Patch to follow.
Created attachment 15335 [details] allows for custom prompt dialog
See also bug #72114
Just looking at the patch... you've patched the project file to change the name.
To look at for 3.1 M4.
If we allow this for views, should also allow it for editors. Need to also handle the case where the workbench prompts for multiple editors at the same time (e.g. close all when multiple editors have unsaved changes). In this case, we'll either need to change to prompting for each part separately, instead of using a single dialog for multiple editors, or come up with some other way of handling batching in a pluggable way (e.g. delegate to a handler specified on the editor type extension?)
Sorry, wasn't able to get to this for M4. In the attached patch, prompt() is responsible not only for prompting, but for doing the save() as well. Is this necessary? Can we leave the actual saving up to the workbench, or are there aspects of this (e.g. progress monitoring labels) that would need to be customizeable as well)? If they can't be separated, then we should be more explicit in method names that it takes over the whole saving process, not just the prompting.
In response to #5, yes this should apply to editors as well. During Workbench shutdown the expected behavior (at least for our RCP apps) is to prompt the user for each dirty editor/view, switch perspectives as needed. We have code to do this in our WorkbenchAdvisor but it's not as robust as we would like. We keep a list of parts that implement ISaveablePart via part activation listeners. In response to #6, yes I think implementors of ISaveablePartWithPrompt should be responsible for all cases resulting from the prompt. In some cases "save" is appropriate, in others there may be cases where "send" or "save as draft" type actions need to take place. If we left the "save" up to the Workbench how difficult would it be for ISaveablePartWithPrompt implementors to manage these different actions? I can code up some test cases for this including the code mentioned in response to comment #5.
The main reason for wanting to keep going through doSave(IProgressMonitor) was to allow the workbench to take care of how progress is handled, without requiring each part to do this. Even if at the UI level the operation is called "Send", it would be good to have some common support for saving the contents of an editor/view. Tracking which views are in which perspectives via listeners is likely to be tricky. I suggest iterating over all perspectives in activation order using (from WorkbenchPage) perspList.getSortedPerspectives(), then iterating over the views using Perspective.getViewReferences() and IViewReference.getView(false). Use false to avoid forcing creation of the view. If the view hasn't been created, it can't have changes.
I propose the following changes: - rename to ISaveablePart2 (extends ISaveablePart) - clarify the API that it's determining whether to save on close, and is only called if the part is dirty - allow a 3-state return value: YES: save the part and proceed with close (using the existing doSave API and progress monitor support) NO: close the part without saving CANCEL: cancel the close This will also allow us to address the problem with editors' handling of isSaveOnCloseNeeded() described in bug 37221 comment 7. Instead of implementing isSaveOnCloseNeeded() to return false, the editor could just implement ISaveablePart2 and return NO. We would then likely just deprecate isSaveOnCloseNeeded() (for both views and editors).
This also needs to handle the WorkbenchPage.closeEditors(IEditorReference[] editorRefs, boolean save) case (used by File > Close All), where multiple editors are processed simultaneously using a single dialog currently. One option would be first process any that implement ISaveablePart2, and then only include those that return YES in the dialog. Also need to ensure the prompt to save on window close or shutdown works the same way (I'm not positive it goes through this method).
Question about default handling of return codes. Currently the default is "cancel". In order for the Workbench to protect itself from bad return codes from implementers of ISaveablePart2, we should either change the default to "no" or provide a typesafe enum for return codes. Parts that returned a value outside of yes|no|cancel would either prevent a user from closing a part or prevent the Workbench from shutting down. Providing typesafe enum may be overkill. Moving the default is not risky since it doesn't look like "default" is ever hit. Thoughts? SaveableHelper.java static boolean savePart(...... ..... switch (choice) { case 0 : //yes break; case 1 : //no return true; default : case 2 : //cancel return false; }
I don't think the default case can get hit currently. Changing the default to NO would be fine. Since we don't use type safe enums anywhere else, let's not start now.
Suggest calling the method promptToSaveOnClose() instead of just prompt() or doPrompt(). We should add a DEFAULT or UNHANDLED result, which would have the same effect as if the part did not implement ISaveablePart2, and make this the default case in the switch. This way, if other API gets added to ISaveablePart2, or yet another extension interface is added, clients can still get the default behaviour and aren't forced to handle the prompting themselves.
In response to comment #13 >> This way, if other API gets added to ISaveablePart2, or yet another extension interface is added, clients can still get the default behaviour and aren't forced to handle the prompting themselves. Wouldn't an Adapter class make more sense for those cases?
Re comment #11 and comment #12 >> don't think the default case can get hit currently. This is not true. Open a file, make a change then close it without saving. When the save prompt is presented, close it using the close button in the title bar of the dialog. The return code in this case is a -1. The default is then hit and results in a cancel event.
So should the Workbench protect itself from parts that do not provide the proper return codes or parts that always return a cancel return code? A part that always returned a cancel return code would do two things, 1) it would prevent the part from ever being closed once open and dirty and 2) would prevent the Workbench from ever shutting down. We could do something like the following to protect against these cases: A) Enable support for ISaveablePart2 via a protected product level ini setting. B) SaveableHelper will always provide the actions for Cancel and No/Close/Dismiss. The implementers of ISaveablePart2 will only be able to provide the other "save-like" actions. Either through a getter on ISaveablePart2 or through an extension point. In my opinion A seems simple and allows for more customization, eg. the dialog doesn't have to be an SWT dialog. B may be overkill, paticularly the extension point options, but seems more safe since the Workbench keeps ownership of some critical actions. Thoughts?
Just revisted comment #9 >> Instead of implementing isSaveOnCloseNeeded() to return false, the editor could just implement ISaveablePart2 and return NO. We would then likely just deprecate isSaveOnCloseNeeded() (for both views and editors). Option B metioned in comment #16 would preclude us from doing the above.
I don't think we should worry too much about the scenario in comment 16. Both A and B sound unnecessarily complex to me. If we're going to allow the part to override the close policy, then we're going to have to trust that they won't do something silly like always return CANCEL. This isn't the only opportunity for wayward parts to hose the environment. I'd also like to phrase the API in such a way that the part isn't required to bring up a dialog. It may be that it just wants to take care of the save silently itself, or always return NO, or whatever.
> Wouldn't an Adapter class make more sense for those cases? Actually, it probably makes sense in this case too.
Deferring to M6. Matt to supply updated patch.
Created attachment 18061 [details] Preliminary patch for feedback This patch introduces the ISaveablePart2 interface and supports "close all" and "close/save all on shutdown" for editors only. This patch is missing shutdown support for Views that implement ISaveablePart2. Patch for that to follow.
Thanks Matt. Tod, could you please review this patch?
Matt this patch is out of date. Could you update it with one that I can check against HEAD?
Created attachment 18505 [details] Updated patch to check against head
Matt I still can't apply this patch to workbench or org.eclipse.ui.internal. Could you maybe just export the 3 classes to a zip?
Created attachment 18533 [details] zip file containing changed files
Any updates here? I would like to move forward with support for "save on close" for saveable view parts that implement ISaveablePart2 but would like some feedback on the previous patch before I do. Thanks
Matt I have the following errors when I apply your changes Severity Description Resource In Folder Location Creation Time 2 The method openEditor(IEditorReference, IEditorReference[], boolean) is undefined for the type EditorAreaHelper EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 773 March 14, 2005 10:32:12 AM 2 The method openEditor(IEditorReference, boolean) is undefined for the type EditorAreaHelper EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 778 March 14, 2005 10:32:12 AM 2 WorkbenchMessages.EditorManager_unableToInitialize cannot be resolved EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 820 March 14, 2005 10:32:12 AM 2 WorkbenchMessages.EditorManager_unableToInstantiate cannot be resolved EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 902 March 14, 2005 10:32:12 AM 2 The method createChildControl() is undefined for the type PartPane EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 1097 March 14, 2005 10:32:12 AM 2 The method createChildControl() is undefined for the type PartPane EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 1101 March 14, 2005 10:32:12 AM 2 The type EditorManager.Editor must implement the inherited abstract method WorkbenchPartReference.createPane() EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 1339 March 14, 2005 10:32:12 AM 2 The method setPane(PartPane) is undefined for the type EditorManager.Editor EditorManager.java org.eclipse.ui.workbench/Eclipse UI/org/eclipse/ui/internal line 1491 March 14, 2005 10:32:12 AM
Created attachment 18768 [details] Updated patch to check against head for 03142005 Tod, this patch should do the trick if you apply it right..........now ;-)
Matt the patch won't load SaveablePart. Lets try the zip again
Can you explain what you mean by "wont load SaveablePart"? I think you mean SaveableHelper, I'll fix up the patch. Any ideas why this is happening?
Created attachment 18771 [details] Updated patch to check against head for 03142005 This patch works for sure, tested it myself ;-) I will remember to test my patches before I submit them. I will remember to test my patches before I submit them. I will remember to test my patches before I submit them. .....
Sorry - I meant SaveableHelper. Send me the files again - the patches don't seem to be doing it.
OK that worked! Looks fine to me - the only thing I would mention is that SaveablePart has the code if (choice == -1) { if (saveable instanceof ISaveablePart2) { choice = ((ISaveablePart2)saveable).promptToSaveOnClose(); } if (choice == -1 || choice == ISaveablePart2.DEFAULT I would make the -1 some sort of constant
Sorry - slip of the button
Patch released to build 20050315
Matt please verify
Verified in I20050330-0500
