Fix issue #160, crashes undoing critical actions like "remove layer" or "remove frame".

The OpenGroup & CloseGroup undoers now save the current SpritePosition
when the Undoer instance is created. Before we were using the current
Layer and Frame at the moment of the OpenGroup creation, it was generating
buggy undoers.

src/CMakeLists.txt

@@ -233,7 +233,6 @@ add_library(aseprite-library
   commands/cmd_paste.cpp
   commands/cmd_play_animation.cpp
   commands/cmd_preview.cpp
-  commands/cmd_redo.cpp
   commands/cmd_refresh.cpp
   commands/cmd_remove_cel.cpp
   commands/cmd_remove_frame.cpp

src/commands/cmd_redo.cpp

@@ -1,96 +0,0 @@
-/* ASEPRITE
- * Copyright (C) 2001-2012  David Capello
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- */
-
-#include "config.h"
-
-#include "app.h"
-#include "base/thread.h"
-#include "commands/command.h"
-#include "document_undo.h"
-#include "document_wrappers.h"
-#include "ini_file.h"
-#include "modules/editors.h"
-#include "modules/gui.h"
-#include "raster/sprite.h"
-#include "widgets/editor/editor.h"
-#include "widgets/status_bar.h"
-
-class RedoCommand : public Command
-{
-public:
-  RedoCommand();
-  Command* clone() { return new RedoCommand(*this); }
-
-protected:
-  bool onEnabled(Context* context);
-  void onExecute(Context* context);
-};
-
-RedoCommand::RedoCommand()
-  : Command("Redo",
-            "Redo",
-            CmdUIOnlyFlag)
-{
-}
-
-bool RedoCommand::onEnabled(Context* context)
-{
-  ActiveDocumentWriter document(context);
-  return
-    document != NULL &&
-    document->getUndo()->canRedo();
-}
-
-void RedoCommand::onExecute(Context* context)
-{
-  ActiveDocumentWriter document(context);
-  DocumentUndo* undo = document->getUndo();
-  Sprite* sprite = document->getSprite();
-
-  if (get_config_bool("Options", "UndoGotoModified", true)) {
-    if (undo->getNextRedoFrame() != sprite->getCurrentFrame() ||
-        undo->getNextRedoLayer() != sprite->getCurrentLayer()) {
-      sprite->setCurrentFrame(undo->getNextRedoFrame());
-      sprite->setCurrentLayer(undo->getNextRedoLayer());
-
-      current_editor->drawSpriteSafe(0, 0, sprite->getWidth(), sprite->getHeight());
-      update_screen_for_document(document);
-      gui_feedback();
-
-      base::this_thread::sleep_for(0.01);
-    }
-  }
-
-  StatusBar::instance()
-    ->showTip(1000, "Redid %s",
-              undo->getNextRedoLabel());
-
-  undo->doRedo();
-  document->generateMaskBoundaries();
-  document->destroyExtraCel(); // Regenerate extras
-
-  update_screen_for_document(document);
-}
-
-//////////////////////////////////////////////////////////////////////
-// CommandFactory
-
-Command* CommandFactory::createRedoCommand()
-{
-  return new RedoCommand;
-}

src/commands/cmd_undo.cpp

@@ -33,18 +33,24 @@
 class UndoCommand : public Command
 {
 public:
-  UndoCommand();
+  enum Type { Undo, Redo };
+
+  UndoCommand(Type type);
   Command* clone() { return new UndoCommand(*this); }
 
 protected:
   bool onEnabled(Context* context);
   void onExecute(Context* context);
+
+private:
+  Type m_type;
 };
 
-UndoCommand::UndoCommand()
-  : Command("Undo",
-            "Undo",
+UndoCommand::UndoCommand(Type type)
+  : Command((type == Undo ? "Undo": "Redo"),
+            (type == Undo ? "Undo": "Redo"),
             CmdUIOnlyFlag)
+  , m_type(type)
 {
 }
 
@@ -53,7 +59,8 @@ bool UndoCommand::onEnabled(Context* context)
   ActiveDocumentWriter document(context);
   return
     document != NULL &&
-    document->getUndo()->canUndo();
+    ((m_type == Undo ? document->getUndo()->canUndo():
+                       document->getUndo()->canRedo()));
 }
 
 void UndoCommand::onExecute(Context* context)
@@ -63,10 +70,15 @@ void UndoCommand::onExecute(Context* context)
   Sprite* sprite = document->getSprite();
 
   if (get_config_bool("Options", "UndoGotoModified", true)) {
-    if (undo->getNextUndoFrame() != sprite->getCurrentFrame() ||
-        undo->getNextUndoLayer() != sprite->getCurrentLayer()) {
-      sprite->setCurrentFrame(undo->getNextUndoFrame());
-      sprite->setCurrentLayer(undo->getNextUndoLayer());
+    SpritePosition spritePosition;
+
+    if (m_type == Undo)
+      spritePosition = undo->getNextUndoSpritePosition();
+    else
+      spritePosition = undo->getNextRedoSpritePosition();
+
+    if (spritePosition != sprite->getCurrentPosition()) {
+      sprite->setCurrentPosition(spritePosition);
 
       current_editor->drawSpriteSafe(0, 0, sprite->getWidth(), sprite->getHeight());
       update_screen_for_document(document);
@@ -77,10 +89,16 @@ void UndoCommand::onExecute(Context* context)
   }
 
   StatusBar::instance()
-    ->showTip(1000, "Undid %s",
-              undo->getNextUndoLabel());
+    ->showTip(1000, "%s %s",
+              (m_type == Undo ? "Undid": "Redid"),
+              (m_type == Undo ? undo->getNextUndoLabel():
+                                undo->getNextRedoLabel()));
+
+  if (m_type == Undo)
+    undo->doUndo();
+  else
+    undo->doRedo();
 
-  undo->doUndo();
   document->generateMaskBoundaries();
   document->destroyExtraCel(); // Regenerate extras
 
@@ -92,5 +110,10 @@ void UndoCommand::onExecute(Context* context)
 
 Command* CommandFactory::createUndoCommand()
 {
-  return new UndoCommand;
+  return new UndoCommand(UndoCommand::Undo);
+}
+
+Command* CommandFactory::createRedoCommand()
+{
+  return new UndoCommand(UndoCommand::Redo);
 }

src/document_undo.cpp

@@ -101,24 +101,14 @@ const char* DocumentUndo::getNextRedoLabel() const
   return getNextRedoGroup()->getLabel();
 }
 
-Layer* DocumentUndo::getNextUndoLayer() const
+SpritePosition DocumentUndo::getNextUndoSpritePosition() const
 {
-  return getNextUndoGroup()->getLayer(m_objects);
+  return getNextUndoGroup()->getSpritePosition();
 }
 
-Layer* DocumentUndo::getNextRedoLayer() const
+SpritePosition DocumentUndo::getNextRedoSpritePosition() const
 {
-  return getNextRedoGroup()->getLayer(m_objects);
-}
-
-FrameNumber DocumentUndo::getNextUndoFrame() const
-{
-  return getNextUndoGroup()->getFrame();
-}
-
-FrameNumber DocumentUndo::getNextRedoFrame() const
-{
-  return getNextRedoGroup()->getFrame();
+  return getNextRedoGroup()->getSpritePosition();
 }
 
 undoers::CloseGroup* DocumentUndo::getNextUndoGroup() const

src/document_undo.h

@@ -22,12 +22,10 @@
 #include "base/compiler_specific.h"
 #include "base/disable_copying.h"
 #include "base/unique_ptr.h"
-#include "raster/frame_number.h"
+#include "raster/sprite_position.h"
 #include "undo/undo_config_provider.h"
 #include "undo/undo_history.h"
 
-class Layer;
-
 namespace undo {
   class ObjectsContainer;
   class Undoer;
@@ -65,11 +63,8 @@ public:
   const char* getNextUndoLabel() const;
   const char* getNextRedoLabel() const;
 
-  Layer* getNextUndoLayer() const;
-  Layer* getNextRedoLayer() const;
-
-  FrameNumber getNextUndoFrame() const;
-  FrameNumber getNextRedoFrame() const;
+  SpritePosition getNextUndoSpritePosition() const;
+  SpritePosition getNextRedoSpritePosition() const;
 
 private:
   size_t getUndoSizeLimit() OVERRIDE;

src/undo_transaction.cpp

@@ -83,8 +83,7 @@ UndoTransaction::UndoTransaction(Document* document, const char* label, undo::Mo
     m_undo->pushUndoer(new undoers::OpenGroup(getObjects(),
                                               m_label,
                                               m_modification,
-                                              m_startLayer = m_sprite->getCurrentLayer(),
-                                              m_startFrame = m_sprite->getCurrentFrame()));
+                                              m_sprite));
 }
 
 UndoTransaction::~UndoTransaction()
@@ -115,8 +114,7 @@ void UndoTransaction::closeUndoGroup()
     m_undo->pushUndoer(new undoers::CloseGroup(getObjects(),
                                                m_label,
                                                m_modification,
-                                               m_startLayer,
-                                               m_startFrame));
+                                               m_sprite));
     m_closed = true;
   }
 }

