Add ObjectsContainer interface in src/undo/ so UndoHistory has a way to

obtain any kind of objects (not just GfxObj).
+ Removed GfxObjId.
+ Added ObjectsContainerImpl.
+ Use UniquePtr for each member in Document to avoid memory leaks in
  Document() ctor.
+ Removed RasterModule class.

src/CMakeLists.txt

@@ -80,6 +80,7 @@ add_library(aseprite-library
   job.cpp
   launcher.cpp
   log.cpp
+  objects_container_impl.cpp
   recent_files.cpp
   resource_finder.cpp
   ui_context.cpp

src/app.cpp

@@ -80,7 +80,6 @@ public:
   // ASE Modules
   FileSystemModule m_file_system_module;
   ToolBox m_toolbox;
-  RasterModule m_raster;
   CommandsModule m_commands_modules;
   UIContext m_ui_context;
   RecentFiles m_recent_files;

src/document.cpp

@@ -25,6 +25,7 @@
 #include "base/scoped_lock.h"
 #include "base/unique_ptr.h"
 #include "file/format_options.h"
+#include "objects_container_impl.h"
 #include "raster/cel.h"
 #include "raster/layer.h"
 #include "raster/mask.h"
@@ -37,7 +38,8 @@
 Document::Document(Sprite* sprite)
   : m_id(WithoutDocumentId)
   , m_sprite(sprite)
-  , m_undoHistory(new UndoHistory(sprite))
+  , m_objects(new ObjectsContainerImpl)
+  , m_undoHistory(new UndoHistory(m_objects))
   , m_filename("Sprite")
   , m_associated_to_file(false)
   , m_mutex(new Mutex)
@@ -48,6 +50,9 @@ Document::Document(Sprite* sprite)
     // Extra cel
   , m_extraCel(NULL)
   , m_extraImage(NULL)
+  // Mask
+  , m_mask(new Mask())
+  , m_maskVisible(true)
 {
   // Boundary stuff
   m_bound.nseg = 0;
@@ -58,10 +63,6 @@ Document::Document(Sprite* sprite)
   m_preferred.scroll_y = 0;
   m_preferred.zoom = 0;
   m_preferred.virgin = true;
-
-  // Mask
-  m_mask = new Mask();
-  m_maskVisible = true;
 }
 
 Document::~Document()
@@ -70,11 +71,6 @@ Document::~Document()
     base_free(m_bound.seg);
 
   destroyExtraCel();
-
-  delete m_mutex;
-  delete m_undoHistory;
-  delete m_sprite;
-  delete m_mask;
 }
 
 Document* Document::createBasicDocument(int imgtype, int width, int height, int ncolors)
@@ -271,10 +267,7 @@ Mask* Document::getMask() const
 
 void Document::setMask(const Mask* mask)
 {
-  if (m_mask)
+  m_mask.reset(mask_new_copy(mask));
-    mask_free(m_mask);
-
-  m_mask = mask_new_copy(mask);
   m_maskVisible = true;
 }
 

src/document.h

@@ -21,6 +21,7 @@
 
 #include "base/disable_copying.h"
 #include "base/shared_ptr.h"
+#include "base/unique_ptr.h"
 #include "document_id.h"
 
 #include <string>
@@ -30,6 +31,7 @@ class FormatOptions;
 class Image;
 class Mask;
 class Mutex;
+class ObjectsContainer;
 class Sprite;
 class UndoHistory;
 struct _BoundSeg;
@@ -158,8 +160,9 @@ private:
   // Unique identifier for this document (it is assigned by Documents class).
   DocumentId m_id;
 
-  Sprite* m_sprite;
+  UniquePtr<Sprite> m_sprite;
-  UndoHistory* m_undoHistory;
+  UniquePtr<ObjectsContainer> m_objects;
+  UniquePtr<UndoHistory> m_undoHistory;
 
   // Document's file name (from where it was loaded, where it is saved).
   std::string m_filename;
@@ -195,7 +198,7 @@ private:
   Image* m_extraImage;
 
   // Current mask.
-  Mask* m_mask;
+  UniquePtr<Mask> m_mask;
   bool m_maskVisible;
 
   DISABLE_COPYING(Document);

src/objects_container_impl.cpp

