Fix crash when importing image to empty frame on hidden layer (#1820)

* Fix crash when importing image to empty frame on hidden layer * Disable timeline hover effect for hidden layers * Document various frame adding methods * Fix potential memory leak in Layer::addNewKeyFrameAt * Clean up some code
pencil2d · Apr 14, 2024 · 37cbd97 · 37cbd97
1 parent dfd68c5
commit 37cbd97
Show file tree

Hide file tree

Showing 7 changed files with 57 additions and 23 deletions.
diff --git a/app/src/timelinecells.cpp b/app/src/timelinecells.cpp
@@ -747,7 +747,10 @@ void TimeLineCells::paintEvent(QPaintEvent*)
             {
                 paintHighlightedFrame(painter, mHighlightedFrame, recTop, standardWidth, recHeight);
             }
-            paintFrameCursorOnCurrentLayer(painter, recTop, standardWidth, recHeight);
+            if (currentLayer->visible())
+            {
+                paintFrameCursorOnCurrentLayer(painter, recTop, standardWidth, recHeight);
+            }
         }
 
         // --- draw the position of the current frame

diff --git a/core_lib/src/interface/editor.cpp b/core_lib/src/interface/editor.cpp
@@ -17,12 +17,9 @@ GNU General Public License for more details.
 
 #include "editor.h"
 
-#include <QApplication>
-#include <QClipboard>
 #include <QTimer>
 #include <QImageReader>
 #include <QDropEvent>
-#include <QMimeData>
 #include <QTemporaryDir>
 
 #include "object.h"
@@ -919,7 +916,13 @@ Status Editor::importBitmapImage(const QString& filePath)
     QImageReader reader(filePath);
 
     Q_ASSERT(layers()->currentLayer()->type() == Layer::BITMAP);
-    auto layer = static_cast<LayerBitmap*>(layers()->currentLayer());
+    const auto layer = static_cast<LayerBitmap*>(layers()->currentLayer());
+
+    if (!layer->visible())
+    {
+        mScribbleArea->showLayerNotVisibleWarning();
+        return Status::SAFE;
+    }
 
     Status status = Status::OK;
     DebugDetails dd;
@@ -942,7 +945,7 @@ Status Editor::importBitmapImage(const QString& filePath)
             break;
         case QImageReader::UnsupportedFormatError:
             errorDesc = tr("Image format is not supported. Please convert the image file to one of the following formats and try again:\n%1")
-                        .arg((QString)reader.supportedImageFormats().join(", "));
+                        .arg(QString::fromUtf8(reader.supportedImageFormats().join(", ")));
             break;
         default:
             errorDesc = tr("An error has occurred while reading the image. Please check that the file is a valid image and try again.");
@@ -956,7 +959,8 @@ Status Editor::importBitmapImage(const QString& filePath)
 
     if (!layer->keyExists(mFrame))
     {
-        addNewKey();
+        const bool ok = addNewKey();
+        Q_ASSERT(ok);
     }
     BitmapImage* bitmapImage = layer->getBitmapImageAtFrame(mFrame);
     BitmapImage importedBitmapImage(pos, img);
@@ -1211,7 +1215,7 @@ KeyFrame* Editor::addNewKey()
     return addKeyFrame(layers()->currentLayerIndex(), currentFrame());
 }
 
-KeyFrame* Editor::addKeyFrame(int layerNumber, int frameIndex)
+KeyFrame* Editor::addKeyFrame(const int layerNumber, int frameIndex)
 {
     Layer* layer = mObject->getLayer(layerNumber);
     Q_ASSERT(layer);
@@ -1237,13 +1241,11 @@ KeyFrame* Editor::addKeyFrame(int layerNumber, int frameIndex)
         }
     }
 
-    bool ok = layer->addNewKeyFrameAt(frameIndex);
-    if (ok)
-    {
-        scrubTo(frameIndex); // currentFrameChanged() emit inside.
-        emit frameModified(frameIndex);
-        layers()->notifyAnimationLengthChanged();
-    }
+    const bool ok = layer->addNewKeyFrameAt(frameIndex);
+    Q_ASSERT(ok); // We already ensured that there is no keyframe at frameIndex, so this should always succeed
+    scrubTo(frameIndex); // currentFrameChanged() emit inside.
+    emit frameModified(frameIndex);
+    layers()->notifyAnimationLengthChanged();
     return layer->getKeyFrameAt(frameIndex);
 }
 

