QQuickMultiPointHandler: store QQuickHandlerPoints, not QQEventPoint*s

QQuickPointerTouchEvent::reset(QEvent *event) reuses instances of
QQuickEventPoint from one touch event to the next, but it makes no
attempt to match up each instance with the same pointId from the event.
So from the perspective of Handlers, each event can have its
touchpoints in a different order, and therefore it's always wrong to
hold onto any QQuickEventPoint pointer between events.  Instead we
use QQuickHandlerPoint for storage: both for exposing to QML, and for
internal storage in handlers that need to remember touchpoint state.

Without this change, any MultiPointHandler could "forget" which
point IDs it had chosen to react to, in any case where the event
contains additional points.  It was using a QVector<QQuickEventPoint *>
to remember the chosen points, but each of those instances might be
assigned a different touchpoint during the handling of the next
touch event (and thus its ID would change "underneath").

Perhaps this went unnoticed until now because 1) the only subclass
of MultiPointHandler was PinchHandler, and we didn't often test
use cases with extra touchpoints beyond the ones involved in the pinch
and 2) on Linux/X11 they stayed in the same order in each event.
But as soon as we try to make DragHandler inherit MultiPointHandler,
it becomes clear that it does not succeed in holding on to a particular
touchpoint while dragging multiple DragHandlers with multiple fingers,
without this patch.

Change-Id: If7e0daa9ed77b263efc09f5ea73dfba6a3c8205c
Reviewed-by: Jan Arve Sæther <jan-arve.saether@qt.io>

5 files changed, 61 insertions, 50 deletions

diff --git a/src/quick/handlers/qquickhandlerpoint.cpp b/src/quick/handlers/qquickhandlerpoint.cpp
index 4bd5d2cbfb..ac24924689 100644
--- a/src/quick/handlers/qquickhandlerpoint.cpp
+++ b/src/quick/handlers/qquickhandlerpoint.cpp
@@ -133,46 +133,42 @@ void QQuickHandlerPoint::reset(const QQuickEventPoint *point)
         m_velocity = point->velocity();
 }
 