@@ -0,0 +1,94 @@
+/* ASE - Allegro Sprite Editor
+ * Copyright (C) 2001-2011  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 "objects_container_impl.h"
+
+ObjectsContainerImpl::ObjectsContainerImpl()
+{
+  m_idCounter = 0;
+}
+
+ObjectsContainerImpl::~ObjectsContainerImpl()
+{
+}
+
+ObjectId ObjectsContainerImpl::addObject(void* object)
+{
+  // First we check if the object is already in the container.
+  std::map<void*, ObjectId>::iterator it = m_ptrToId.find(object);
+  if (it != m_ptrToId.end())
+    return it->second;		// So we return the already assigned ID
+
+  // In other case we add the new object
+  ObjectId id = ++m_idCounter;
+
+  m_idToPtr[id] = object;
+  m_ptrToId[object] = id;
+
+  return id;
+}
+
+void ObjectsContainerImpl::insertObject(ObjectId id, void* object)
+{
+  std::map<ObjectId, void*>::iterator it1 = m_idToPtr.find(id);
+  if (it1 != m_idToPtr.end()) {
+    ASSERT(false && "You've inserted two times the same object");
+    throw ExistentObjectException();
+  }
+
+  std::map<void*, ObjectId>::iterator it2 = m_ptrToId.find(object);
+  if (it2 != m_ptrToId.end()) {
+    ASSERT(false && "You've inserted two times the same object");
+    throw ExistentObjectException();
+  }
+
+  m_idToPtr[id] = object;
+  m_ptrToId[object] = id;
+}
+
+void ObjectsContainerImpl::removeObject(ObjectId id)
+{
+  std::map<ObjectId, void*>::iterator it1 = m_idToPtr.find(id);
+  if (it1 == m_idToPtr.end()) {
+    ASSERT(false && "You want to remove a non-existent object");
+    throw ObjectNotFoundException();
+  }
+
+  void* ptr = it1->second;
+  std::map<void*, ObjectId>::iterator it2 = m_ptrToId.find(ptr);
+  if (it2 == m_ptrToId.end()) {
+    ASSERT(false && "You want to remove a non-existent object");
+    throw ObjectNotFoundException();
+  }
+
+  m_idToPtr.erase(it1);
+  m_ptrToId.erase(it2);
+}
+
+void* ObjectsContainerImpl::getObject(ObjectId id)
+{
+  std::map<ObjectId, void*>::iterator it = m_idToPtr.find(id);
+  if (it == m_idToPtr.end()) {
+    ASSERT(false && "You want to get information about a non-existent object");
+    throw ObjectNotFoundException();
+  }
+
+  return it->second;
+}

src/objects_container_impl.h

@@ -1,24 +1,45 @@
-/* ASE - Allegro Sprite Editor
+/* ASE - Allegro Sprite Editor
- * Copyright (C) 2001-2011  David Capello
+ * Copyright (C) 2001-2011  David Capello
- *
+ *
- * This program is free software; you can redistribute it and/or modify
+ * 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
+ * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
+ * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
+ * (at your option) any later version.
- *
+ *
- * This program is distributed in the hope that it will be useful,
+ * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
+ * GNU General Public License for more details.
- *
+ *
- * You should have received a copy of the GNU General Public License
+ * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
+ * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- */
+ */
-
+
-#ifndef RASTER_GFXOBJ_ID_H_INCLUDED
+#ifndef DOC_OBJECTS_CONTAINER_H_INCLUDED
-#define RASTER_GFXOBJ_ID_H_INCLUDED
+#define DOC_OBJECTS_CONTAINER_H_INCLUDED
-
+
-typedef unsigned int GfxObjId;
+#include "undo/objects_container.h"
-
+
-#endif
+#include <map>
+
+class ObjectsContainerImpl : public ObjectsContainer
+{
+public:
+  ObjectsContainerImpl();
+  ~ObjectsContainerImpl();
+
+  // ObjectsContainer Implementation
+
+  ObjectId addObject(void* object);
+  void insertObject(ObjectId id, void* object);
+  void removeObject(ObjectId id);
+  void* getObject(ObjectId id);
+
+private:
+  ObjectId m_idCounter;
+  std::map<ObjectId, void*> m_idToPtr;
+  std::map<void*, ObjectId> m_ptrToId;
+};
+
+#endif

src/objects_container_impl_unittest.cpp

