Fix issue #258 - bug moving a cel from a transparent layer to background

The problem was in DocumentApi::moveCel(). We cannot move a Cel between
layers and frames at the same time easily (it's not possible to do it
without being in a temporal invalid state, e.g. where two cels are in the
same frame position). It's better if we create completely new cel for
the target and remove the previous one in all cases (undoers are already
prepared to do those operations correctly).

- Add Sprite::getImageRefs() member function.

src/app/document_api.cpp

@@ -498,20 +498,13 @@ void DocumentApi::removeCel(LayerImage* layer, Cel* cel)
   ev.cel(cel);
   m_document->notifyObservers<DocumentEvent&>(&DocumentObserver::onRemoveCel, ev);
 
-  // Find if the image that use the cel to remove, is used by another
-  // cels.
-  bool used = false;
-  for (FrameNumber frame(0); frame<sprite->getTotalFrames(); ++frame) {
-    Cel* it = layer->getCel(frame);
-    if (it && it != cel && it->getImage() == cel->getImage()) {
-      used = true;
-      break;
-    }
-  }
+  // Find if the image that use this cel we are going to remove, is
+  // used by other cels.
+  size_t refs = sprite->getImageRefs(cel->getImage());
 
   // If the image is only used by this cel, we can remove the image
   // from the stock.
-  if (!used)
+  if (refs == 1)
     removeImageFromStock(sprite, cel->getImage());
 
   if (undoEnabled())
