From e85836a4529aee884970ee184feb0ce676b0dfbb Mon Sep 17 00:00:00 2001 From: scribblemaniac Date: Fri, 22 Nov 2024 01:01:18 -0700 Subject: [PATCH 1/6] Fix memory leak in ClipboardManager Since the frames stored in clipboardmanager are clones, they are not deleted by any layer and must be managed by clipboard manager. --- core_lib/src/interface/editor.cpp | 12 ++++++------ core_lib/src/managers/clipboardmanager.cpp | 22 +++++++++++++++++++++- core_lib/src/managers/clipboardmanager.h | 8 +++++++- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/core_lib/src/interface/editor.cpp b/core_lib/src/interface/editor.cpp index 6642a7e701..db0263b59d 100644 --- a/core_lib/src/interface/editor.cpp +++ b/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(keyClone); + auto soundClip = static_cast(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")); diff --git a/core_lib/src/managers/clipboardmanager.cpp b/core_lib/src/managers/clipboardmanager.cpp index cfb1ffb140..48d74a94df 100644 --- a/core_lib/src/managers/clipboardmanager.cpp +++ b/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) @@ -90,9 +94,25 @@ void ClipboardManager::copySelectedFrames(const Layer* currentLayer) { mFramesType = currentLayer->type(); } +std::map ClipboardManager::getClipboardFrames() +{ + std::map 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; diff --git a/core_lib/src/managers/clipboardmanager.h b/core_lib/src/managers/clipboardmanager.h index 04d3b82eea..4ab7c1df4e 100644 --- a/core_lib/src/managers/clipboardmanager.h +++ b/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 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 getClipboardFrames(); Layer::LAYER_TYPE framesLayerType() const { return mFramesType; } bool framesIsEmpty() const { return mFrames.empty(); } From b62abc5fdf6562e490a3a0a63b235f86aa1b2881 Mon Sep 17 00:00:00 2001 From: scribblemaniac Date: Sat, 23 Nov 2024 23:25:26 -0700 Subject: [PATCH 2/6] Use different naming scheme for cloned frames The old approach added a 12 character suffix to the source frame's filename. This works fine when cloning an normal frame, but cloning cloned frames would result in multiple suffixes, rapidly increaing the length of the file name. While untested, I suspect this could cause serious issues when there is a small limit on the file name or path length (such as on Windows). --- core_lib/src/graphics/bitmap/bitmapimage.cpp | 25 ++++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapimage.cpp b/core_lib/src/graphics/bitmap/bitmapimage.cpp index fc1f1cc15a..99cd6b031e 100644 --- a/core_lib/src/graphics/bitmap/bitmapimage.cpp +++ b/core_lib/src/graphics/bitmap/bitmapimage.cpp @@ -18,6 +18,7 @@ GNU General Public License for more details. #include #include +#include #include #include #include @@ -108,14 +109,24 @@ BitmapImage* BitmapImage::clone() const 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); + QDir tempDir = finfo.canonicalPath(); + tempDir.cd("temp"); + tempDir.mkpath("."); - bool ok = QFile::copy(fileName(), newFileName); + QString newFilePath; + QFileInfo newFileInfo; + do + { + newFilePath = QString("%1/%2.%3") + .arg(tempDir.canonicalPath()) + .arg(uniqueString(12)) + .arg(finfo.suffix()); + newFileInfo.setFile(newFilePath); + } + while (newFileInfo.exists()); + + b->setFileName(newFilePath); + bool ok = QFile::copy(fileName(), newFilePath); Q_ASSERT(ok); qDebug() << "COPY>" << fileName(); } From d7ba579e4bc5962fa5244a930b3749b2a8b0f21b Mon Sep 17 00:00:00 2001 From: scribblemaniac Date: Sat, 23 Nov 2024 23:58:37 -0700 Subject: [PATCH 3/6] Always copy backing file if image is not modified Non-modified frames can be unloaded. If the copied frame is loaded when copied and then later unloaded, it can end up with no data in a file and no data in memory, wiping the frame. --- core_lib/src/graphics/bitmap/bitmapimage.cpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/core_lib/src/graphics/bitmap/bitmapimage.cpp b/core_lib/src/graphics/bitmap/bitmapimage.cpp index 99cd6b031e..127fe6eeeb 100644 --- a/core_lib/src/graphics/bitmap/bitmapimage.cpp +++ b/core_lib/src/graphics/bitmap/bitmapimage.cpp @@ -101,7 +101,7 @@ 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. @@ -109,21 +109,15 @@ BitmapImage* BitmapImage::clone() const Q_ASSERT(finfo.isAbsolute()); Q_ASSERT(QFile::exists(fileName())); - QDir tempDir = finfo.canonicalPath(); - tempDir.cd("temp"); - tempDir.mkpath("."); - QString newFilePath; - QFileInfo newFileInfo; do { - newFilePath = QString("%1/%2.%3") - .arg(tempDir.canonicalPath()) + newFilePath = QString("%1/temp-%2.%3") + .arg(finfo.canonicalPath()) .arg(uniqueString(12)) .arg(finfo.suffix()); - newFileInfo.setFile(newFilePath); } - while (newFileInfo.exists()); + while (QFile::exists(newFilePath)); b->setFileName(newFilePath); bool ok = QFile::copy(fileName(), newFilePath); From d9e9ff435634551063a3149e2a8a3325cd795b76 Mon Sep 17 00:00:00 2001 From: scribblemaniac Date: Sun, 24 Nov 2024 00:26:39 -0700 Subject: [PATCH 4/6] Don't force file name or modification state of cloned keyframes Now that the backing file is properly copied, we do not need to set duplicated keyframes as modified. If the source frame is modified, the cloned frame will be modified as well and remain in memory. If the source frame is not modified, then it can be copied without loading it into memory, or unloaded after copying. --- app/src/actioncommands.cpp | 9 --------- core_lib/src/managers/clipboardmanager.cpp | 11 +++++++---- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/app/src/actioncommands.cpp b/app/src/actioncommands.cpp index 347779eeb5..11fba978c9 100644 --- a/app/src/actioncommands.cpp +++ b/app/src/actioncommands.cpp @@ -758,10 +758,6 @@ void ActionCommands::duplicateLayer() { mEditor->sound()->processSound(static_cast(key)); } - else - { - key->modification(); - } }); if (!fromLayer->keyExists(1)) { toLayer->removeKeyFrame(1); @@ -803,11 +799,6 @@ void ActionCommands::duplicateKey() mEditor->sound()->processSound(dynamic_cast(dupKey)); showSoundClipWarningIfNeeded(); } - else - { - dupKey->setFileName(""); // don't share filename - dupKey->modification(); - } mEditor->layers()->notifyAnimationLengthChanged(); emit mEditor->layers()->currentLayerChanged(mEditor->layers()->currentLayerIndex()); // trigger timeline repaint. diff --git a/core_lib/src/managers/clipboardmanager.cpp b/core_lib/src/managers/clipboardmanager.cpp index 48d74a94df..67ea7e07f0 100644 --- a/core_lib/src/managers/clipboardmanager.cpp +++ b/core_lib/src/managers/clipboardmanager.cpp @@ -79,17 +79,20 @@ 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(); } From 8cf7ebdd9a0b498fa7c2a57bc5980f86683c08bb Mon Sep 17 00:00:00 2001 From: scribblemaniac Date: Mon, 9 Dec 2024 00:50:11 -0700 Subject: [PATCH 5/6] Copy data diretory to temporary directory when using pcl This change is being made so that modifications to the working directory are not equivalent to saving the project when working with the pcl format. --- core_lib/src/structure/filemanager.cpp | 75 +++++++++++++++++++++++--- core_lib/src/structure/filemanager.h | 2 + 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/core_lib/src/structure/filemanager.cpp b/core_lib/src/structure/filemanager.cpp index 898ebb824b..6289ce84c1 100644 --- a/core_lib/src/structure/filemanager.cpp +++ b/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 diff --git a/core_lib/src/structure/filemanager.h b/core_lib/src/structure/filemanager.h index 9fc81a1cae..0b1137a776 100644 --- a/core_lib/src/structure/filemanager.h +++ b/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); From f20c53b057652fa0c5d8922ef9e59cb5c0be4e1d Mon Sep 17 00:00:00 2001 From: scribblemaniac Date: Mon, 9 Dec 2024 00:57:07 -0700 Subject: [PATCH 6/6] Update changelog --- ChangeLog.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ChangeLog.md b/ChangeLog.md index ea550e1595..8939fe888b 100644 --- a/ChangeLog.md +++ b/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