From 8e0711cafd7e78c6e5d77fdda6be91135a355cd1 Mon Sep 17 00:00:00 2001 From: Jan Arve Saether Date: Wed, 25 Mar 2020 16:53:18 +0100 Subject: Fix signal emission order for zero-duration animations The order for non-zero-duration animations are: * started() * runningChanged(true) * stopped() * runningChanged(false) * finished() This patch tries to ensure that zero-duration animations have the same signal emission order. The problem was that when setRunning(true) was called on zero-duration animation, it would call itself (setRunning(false)) lower in the call stack in order to immediately stop again. This had the implication that we could emit stopped() even before started() was emitted (since the recursive call to setRunning(false) would actually complete before the ancestor stack frame setRunning(true) was completed) To fix this we emit started() *before* we call start() on the animationInstance. There is still a bug in that runningChanged(true) is still not emitted for a zero-duration animation, but this patch should improve the current behavior in the sense that stopped() is not emitted _before_ started(). Task-number: QTBUG-48193 Change-Id: Ic2bc85e648e6746f6a058e2e9136515e7fdb6192 Reviewed-by: Mitch Curtis Reviewed-by: Volker Hilsheimer Reviewed-by: Ulf Hermann --- src/quick/util/qquickanimation.cpp | 20 +++++---- .../quick/qquickanimations/data/signalorder.qml | 27 +++++++++++ .../quick/qquickanimations/qquickanimations.pro | 1 + .../qquickanimations/tst_qquickanimations.cpp | 52 ++++++++++++++++++++++ 4 files changed, 92 insertions(+), 8 deletions(-) create mode 100644 tests/auto/quick/qquickanimations/data/signalorder.qml diff --git a/src/quick/util/qquickanimation.cpp b/src/quick/util/qquickanimation.cpp index 90c725a67f..e5e25d141b 100644 --- a/src/quick/util/qquickanimation.cpp +++ b/src/quick/util/qquickanimation.cpp @@ -176,11 +176,8 @@ void QQuickAbstractAnimationPrivate::commence() animationInstance = new QQuickAnimatorProxyJob(animationInstance, q); animationInstance->addAnimationChangeListener(this, QAbstractAnimationJob::Completion); } + emit q->started(); animationInstance->start(); - if (animationInstance->isStopped()) { - running = false; - emit q->stopped(); - } } } @@ -287,10 +284,8 @@ void QQuickAbstractAnimation::setRunning(bool r) d->animationInstance->setLoopCount(d->animationInstance->currentLoop() + d->loopCount); supressStart = true; //we want the animation to continue, rather than restart } - if (!supressStart) { + if (!supressStart) d->commence(); - emit started(); - } } else { if (d->paused) { d->paused = false; //reset paused state to false when stopped @@ -308,7 +303,16 @@ void QQuickAbstractAnimation::setRunning(bool r) } } - emit runningChanged(d->running); + + if (r == d->running) { + // This might happen if we start an animation with 0 duration: This will result in that + // commence() will emit started(), and then when it starts it will call setCurrentTime(0), + // (which is both start and end time of the animation), so it will also end up calling + // setRunning(false) (recursively) and stop the animation. + // Therefore, the state of d->running will in that case be different than r if we are back in + // the root stack frame of the recursive calls to setRunning() + emit runningChanged(d->running); + } } /*! diff --git a/tests/auto/quick/qquickanimations/data/signalorder.qml b/tests/auto/quick/qquickanimations/data/signalorder.qml new file mode 100644 index 0000000000..35029b0c84 --- /dev/null +++ b/tests/auto/quick/qquickanimations/data/signalorder.qml @@ -0,0 +1,27 @@ +import QtQuick 2.12 + + +Item { + id: wrapper + width: 400; height: 400 + + Rectangle { + id: greenRect + width: 50; height: 50 + color: "red" + + ColorAnimation on color { + id: colorAnimation + objectName: "ColorAnimation" + to: "green" + duration: 10 + running: false + } + + ParallelAnimation { + id: parallelAnimation + objectName: "ParallelAnimation" + running: false + } + } +} diff --git a/tests/auto/quick/qquickanimations/qquickanimations.pro b/tests/auto/quick/qquickanimations/qquickanimations.pro index 94f694181d..6998c023db 100644 --- a/tests/auto/quick/qquickanimations/qquickanimations.pro +++ b/tests/auto/quick/qquickanimations/qquickanimations.pro @@ -64,6 +64,7 @@ OTHER_FILES += \ data/scriptActionCrash.qml \ data/sequentialAnimationNullChildBug.qml \ data/signals.qml \ + data/signalorder.qml \ data/transitionAssignmentBug.qml \ data/valuesource.qml \ data/valuesource2.qml diff --git a/tests/auto/quick/qquickanimations/tst_qquickanimations.cpp b/tests/auto/quick/qquickanimations/tst_qquickanimations.cpp index 55957fa71a..e62d49ed6b 100644 --- a/tests/auto/quick/qquickanimations/tst_qquickanimations.cpp +++ b/tests/auto/quick/qquickanimations/tst_qquickanimations.cpp @@ -89,6 +89,8 @@ private slots: void easingProperties(); void rotation(); void startStopSignals(); + void signalOrder_data(); + void signalOrder(); void runningTrueBug(); void nonTransitionBug(); void registrationBug(); @@ -1323,6 +1325,56 @@ void tst_qquickanimations::startStopSignals() QVERIFY(timer.elapsed() >= 200); } +void tst_qquickanimations::signalOrder_data() +{ + QTest::addColumn("animationType"); + QTest::addColumn("duration"); + + QTest::addRow("ColorAnimation, duration = 10") << QByteArray("ColorAnimation") << 10; + QTest::addRow("ColorAnimation, duration = 0") << QByteArray("ColorAnimation") << 0; + QTest::addRow("ParallelAnimation, duration = 0") << QByteArray("ParallelAnimation") << 0; +} + +void tst_qquickanimations::signalOrder() +{ + QFETCH(QByteArray, animationType); + QFETCH(int, duration); + + QQmlEngine engine; + QQmlComponent c(&engine, testFileUrl("signalorder.qml")); + QScopedPointer obj(c.create()); + auto *root = qobject_cast(obj.data()); + QVERIFY(root); + QQuickAbstractAnimation *animation = root->findChild(animationType); + + const QVector signalsToConnect = { + &QQuickAbstractAnimation::started, + &QQuickAbstractAnimation::stopped, + &QQuickAbstractAnimation::finished + }; + const QVector expectedSignalOrder = { + "started", + "stopped", + "finished" + }; + + QVector actualSignalOrder; + + for (int i = 0; i < signalsToConnect.size(); ++i) { + const char *str = expectedSignalOrder.at(i); + connect(animation, signalsToConnect.at(i) , [str, &actualSignalOrder] () { + actualSignalOrder.append(str); + }); + } + QSignalSpy finishedSpy(animation, SIGNAL(finished())); + if (QQuickColorAnimation *colorAnimation = qobject_cast(animation)) + colorAnimation->setDuration(duration); + + animation->start(); + QTRY_VERIFY(finishedSpy.count()); + QCOMPARE(actualSignalOrder, expectedSignalOrder); +} + void tst_qquickanimations::runningTrueBug() { //ensure we start correctly when "running: true" is explicitly set -- cgit v1.2.3