Community
Participate
Working Groups
You seemed to recall that the ISaveable mechanism was not fully enabled for some obscure reasons of compatilibity, but that it needed to be reactivated.
See bug 72114. The work is to take out the requirement for: org.eclipse.ui.workbench/fix72114=true Also to make it work for editors (bug 37221). Also to support doSaveAs for views. Question: should view dirty state be indicated automatically in tab, as for editors?
I think dirty state indication should be a function of the Presentation so RCP applications can override any default behavior.
For such a basic thing, it would be regrettable to force people into presentations. However nothing should prevent presentations to overrider it. Basically I want two mechanisms :-)
I believe showing the dirty state is already a presentation responsibility. Stefan, can you confirm?
Also need to support Save All.
The rendering of the dirty state is a presentation responsibility. It would make sense to render it for views as well as editors in the default presentation.
Bug 72114 and bug 37221 have been fixed. Still more to do here in M6.
Anthony and Matt, is there anything critical missing from the current support need to support your scenarios on 3.1? I'm moving this to M7 as I'm assuming any further changes will not require new API. Please shout ASAP if you think this is not the case.
No new API required. I tried out ISaveablePart in M5 and editors are fine, I can now close a dirty editor without prompt to save by isSaveOnCloseNeeded() = false. Views are almost there, I have isDirty() = true and isSaveOnCloseNeeded() = true and am prompted to save on view close, but not workbench exit.
I don't see a need for new API either.
*** Bug 43464 has been marked as a duplicate of this bug. ***
Created attachment 20677 [details] patch to fix this bug This is the first cut of a patch to fix this bug. Please provide feedback.
This patch has a few issues: 1. It will force materialization of views that were restored but not materialized from the previous session, due to getPart(true) in the getViews(Perspective) method. 2. It checks isSaveOnCloseNeeded on all views, not just dirty views. This differs from how editors are treated. 3. Save All is not fully supported (see comment 5). Although I don't think the current patch was intended to address this, it's pretty clear from testing it that we should. Although views are included in the dialog you get from Save All, Save All is currently only enabled if there is a dirty editor. It should be enabled iff there are any dirty editors or views in the current perspective. 4. It doesn't look like the perspective switching for ISaveablePart2 will work as expected, since getSortedPerspectives() returns the most recently used perspective -last- in the list. I assume we want it to show the most recently used perspective showing the view, so getFirstPerspectiveWithView should look through the list in reverse order. (1) and (2) can be addressed together by getting the dirty views before checking for isSaveOnCloseNeeded(), similarly to how EditorManager.getDirtyEditors does it. Note that collectDirtyEditors uses getPart(false) instead of getPart(true), since a non-materialized editor cannot be dirty. There is no way to check isSaveOnCloseNeeded() without restoring the view. For (3) take a look at SaveAllAction's current enablement logic. It could be changed to track all parts, and make use of a WorkbenchPage.getDirtyViews() method as suggested above (though it would be more efficient to optimize this to an isAnyPartDirty() call).
To test this in the IDE, you can use Window > Show View > Other... > Other > Saveable Mock View, which comes from org.eclipse.ui.tests. It has checkbox buttons for isDirty, isSaveOnCloseNeeded() etc.
Nick thanks for the great feedback. I have updated the patch and will post it after I run it through the gamut of tests. To respond to 4. in comment #13, I think you have it backwards, according the the Javadoc at least ;-) /** * Returns the perspectives in activation order (oldest first). * * @return all open Perspective objects * @since 3.1 */
Created attachment 20731 [details] updated patch based on Nick's feedback with some optimizations
I notice in the patch to SaveAllAction.partOpened you have a comment: + // We need to temporarily cache the opened part + // because saveable views are not registered + // with a perspective until after this method + // is called. We can't pass it through to + // update because it's protected. An async + // call to update may be a better approach + openPart = (ISaveablePart)part; This should not be necessary. The partOpened event should only be sent after the view has been fully opened and added to the page and perspective. If you're not seeing this, does it occur when a view is first opened, or in the startup sequence when restoring from a previous session?
I'm seeing this when the part is first opened. The call to perspective.getViews does not return the view that was just opened when called from SaveAllAction.partOpened.
Sure enough. I've filed bug 93784 for this problem.
Does this patch/bug now depend on bug 93784? Is there anything else about the patch you don't like?
I've changed the workaround in SaveAllAction to: // workaround for Bug 93784 [WorkbenchParts] View not yet added to perspective when partOpened sent if (openPart != null && openPart.isDirty()) { setEnabled(true); } Previously it would disable the action if the new part was not dirty, even if others in the page still were dirty.
Bug 93784 will not be addressed for 3.1, so we will leave the workaround in. I'm still reviewing the rest of the patch.
It doesn't look like EditorManager.saveAll will properly handle the case where there are ISaveablePart2 views from windows other than window passed in (Workbench.saveAllEditors passes in the active one, or the first one if the active window couldn't be determined. In particular, page.getFirstPerspectiveWithView will return null if the view is in a different window or a different page in the same window (though the latter is highly unlikely since 2.1). Also, getActivePage() and getActivePerspective() can both return null. All of these null cases should be checked for. Should also talk to the part's page (part.getSite().gePage()) for revealing the part, not the active page of the given window.
I've released the patches, with some extra changes to SaveAllAction and EditorManager.saveAll to handle the issues above. I still doubt this is properly handling the case of a dirty ISaveablePart2 in a non-active window. We will probably need to force the window to be activated to show the part. Matt, would you be able to test this case? Might be worth adding a new test view SaveableMockView2, extending SaveableMockView, implementing ISaveablePart2, and with its own id.
In response to comment #24 I tested the case where there are two windows. The active window has no dirty parts and the inactive window has some dirty parts that implement ISaveablePart2. The results were okay but not perfect. The custom prompt dialog and perspective switching work as expected. The one problem I encountered was that the window that contains the ISaveablePart2 needs to be activiated.
Created attachment 20757 [details] updated patch to fix active window issues This one-liner patch will address the issue of multiple windows.
Created attachment 20760 [details] Smaller faster patch I don't like the way the existing patch introduces more special-case code for views and editors. Also, the search for parts runs in O(n^2 * p) worst-case time (n = number of parts, p = number of perspectives) because of the way it iterates over all perspectives (each of which can contain every part). This can be done in linear time by calling WorkbenchPage.getAllParts. Performance is an issue since this code will run on each page event. This patch reduces the code size by merging the editor and view code paths, and improves the runtime. Note that even with this patch the runtime of SaveAllAction.updateState is still much too slow (you'll see it in part activation if a very large number of parts are open). It should incrementally update its enablement state and never call anything that runs in worse than constant time.
Nick and Matt, I didn't want to step on your toes so I haven't committed this patch yet. But if you approve, can one of you please do so?
Btw, this change seems to have introduced a new modal dialog to the test suites, which (at least on my machine) prevents them from finishing unless it is manually closed.
None of the patches above include changes to the test suite, but I'll have a look out for any blocking dialog. Looking at Stefan's patch now...
I was going to congratulate Matt for being the first one to introduce the "dirtyParts" variable into the Platform UI code, but I think credit has to go to Stefan for adding the getDirtyParts() method.
Stefan's patch looks great so far. It's missing a fix for the window activiation issue. I've posted a patch for that. I thought getDirtyViews would have been generally useful for EditorManager or even WorkbenchPage. With Stefan's patch we only have getDirtyEditors vs. getDirtyParts. Just my 2 cents.
I've applied Stefan's patch, with a minor tweak to EditorManager.saveAll to avoid creating the list if it would be empty, and to give it the exact size initially otherwise. Note that using getDirtyParts() will change the order of processing of Workbench.saveAllEditors to process views before editors. Previously it processed editors before views. But I don't think that it really matters. I didn't want to change the ordering in getAllParts() since it's also used when firing the partOpened events when restoring the workbench. If we need a getDirtyViews() in the future, we can add it. Matt, your patch in comment 26 is not the right one.
Re: comment 32 I've been working toward unifying the editor and view code paths. I hope we'll never need to add another get*Views or get*Editors method to workbenchPage. :-)
Created attachment 20801 [details] updated patch to fix active window issues In the previous active window patch the window would not be restored/brought to front, if it was in a minimized state. This revised patch should do the trick.
The patch does the window activation before calling page.bringToTop(part), which could result in flicker. I've applied the patch, but could you check this case out Matt?
Waiting on response to last question to Matt. Any further changes here will have to wait until RC1.
Nick, I don't notice any flicker at all in testing. Thanks.
Closing then. Anthony, could you please verify in 3.1 M7 that all the issues RAD cares about are covered?
Looks good, I gave it a go and all of the issues we were looking for are covered and working: - allow no prompt to save when closing a dirty editor. - allow prompt to save when closing a dirty view. - exit workbench prompts as appropriate from above.
Thanks Anthony. Marking as verified.