@@ -583,16 +576,17 @@ void DocumentApi::moveCel(Sprite* sprite,
   ASSERT(srcFrame >= 0 && srcFrame < sprite->getTotalFrames());
   ASSERT(dstFrame >= 0 && dstFrame < sprite->getTotalFrames());
 
+  // Background to any other layer, we use copyCel() instead.
   if (srcLayer->isBackground()) {
     copyCel(sprite, srcLayer, dstLayer, srcFrame, dstFrame, bgcolor);
     return;
   }
-
-  Cel* srcCel = srcLayer->getCel(srcFrame);
-  Cel* dstCel = dstLayer->getCel(dstFrame);
+  // In this we copy from a transparent layer to another layer...
 
   // Remove the dstCel (if it exists) because it must be replaced with
   // srcCel.
+  Cel* srcCel = srcLayer->getCel(srcFrame);
+  Cel* dstCel = dstLayer->getCel(dstFrame);
   if ((dstCel != NULL) && (!dstLayer->isBackground() || srcCel != NULL))
     removeCel(dstLayer, dstCel);
 
@@ -603,17 +597,14 @@ void DocumentApi::moveCel(Sprite* sprite,
     }
     // Move the cel between different layers.
     else {
-      if (undoEnabled())
-        m_undoers->pushUndoer(new undoers::RemoveCel(getObjects(), srcLayer, srcCel));
-      srcLayer->removeCel(srcCel);
-
-      srcCel->setFrame(dstFrame);
+      Cel* newCel = new Cel(*srcCel);
+      newCel->setFrame(dstFrame);
 
       // If we are moving a cel from a transparent layer to the
       // background layer, we have to clear the background of the
       // image.
-      if (!srcLayer->isBackground() &&
-          dstLayer->isBackground()) {
+      ASSERT(!srcLayer->isBackground());
+      if (dstLayer->isBackground()) {
         Image* srcImage = sprite->getStock()->getImage(srcCel->getImage());
         Image* dstImage = crop_image(srcImage,
           -srcCel->getX(),
@@ -621,24 +612,17 @@ void DocumentApi::moveCel(Sprite* sprite,
           sprite->getWidth(),
           sprite->getHeight(), 0);
 
-        if (undoEnabled()) {
-          m_undoers->pushUndoer(new undoers::ReplaceImage(getObjects(),
-              sprite->getStock(), srcCel->getImage()));
-          m_undoers->pushUndoer(new undoers::SetCelPosition(getObjects(), srcCel));
-          m_undoers->pushUndoer(new undoers::SetCelOpacity(getObjects(), srcCel));
-        }
-
         clear_image(dstImage, bgcolor);
         composite_image(dstImage, srcImage, srcCel->getX(), srcCel->getY(), 255, BLEND_MODE_NORMAL);
 
-        srcCel->setPosition(0, 0);
-        srcCel->setOpacity(255);
-
-        sprite->getStock()->replaceImage(srcCel->getImage(), dstImage);
-        delete srcImage;
+        newCel->setPosition(0, 0);
+        newCel->setOpacity(255);
+        newCel->setImage(addImageInStock(sprite, dstImage));
       }
 
-      addCel(dstLayer, srcCel);
+      // Add and the remove, so the Stock's image is reused.
+      addCel(dstLayer, newCel);
+      removeCel(srcLayer, srcCel);
     }
   }
 

src/raster/layer.cpp

@@ -137,10 +137,10 @@ void LayerImage::destroyAllCels()
   m_cels.clear();
 }
 
-void LayerImage::getCels(CelList& cels)
+void LayerImage::getCels(CelList& cels) const
 {
-  CelIterator it = getCelBegin();
-  CelIterator end = getCelEnd();
+  CelConstIterator it = getCelBegin();
+  CelConstIterator end = getCelEnd();
 
   for (; it != end; ++it)
     cels.push_back(*it);
@@ -259,10 +259,10 @@ int LayerFolder::getMemSize() const
   return size;
 }
 
-void LayerFolder::getCels(CelList& cels)
+void LayerFolder::getCels(CelList& cels) const
 {
-  LayerIterator it = getLayerBegin();
-  LayerIterator end = getLayerEnd();
+  LayerConstIterator it = getLayerBegin();
+  LayerConstIterator end = getLayerEnd();
 
   for (; it != end; ++it)
     (*it)->getCels(cels);

src/raster/layer.h

@@ -20,6 +20,7 @@
 #define RASTER_LAYER_H_INCLUDED
 #pragma once
 
+#include "base/compiler_specific.h"
 #include "raster/blend.h"
 #include "raster/frame_number.h"
 #include "raster/object.h"
@@ -80,7 +81,7 @@ namespace raster {
     uint32_t getFlags() const { return m_flags; }
     void setFlags(uint32_t flags) { m_flags = flags; }
 
-    virtual void getCels(CelList& cels) = 0;
+    virtual void getCels(CelList& cels) const = 0;
 
   private:
     std::string m_name;           // layer name
@@ -111,7 +112,7 @@ namespace raster {
     const Cel* getCel(FrameNumber frame) const;
     Cel* getCel(FrameNumber frame);
 
-    void getCels(CelList& cels);
+    void getCels(CelList& cels) const OVERRIDE;
 
     void configureAsBackground();
 
@@ -152,7 +153,7 @@ namespace raster {
     Layer* getFirstLayer() { return (m_layers.empty() ? NULL: m_layers.front()); }
     Layer* getLastLayer() { return (m_layers.empty() ? NULL: m_layers.back()); }
 
-    void getCels(CelList& cels);
+    void getCels(CelList& cels) const OVERRIDE;
 
   private:
     void destroyAllLayers();

src/raster/sprite.cpp

@@ -347,11 +347,24 @@ Stock* Sprite::getStock() const
   return m_stock;
 }
 
-void Sprite::getCels(CelList& cels)
+void Sprite::getCels(CelList& cels) const
 {
   getFolder()->getCels(cels);
 }
 
+size_t Sprite::getImageRefs(int imageIndex) const
+{
+  CelList cels;
+  getCels(cels);
+
+  size_t refs = 0;
+  for (CelList::iterator it=cels.begin(), end=cels.end(); it != end; ++it)
+    if ((*it)->getImage() == imageIndex)
+      ++refs;
+
+  return refs;
+}
+
 void Sprite::remapImages(FrameNumber frameFrom, FrameNumber frameTo, const std::vector<uint8_t>& mapping)
 {
   ASSERT(m_format == IMAGE_INDEXED);

src/raster/sprite.h

@@ -120,7 +120,10 @@ namespace raster {
 
     Stock* getStock() const;
 
-    void getCels(CelList& cels);
+    void getCels(CelList& cels) const;
+
+    // Returns the how many cels are referencing the given imageIndex.
+    size_t getImageRefs(int imageIndex) const;
 
     void remapImages(FrameNumber frameFrom, FrameNumber frameTo, const std::vector<uint8_t>& mapping);