Bug 54193 - [New editor] Saving the new editor does not work
Summary: [New editor] Saving the new editor does not work
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.0   Edit
Hardware: PC Windows XP
: P1 normal (vote)
Target Milestone: 3.0 M9   Edit
Assignee: Dejan Glozic CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-03-09 14:52 EST by Wassim Melhem CLA
Modified: 2004-05-28 02:09 EDT (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Wassim Melhem CLA 2004-03-09 14:52:43 EST
Make changes in the new editor and try to save.  The results are predictable.
Comment 1 Dejan Glozic CLA 2004-03-09 15:23:15 EST
Can you clarify 'predictable'?
Comment 2 Wassim Melhem CLA 2004-03-09 15:57:53 EST
I meant "unpredictable", like what we saw when the first page was not 
initialized correctly, yet the source page/outline was showing the correct 
content.  When the editor was then opened in a plain text editor and saved. 
The next time we reopened the new editor, everything initialized correctly.
Comment 3 Dejan Glozic CLA 2004-03-26 18:30:00 EST
With the key bindings working better and a surgical fix in 'mustSave' method 
of the input context, saving from source now works predictably.

Later on, I must ask Kai what subtle difference is between the following two 
methods (we were calling 'mustSave...' and should have been 
calling 'canSave...'):

	/**
	 * Returns whether the document provided for the given element must be 
saved.
	 *
	 * @param element the element, or <code>null</code>
	 * @return <code>true</code> if the document must be saved, and
	 *   <code>false</code> otherwise (including the element is 
<code>null</code>)
	 */
	boolean mustSaveDocument(Object element);
	
	/**
	 * Returns whether the document provided for the given element differs 
from
	 * its original state which would required that it be saved.
	 *
	 * @param element the element, or <code>null</code>
	 * @return <code>true</code> if the document can be saved, and
	 *   <code>false</code> otherwise (including the element is 
<code>null</code>)
	 */
	boolean canSaveDocument(Object element);
Comment 4 Wassim Melhem CLA 2004-04-13 23:01:20 EDT
We're not there yet.

1. Create Hello world plug-in.
2. Go to dependencies page.  Move core.runtime.compatibility up and then back 
down.
3. Go to source page, no changes were made to text.  This is pretty good from 
a text edit point of view, since the up/down operations canceled each other 
out and thus the document did not need to be modified.  Try closing the 
editor.  It will close without asking you to save, but the editor tab still 
had a *.


1. Create a Hello world plug-in
2. Go to dependencies page.  Move core.runtime.compatibility up.
3. Go to the source page.  The document has updated correctly. 
4. Right-click and select Revert.   The document goes back to its original 
state, and the * goes way.  All good.
5. Go to the dependencies page.  Try closing the editor.  It will prompt you 
to save..????
Comment 5 Wassim Melhem CLA 2004-04-21 20:15:26 EDT
Not too crazy about the Revert either.  Try reverting when you're in forms.  
Not pretty.
Comment 6 Dejan Glozic CLA 2004-04-24 17:36:45 EDT
I am not very hopeful about this - the old design is that any change to the 
model makes it dirty. If, for example, you go in, type a different plug-in 
name, then type the original one on top, we don't sense that you are back to 
the original value and cancel the dirty state. Changes to the model are 
irreversable i.e. the model is dirty until it is saved or until 'Revert' 
operation is called.

Your 'up/down' operations also make the model dirty and we don't check if you 
are back with the original content. I would rather be consistent with 2.1 
behaviour and always be dirty when changes are made.
Comment 7 Wassim Melhem CLA 2004-04-24 17:45:42 EDT
I think the problem here is the inconsistency with what the text editor thinks 
is dirty and what the overall model thinks.

As you observed, moving up/down marks the model dirty, but no text operations 
are necessary as they cancel each other out.  so when you flip to the source 
page, you are able to close without any prompt to save because the text was 
unaffected.  I think you should be forcing a 'Save' here.
Comment 8 Wassim Melhem CLA 2004-04-24 17:46:40 EDT
By forcing a 'save', I meant forcing a prompt to save.
Comment 9 Wassim Melhem CLA 2004-04-24 18:06:13 EDT
Not sure why there is a disconnect between the model and the document.  
Shouldn't the context(s) be the determining factor whether the editor needs to 
be saved or not?
Comment 10 Dejan Glozic CLA 2004-04-30 10:19:06 EDT
Wassim, please append a scenario that does not involve mutually cancelling 
up/down (swap) events.
Comment 11 Wassim Melhem CLA 2004-04-30 10:25:38 EDT
Gladly, I could provide 100.  But here is a simple one.

1. Create a Hello world plug-in.
2. Go to the Dependencies page.  Mark org.eclipse.ui as exported.
3. Go to the source page.  From the context menu, save.
4. Switch to the dependencies page.  Try to close the editor.  It will prompt 
you to save????

This will get you going.
Comment 12 Dejan Glozic CLA 2004-04-30 11:20:29 EDT
It did get me going :-). Care to explain why is the model marked dirty when 
going back to the dependency page? Let's review the scenario again:

