Bugzilla – Full Text Bug Listing
| Summary: | [EditorMgmt] EditorPart#isSaveOnCloseNeeded() not called when closing Editor Parts via "Close All" or "Close Others" | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Phil Beauvoir <p.beauvoir> |
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> |
| Status: | RESOLVED WORKSFORME | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | emoffatt |
| Version: | 4.4 | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Windows 8 | ||
| Whiteboard: | |||
|
Description
Phil Beauvoir
This is a regression and has appeared in Integration Build id: I20140218-0800. In Eclipse 4.4 M5 Build id: I20140123-1600 this all worked as expected, isSaveOnCloseNeeded() was called on closing Editor Parts in this way. For information, the stack trace on a breakpoint at isSaveOnCloseNeeded() on my subclass was: Thread [main] (Suspended (breakpoint at line 498 in AbstractDiagramEditor)) ArchimateDiagramEditor(AbstractDiagramEditor).isSaveOnCloseNeeded() line: 498 WorkbenchPage.saveSaveable(ISaveablePart, IWorkbenchPart, boolean, boolean) line: 3760 WorkbenchWindow$7.save(MPart, boolean) line: 563 PartServiceImpl.savePart(MPart, boolean) line: 1268 StackRenderer.closeSiblingParts(MPart, boolean) line: 1458 StackRenderer.access$6(StackRenderer, MPart, boolean) line: 1430 StackRenderer$18.widgetSelected(SelectionEvent) line: 1379 TypedListener.handleEvent(Event) line: 248 EventTable.sendEvent(Event) line: 84 Display.sendEvent(EventTable, Event) line: 4353 MenuItem(Widget).sendEvent(Event) line: 1061 Display.runDeferredEvents() line: 4172 Display.readAndDispatch() line: 3761 PartRenderingEngine$9.run() line: 1122 Realm.runWithDefault(Realm, Runnable) line: 332 PartRenderingEngine.run(MApplicationElement, IEclipseContext) line: 1006 E4Workbench.createAndRunUI(MApplicationElement) line: 146 Workbench$5.run() line: 615 Realm.runWithDefault(Realm, Runnable) line: 332 Workbench.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 566 PlatformUI.createAndRunWorkbench(Display, WorkbenchAdvisor) line: 150 Application.start(IApplicationContext) line: 78 EclipseAppHandle.run(Object) line: 196 EclipseAppLauncher.runApplication(Object) line: 109 EclipseAppLauncher.start(Object) line: 80 EclipseStarter.run(Object) line: 372 EclipseStarter.run(String[], Runnable) line: 226 NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method] NativeMethodAccessorImpl.invoke(Object, Object[]) line: 57 DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43 Method.invoke(Object, Object...) line: 606 Main.invokeFramework(String[], URL[]) line: 636 Main.basicRun(String[]) line: 591 Main.run(String[]) line: 1450 Main.main(String[]) line: 1426 But this is not called in I build I20140218-0800 Looking through recent changes I found this: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=14734da604e6240048c4d62ef1279c9c7787d6a2 related to a recent fix for: https://bugs.eclipse.org/bugs/show_bug.cgi?id=396318 Save Resources dialog missing when selecting Close All from editor tab Is this it? (In reply to comment #2) > Looking through recent changes I found this: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=14734da604e6240048c4d62ef1279c9c7787d6a2 > > related to a recent fix for: > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=396318 > Save Resources dialog missing when selecting Close All from editor tab > > Is this it? No. Close All and Close Others still do not call isSaveOnCloseNeeded(). (In reply to Paul Elder from comment #3) > (In reply to comment #2) > > Looking through recent changes I found this: > > > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=14734da604e6240048c4d62ef1279c9c7787d6a2 > > > > related to a recent fix for: > > > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=396318 > > Save Resources dialog missing when selecting Close All from editor tab > > > > Is this it? > > No. Close All and Close Others still do not call isSaveOnCloseNeeded(). That's what I mean isn't it? If Close All and Close Others do not call isSaveOnCloseNeeded() then that's the cause of this bug. In 3.8 they do. (In reply to Paul Elder from comment #3) > (In reply to comment #2) > > Looking through recent changes I found this: > > > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=14734da604e6240048c4d62ef1279c9c7787d6a2 > > > > related to a recent fix for: > > > > https://bugs.eclipse.org/bugs/show_bug.cgi?id=396318 > > Save Resources dialog missing when selecting Close All from editor tab > > > > Is this it? > > No. Close All and Close Others still do not call isSaveOnCloseNeeded(). Perhaps I didn't make myself clear. I mean, is this recent change what caused a regression so that Close All and Close Others do not call isSaveOnCloseNeeded() (In reply to Phillipus B from comment #5) > Perhaps I didn't make myself clear. > > I mean, is this recent change what caused a regression so that Close All and > Close Others do not call isSaveOnCloseNeeded() Ah, I understand. Yes, the fix for bug 396318 appears to have caused this regression. Ironically, that bug was trying to delegate from the e4 UI back more completely to the 3.x compatibility layer so that the correct save dialogs would display. But, it seems that code doesn't respect ISaveablePart, ISaveablePart2 or any of the Saveables infrastructure found in 3.x. I am working on it. (In reply to Paul Elder from comment #6) > (In reply to Phillipus B from comment #5) > > > Perhaps I didn't make myself clear. > > > > I mean, is this recent change what caused a regression so that Close All and > > Close Others do not call isSaveOnCloseNeeded() > > Ah, I understand. Yes, the fix for bug 396318 appears to have caused this > regression. Ironically, that bug was trying to delegate from the e4 UI back > more completely to the 3.x compatibility layer so that the correct save > dialogs would display. But, it seems that code doesn't respect > ISaveablePart, ISaveablePart2 or any of the Saveables infrastructure found > in 3.x. I am working on it. That's superb, thanks. :-) A polite enquiry - will it be fixed for M6? (In reply to Phillipus B from comment #8) > A polite enquiry - will it be fixed for M6? Unfortunately, it did not get into M6 (contributions shut down last week). (In reply to Paul Elder from comment #9) > (In reply to Phillipus B from comment #8) > > A polite enquiry - will it be fixed for M6? > > Unfortunately, it did not get into M6 (contributions shut down last week). OK. I appreciate that. Thanks for looking at it. :-) Target 4.4 M7 removed? Will it be fixed for 4.4 at all? (In reply to Phillipus B from comment #11) > Target 4.4 M7 removed? Will it be fixed for 4.4 at all? No PW (In reply to Paul Webster from comment #12) > (In reply to Phillipus B from comment #11) > > Target 4.4 M7 removed? Will it be fixed for 4.4 at all? > > No > > PW I'm left wondering why. Seems strange that it seems to be a regression and wont be fixed. Resources. The people that could fix this bug are either busy or have moved on. If you contribute a fix we would look at it for Luna, but I understand that it's not easy to fix. PW (In reply to Paul Webster from comment #14) > Resources. The people that could fix this bug are either busy or have moved > on. > > If you contribute a fix we would look at it for Luna, but I understand that > it's not easy to fix. > > PW I'm sorry but I wouldn't know where to start on fixing this. Just to add some more info as to why this turns out to be more difficult to fix than it seems... The problem is that the Close & Close Others are pure 'e4' menu elements, created by the stack renderer (i.e. not a part of the normal IDE Commands infrastructure). This makes it difficult to have them access IDE-specific code as part of their implementation. We'd either have to move the save code out into the 'e4' layer or provide a new mechanism to allow for contributions (and *overrides*) to the CTabItem's context menu... I've verified in Eclipse 4.4.1 RC4 and 4.5 M2 that this issue has been fixed by changes made in the fix for the bug 410749. Phil, could you verify and confirm it works as expected for you? (In reply to Wojciech Sudol from comment #17) > I've verified in Eclipse 4.4.1 RC4 and 4.5 M2 that this issue has been fixed > by changes made in the fix for the bug 410749. > > Phil, could you verify and confirm it works as expected for you? Wojciech: Thank-you, thank-you, thank-you, thank-you, thank-you, thank-you, thank-you, thank-you, thank-you, thank-you! Verified and confirmed. :-) (In reply to Phil Beauvoir from comment #18) > Verified and confirmed. Thank you for the verification. Closing as 'worksforme'. |