Skip to content

Commit

Permalink
AC-1873 - Fix frame not being drawn on the timeline
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@MrStevns
MrStevns committed Aug 25, 2024
1 parent 09ef787 commit f2f1c31
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 45 deletions.
25 changes: 12 additions & 13 deletions app/src/timelinecells.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,12 +444,19 @@ void TimeLineCells::paintFrames(QPainter& painter, QColor trackCol, const Layer*

int recHeight = height - 4;

layer->foreachKeyFrame([&](KeyFrame* key)
const QList<int> 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.
Expand All @@ -463,19 +470,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
Expand Down Expand Up @@ -729,7 +728,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);
Expand Down
2 changes: 0 additions & 2 deletions core_lib/src/structure/keyframe.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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;
Expand Down
4 changes: 0 additions & 4 deletions core_lib/src/structure/keyframe.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }

Expand All @@ -65,7 +62,6 @@ class KeyFrame
int mFrame = -1;
int mLength = 1;
bool mIsModified = true;
bool mIsSelected = false;
QString mAttachedFileName;

std::vector<KeyFrameEventListener*> mEventListeners;
Expand Down
14 changes: 2 additions & 12 deletions core_lib/src/structure/layer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -383,9 +381,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)
Expand All @@ -410,7 +406,6 @@ void Layer::setFrameSelected(int position, bool isSelected)
mSelectedFrames_byLast.removeOne(startPosition);
mSelectedFrames_byPosition.removeOne(startPosition);
}
keyFrame->setSelected(isSelected);
}
}

Expand Down Expand Up @@ -496,11 +491,6 @@ void Layer::deselectAll()
{
mSelectedFrames_byLast.clear();
mSelectedFrames_byPosition.clear();

for (auto pair : mKeyFrames)
{
pair.second->setSelected(false);
}
}

bool Layer::canMoveSelectedFramesToOffset(int offset) const
Expand Down
2 changes: 2 additions & 0 deletions core_lib/src/structure/layer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ class Layer
/** Clear the list of dirty keyframes */
void clearDirtyFrames() { mDirtyFrames.clear(); }

std::map<int, KeyFrame*, std::greater<int>> keyframes() const { return mKeyFrames; }

protected:
virtual KeyFrame* createKeyFrame(int position) = 0;
bool loadKey(KeyFrame*);
Expand Down
28 changes: 14 additions & 14 deletions tests/src/test_layer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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;
Expand Down

0 comments on commit f2f1c31

Please sign in to comment.