src/undo_transaction.h

@@ -147,8 +147,6 @@ private:
   bool m_enabledFlag;
   const char* m_label;
   undo::Modification m_modification;
-  Layer* m_startLayer;
-  FrameNumber m_startFrame;
 };
 
 #endif

src/undoers/close_group.cpp

@@ -20,6 +20,7 @@
 
 #include "undoers/close_group.h"
 
+#include "raster/sprite.h"
 #include "undo/objects_container.h"
 #include "undo/undoers_collector.h"
 #include "undoers/open_group.h"
@@ -27,11 +28,14 @@
 using namespace undo;
 using namespace undoers;
 
-CloseGroup::CloseGroup(undo::ObjectsContainer* objects, const char* label, undo::Modification modification, Layer* layer, FrameNumber frame)
+CloseGroup::CloseGroup(undo::ObjectsContainer* objects,
+                       const char* label,
+                       undo::Modification modification,
+                       Sprite* sprite)
   : m_label(label)
   , m_modification(modification)
-  , m_activeLayerId(objects->addObject(layer))
-  , m_activeFrame(frame)
+  , m_spriteId(objects->addObject(sprite))
+  , m_spritePosition(sprite->getCurrentPosition())
 {
 }
 
@@ -42,11 +46,7 @@ void CloseGroup::dispose()
 
 void CloseGroup::revert(ObjectsContainer* objects, UndoersCollector* redoers)
 {
-  redoers->pushUndoer(new OpenGroup(objects, m_label, m_modification,
-                                    getLayer(objects), m_activeFrame));
-}
+  Sprite* sprite = objects->getObjectT<Sprite>(m_spriteId);
 