-void QQuickHandlerPoint::reset(const QVector<QQuickEventPoint *> &points)
+void QQuickHandlerPoint::reset(const QVector<QQuickHandlerPoint> &points)
 {
     if (points.isEmpty()) {
         qWarning("reset: no points");
         return;
     }
     if (points.count() == 1) {
-        reset(points.first());
+        *this = points.first(); // copy all values
         return;
     }
     // all points are required to be from the same event
-    const QQuickPointerEvent *event = points.first()->pointerEvent();
     QPointF posSum;
     QPointF scenePosSum;
+    QPointF pressPosSum;
+    QPointF scenePressPosSum;
     QVector2D velocitySum;
     qreal pressureSum = 0;
     QSizeF ellipseDiameterSum;
-    bool press = false;
-    const QQuickPointerTouchEvent *touchEvent = event->asPointerTouchEvent();
-    for (const QQuickEventPoint *point : qAsConst(points)) {
-        posSum += point->position();
-        scenePosSum += point->scenePosition();
-        velocitySum += point->velocity();
-        if (touchEvent) {
-            pressureSum += static_cast<const QQuickEventTouchPoint *>(point)->pressure();
-            ellipseDiameterSum += static_cast<const QQuickEventTouchPoint *>(point)->ellipseDiameters();
-        }
-        if (point->state() == QQuickEventPoint::Pressed)
-            press = true;
+    for (const QQuickHandlerPoint &point : points) {
+        posSum += point.position();
+        scenePosSum += point.scenePosition();
+        pressPosSum += point.pressPosition();
+        scenePressPosSum += point.scenePressPosition();
+        velocitySum += point.velocity();
+        pressureSum += point.pressure();
+        ellipseDiameterSum += point.ellipseDiameters();
     }
     m_id = 0;
     m_uniqueId = QPointingDeviceUniqueId();
-    m_pressedButtons = event->buttons();
-    m_pressedModifiers = event->modifiers();
+    // all points are required to be from the same event, so pressed buttons and modifiers should be the same
+    m_pressedButtons = points.first().pressedButtons();
+    m_pressedModifiers = points.first().modifiers();
     m_position = posSum / points.size();
     m_scenePosition = scenePosSum / points.size();
-    if (press) {
-        m_pressPosition = m_position;
-        m_scenePressPosition = m_scenePosition;
-    }
+    m_pressPosition = pressPosSum / points.size();
+    m_scenePressPosition = scenePressPosSum / points.size();
     m_velocity = velocitySum / points.size();
     m_rotation = 0; // averaging the rotations of all the points isn't very sensible
     m_pressure = pressureSum / points.size();
diff --git a/src/quick/handlers/qquickhandlerpoint_p.h b/src/quick/handlers/qquickhandlerpoint_p.h
index 1dff52942a..946b9aac0b 100644
--- a/src/quick/handlers/qquickhandlerpoint_p.h
+++ b/src/quick/handlers/qquickhandlerpoint_p.h
@@ -93,7 +93,7 @@ public:
 
     void reset();
     void reset(const QQuickEventPoint *point);
-    void reset(const QVector<QQuickEventPoint *> &points);
+    void reset(const QVector<QQuickHandlerPoint> &points);
 
 private:
     int m_id;
diff --git a/src/quick/handlers/qquickmultipointhandler.cpp b/src/quick/handlers/qquickmultipointhandler.cpp
index 9a3732ff84..b08b00e5a7 100644
--- a/src/quick/handlers/qquickmultipointhandler.cpp
+++ b/src/quick/handlers/qquickmultipointhandler.cpp
@@ -85,15 +85,31 @@ bool QQuickMultiPointHandler::wantsPointerEvent(QQuickPointerEvent *event)
 
     const QVector<QQuickEventPoint *> candidatePoints = eligiblePoints(event);
     const bool ret = (candidatePoints.size() >= minimumPointCount() && candidatePoints.size() <= maximumPointCount());
-    if (ret)
-        m_currentPoints = candidatePoints;
+    if (ret) {
+        const int c = candidatePoints.count();
+        m_currentPoints.resize(c);
+        for (int i = 0; i < c; ++i)
+            m_currentPoints[i].reset(candidatePoints[i]);
+    } else {
+        m_currentPoints.clear();
+    }
     return ret;
 }
 
 void QQuickMultiPointHandler::handlePointerEventImpl(QQuickPointerEvent *event)
 {
     QQuickPointerHandler::handlePointerEventImpl(event);
+    // event's points can be reordered since the previous event, which is why m_currentPoints
+    // is _not_ a shallow copy of the QQuickPointerTouchEvent::m_touchPoints vector.
+    // So we have to update our m_currentPoints instances based on the given event.
+    for (QQuickHandlerPoint &p : m_currentPoints) {
+        const QQuickEventPoint *ep = event->pointById(p.id());
+        if (ep)
+            p.reset(ep);
+    }
+    QPointF sceneGrabPos = m_centroid.sceneGrabPosition();
     m_centroid.reset(m_currentPoints);
+    m_centroid.m_sceneGrabPosition = sceneGrabPos; // preserve as it was
     emit centroidChanged();
 }
 
@@ -175,24 +191,18 @@ void QQuickMultiPointHandler::setMaximumPointCount(int maximumPointCount)
 
 bool QQuickMultiPointHandler::hasCurrentPoints(QQuickPointerEvent *event)
 {
-    bool ret = true;
-    int c = event->pointCount();
-    if (c < m_currentPoints.size())
+    if (event->pointCount() < m_currentPoints.size() || m_currentPoints.size() == 0)
         return false;
     // TODO optimize: either ensure the points are sorted,
     // or use std::equal with a predicate
-    for (int i = 0; ret && i < c; ++i) {
-        if (event->point(i)->state() == QQuickEventPoint::Released)
+    for (const QQuickHandlerPoint &p : qAsConst(m_currentPoints)) {
+        const QQuickEventPoint *ep = event->pointById(p.id());
+        if (!ep)
+            return false;
+        if (ep->state() == QQuickEventPoint::Released)
             return false;
-        bool found = false;
-        int pointId = event->point(i)->pointId();
-        for (QQuickEventPoint *o : qAsConst(m_currentPoints))
-            if (o && pointId == o->pointId())
-                found = true;
-        if (!found)
-            ret = false;
     }
-    return ret;
+    return true;
 }
 
 qreal QQuickMultiPointHandler::averageTouchPointDistance(const QPointF &ref)
@@ -200,8 +210,8 @@ qreal QQuickMultiPointHandler::averageTouchPointDistance(const QPointF &ref)
     qreal ret = 0;
     if (Q_UNLIKELY(m_currentPoints.size() == 0))
         return ret;
-    for (QQuickEventPoint *point : qAsConst(m_currentPoints))
-        ret += QVector2D(point->scenePosition() - ref).length();
+    for (const QQuickHandlerPoint &p : m_currentPoints)
+        ret += QVector2D(p.scenePosition() - ref).length();
     return ret / m_currentPoints.size();
 }
 
@@ -211,8 +221,8 @@ qreal QQuickMultiPointHandler::averageStartingDistance(const QPointF &ref)
     qreal ret = 0;
     if (Q_UNLIKELY(m_currentPoints.size() == 0))
         return ret;
-    for (QQuickEventPoint *point : qAsConst(m_currentPoints))
-        ret += QVector2D(point->sceneGrabPosition() - ref).length();
+    for (const QQuickHandlerPoint &p : m_currentPoints)
+        ret += QVector2D(p.sceneGrabPosition() - ref).length();
     return ret / m_currentPoints.size();
 }
 
