From 89fa26b839cfb5fc2714194b79afb32883af8be0 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 25 Aug 2024 10:16:35 +0200 Subject: [PATCH 1/4] AC-1873 - Fix frame not being drawn on the timeline The reason for this is that while we keep a list of keyframes to move and paint keyframes on the timeline, a keyframe also has its own "isSelected" state. This state is backed up through undo/redo operations which the layer::getSelectedFrames is not. The solution is to remove isSelected from Keyframe, as there should only be one truth. --- app/src/timelinecells.cpp | 25 ++++++++++++------------- core_lib/src/structure/keyframe.cpp | 2 -- core_lib/src/structure/keyframe.h | 4 ---- core_lib/src/structure/layer.cpp | 14 ++------------ core_lib/src/structure/layer.h | 2 ++ tests/src/test_layer.cpp | 28 ++++++++++++++-------------- 6 files changed, 30 insertions(+), 45 deletions(-) diff --git a/app/src/timelinecells.cpp b/app/src/timelinecells.cpp index 1be3201072..fd50401614 100644 --- a/app/src/timelinecells.cpp +++ b/app/src/timelinecells.cpp @@ -445,12 +445,19 @@ void TimeLineCells::paintFrames(QPainter& painter, QColor trackCol, const Layer* int recHeight = height - 4; - layer->foreachKeyFrame([&](KeyFrame* key) + const QList selectedFrames = layer->getSelectedFramesByPos(); + for (auto pair : layer->keyframes()) { + const KeyFrame* key = pair.second; int framePos = key->pos(); int recWidth = standardWidth; int recLeft = getFrameX(framePos) - recWidth; + // Selected frames are painted separately + if (selectedFrames.contains(framePos)) { + continue; + } + if (key->length() > 1) { // This is especially for sound clips. @@ -464,19 +471,11 @@ void TimeLineCells::paintFrames(QPainter& painter, QColor trackCol, const Layer* // Paint the frame contents if (selected) { - if (key->isSelected()) { - painter.setBrush(QColor(60, 60, 60)); - } - else - { - painter.setBrush(QColor(trackCol.red(), trackCol.green(), trackCol.blue(), 150)); - } + painter.setBrush(QColor(trackCol.red(), trackCol.green(), trackCol.blue(), 150)); } - if (!key->isSelected()) { - painter.drawRect(recLeft, recTop, recWidth, recHeight); - } - }); + painter.drawRect(recLeft, recTop, recWidth, recHeight); + } } void TimeLineCells::paintCurrentFrameBorder(QPainter &painter, int recLeft, int recTop, int recWidth, int recHeight) const @@ -730,7 +729,7 @@ void TimeLineCells::paintEvent(QPaintEvent*) int currentFrame = mEditor->currentFrame(); Layer* currentLayer = mEditor->layers()->currentLayer(); KeyFrame* keyFrame = currentLayer->getKeyFrameWhichCovers(currentFrame); - if (keyFrame != nullptr && !keyFrame->isSelected()) + if (keyFrame != nullptr) { int recWidth = keyFrame->length() == 1 ? mFrameSize - 2 : mFrameSize * keyFrame->length(); int recLeft = getFrameX(keyFrame->pos()) - (mFrameSize - 2); diff --git a/core_lib/src/structure/keyframe.cpp b/core_lib/src/structure/keyframe.cpp index 88282e3632..b03b2f03e2 100644 --- a/core_lib/src/structure/keyframe.cpp +++ b/core_lib/src/structure/keyframe.cpp @@ -27,7 +27,6 @@ KeyFrame::KeyFrame(const KeyFrame& k2) mFrame = k2.mFrame; mLength = k2.mLength; mIsModified = k2.mIsModified; - mIsSelected = k2.mIsSelected; mAttachedFileName = k2.mAttachedFileName; // intentionally not copying event listeners } @@ -50,7 +49,6 @@ KeyFrame& KeyFrame::operator=(const KeyFrame& k2) mFrame = k2.mFrame; mLength = k2.mLength; mIsModified = k2.mIsModified; - mIsSelected = k2.mIsSelected; mAttachedFileName = k2.mAttachedFileName; // intentionally not copying event listeners return *this; diff --git a/core_lib/src/structure/keyframe.h b/core_lib/src/structure/keyframe.h index 4412bb55cb..361ce58bb8 100644 --- a/core_lib/src/structure/keyframe.h +++ b/core_lib/src/structure/keyframe.h @@ -45,9 +45,6 @@ class KeyFrame void setModified(bool b) { mIsModified = b; } bool isModified() const { return mIsModified; } - void setSelected(bool b) { mIsSelected = b; } - bool isSelected() const { return mIsSelected; } - QString fileName() const { return mAttachedFileName; } void setFileName(QString strFileName) { mAttachedFileName = strFileName; } @@ -65,7 +62,6 @@ class KeyFrame int mFrame = -1; int mLength = 1; bool mIsModified = true; - bool mIsSelected = false; QString mAttachedFileName; std::vector mEventListeners; diff --git a/core_lib/src/structure/layer.cpp b/core_lib/src/structure/layer.cpp index 97a18417c7..c855665baa 100644 --- a/core_lib/src/structure/layer.cpp +++ b/core_lib/src/structure/layer.cpp @@ -222,9 +222,7 @@ bool Layer::removeKeyFrame(int position) if (frame) { - if (frame->isSelected()) { - removeFromSelectionList(frame->pos()); - } + removeFromSelectionList(frame->pos()); mKeyFrames.erase(frame->pos()); markFrameAsDirty(frame->pos()); delete frame; @@ -384,9 +382,7 @@ bool Layer::isFrameSelected(int position) const KeyFrame* keyFrame = getKeyFrameWhichCovers(position); if (keyFrame == nullptr) { return false; } - int frameFound = mSelectedFrames_byLast.contains(keyFrame->pos()); - Q_ASSERT(!frameFound || keyFrame->isSelected()); - return frameFound; + return mSelectedFrames_byLast.contains(keyFrame->pos()); } void Layer::setFrameSelected(int position, bool isSelected) @@ -411,7 +407,6 @@ void Layer::setFrameSelected(int position, bool isSelected) mSelectedFrames_byLast.removeOne(startPosition); mSelectedFrames_byPosition.removeOne(startPosition); } - keyFrame->setSelected(isSelected); } } @@ -497,11 +492,6 @@ void Layer::deselectAll() { mSelectedFrames_byLast.clear(); mSelectedFrames_byPosition.clear(); - - for (auto pair : mKeyFrames) - { - pair.second->setSelected(false); - } } bool Layer::canMoveSelectedFramesToOffset(int offset) const diff --git a/core_lib/src/structure/layer.h b/core_lib/src/structure/layer.h index 2e8897d47a..2bf49643f6 100644 --- a/core_lib/src/structure/layer.h +++ b/core_lib/src/structure/layer.h @@ -170,6 +170,8 @@ class Layer /** Clear the list of dirty keyframes */ void clearDirtyFrames() { mDirtyFrames.clear(); } + std::map> keyframes() const { return mKeyFrames; } + protected: virtual KeyFrame* createKeyFrame(int position) = 0; bool loadKey(KeyFrame*); diff --git a/tests/src/test_layer.cpp b/tests/src/test_layer.cpp index 73478d105d..1e49d9bf8e 100644 --- a/tests/src/test_layer.cpp +++ b/tests/src/test_layer.cpp @@ -355,8 +355,8 @@ TEST_CASE("Layer::moveKeyFrame(int position, int offset)") layer->moveKeyFrame(2, 1); // Confirm that both frames are still selected. - REQUIRE(layer->getKeyFrameAt(2)->isSelected()); - REQUIRE(layer->getKeyFrameAt(3)->isSelected()); + REQUIRE(layer->isFrameSelected(2)); + REQUIRE(layer->isFrameSelected(3)); // Verify that poiners has been swapped REQUIRE(frame1 == layer->getKeyFrameAt(3)); @@ -376,8 +376,8 @@ TEST_CASE("Layer::moveKeyFrame(int position, int offset)") layer->moveKeyFrame(2, 1); // Confirm that both frames are still selected. - REQUIRE(layer->getKeyFrameAt(2)->isSelected()); - REQUIRE_FALSE(layer->getKeyFrameAt(3)->isSelected()); + REQUIRE(layer->isFrameSelected(2)); + REQUIRE_FALSE(layer->isFrameSelected(3)); } SECTION("move non selected frame across multiple selected frames") @@ -402,11 +402,11 @@ TEST_CASE("Layer::moveKeyFrame(int position, int offset)") layer->moveKeyFrame(8, 1); // Confirm that both frames are still selected. - REQUIRE(layer->getKeyFrameAt(4)->isSelected()); - REQUIRE(layer->getKeyFrameAt(5)->isSelected()); - REQUIRE(layer->getKeyFrameAt(6)->isSelected()); - REQUIRE(layer->getKeyFrameAt(7)->isSelected()); - REQUIRE_FALSE(layer->getKeyFrameAt(9)->isSelected()); + REQUIRE(layer->isFrameSelected(4)); + REQUIRE(layer->isFrameSelected(5)); + REQUIRE(layer->isFrameSelected(6)); + REQUIRE(layer->isFrameSelected(7)); + REQUIRE_FALSE(layer->isFrameSelected(9)); } SECTION("move selected frame across multiple not selected frames") @@ -433,11 +433,11 @@ TEST_CASE("Layer::moveKeyFrame(int position, int offset)") layer->moveKeyFrame(4, -1); // Confirm that both frames are still selected. - REQUIRE(layer->getKeyFrameAt(3)->isSelected()); - REQUIRE_FALSE(layer->getKeyFrameAt(5)->isSelected()); - REQUIRE_FALSE(layer->getKeyFrameAt(6)->isSelected()); - REQUIRE_FALSE(layer->getKeyFrameAt(7)->isSelected()); - REQUIRE_FALSE(layer->getKeyFrameAt(8)->isSelected()); + REQUIRE(layer->isFrameSelected(3)); + REQUIRE_FALSE(layer->isFrameSelected(5)); + REQUIRE_FALSE(layer->isFrameSelected(6)); + REQUIRE_FALSE(layer->isFrameSelected(7)); + REQUIRE_FALSE(layer->isFrameSelected(8)); } delete obj; From 5afd77d1894f6a008a51a965335a11a9338384b7 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Sun, 25 Aug 2024 12:49:38 +0200 Subject: [PATCH 2/4] Only deselect layer if the index has changed --- core_lib/src/managers/layermanager.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core_lib/src/managers/layermanager.cpp b/core_lib/src/managers/layermanager.cpp index 927988f4b6..6e03428021 100644 --- a/core_lib/src/managers/layermanager.cpp +++ b/core_lib/src/managers/layermanager.cpp @@ -112,7 +112,9 @@ void LayerManager::setCurrentLayer(int layerIndex) // Deselect frames of previous layer. Layer* previousLayer = currentLayer(); - previousLayer->deselectAll(); + if (previousLayer != object()->getLayer(layerIndex)) { + previousLayer->deselectAll(); + } emit currentLayerWillChange(layerIndex); From 561558441cac0ecc7268cfca83d4c3b4fef98431 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Wed, 4 Jun 2025 18:37:05 +0200 Subject: [PATCH 3/4] TimeLineCells: Use ForEach and return --- app/src/timelinecells.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/src/timelinecells.cpp b/app/src/timelinecells.cpp index fd50401614..2f5dacc439 100644 --- a/app/src/timelinecells.cpp +++ b/app/src/timelinecells.cpp @@ -446,16 +446,15 @@ void TimeLineCells::paintFrames(QPainter& painter, QColor trackCol, const Layer* int recHeight = height - 4; const QList selectedFrames = layer->getSelectedFramesByPos(); - for (auto pair : layer->keyframes()) + layer->foreachKeyFrame([&](KeyFrame* key) { - const KeyFrame* key = pair.second; int framePos = key->pos(); int recWidth = standardWidth; int recLeft = getFrameX(framePos) - recWidth; // Selected frames are painted separately if (selectedFrames.contains(framePos)) { - continue; + return; } if (key->length() > 1) @@ -475,7 +474,7 @@ void TimeLineCells::paintFrames(QPainter& painter, QColor trackCol, const Layer* } painter.drawRect(recLeft, recTop, recWidth, recHeight); - } + }); } void TimeLineCells::paintCurrentFrameBorder(QPainter &painter, int recLeft, int recTop, int recWidth, int recHeight) const From de67de3c1e19de0b7d69298b617b2a39cfd6d5b9 Mon Sep 17 00:00:00 2001 From: MrStevns Date: Thu, 5 Jun 2025 09:12:05 +0200 Subject: [PATCH 4/4] Cleanup unused function --- core_lib/src/structure/layer.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/core_lib/src/structure/layer.h b/core_lib/src/structure/layer.h index 2bf49643f6..2e8897d47a 100644 --- a/core_lib/src/structure/layer.h +++ b/core_lib/src/structure/layer.h @@ -170,8 +170,6 @@ class Layer /** Clear the list of dirty keyframes */ void clearDirtyFrames() { mDirtyFrames.clear(); } - std::map> keyframes() const { return mKeyFrames; } - protected: virtual KeyFrame* createKeyFrame(int position) = 0; bool loadKey(KeyFrame*);