@@ -0,0 +1,83 @@
+/* ASE - Allegro Sprite Editor
+ * Copyright (C) 2001-2011  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 <gtest/gtest.h>
+
+#include "objects_container_impl.h"
+
+TEST(ObjectsContainerImpl, AddObjectReturnsSameIdForSameObject)
+{
+  ObjectsContainerImpl objs;
+  int a, b;
+
+  ObjectId idA = objs.addObject(&a);
+  ObjectId idB = objs.addObject(&b);
+
+  EXPECT_NE(idA, idB);
+
+  EXPECT_EQ(idA, objs.addObject(&a));
+  EXPECT_EQ(idB, objs.addObject(&b));
+}
+
+TEST(ObjectsContainerImpl, GetObjectAndRemoveObject)
+{
+  ObjectsContainerImpl objs;
+  int a, b;
+
+  ObjectId idA = objs.addObject(&a);
+  ObjectId idB = objs.addObject(&b);
+  EXPECT_EQ(&a, objs.getObjectT<int>(idA));
+  EXPECT_EQ(&b, objs.getObjectT<int>(idB));
+
+  objs.removeObject(idA);
+  objs.removeObject(idB);
+  EXPECT_THROW(objs.getObject(idA), ObjectNotFoundException);
+  EXPECT_THROW(objs.getObject(idB), ObjectNotFoundException);
+
+  EXPECT_THROW(objs.removeObject(idA), ObjectNotFoundException);
+  EXPECT_THROW(objs.removeObject(idB), ObjectNotFoundException);
+}
+
+TEST(ObjectsContainerImpl, InsertExistObjectsThrows)
+{
+  ObjectsContainerImpl objs;
+  int a, b;
+
+  ObjectId id1 = objs.addObject(&a);
+  ObjectId id2 = id1 + 1;
+
+  // Existent ID and pointer
+  EXPECT_THROW(objs.insertObject(id1, &a), ExistentObjectException);
+
+  // Existent pointer
+  EXPECT_THROW(objs.insertObject(id2, &a), ExistentObjectException);
+
+  // Existent ID
+  EXPECT_THROW(objs.insertObject(id1, &b), ExistentObjectException);
+
+  // OK, new object with new ID
+  EXPECT_NO_THROW(objs.insertObject(id2, &b));
+}
+
+int main(int argc, char** argv)
+{
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}

src/raster/gfxobj.cpp

@@ -20,114 +20,21 @@
 
 #include "raster/gfxobj.h"
 
-#include "base/mutex.h"
-#include "base/scoped_lock.h"
-
-#include <cstring>
-#include <map>
-#include <utility>
-
-static Mutex* objects_mutex;
-static GfxObjId object_id = 0;		         // Last object ID created
-static std::map<GfxObjId, GfxObj*>* objects_map; // Graphics objects map
-
-static void insert_gfxobj(GfxObj* gfxobj);
-static void erase_gfxobj(GfxObj* gfxobj);
-
-RasterModule::RasterModule()
-{
-  objects_map = new std::map<GfxObjId, GfxObj*>;
-  objects_mutex = new Mutex();
-}
-
-RasterModule::~RasterModule()
-{
-#ifndef MEMLEAK
-  ASSERT(objects_map->empty());
-#endif
-
-  delete objects_map;
-  delete objects_mutex;
-}
-
-//////////////////////////////////////////////////////////////////////
-// GfxObj class
-
 GfxObj::GfxObj(GfxObjType type)
 {
   m_type = type;
-  assign_id();
 }
 
 GfxObj::GfxObj(const GfxObj& gfxobj)
 {
   m_type = gfxobj.m_type;
-  assign_id();
 }
 
 GfxObj::~GfxObj()
 {
-  ScopedLock lock(*objects_mutex);
-
-  // we have to remove this object from the map
-  erase_gfxobj(this);
 }
 
 int GfxObj::getMemSize() const
 {
   return sizeof(GfxObj);
 }
-
-void GfxObj::assign_id()
-{
-  ScopedLock lock(*objects_mutex);
-
-  // we have to assign an ID for this object
-  m_id = ++object_id;
-
-  // and here we add the object in the map of graphics-objects
-  insert_gfxobj(this);
-}
-
-// static
-GfxObj* GfxObj::find(GfxObjId id)
-{
-  GfxObj* ret = NULL;
-  {
-    ScopedLock lock(*objects_mutex);
-
-    std::map<GfxObjId, GfxObj*>::iterator
-      it = objects_map->find(id);
-
-    if (it != objects_map->end())
-      ret = it->second;
-  }
-  return ret;
-}
-
-void GfxObj::_setGfxObjId(GfxObjId id)
-{
-  ASSERT(find(m_id) == this);
-  ASSERT(find(id) == NULL);
-
-  ScopedLock lock(*objects_mutex);
-
-  erase_gfxobj(this);		// Remove the object from the map
-  m_id = id;			// Change the ID
-  insert_gfxobj(this);		// Insert the object again in the map
-}
-
-static void insert_gfxobj(GfxObj* gfxobj)
-{
-  objects_map->insert(std::make_pair(gfxobj->getId(), gfxobj));
-}
-
-static void erase_gfxobj(GfxObj* gfxobj)
-{
-  std::map<GfxObjId, GfxObj*>::iterator it
-    = objects_map->find(gfxobj->getId());
-  
-  ASSERT(it != objects_map->end());
-
-  objects_map->erase(it);
-}

src/raster/gfxobj.h

@@ -20,7 +20,6 @@
 #define RASTER_GFXOBJ_H_INCLUDED
 
 #include <list>
-#include "raster/gfxobj_id.h"
 
 enum GfxObjType {
   GFXOBJ_CEL,
@@ -32,7 +31,6 @@ enum GfxObjType {
   GFXOBJ_PATH,
   GFXOBJ_SPRITE,
   GFXOBJ_STOCK,
-  GFXOBJ_UNDO,
   GFXOBJ_RGBMAP,
 };
 
@@ -54,35 +52,16 @@ public:
   GfxObj(const GfxObj& gfxobj);
   virtual ~GfxObj();
 
-  GfxObjId getId() const { return m_id; }
   GfxObjType getType() const { return m_type; }
 
   // Returns the approximate amount of memory (in bytes) which this
   // object use.
   virtual int getMemSize() const;
 
-  // Returns a GfxObj by its ID. If it is not found, returns NULL.
-  static GfxObj* find(GfxObjId id);
-
-  // Changes the ID of an existent GfxObj. This function is used by
-  // Undo to change the ID of objects that were back to life (and
-  // should not be used in other places).
-  void _setGfxObjId(GfxObjId id);
-
 private:
-  void assign_id();
-
   GfxObjType m_type;
-  GfxObjId m_id;
 
   GfxObj& operator=(const GfxObj&);
 };
 
-class RasterModule
-{
-public:
-  RasterModule();
-  ~RasterModule();
-};
-
 #endif

src/raster/rgbmap.cpp

@@ -18,17 +18,18 @@
 
 #include "config.h"
 
-#include <allegro.h>
+#include "raster/rgbmap.h"
 
 #include "raster/palette.h"
-#include "raster/rgbmap.h"
+
+#include <allegro.h>
 
 class RgbMapImpl
 {
 public:
   RgbMapImpl() {
     m_allegMap = new RGB_MAP;
-    m_palette_id = 0;
+    m_palette = NULL;
     m_modifications = 0;
   }
 
@@ -37,12 +38,12 @@ public:
   }
 
   bool match(const Palette* palette) const {
-    return (m_palette_id == palette->getId() &&
+    return (m_palette == palette &&
 	    m_modifications == palette->getModifications());
   }
 
   void regenerate(const Palette* palette) {
-    m_palette_id = palette->getId();
+    m_palette = palette;
     m_modifications = palette->getModifications();
 
     PALETTE allegPal;
@@ -68,7 +69,7 @@ public:
 
 private:
   RGB_MAP* m_allegMap;
-  GfxObjId m_palette_id;
+  const Palette* m_palette;
   int m_modifications;
 };
 

src/undo/objects_container.h

@@ -0,0 +1,93 @@
+/* ASE - Allegro Sprite Editor
+ * Copyright (C) 2001-2011  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
+ */
+
+#ifndef UNDO_OBJECTS_CONTAINER_H_INCLUDED
+#define UNDO_OBJECTS_CONTAINER_H_INCLUDED
+
+#include "base/exception.h"
+
+typedef ase_uint32 ObjectId;
+
+// Thrown when you use ObjectsContainer::insertObject() with an
+// existent ID or pointer.
+class ExistentObjectException : base::Exception
+{
+public:
+  ExistentObjectException() { }
+};
+
+// Thrown when an object is not found when
+// ObjectsContainer::getObject() or removeObject() methods are used.
+class ObjectNotFoundException : base::Exception
+{
+public:
+  ObjectNotFoundException() { }
+};
+
+// A container of any kind of object used to generate serializable
+// references (ID) to instances in memory. It converts a pointer to a
+// 32-bits ID, and then you can get back the original object pointer
+// from the ID.
+//
+// If the original pointer is deleted, you must use removeObject(),
+// and if the object is re-created (e.g. by an undo operation),
+// the object can be added back to the container using insertObject().
+class ObjectsContainer
+{
+public:
+  virtual ~ObjectsContainer() { };
+
+  // Adds an object in the container and returns an ID, so then you
+  // can retrieve the original object pointer using getObject()
+  // method. If the object is already in the container, the returned
+  // ID must be the same as the already assigned. It means that this
+  // method cannot return different IDs for the same given "void*"
+  // pointer.
+  virtual ObjectId addObject(void* object) = 0;
+
+  // Inserts an existent object with the specific ID. If an object
+  // with the given ID or the pointer already exist in the container,
+  // it should throw an ExistentObjectException. This method is used
+  // to insert back in the container a previously removed object
+  // with a removeObject() call.
+  virtual void insertObject(ObjectId id, void* object) = 0;
+
+  // Removes the given object from the container because it cannot be
+  // referenced anymore. Anyway the ID is not re-used by following
+  // calls to addObject(), so the object can be added back into the
+  // container with the same ID using insertObject() method. If the
+  // object does not exist in the container, it throws an
+  // ObjectNotFoundException exception.
+  virtual void removeObject(ObjectId id) = 0;
+
+  // Returns the object pointer associated to the given ID. The
+  // pointer is the same as the used in addObject() or insertObject()
+  // method. If the object does not exist, it throws an
+  // ObjectNotFoundException.
+  virtual void* getObject(ObjectId id) = 0;
+
+  // Helper method to cast getObject() to the expected object type.
+  template<class T>
+  T* getObjectT(ObjectId id)
+  {
+    return reinterpret_cast<T*>(getObject(id));
+  }
+  
+};
+
+#endif