@@ -220,9 +230,9 @@ QVector<QQuickMultiPointHandler::PointData> QQuickMultiPointHandler::angles(cons
 {
     QVector<PointData> angles;
     angles.reserve(m_currentPoints.count());
-    for (QQuickEventPoint *point : qAsConst(m_currentPoints)) {
-        qreal angle = QLineF(ref, point->scenePosition()).angle();
-        angles.append(PointData(point->pointId(), -angle));     // convert to clockwise, to be consistent with QQuickItem::rotation
+    for (const QQuickHandlerPoint &p : m_currentPoints) {
+        qreal angle = QLineF(ref, p.scenePosition()).angle();
+        angles.append(PointData(p.id(), -angle));     // convert to clockwise, to be consistent with QQuickItem::rotation
     }
     return angles;
 }
diff --git a/src/quick/handlers/qquickmultipointhandler_p.h b/src/quick/handlers/qquickmultipointhandler_p.h
index 06045771fb..d1cf67b2cb 100644
--- a/src/quick/handlers/qquickmultipointhandler_p.h
+++ b/src/quick/handlers/qquickmultipointhandler_p.h
@@ -106,7 +106,7 @@ protected:
     bool grabPoints(QVector<QQuickEventPoint *> points);
 
 protected:
-    QVector<QQuickEventPoint *> m_currentPoints;
+    QVector<QQuickHandlerPoint> m_currentPoints;
     QQuickHandlerPoint m_centroid;
     int m_minimumPointCount;
     int m_maximumPointCount;
diff --git a/src/quick/handlers/qquickpinchhandler.cpp b/src/quick/handlers/qquickpinchhandler.cpp
index c251ae6d36..e0b798b638 100644
--- a/src/quick/handlers/qquickpinchhandler.cpp
+++ b/src/quick/handlers/qquickpinchhandler.cpp
@@ -317,8 +317,8 @@ void QQuickPinchHandler::onActiveChanged()
 void QQuickPinchHandler::handlePointerEventImpl(QQuickPointerEvent *event)
 {
     if (Q_UNLIKELY(lcPinchHandler().isDebugEnabled())) {
-        for (QQuickEventPoint *point : qAsConst(m_currentPoints))
-            qCDebug(lcPinchHandler) << point->state() << point->sceneGrabPosition() << "->" << point->scenePosition();
+        for (const QQuickHandlerPoint &p : m_currentPoints)
+            qCDebug(lcPinchHandler) << hex << p.id() << p.sceneGrabPosition() << "->" << p.scenePosition();
     }
     QQuickMultiPointHandler::handlePointerEventImpl(event);
 
@@ -354,10 +354,15 @@ void QQuickPinchHandler::handlePointerEventImpl(QQuickPointerEvent *event)
 #endif // QT_CONFIG(gestures)
     {
         bool containsReleasedPoints = event->isReleaseEvent();
+        QVector<QQuickEventPoint *> chosenPoints;
+        for (const QQuickHandlerPoint &p : m_currentPoints) {
+            QQuickEventPoint *ep = event->pointById(p.id());
+            chosenPoints << ep;
+        }
         if (!active()) {
             // Verify that at least one of the points has moved beyond threshold needed to activate the handler
-            for (QQuickEventPoint *point : qAsConst(m_currentPoints)) {
-                if (!containsReleasedPoints && QQuickWindowPrivate::dragOverThreshold(point) && grabPoints(m_currentPoints)) {
+            for (QQuickEventPoint *point : qAsConst(chosenPoints)) {
+                if (!containsReleasedPoints && QQuickWindowPrivate::dragOverThreshold(point) && grabPoints(chosenPoints)) {
                     setActive(true);
                     break;
                 } else {
@@ -386,7 +391,7 @@ void QQuickPinchHandler::handlePointerEventImpl(QQuickPointerEvent *event)
         m_startAngles = std::move(newAngles);
 
         if (!containsReleasedPoints)
-            acceptPoints(m_currentPoints);
+            acceptPoints(chosenPoints);
     }
 
     QPointF centroidParentPos;