1. Create a Hello world plug-in.
2. Go to the Dependencies page.  Mark org.eclipse.ui as exported.
3. Go to the source page.  From the context menu, save.
4. Switch to the dependencies page.  Try to close the editor.  It will prompt 
you to save????

When I debug step 4, it prompts you to save because plug-in model claims it is 
dirty. But it should be, because it was reconciled while in the source page, 
and no changes were made in the dependencies page. It should be clean at that 
point.
Comment 13 Wassim Melhem CLA 2004-04-30 11:25:34 EDT
As I said in a previous comment, the decision on whether or not to save should 
really come from the input context.  When one saves, it should marked 
as "clean" or whatever terminology you want to use.
I still don't understand the disconnect between an IDocument and the model.  
Why are they even being asked if they are dirty?  The question should be asked 
to the InputContext which has a combined view of both and is in the best 
position to answer.
Comment 14 Dejan Glozic CLA 2004-04-30 11:43:02 EDT
Because in the GUI context, a model can be dirty while the document is 
still 'clean'. That is because the model has been changed by Gui actions, but 
only when you switch to source page or when you initiate a 'save' operation 
will model be flushed into the doc. 

When a user leaves the source page and goes into any of the GUI pages, the 
model must be clean, period. It must be clean because it is presumably in sync 
with the document. Let me summarize:

Operation               Model    Document
Clean GUi page          clean    clean
Change in the GUI       dirty    clean
Switch to source        dirty    dirty
doc->model sync         clean    dirty
save                    clean    clean
switch to GUI           clean    clean
Change in GUi           dirty    clean

and so on.



Comment 15 Dejan Glozic CLA 2004-04-30 11:59:27 EDT
Just for clarification, here is the method 'isDirty' from PDEFormEditor:

	public boolean isDirty() {
		IFormPage page = getActivePageInstance();
		if ((page != null && page.isDirty()) || 
inputContextManager.isDirty())
			return true;
		return super.isDirty();
	}

A page may be dirty if there are changes in text fields that are yet to be 
committed to the model.

InputContextManager.isDirty:

	public boolean isDirty() {
		for (Enumeration enum=inputContexts.elements(); 
enum.hasMoreElements();) {
			InputContext context = (InputContext)enum.nextElement
();
			if (context.mustSave())
				return true;
		}
		return false;
	}

i.e. input context manager is dirty if at least one input context must be 
saved.

Finally, input context must be saved if:

	public boolean mustSave() {
		if (!fIsSourceMode) {
			if (model instanceof IEditable) {
				if (((IEditable)model).isDirty())
					return true;
			}
		}
		return documentProvider.canSaveDocument(input);
	}
i.e. it must be saved if:

1) model is editable and is dirty
2) document is dirty

Summary:

Editor is dirty if:

1) There are uncommited changes in text fields (in GUI)
2) The model is dirty
3) The document is dirty

We are having a problem because case 2) returns true when we switch from 
source to Dependencies page even though the model should be clean at that 
point.
Comment 16 Dejan Glozic CLA 2004-05-01 12:29:39 EDT
With the fix in the model confirmed as working, do we need to keep this defect 
for the swap scenario?
Comment 17 Wassim Melhem CLA 2004-05-01 17:33:47 EDT
Yes, I think we should do something about the swap scenario.
If two UI ops cancel each other out, and we then switch to the source page.  
The stack of operations is empty, therefore we should not fire a gratuitous 
WORLD_CHANGED event.  This would keep the model in a dirty state.  So if you 
try to close from the source page, you will be prompted to save, even if the 
document was technically untouched.  This would be consistent with the * that 
appears on the editor tab.
Comment 18 Wassim Melhem CLA 2004-05-28 02:09:54 EDT
Closing.