Auto-save for the Dart Editor.

Auto-save for the Dart Editor.

Note: this CL is not done yet, it will include a preference to turn on auto-save.  I wanted to push it out to people to get feedback.

I thought that it would be smart to include it in the reconciliation logic to prevent extra reconciliation from being triggered from the save.  Also, by including it with reconciliation, it prevents us from needing to add a timer/loop to automatically save in the Editor.

http://code.google.com/p/dart/issues/detail?id=2960

[Brian Wilkerson, 2012-10-18 17:53:20]

Other than the fact that I think this whole "feature" is a really bad idea, even
when it's controlled by a flag, (I much prefer Dan's suggestion of
auto-capturing the state of modified editors and restoring them on start-up if
Editor crashes), I have a couple of questions:

Does reconcile get used outside the editor context (such as in a refactoring)?
The editor is where the timer is that you're trying to piggy-back on, reconcile
is just the operation tied to the timer. Seems like a more direct tie-in might
be safer. I'd hate to start auto saving as part of an operation that might be
cancelled.

I think that saving the file, even within reconcile, will cause a resource
changed event to be generated. If so, are we going to duplicate the effort of
updating the model? (Something that's easier for you to test than it is for me
to reason about.)

http://codereview.chromium.org/11186051/diff/1/editor/tools/plugins/com.googl...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/model/CompilationUnitImpl.java
(right):

http://codereview.chromium.org/11186051/diff/1/editor/tools/plugins/com.googl...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/model/CompilationUnitImpl.java:886:
private long lastSavedTime = 0;
This needs to be stored in the info object. The impl objects are just handles
that are created and destroyed all over the place. The info objects are where
long lived data resides.

In addition, I'm not sure this is the right information to store. Let's say the
user opens a file that was last modified and saved two days ago. We will think
we need to auto-save as part of the first reconcile of that file. I'm wondering
if the auto-save interval shouldn't be measured from the time at which the file
was first made dirty rather than the time at which it was last saved.

http://codereview.chromium.org/11186051/diff/1/editor/tools/plugins/com.googl...
File
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/operation/ReconcileWorkingCopyOperation.java
(right):

http://codereview.chromium.org/11186051/diff/1/editor/tools/plugins/com.googl...
editor/tools/plugins/com.google.dart.tools.core/src/com/google/dart/tools/core/internal/operation/ReconcileWorkingCopyOperation.java:125:
System.out.println("\t\t\tReconcileWorkingCopyOperation.makeConsistent() Save
");
nit: debugging output should be removed before commit

http://codereview.chromium.org/11186051/diff/1/editor/tools/plugins/com.googl...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java
(right):

http://codereview.chromium.org/11186051/diff/1/editor/tools/plugins/com.googl...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/CompilationUnitEditor.java:1813:
// Do we want recordSaveTime() as part of the interface, or should we leave it
CompilationUnitImpl
No, we don't want recordSaveTime() to be part of the public API.

[jwren, 2012-10-22 11:56:37]

Updated the CL to include a new preference "Auto save".  I moved the "time since
last saved" into a class that does the does the save for each of the editors.

I moved the "time since last save" value into the "info" object, but it seemed
like the information was not long-lived.  I didn't want to put it back into
CompilationUnitImpl, so I moved it into the new class AutoSaveDartEditorManager,
which I believe is more than sufficient for our use case.

Finally, I did check that by calling the save action like I am, a reconcile call
is not generated, meaning this doesn't set off some infinite loop.

[Brian Wilkerson, 2012-10-22 13:57:17]

I still think this whole "feature" is a really bad idea (I much prefer Dan's
suggestion of auto-capturing the state of modified editors and restoring them on
start-up if Editor crashes).

And I still have concerns about the UX for this feature. We're capturing the
current time at which the editor is opened and pretending that it's the last
time the file was saved. That means that if I open an editor, take a call, then
type one character (followed by enough of a pause to invoke reconcile), the file
will be saved. After that, it won't be saved for 10 seconds. If I'm saving to a
network drive this could introduce a noticeable lag when I type the first
character. Have you tested the performance of the editor with this in place?

In any case, other than one minor nit and the fact that I don't think we should
implement this feature at all, the code LGTM.

http://codereview.chromium.org/11186051/diff/10001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/AutoSaveDartEditorManager.java
(right):

http://codereview.chromium.org/11186051/diff/10001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/AutoSaveDartEditorManager.java:54:
e.printStackTrace();
This should either be replaced by a log message or a comment explaining the
effect of ignoring the exception (or both).

[messick, 2012-10-22 15:02:06]

I'm concerned that reconciliation may not be the best place to hook auto-save.
Long-term, I think we do want auto-save, but it needs to work with all the other
stuff that happens. How does this interact with the remove-trailing-whitespace
feature? How will it interact with the reformat-on-save feature (that is planned
but not implemented)? How does it work with refactoring, which may or may not
need to save? I'd prefer see answers to those questions in a UX design doc,
rather than embedded in a review thread, unless the answers are trivial and I
just haven't woken up enough to realize it.

[danrubel, 2012-10-22 15:03:15]

lgtm

http://codereview.chromium.org/11186051/diff/10001/editor/tools/plugins/com.g...
File
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/AutoSaveDartEditorManager.java
(right):

http://codereview.chromium.org/11186051/diff/10001/editor/tools/plugins/com.g...
editor/tools/plugins/com.google.dart.tools.ui/src/com/google/dart/tools/ui/internal/text/editor/AutoSaveDartEditorManager.java:54:
e.printStackTrace();
On 2012/10/22 13:57:17, Brian Wilkerson wrote:
> This should either be replaced by a log message or a comment explaining the
> effect of ignoring the exception (or both).

+1 for logging

[scheglov, 2012-10-22 15:26:43]

lgtm, but I'm also concerned if auto-save is what we want.
I'd more liked to have saved state and analyze everything on disk and in
editors.