Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 372992

Summary: EditorPart#isSaveOnCloseNeeded() not called when closing Editor Part
Product: [Eclipse Project] Platform Reporter: <h1055071>
Component: UIAssignee: Oleg Besedin <ob1.eclipse>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: daniel_megert, elias, Lars.Vogel, p.beauvoir, pwebster, remy.suen
Version: 4.2   
Target Milestone: 4.2 M7   
Hardware: PC   
OS: All   
Whiteboard:

Description CLA 2012-03-01 11:05:17 EST
My 3.7 RCP app is running in E4 compatibility layer, using Eclipse 4.2 M5 and later.

I have Editor parts that sub-class EditorPart. I over-ride isSaveOnCloseNeeded() to return false so that the "... has been modified. Save changes?" dialog is *not* shown by the workbench if the editor is dirty and the user manually closes the editor part.

However in E4 isSaveOnCloseNeeded() is not called and the dialog is shown.
Comment 1 Remy Suen CLA 2012-03-01 12:29:55 EST
This works when using Ctrl+W but not when clicking on the 'X'.

When the 'X' is clicked on, we first ask the user to save the part before we close it as the isSaveOnCloseNeeded() concept is not in Eclipse 4.
Comment 2 CLA 2012-03-01 12:41:48 EST
(In reply to comment #1)
> This works when using Ctrl+W but not when clicking on the 'X'.
> 
> When the 'X' is clicked on, we first ask the user to save the part before we
> close it as the isSaveOnCloseNeeded() concept is not in Eclipse 4.

So, can it be fixed? Or is this intended for Ecliopse 4?
Comment 3 Remy Suen CLA 2012-03-01 13:31:36 EST
(In reply to comment #2)
> So, can it be fixed?

It needs to be fixed for compatibility reasons anyway, yes.
Comment 4 Eric Moffatt CLA 2012-04-17 13:23:08 EDT
Will / has this ever shown up in the IDE itself ? If not then we'll likely defer this to 4.2.1...
Comment 5 CLA 2012-04-17 13:29:42 EDT
So the "compatability" layer will be broken for Eclipse 4.2? (This and other bugs I've found).
Comment 6 Paul Webster CLA 2012-04-17 13:34:06 EDT
(In reply to comment #5)
> So the "compatability" layer will be broken for Eclipse 4.2? (This and other
> bugs I've found).

no, there will be bugs in it, same as 3.8 (which has bugs numbering in the thousands)

PW
Comment 7 CLA 2012-04-17 15:15:56 EDT
(In reply to comment #4)
> Will / has this ever shown up in the IDE itself ? If not then we'll likely
> defer this to 4.2.1...

Maybe not, but it shows up in our RCP application. We won't be "upgrading" to Eclipse 4 anytime soon.
Comment 8 Dani Megert CLA 2012-04-18 08:14:45 EDT
(In reply to comment #4)
> Will / has this ever shown up in the IDE itself ? If not then we'll likely
> defer this to 4.2.1...

In the SDK there are several callers (outside Platform UI) and one override.
Comment 9 Remy Suen CLA 2012-04-18 08:20:40 EDT
(In reply to comment #8)
> In the SDK there are several callers (outside Platform UI) and one override.

I would be concerned about the overrides, not really worried about the callers.
Comment 10 Dani Megert CLA 2012-04-18 08:25:15 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > In the SDK there are several callers (outside Platform UI) and one override.
> 
> I would be concerned about the overrides, not really worried about the callers.

Agreed.
Comment 11 Dani Megert CLA 2012-04-18 08:27:11 EDT
We also have to check whether only EditorPart.isSaveOnCloseNeeded() is missing, or whether this is a general problem i.e. affects org.eclipse.ui.ISaveablePart.isSaveOnCloseNeeded(). There, we have several overrides.
Comment 12 CLA 2012-04-18 08:32:18 EDT
My use of this is that our RCP tool (http://archi.cetis.ac.uk) uses several GEF-based Editor parts to edit different diagrams for the same model. The "dirty" state is maintained for the model, not for each Editor part. Thus we don't want to prompt the user to save the editor part when it is closed. This was easy to implement in Eclipse 3.x by simply returning false when over-riding sSaveOnCloseNeeded(). Now in the e4 compatibility layer this no longer works so our app cannot be used beyond 3.x.
Comment 14 Remy Suen CLA 2012-04-23 14:16:35 EDT
(In reply to comment #13)
> Fixed:
> 
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8ab339d1d4c6bc5ba32c44bdf870b29bbe1903fa

This feels off to me. This means a request to save will simply be ignored if the editor returns 'false' with isSaveOnCloseNeeded(), no?

The problem here is that we have no way of distinguishing between a save request and a save request that's been triggered by a close request.
Comment 15 Oleg Besedin CLA 2012-04-23 14:43:27 EDT
(In reply to comment #14)
> (In reply to comment #13)
> > Fixed:
> > 
> > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=8ab339d1d4c6bc5ba32c44bdf870b29bbe1903fa
> 
> This feels off to me. This means a request to save will simply be ignored if
> the editor returns 'false' with isSaveOnCloseNeeded(), no?
> 
> The problem here is that we have no way of distinguishing between a save
> request and a save request that's been triggered by a close request.

The ISaveHandler#promptToSave() is not called on a normal save action (Ctrl+S/Menu/Toolbar). It is called on part close. As far as I know, normal save is not affected by this change.
Comment 16 Remy Suen CLA 2012-04-23 14:46:10 EDT
(In reply to comment #15)
> The ISaveHandler#promptToSave() is not called on a normal save action
> (Ctrl+S/Menu/Toolbar). It is called on part close. As far as I know, normal
> save is not affected by this change.

If someone goes through the EPartService's savePart(MPart, boolean) method then this becomes a problem.
Comment 17 CLA 2012-04-23 14:49:08 EDT
(In reply to comment #16)
> (In reply to comment #15)
> > The ISaveHandler#promptToSave() is not called on a normal save action
> > (Ctrl+S/Menu/Toolbar). It is called on part close. As far as I know, normal
> > save is not affected by this change.
> 
> If someone goes through the EPartService's savePart(MPart, boolean) method then
> this becomes a problem.

Not as much a problem as breaking backwards compatibility?
Comment 18 Remy Suen CLA 2012-04-23 15:12:38 EDT
(In reply to comment #17)
> (In reply to comment #16)
> > If someone goes through the EPartService's savePart(MPart, boolean) method then
> > this becomes a problem.
> 
> Not as much a problem as breaking backwards compatibility?

Not my decision to make. Though having new API not do something correctly with pre-existing clients could also be interpreted as breaking backwards compatibility.

In any case, if there are no plans to reopen this and fix it correctly, then please open a new bug to track the problem with the now broken 4.x API.
Comment 19 CLA 2012-04-23 15:19:33 EDT
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > If someone goes through the EPartService's savePart(MPart, boolean) method then
> > > this becomes a problem.
> > 
> > Not as much a problem as breaking backwards compatibility?
> 
> Not my decision to make. Though having new API not do something correctly with
> pre-existing clients could also be interpreted as breaking backwards
> compatibility.
> 
> In any case, if there are no plans to reopen this and fix it correctly, then
> please open a new bug to track the problem with the now broken 4.x API.

And the 4.x API isn't already broken? As well as the totally unusable broken "compatibility layer"? With this fiasco I am abandoning Eclipse RCP development. With e4, Eclipse jumped the shark...
Comment 20 Oleg Besedin CLA 2012-04-24 10:04:46 EDT
(In reply to comment #18)
> In any case, if there are no plans to reopen this and fix it correctly, then
> please open a new bug to track the problem with the now broken 4.x API.

Done: bug 377519 . 

It is not possible to fix it "correctly" at this point of the release cycle. A sizable block of functionality needs to be moved from 3.x into e4 code.
Comment 21 Oleg Besedin CLA 2012-05-01 14:04:30 EDT
Verified in I20120430-1800.
Comment 22 Phil Beauvoir CLA 2013-06-23 10:28:56 EDT
This is a bug again in E4 RC4. Build id: I20130605-2000


Please re-open.

isSaveOnCloseNeeded() is not being called just as before.


Back to Eclipse 3.8 I go...again...

:sigh:
Comment 23 Phil Beauvoir CLA 2013-06-24 03:01:13 EDT
Re-submitted as Bug #411465