diff --git a/core_lib/src/interface/editor.h b/core_lib/src/interface/editor.h
@@ -182,6 +182,12 @@ class Editor : public QObject
     void scrubForward();
     void scrubBackward();
 
+    /**
+     * Attempts to create a new keyframe at the current frame and layer. If the current frame already holds a keyframe,
+     * the new one will instead be created on the first empty frame that follows. If the current layer is not visible, a
+     * warning dialog will be shown to the user and no new keyframe is created.
+     * @return The newly created keyframe or `nullptr` if the layer is not visible
+     */
     KeyFrame* addNewKey();
     void removeKey();
 
@@ -268,7 +274,16 @@ class Editor : public QObject
     bool mAutosaveNeverAskAgain = false;
 
     void makeConnections();
-    KeyFrame* addKeyFrame(int layerNumber, int frameNumber);
+    /**
+     * Attempts to create a new keyframe at the given position and layer. If a keyframe already exists at the position,
+     * the new one will instead be created on the first empty frame that follows. If the layer is not visible, a warning
+     * dialog will be shown to the user and no new keyframe is created.
+     * @param layerNumber The number of an existing layer on which the keyframe is to be created
+     * @param frameIndex The desired position of the new keyframe
+     * @return The newly created keyframe or `nullptr` if the layer is not visible
+     * @see Editor::addNewKey()
+     */
+    KeyFrame* addKeyFrame(int layerNumber, int frameIndex);
 
     QList<QTemporaryDir*> mTemporaryDirs;
 

diff --git a/core_lib/src/structure/layer.cpp b/core_lib/src/structure/layer.cpp
@@ -172,7 +172,12 @@ bool Layer::addNewKeyFrameAt(int position)
     if (position <= 0) return false;
 
     KeyFrame* key = createKeyFrame(position);
-    return addKeyFrame(position, key);
+    if (!addKeyFrame(position, key))
+    {
+        delete key;
+        return false;
+    }
+    return true;
 }
 
 void Layer::addOrReplaceKeyFrame(int position, KeyFrame* pKeyFrame)

diff --git a/core_lib/src/structure/layer.h b/core_lib/src/structure/layer.h
@@ -94,8 +94,19 @@ class Layer : public QObject
     */
     bool insertExposureAt(int position);
 
+    /**
+     * Creates a new keyframe at the given position, unless one already exists.
+     * @param position The position of the new keyframe
+     * @return false if a keyframe already exists at the position, true if the new keyframe was successfully added
+     */
     bool addNewKeyFrameAt(int position);
     void addOrReplaceKeyFrame(int position, KeyFrame* pKeyFrame);
+    /**
+     * Adds a keyframe at the given position, unless one already exists.
+     * @param position The new position of the keyframe
+     * @param pKeyFrame The keyframe to add. Its previous position will be overwritten
+     * @return false if a keyframe already exists at the position, true if the keyframe was successfully added
+     */
     virtual bool addKeyFrame(int position, KeyFrame* pKeyFrame);
     virtual bool removeKeyFrame(int position);
     bool swapKeyFrames(int position1, int position2);

diff --git a/core_lib/src/util/pencilerror.cpp b/core_lib/src/util/pencilerror.cpp
@@ -17,9 +17,7 @@ GNU General Public License for more details.
 
 #include "pencilerror.h"
 #include <map>
-#include <QObject>
 #include <QSysInfo>
-#include "pencildef.h"
 
 DebugDetails::DebugDetails()
 {

diff --git a/core_lib/src/util/pencilerror.h b/core_lib/src/util/pencilerror.h
@@ -45,7 +45,7 @@ class Status
         OK = 0,
         SAFE,
         FAIL,
-		CANCELED,
+        CANCELED,
         FILE_NOT_FOUND,
         NOT_SUPPORTED,
         INVALID_ARGUMENT,
@@ -65,8 +65,8 @@ class Status
         // Sound
         ERROR_LOAD_SOUND_FILE,
 
-		// Export
-		ERROR_FFMPEG_NOT_FOUND,
+        // Export
+        ERROR_FFMPEG_NOT_FOUND,
 
         // Layer
         ERROR_NEED_AT_LEAST_ONE_CAMERA_LAYER
@@ -100,7 +100,7 @@ class Status
 
 #ifndef STATUS_CHECK
 #define STATUS_CHECK( x )\
-	{ Status st = (x); if (!st.ok()) { return st; } }
+    { Status st = (x); if (!st.ok()) { return st; } }
 #endif
 
 #ifndef STATUS_FAILED