Bug 131068 - [misc] Closing one of several open editors prompts user to save modified file
Summary: [misc] Closing one of several open editors prompts user to save modified file
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 3.2   Edit
Hardware: All All
: P2 enhancement (vote)
Target Milestone: 3.3 M5   Edit
Assignee: Dani Megert CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 17102 106689 169669 (view as bug list)
Depends on:
Blocks: 167607
  Show dependency tree
 
Reported: 2006-03-09 06:31 EST by Max Gilead CLA
Modified: 2007-04-23 13:44 EDT (History)
9 users (show)

See Also:


Attachments
work in progress (4.31 KB, patch)
2006-03-14 18:07 EST, Boris Bokowski CLA
no flags Details | Diff
work in progress (4.31 KB, patch)
2006-03-26 23:51 EST, Boris Bokowski CLA
no flags Details | Diff
patch against org.eclipse.ui.workbench.texteditor (5.32 KB, patch)
2006-04-12 17:35 EDT, Boris Bokowski CLA
no flags Details | Diff
helper plugin jar (12.06 KB, application/x-java-archive)
2006-04-12 17:45 EDT, Boris Bokowski CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Max Gilead CLA 2006-03-09 06:31:11 EST
Closing one of several open editors (all showing the same, modified, file) prompts user to save changes. Such dialog is confusing (correct answer is 'No' ;) ).
Comment 1 Dani Megert CLA 2006-03-13 05:18:00 EST
Nick, according to bug 112225 it would be the right thing not to prompt and could be solved using the new ISaveableModel* support, right?
Comment 2 Nick Edgar CLA 2006-03-14 08:14:06 EST
Yes, if the text editors implements ISaveableModelSource, with the ISaveableModel mapping to the underlying IDocument, this prompt could be avoided for editors on the same document in the same window.

I think we should do this for 3.2.  
Comment 3 Boris Bokowski CLA 2006-03-14 10:30:56 EST
I am working on this right now. May I take this bug?
Comment 4 Dani Megert CLA 2006-03-14 11:01:32 EST
Yes please ;-)

Do you intend to provide a patch for Text? I don't see how you could solve this just on the UI side.
Comment 5 Boris Bokowski CLA 2006-03-14 11:10:58 EST
Yes, the plan is to provide a patch.
Comment 6 Boris Bokowski CLA 2006-03-14 18:07:57 EST
Created attachment 36292 [details]
work in progress

Just to save my current state...
Comment 7 Nick Edgar CLA 2006-03-15 16:30:38 EST
Thanks Boris.
Comment 8 Boris Bokowski CLA 2006-03-26 23:51:17 EST
Created attachment 36967 [details]
work in progress
Comment 9 Boris Bokowski CLA 2006-04-12 14:41:59 EDT
Daniel, I forgot to talk to you about this earlier but would like to see it in 3.2.  Could you have a look at the latest patch?  Do you think we can still do this?
Comment 10 Dani Megert CLA 2006-04-12 16:20:37 EDT
I'm reluctant to do and bring this change to the PMC at this point.

Boris, how do you assess the risk when applying the patch? In addition what would be the ETA for a clean patch which also handles editor input and document provider changes and correct formatting as it is used in Platform Text?
Comment 11 Boris Bokowski CLA 2006-04-12 17:35:45 EDT
Created attachment 38458 [details]
patch against org.eclipse.ui.workbench.texteditor

Sorry about that, I saw two patches and thought the newer of the two was my current state. Here is the patch I wanted you to look at.

I deleted the spaces between assigned variables and the "=", but I'm sure there are other differences between the default formatter settings and the settings you are using.  Are your formatter settings available somewhere?
Comment 12 Boris Bokowski CLA 2006-04-12 17:45:17 EDT
Created attachment 38459 [details]
helper plugin jar

This helper plugin provides a view (Model->Models) that displays the list of open "Saveable" objects and their dirty state.  It can be used while testing, to verify that the internal state of the Workbench is correct.

Note that without the patch, if you open a second editor on a file, the view contains two entries.  With the patch, there is only one entry.

I have tested the patch by checking "Close editors automatically" in the preferences with a low number of editors that should be open concurrently.  I would be interested to know if there are other interesting cases like this that I should test.
Comment 13 Boris Bokowski CLA 2006-04-12 17:52:11 EDT
As for the risk, it is hard to say since I am not familiar enough with the text editor.  If updateDocumentProvider() is guaranteed to be called when either the document provider or the editor input changes, I think the risk is low.  If the assumption is wrong, we should not make this change now.

Ultimately, it is your call.  I would like to see the patch in if possible, because it makes use of API that would otherwise not be used by the SDK itself.
Comment 14 Dani Megert CLA 2006-04-13 11:31:16 EDT
Boris,
I'll take a look at this for RC2. Did you also make tests with multi-page editors like the PDE Manifest editor?
Comment 15 Dani Megert CLA 2006-04-18 11:05:27 EDT
The latest patch does not work for non-multipage editors and also only works per editor type i.e. if I open A.java with the Text and the Java editor I still get asked when closing one of the two. This gives an inconsistent story to the user and might be confusing.

I prefer to consistently get (or not get) the dialog in all cases and therefore will not apply this patch for 3.2. We should work towards a more complete story for 3.3. This will also give us time to carefully test the new code and let others, like multi-page editors adopt to the new behavior i.e. not ask to save if another editor still works on the same model object (or a higher level model that contains the one which gets closed).
Comment 16 Andrew Bachmann CLA 2006-04-19 21:16:52 EDT
it seems like there is a related issue:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=106689
Comment 17 Boris Bokowski CLA 2006-04-19 22:55:51 EDT
*** Bug 106689 has been marked as a duplicate of this bug. ***
Comment 18 Dani Megert CLA 2006-07-07 08:51:51 EDT
I'll look into the problem where the textual (e.g. Text and Java) editor interact which each other but will probably have to move it back to Platform UI for the multi-page editor scenario.
Comment 19 Markus Keller CLA 2006-11-20 10:06:45 EST
See also bug 17102.
Comment 20 Dani Megert CLA 2006-12-18 12:42:19 EST
First cut of a fix is in HEAD.
Available in builds > N20061218-0010.

NOTE: bug 17102 in Platform UI land is almost a dup of this one. I close this one as the text part is done. Open issue as mentioned in comment 15 might be multi-page editors. The following scenario works: we can open the PDE editor plus e.g. the text editor on plugin.xml and when the file is dirty you can close the text editor without getting a dialog.
Comment 21 Dani Megert CLA 2006-12-18 13:25:53 EST
Filed bug 168435 which reports that the user cannot undo his close strategy.
Comment 22 Boris Bokowski CLA 2006-12-18 14:15:05 EST
*** Bug 17102 has been marked as a duplicate of this bug. ***
Comment 23 Dani Megert CLA 2007-02-07 11:57:26 EST
Verified in I20070207-0800.
Comment 24 Boris Bokowski CLA 2007-04-23 13:44:03 EDT
*** Bug 169669 has been marked as a duplicate of this bug. ***