-Layer* CloseGroup::getLayer(undo::ObjectsContainer* objects)
-{
-  return objects->getObjectT<Layer>(m_activeLayerId);
+  redoers->pushUndoer(new OpenGroup(objects, m_label, m_modification, sprite));
 }

src/undoers/close_group.h

@@ -20,18 +20,22 @@
 #define UNDOERS_CLOSE_GROUP_H_INCLUDED
 
 #include "base/compiler_specific.h"
-#include "raster/frame_number.h"
+#include "raster/sprite_position.h"
 #include "undo/object_id.h"
 #include "undo/undoer.h"
 
-class Layer;
+class Sprite;
 
 namespace undoers {
 
 class CloseGroup : public undo::Undoer
 {
 public:
-  CloseGroup(undo::ObjectsContainer* objects, const char* label, undo::Modification modification, Layer* layer, FrameNumber frame);
+  CloseGroup(undo::ObjectsContainer* objects,
+             const char* label,
+             undo::Modification modification,
+             Sprite* sprite);
+
   void dispose() OVERRIDE;
   size_t getMemSize() const OVERRIDE { return sizeof(*this); }
   undo::Modification getModification() const { return m_modification; }
@@ -40,14 +44,13 @@ public:
   void revert(undo::ObjectsContainer* objects, undo::UndoersCollector* redoers) OVERRIDE;
 
   const char* getLabel() { return m_label; }
-  FrameNumber getFrame() { return m_activeFrame; }
-  Layer* getLayer(undo::ObjectsContainer* objects);
+  const SpritePosition& getSpritePosition() { return m_spritePosition; }
 
 private:
   const char* m_label;
   undo::Modification m_modification;
-  undo::ObjectId m_activeLayerId;
-  FrameNumber m_activeFrame;
+  undo::ObjectId m_spriteId;
+  SpritePosition m_spritePosition;
 };
 
 } // namespace undoers

