Fix some issues related to frame copying & change pcl working directory

ChangeLog.md

@@ -3,12 +3,13 @@
 ### Features
 + Introduce new Undo/Redo system [#1817](https://github.com/pencil2d/pencil/pull/1817)
 
-### Enhancements
+### Enhancements/Changes
 + Add checkbox to allow polyline to close automatically [#1863](https://github.com/pencil2d/pencil/pull/1863)
 + Maintain active layer track in view - [#1867](https://github.com/pencil2d/pencil/pull/1867)
 + Update shortcuts [#1866](https://github.com/pencil2d/pencil/pull/1866)
 + Improve dock layout for lower resolutions [#1840](https://github.com/pencil2d/pencil/pull/1840)
 + Add ability to remove Last Polyline Segment using backspace [#1861](https://github.com/pencil2d/pencil/pull/1861)
++ Changed handling of pcl projects - [#1896](https://github.com/pencil2d/pencil/pull/1896)
 
 ### Bugfixes:
 + Do not make a new keyframe if double clicking on an existing keyframe - [#1851](https://github.com/pencil2d/pencil/pull/1851)
@@ -18,6 +19,9 @@
 + Avoid updating width/feather sliders for tools that don’t use them [cce3107](https://github.com/pencil2d/pencil/commit/cce31079c871fcc04e957c44d5c6e65990f635f1)
 + Fix fill misbehaving when drawing was partly outside border [#1865](https://github.com/pencil2d/pencil/pull/1865)
 + Fix clearing selection with the delete shortcut [#1892](https://github.com/pencil2d/pencil/pull/1892)
++ Fixed memory leak when copying bitmap keyframes - [#1896](https://github.com/pencil2d/pencil/pull/1896)
++ Fixed potential issue on some systems when repeatedly copying bitmap frames - [#1896](https://github.com/pencil2d/pencil/pull/1896)
++ Fixed bitmap frame wipe that can occur under specific situations when using keyframe copy & paste - [#1896](https://github.com/pencil2d/pencil/pull/1896)
 
 ## Pencil2D v0.7.0 - 12 July 2024
 

app/src/actioncommands.cpp

@@ -758,10 +758,6 @@ void ActionCommands::duplicateLayer()
         {
             mEditor->sound()->processSound(static_cast<SoundClip*>(key));
         }
-        else
-        {
-            key->modification();
-        }
     });
     if (!fromLayer->keyExists(1)) {
         toLayer->removeKeyFrame(1);
@@ -803,11 +799,6 @@ void ActionCommands::duplicateKey()
         mEditor->sound()->processSound(dynamic_cast<SoundClip*>(dupKey));
         showSoundClipWarningIfNeeded();
     }
-    else
-    {
-        dupKey->setFileName(""); // don't share filename
-        dupKey->modification();
-    }
 
     mEditor->layers()->notifyAnimationLengthChanged();
     emit mEditor->layers()->currentLayerChanged(mEditor->layers()->currentLayerIndex()); // trigger timeline repaint.

core_lib/src/graphics/bitmap/bitmapimage.cpp

@@ -18,6 +18,7 @@ GNU General Public License for more details.
 
 #include <cmath>
 #include <QDebug>
+#include <QDir>
 #include <QFile>
 #include <QFileInfo>
 #include <QPainterPath>
@@ -100,22 +101,26 @@ BitmapImage* BitmapImage::clone() const
     b->setFileName(""); // don't link to the file of the source bitmap image
 
     const bool validKeyFrame = !fileName().isEmpty();
-    if (validKeyFrame && !isLoaded())
+    if (validKeyFrame && !isModified())
     {
         // This bitmapImage is temporarily unloaded.
         // since it's not in the memory, we need to copy the linked png file to prevent data loss.
         QFileInfo finfo(fileName());
         Q_ASSERT(finfo.isAbsolute());
         Q_ASSERT(QFile::exists(fileName()));
 
-        QString newFileName = QString("%1/%2-%3.%4")
-            .arg(finfo.canonicalPath())
-            .arg(finfo.completeBaseName())
-            .arg(uniqueString(12))
-            .arg(finfo.suffix());
-        b->setFileName(newFileName);
+        QString newFilePath;
+        do
+        {
+            newFilePath = QString("%1/temp-%2.%3")
+                .arg(finfo.canonicalPath())
+                .arg(uniqueString(12))
+                .arg(finfo.suffix());
+        }
+        while (QFile::exists(newFilePath));
 
-        bool ok = QFile::copy(fileName(), newFileName);
+        b->setFileName(newFilePath);
+        bool ok = QFile::copy(fileName(), newFilePath);
         Q_ASSERT(ok);
         qDebug() << "COPY>" << fileName();
     }

core_lib/src/interface/editor.cpp

@@ -330,19 +330,19 @@ void Editor::pasteToFrames()
             currentLayer->moveSelectedFrames(1);
         }
 
+        KeyFrame* key = it->second;
         // It's a bug if the keyframe is nullptr at this point...
-        Q_ASSERT(it->second != nullptr);
+        Q_ASSERT(key != nullptr);
 
         // TODO: undo/redo implementation
-        KeyFrame* keyClone = it->second->clone();
-        currentLayer->addKeyFrame(newPosition, keyClone);
+        currentLayer->addKeyFrame(newPosition, key);
         if (currentLayer->type() == Layer::SOUND)
         {
-            auto soundClip = static_cast<SoundClip*>(keyClone);
+            auto soundClip = static_cast<SoundClip*>(key);
             sound()->loadSound(soundClip, soundClip->fileName());
         }
 
-        currentLayer->setFrameSelected(keyClone->pos(), true);
+        currentLayer->setFrameSelected(key->pos(), true);
     }
 }
 
@@ -354,7 +354,7 @@ void Editor::paste()
 
     if (!canPaste()) { return; }
 
-    if (clipboards()->getClipboardFrames().empty()) {
+    if (clipboards()->framesIsEmpty()) {
 
         backup(tr("Paste"));
 

core_lib/src/managers/clipboardmanager.cpp

@@ -28,7 +28,11 @@ ClipboardManager::ClipboardManager(Editor* editor) : BaseManager(editor, "Clipbo
 
 ClipboardManager::~ClipboardManager()
 {
-
+    for (auto it : mFrames)
+    {
+        KeyFrame* frame = it.second;
+        delete frame;
+    }
 }
 
 void ClipboardManager::setFromSystemClipboard(const QPointF& pos, const Layer* layer)
@@ -75,24 +79,43 @@ void ClipboardManager::copyVectorImage(const VectorImage* vectorImage)
     mVectorImage = *vectorImage->clone();
 }
 
-void ClipboardManager::copySelectedFrames(const Layer* currentLayer) {
+void ClipboardManager::copySelectedFrames(const Layer* currentLayer)
+{
     resetStates();
 
     for (int pos : currentLayer->selectedKeyFramesPositions()) {
         KeyFrame* keyframe = currentLayer->getKeyFrameAt(pos);
-
         Q_ASSERT(keyframe != nullptr);
 
-        keyframe->loadFile();
+        KeyFrame* newKeyframe = keyframe->clone();
+        // Unload unmodified keyframes now as they won't ever get unloaded
+        // by activeframepool while in clipboard manager.
+        newKeyframe->unloadFile();
 
-        mFrames.insert(std::make_pair(keyframe->pos(), keyframe->clone()));
+        mFrames.insert(std::make_pair(keyframe->pos(), newKeyframe));
     }
     mFramesType = currentLayer->type();
 }
 
+std::map<int, KeyFrame*> ClipboardManager::getClipboardFrames()
+{
+    std::map<int, KeyFrame*> resultMap;
+    for (auto it : mFrames)
+    {
+        resultMap.insert(std::make_pair(it.first, it.second->clone()));
+    }
+    return resultMap;
+}
+
 void ClipboardManager::resetStates()
 {
+    for (auto it : mFrames)
+    {
+        KeyFrame* frame = it.second;
+        delete frame;
+    }
     mFrames.clear();
+
     mBitmapImage = BitmapImage();
     mVectorImage = VectorImage();
     mFramesType = Layer::LAYER_TYPE::UNDEFINED;

core_lib/src/managers/clipboardmanager.h

@@ -61,7 +61,13 @@ class ClipboardManager: public BaseManager
 
     const BitmapImage& getBitmapClipboard() const { return mBitmapImage; }
     const VectorImage& getVectorClipboard() const { return mVectorImage; }
-    std::map<int, KeyFrame*> getClipboardFrames() { return mFrames; }
+
+    /** Return a copy of all clipboard frames keyed by their position.
+     *
+     *  The caller takes ownership of the returned keyframe objects and is
+     *  responsible for deleting them when no longer in use.
+     */
+    std::map<int, KeyFrame*> getClipboardFrames();
 
     Layer::LAYER_TYPE framesLayerType() const { return mFramesType; }
     bool framesIsEmpty() const { return mFrames.empty(); }

core_lib/src/structure/filemanager.cpp

@@ -48,8 +48,8 @@ Object* FileManager::load(const QString& sFileName)
     obj->setFilePath(sFileName);
     obj->createWorkingDir();
 
-    QString strMainXMLFile;
-    QString strDataFolder;
+    QString strMainXMLFile = QDir(obj->workingDir()).filePath(PFF_XML_FILE_NAME);
+    QString strDataFolder = QDir(obj->workingDir()).filePath(PFF_DATA_DIR);
 
     // Test file format: new zipped .pclx or old .pcl?
     bool isArchive = isArchiveFormat(sFileName);
@@ -59,8 +59,15 @@ Object* FileManager::load(const QString& sFileName)
     {
         dd << fileFormat.arg(".pcl");
 
-        strMainXMLFile = sFileName;
-        strDataFolder = strMainXMLFile + "." + PFF_OLD_DATA_DIR;
+        if (!QFile::copy(sFileName, strMainXMLFile))
+        {
+            dd << "Failed to copy main xml file";
+        }
+        Status st = copyDir(sFileName + "." + PFF_OLD_DATA_DIR, strDataFolder);
+        if (!st.ok())
+        {
+            dd.collect(st.details());
+        }
     }
     else
     {
@@ -89,9 +96,6 @@ Object* FileManager::load(const QString& sFileName)
                 return nullptr;
             }
         }
-
-        strMainXMLFile = QDir(workingDirPath).filePath(PFF_XML_FILE_NAME);
-        strDataFolder = QDir(workingDirPath).filePath(PFF_DATA_DIR);
     }
 
     obj->setDataDir(strDataFolder);
@@ -754,6 +758,63 @@ Status FileManager::writePalette(const Object* object, const QString& dataFolder
     return Status::OK;
 }
 
+/** Copy a directory to another directory recursively (depth-first).
+ *
+ *  If any of the files being copied already exist at the destination, they will
+ *  not be overwritten and a non-ok status will be returned.
+ *
+ *  If any part of the copy fails, this function will still attempt to copy
+ *  as many files as possible before returning a non-ok status.
+ *
+ * @param src The source directory to copy.
+ * @param dst The target directory to copy to.
+ *
+ * @return Status indicating the success or failure of the copy.
+ */
+Status FileManager::copyDir(const QDir src, const QDir dst)
+{
+    if (!src.exists()) return Status::FILE_NOT_FOUND;
+
+    DebugDetails dd;
+    bool isOkay = true;
+
+    if (!dst.mkpath(dst.absolutePath()))
+    {
+        dd << ("Failed to create target directory: " + dst.absolutePath());
+        return Status(Status::FAIL, dd);
+    }
+
+    foreach (QString dirName, src.entryList(QDir::Dirs | QDir::NoDotAndDotDot))
+    {
+        Status st = copyDir(src.filePath(dirName), dst.filePath(dirName));
+        if (!st.ok())
+        {
+            isOkay = false;
+            dd.collect(st.details());
+        }
+    }
+
+    foreach (QString fileName, src.entryList(QDir::Files))
+    {
+        if (!QFile::copy(src.filePath(fileName), dst.filePath(fileName)))
+        {
+            isOkay = false;
+            dd << "Failed to copy file"
+               << ("Source path: " + src.filePath(fileName))
+               << ("Destination path: " + dst.filePath(fileName));
+        }
+    }
+
+    if (isOkay)
+    {
+        return Status::OK;
+    }
+    else
+    {
+        return Status(Status::FAIL, dd);
+    }
+}
+
 Status FileManager::unzip(const QString& strZipFile, const QString& strUnzipTarget)
 {
     // removes the previous directory first  - better approach

core_lib/src/structure/filemanager.h

@@ -30,6 +30,7 @@ GNU General Public License for more details.
 
 class Object;
 class ObjectData;
+class QDir;
 
 
 class FileManager : public QObject
@@ -55,6 +56,7 @@ class FileManager : public QObject
     void progressRangeChanged(int maxValue);
 
 private:
+    Status copyDir(const QDir src, const QDir dst);
     Status unzip(const QString& strZipFile, const QString& strUnzipTarget);
 
     bool loadObject(Object*, const QDomElement& root);