QAbstractAnimation::setCurrentTime() - fix bindable property usage

The setter is complicated, because it reads the values of four
properties (loopCount, totalCurrentTime, currentLoop, and direction)
and modifies the values of two properties (totalCurrentTime and
currentLoop).

For the read-only properties, inroduce local variables, so that the
binding evaluation is done only once, and re-use the local variables
afterwards.

For the read-write properties, use {set}ValueBypassingBindings()
methods, because we are anyway explicitly calling notify() afterwards.
Intorduce a newCurrentLoop local variable to calculate the new value
of currentLoop, and update the actual property only once.
Use the newCurrentLoop and msecs variables instead of accessing the
actual properties when determining if we need to notify about
property changes.

The other complexity is that the setter calls a virtual
updateCurrentTime() method in the middle of the calculation.
Subclasses can override the default implementation and modify the
properties there (at least QSequentialAnimationGroup updates
totalCurrentTime property under certain conditions). Keep it in
mind and re-read the property values after the call to the virtual
method.

As a drive-by, make some pre-existing variables const.

Task-number: QTBUG-116346
Pick-to: 6.6 6.5
Change-Id: I0a48bcabde15333e80ea2d54e308cb9438b310e7
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

1 files changed, 27 insertions, 14 deletions

diff --git a/src/corelib/animation/qabstractanimation.cpp b/src/corelib/animation/qabstractanimation.cpp
index 4965ed4fd5..c32053db82 100644
--- a/src/corelib/animation/qabstractanimation.cpp
+++ b/src/corelib/animation/qabstractanimation.cpp
@@ -1301,46 +1301,59 @@ void QAbstractAnimation::setCurrentTime(int msecs)
     msecs = qMax(msecs, 0);
 
     // Calculate new time and loop.
-    int dura = duration();
-    int totalDura = dura <= 0 ? dura : ((d->loopCount < 0) ? -1 : dura * d->loopCount);
+    const int dura = duration();
+    const int totalLoopCount = d->loopCount;
+    const int totalDura = dura <= 0 ? dura : ((totalLoopCount < 0) ? -1 : dura * totalLoopCount);
     if (totalDura != -1)
         msecs = qMin(totalDura, msecs);
 
-    const int oldCurrentTime = d->totalCurrentTime;
-    d->totalCurrentTime = msecs;
+    d->totalCurrentTime.removeBindingUnlessInWrapper();
+
+    const int oldCurrentTime = d->totalCurrentTime.valueBypassingBindings();
+    d->totalCurrentTime.setValueBypassingBindings(msecs);
+
+    QAbstractAnimation::Direction currentDirection = d->direction;
 
     // Update new values.
-    int oldLoop = d->currentLoop;
-    d->currentLoop = ((dura <= 0) ? 0 : (msecs / dura));
-    if (d->currentLoop == d->loopCount) {
+    const int oldLoop = d->currentLoop.valueBypassingBindings();
+    int newCurrentLoop = (dura <= 0) ? 0 : (msecs / dura);
+    if (newCurrentLoop == totalLoopCount) {
         //we're at the end
         d->currentTime = qMax(0, dura);
-        d->currentLoop = qMax(0, d->loopCount - 1);
+        newCurrentLoop = qMax(0, totalLoopCount - 1);
     } else {
-        if (d->direction == Forward) {
+        if (currentDirection == Forward) {
             d->currentTime = (dura <= 0) ? msecs : (msecs % dura);
         } else {
             d->currentTime = (dura <= 0) ? msecs : ((msecs - 1) % dura) + 1;
             if (d->currentTime == dura)
-                d->currentLoop = d->currentLoop - 1;
+                newCurrentLoop = newCurrentLoop - 1;
         }
     }
+    d->currentLoop.setValueBypassingBindings(newCurrentLoop);
 
+    // this is a virtual function, so it can update the properties as well
     updateCurrentTime(d->currentTime);
-    if (d->currentLoop != oldLoop)
+
+    // read the property values again
+    newCurrentLoop = d->currentLoop.valueBypassingBindings();
+    currentDirection = d->direction;
+    const int newTotalCurrentTime = d->totalCurrentTime.valueBypassingBindings();
+
+    if (newCurrentLoop != oldLoop)
         d->currentLoop.notify();
 
     /* Notify before calling stop: As seen in tst_QSequentialAnimationGroup::clear
      * we might delete the animation when stop is called. Thus after stop no member
      * of the object must be used anymore.
      */
-    if (oldCurrentTime != d->totalCurrentTime)
+    if (oldCurrentTime != newTotalCurrentTime)
         d->totalCurrentTime.notify();
     // All animations are responsible for stopping the animation when their
     // own end state is reached; in this case the animation is time driven,
     // and has reached the end.
-    if ((d->direction == Forward && d->totalCurrentTime == totalDura)
-        || (d->direction == Backward && d->totalCurrentTime == 0)) {
+    if ((currentDirection == Forward && newTotalCurrentTime == totalDura)
+        || (currentDirection == Backward && newTotalCurrentTime == 0)) {
         stop();
     }
 }