src/undoers/open_group.cpp

@@ -20,6 +20,7 @@
 
 #include "undoers/open_group.h"
 
+#include "raster/sprite.h"
 #include "undo/objects_container.h"
 #include "undo/undoers_collector.h"
 #include "undoers/close_group.h"
@@ -27,11 +28,11 @@
 using namespace undo;
 using namespace undoers;
 
-OpenGroup::OpenGroup(undo::ObjectsContainer* objects, const char* label, undo::Modification modification, Layer* layer, FrameNumber frame)
+OpenGroup::OpenGroup(undo::ObjectsContainer* objects, const char* label, undo::Modification modification, Sprite* sprite)
   : m_label(label)
   , m_modification(modification)
-  , m_activeLayerId(objects->addObject(layer))
-  , m_activeFrame(frame)
+  , m_spriteId(objects->addObject(sprite))
+  , m_spritePosition(sprite->getCurrentPosition())
 {
 }
 
@@ -42,7 +43,7 @@ void OpenGroup::dispose()
 
 void OpenGroup::revert(ObjectsContainer* objects, UndoersCollector* redoers)
 {
-  Layer* layer = objects->getObjectT<Layer>(m_activeLayerId);
+  Sprite* sprite = objects->getObjectT<Sprite>(m_spriteId);
 
-  redoers->pushUndoer(new CloseGroup(objects, m_label, m_modification, layer, m_activeFrame));
+  redoers->pushUndoer(new CloseGroup(objects, m_label, m_modification, sprite));
 }

src/undoers/open_group.h

@@ -20,7 +20,7 @@
 #define UNDOERS_OPEN_GROUP_H_INCLUDED
 
 #include "base/compiler_specific.h"
-#include "raster/frame_number.h"
+#include "raster/sprite_position.h"
 #include "undo/object_id.h"
 #include "undo/undoer.h"
 
@@ -31,7 +31,7 @@ namespace undoers {
 class OpenGroup : public undo::Undoer
 {
 public:
-  OpenGroup(undo::ObjectsContainer* objects, const char* label, undo::Modification modification, Layer* layer, FrameNumber frame);
+  OpenGroup(undo::ObjectsContainer* objects, const char* label, undo::Modification modification, Sprite* sprite);
   void dispose() OVERRIDE;
   size_t getMemSize() const OVERRIDE { return sizeof(*this); }
   undo::Modification getModification() const { return m_modification; }
@@ -39,11 +39,13 @@ public:
   bool isCloseGroup() const OVERRIDE { return false; }
   void revert(undo::ObjectsContainer* objects, undo::UndoersCollector* redoers) OVERRIDE;
 
+  const SpritePosition& getSpritePosition() { return m_spritePosition; }
+
 private:
   const char* m_label;
   undo::Modification m_modification;
-  undo::ObjectId m_activeLayerId;
-  FrameNumber m_activeFrame;
+  undo::ObjectId m_spriteId;
+  SpritePosition m_spritePosition;
 };
 
 } // namespace undoers