Merge "Fix undo problems in the XML editor (issue #15901)"

2 files changed, 64 insertions, 74 deletions

diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlEditor.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlEditor.java
index bd12e27..7baed28 100644
--- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlEditor.java
+++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/AndroidXmlEditor.java
@@ -794,11 +794,55 @@ public abstract class AndroidXmlEditor extends FormEditor implements IResourceCh
      * @param editAction Something that will change the XML.
      */
     public final void wrapEditXmlModel(Runnable editAction) {
+        wrapEditXmlModel(editAction, null);
+    }
+
+    /**
+     * Executor which performs the given action under an edit lock (and optionally as a
+     * single undo event).
+     *
+     * @param editAction the action to be executed
+     * @param undoLabel if non null, the edit action will be run as a single undo event
+     *            and the label used as the name of the undoable action
+     */
+    private final void wrapEditXmlModel(Runnable editAction, String undoLabel) {
         IStructuredModel model = null;
+        int undoReverseCount = 0;
         try {
+
             if (mIsEditXmlModelPending == 0) {
                 try {
                     model = getModelForEdit();
+                    if (undoLabel != null) {
+                        // Run this action as an undoable unit.
+                        // We have to do it more than once, because in some scenarios
+                        // Eclipse WTP decides to cancel the current undo command on its
+                        // own -- see http://code.google.com/p/android/issues/detail?id=15901
+                        // for one such call chain. By nesting these calls several times
+                        // we've incrementing the command count such that a couple of
+                        // cancellations are ignored. Interfering which this mechanism may
+                        // sound dangerous, but it appears that this undo-termination is
+                        // done for UI reasons to anticipate what the user wants, and we know
+                        // that in *our* scenarios we want the entire unit run as a single
+                        // unit. Here's what the documentation for
+                        // IStructuredTextUndoManager#forceEndOfPendingCommand says
+                        //   "Normally, the undo manager can figure out the best
+                        //    times when to end a pending command and begin a new
+                        //    one ... to the structure of a structured
+                        //    document. There are times, however, when clients may
+                        //    wish to override those algorithms and end one earlier
+                        //    than normal. The one known case is for multi-page
+                        //    editors. If a user is on one page, and type '123' as
+                        //    attribute value, then click around to other parts of
+                        //    page, or different pages, then return to '123|' and
+                        //    type 456, then "undo" they typically expect the undo
+                        //    to just undo what they just typed, the 456, not the
+                        //    whole attribute value."
+                        for (int i = 0; i < 4; i++) {
+                            model.beginRecording(this, undoLabel);
+                            undoReverseCount++;
+                        }
+                    }
                     model.aboutToChangeModel();
                 } catch (Throwable t) {
                     // This is never supposed to happen unless we suddenly don't have a model.
@@ -812,8 +856,18 @@ public abstract class AndroidXmlEditor extends FormEditor implements IResourceCh
         } finally {
             mIsEditXmlModelPending--;
             if (model != null) {
-                // Notify the model we're done modifying it. This must *always* be executed.
-                model.changedModel();
+                try {
+                    // Notify the model we're done modifying it. This must *always* be executed.
+                    model.changedModel();
+
+                    // Clean up the undo unit. This is done more than once as explained
+                    // above for beginRecording.
+                    for (int i = 0; i < undoReverseCount; i++) {
+                        model.endRecording(this);
+                    }
+                } catch (Exception e) {
+                    AdtPlugin.log(e, "Failed to clean up undo unit");
+                }
                 model.releaseFromEdit();
 
                 if (mIsEditXmlModelPending < 0) {
@@ -828,7 +882,7 @@ public abstract class AndroidXmlEditor extends FormEditor implements IResourceCh
 
     /**
      * Creates an "undo recording" session by calling the undoableAction runnable
-     * using {@link #beginUndoRecording(String)} and {@link #endUndoRecording()}.
+     * under an undo session.
      * <p/>
      * This also automatically starts an edit XML session, as if
      * {@link #wrapEditXmlModel(Runnable)} had been called.
@@ -838,88 +892,24 @@ public abstract class AndroidXmlEditor extends FormEditor implements IResourceCh
      *
      * @param label The label for the undo operation. Can be null. Ideally we should really try
      *              to put something meaningful if possible.
+     * @param undoableAction the action to be run as a single undoable unit
      */
     public void wrapUndoEditXmlModel(String label, Runnable undoableAction) {
-        boolean recording = false;
-        try {
-            recording = beginUndoRecording(label);
-
-            if (!recording) {
-                // This can only happen if we don't have an underlying model to edit
-                // or it's not a structured document, which in this context is
-                // highly unlikely. Abort the operation in this case.
-                AdtPlugin.logAndPrintError(
-                    null, //exception,
-                    getProject() != null ? getProject().getName() : "XML Editor", //$NON-NLS-1$ //tag
-                    "Action '%s' failed: could not start an undo session, document might be corrupt.", //$NON-NLS-1$
-                    label);
-                return;
-            }
-
-            wrapEditXmlModel(undoableAction);
-        } finally {
-            if (recording) {
-                endUndoRecording();
-            }
-        }
+        assert label != null : "All undoable actions should have a label";
+        wrapEditXmlModel(undoableAction, label == null ? "" : label); //$NON-NLS-1$
     }
 
     /**
      * Returns true when the runnable of {@link #wrapEditXmlModel(Runnable)} is currently
-     * being executed. This means it is safe to actually edit the XML model returned
-     * by {@link #getModelForEdit()}.
+     * being executed. This means it is safe to actually edit the XML model.
+     *
+     * @return true if the XML model is already locked for edits
      */
     public boolean isEditXmlModelPending() {
         return mIsEditXmlModelPending > 0;
     }
 
     /**
-     * Starts an "undo recording" session. This is managed by the underlying undo manager
-     * associated to the structured XML model.
-     * <p/>
-     * There <em>must</em> be a corresponding call to {@link #endUndoRecording()}.
-     * <p/>
-     * beginUndoRecording/endUndoRecording calls can be nested (inner calls are ignored, only one
-     * undo operation is recorded.)
-     * To guarantee that, only access this via {@link #wrapUndoEditXmlModel(String, Runnable)}.
-     *
-     * @param label The label for the undo operation. Can be null but we should really try to put
-     *              something meaningful if possible.
-     * @return True if the undo recording actually started, false if any kind of error occurred.
-     *         {@link #endUndoRecording()} should only be called if True is returned.
-     */
-    private boolean beginUndoRecording(String label) {
-        IStructuredModel model = getModelForEdit();
-        if (model != null) {
-            try {
-                model.beginRecording(this, label);
-                return true;
-            } finally {
-                model.releaseFromEdit();
-            }
-        }
-        return false;
-    }
-
-    /**
-     * Ends an "undo recording" session.
-     * <p/>
-     * This is the counterpart call to {@link #beginUndoRecording(String)} and should only be
-     * used if the initial call returned true.
-     * To guarantee that, only access this via {@link #wrapUndoEditXmlModel(String, Runnable)}.
-     */
-    private void endUndoRecording() {
-        IStructuredModel model = getModelForEdit();
-        if (model != null) {
-            try {
-                model.endRecording(this);
-            } finally {
-                model.releaseFromEdit();
-            }
-        }
-    }
-
-    /**
      * Returns the XML {@link Document} or null if we can't get it
      */
     protected final Document getXmlDocument(IStructuredModel model) {
diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/gre/RulesEngine.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/gre/RulesEngine.java
index 5b2d430..49a4b01 100755
--- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/gre/RulesEngine.java
+++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/gre/RulesEngine.java
@@ -535,7 +535,7 @@ public class RulesEngine {
         // (For example, a ScrollView parent can go and set all its children's layout params to
         // fill the parent.)
         if (!editor.isEditXmlModelPending()) {
-            editor.wrapUndoEditXmlModel("Customize creation", new Runnable() {
+            editor.wrapEditXmlModel(new Runnable() {
                 public void run() {
                     callCreateHooks(editor, insertType,
                             parentRule, parentNode, childRule, newNode);