src/undo/undo_history.h

@@ -20,16 +20,17 @@
 #define UNDO_UNDO_HISTORY_H_INCLUDED
 
 #include "base/exception.h"
-#include "raster/gfxobj.h"
 
 #include <vector>
 
 class Cel;
 class Dirty;
 class Document;
+class GfxObj;
 class Image;
 class Layer;
 class Mask;
+class ObjectsContainer;
 class Palette;
 class Sprite;
 class Stock;
@@ -41,10 +42,10 @@ public:
   UndoException(const char* msg) throw() : base::Exception(msg) { }
 };
 
-class UndoHistory : public GfxObj
+class UndoHistory
 {
 public:
-  UndoHistory(Sprite* sprite);
+  UndoHistory(ObjectsContainer* objects);
   virtual ~UndoHistory();
 
   bool isEnabled() const;
@@ -69,7 +70,7 @@ public:
 
   void undo_open();
   void undo_close();
-  void undo_data(GfxObj *gfxobj, void *data, int size);
+  void undo_data(void* object, void* fieldAddress, int fieldSize);
   void undo_image(Image *image, int x, int y, int w, int h);
   void undo_flip(Image *image, int x1, int y1, int x2, int y2, bool horz);
   void undo_dirty(Image* image, Dirty *dirty);
@@ -103,12 +104,14 @@ public:
     undo_data(gfxobj, (void*)(value_address), sizeof(double));
   }
 
+  ObjectsContainer* getObjects() const { return m_objects; }
+
 private:
   void runUndo(int state);
   void discardTail();
   void updateUndo();
 
-  Sprite* m_sprite;		// Related sprite
+  ObjectsContainer* m_objects;	// Container of objects to insert & retrieve objects by ID
   UndoStream* m_undoStream;
   UndoStream* m_redoStream;
   int m_diffCount;

src/undo/undo_stream.h

@@ -40,6 +40,11 @@ public:
     return m_undo;
   }
 
+  ObjectsContainer* getObjects() const
+  {
+    return m_undo->getObjects();
+  }
+
   iterator begin() { return m_chunks.begin(); }
   iterator end() { return m_chunks.end(); }
   bool empty() const { return m_chunks.empty(); }