Fix QQuickItem change listeners

All listeners should get invoked, even if they remove themselves
while iterating the listeners. An index-based loop would skip the
next listener in the list. This change replaces the QPODVector with
a QVector, so we can make a cheap copy before iterating the listeners.

Change-Id: I2430b3763184a40ad1c5c3a68d36fecafcadb3ee
Task-number: QTBUG-53453
Reviewed-by: Robin Burchell <robin.burchell@viroteck.net>

2 files changed, 45 insertions, 39 deletions

diff --git a/src/quick/items/qquickitem.cpp b/src/quick/items/qquickitem.cpp
index 3573788855..a8c9d34110 100644
--- a/src/quick/items/qquickitem.cpp
+++ b/src/quick/items/qquickitem.cpp
@@ -2336,8 +2336,9 @@ QQuickItem::~QQuickItem()
     while (!d->childItems.isEmpty())
         d->childItems.constFirst()->setParentItem(0);
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        QQuickAnchorsPrivate *anchor = d->changeListeners.at(ii).listener->anchorPrivate();
+    const auto listeners = d->changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
+        QQuickAnchorsPrivate *anchor = change.listener->anchorPrivate();
         if (anchor)
             anchor->clearItem(this);
     }
@@ -2346,14 +2347,13 @@ QQuickItem::~QQuickItem()
         update item anchors that depended on us unless they are our child (and will also be destroyed),
         or our sibling, and our parent is also being destroyed.
     */
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        QQuickAnchorsPrivate *anchor = d->changeListeners.at(ii).listener->anchorPrivate();
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
+        QQuickAnchorsPrivate *anchor = change.listener->anchorPrivate();
         if (anchor && anchor->item && anchor->item->parentItem() && anchor->item->parentItem() != this)
             anchor->update();
     }
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = d->changeListeners.at(ii);
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::Destroyed)
             change.listener->itemDestroyed(this);
     }
@@ -3595,8 +3595,8 @@ QQuickAnchors *QQuickItemPrivate::anchors() const
 void QQuickItemPrivate::siblingOrderChanged()
 {
     Q_Q(QQuickItem);
-    for (int ii = 0; ii < changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+    const auto listeners = changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::SiblingOrder) {
             change.listener->itemSiblingOrderChanged(q);
         }
@@ -3704,8 +3704,8 @@ void QQuickItem::geometryChanged(const QRectF &newGeometry, const QRectF &oldGeo
     bool widthChange = (newGeometry.width() != oldGeometry.width());
     bool heightChange = (newGeometry.height() != oldGeometry.height());
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = d->changeListeners.at(ii);
+    const auto listeners = d->changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::Geometry) {
             if (change.gTypes == QQuickItemPrivate::GeometryChange) {
                 change.listener->itemGeometryChanged(this, newGeometry, oldGeometry);
@@ -3841,7 +3841,7 @@ void QQuickItemPrivate::removeItemChangeListener(QQuickItemChangeListener *liste
 void QQuickItemPrivate::updateOrAddGeometryChangeListener(QQuickItemChangeListener *listener, GeometryChangeTypes types)
 {
     ChangeListener change(listener, types);
-    int index = changeListeners.find(change);
+    int index = changeListeners.indexOf(change);
     if (index > -1)
         changeListeners[index].gTypes = change.gTypes;  //we may have different GeometryChangeTypes
     else
@@ -3855,7 +3855,7 @@ void QQuickItemPrivate::updateOrRemoveGeometryChangeListener(QQuickItemChangeLis
     if (types == NoChange) {
         changeListeners.removeOne(change);
     } else {
-        int index = changeListeners.find(change);
+        int index = changeListeners.indexOf(change);
         if (index > -1)
             changeListeners[index].gTypes = change.gTypes;  //we may have different GeometryChangeTypes
     }
@@ -4264,8 +4264,8 @@ void QQuickItem::setBaselineOffset(qreal offset)
 
     d->baselineOffset = offset;
 
-    for (int ii = 0; ii < d->changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = d->changeListeners.at(ii);
+    const auto listeners = d->changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::Geometry) {
             QQuickAnchorsPrivate *anchor = change.listener->anchorPrivate();
             if (anchor)
@@ -5936,66 +5936,72 @@ void QQuickItemPrivate::itemChange(QQuickItem::ItemChange change, const QQuickIt
 {
     Q_Q(QQuickItem);
     switch (change) {
-    case QQuickItem::ItemChildAddedChange:
+    case QQuickItem::ItemChildAddedChange: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Children) {
                 change.listener->itemChildAdded(q, data.item);
             }
         }
         break;
-    case QQuickItem::ItemChildRemovedChange:
+    }
+    case QQuickItem::ItemChildRemovedChange: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Children) {
                 change.listener->itemChildRemoved(q, data.item);
             }
         }
         break;
+    }
     case QQuickItem::ItemSceneChange:
         q->itemChange(change, data);
         break;
-    case QQuickItem::ItemVisibleHasChanged:
+    case QQuickItem::ItemVisibleHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Visibility) {
                 change.listener->itemVisibilityChanged(q);
             }
         }
         break;
-    case QQuickItem::ItemParentHasChanged:
+    }
+    case QQuickItem::ItemParentHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Parent) {
                 change.listener->itemParentChanged(q, data.item);
             }
         }
         break;
-    case QQuickItem::ItemOpacityHasChanged:
+    }
+    case QQuickItem::ItemOpacityHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Opacity) {
                 change.listener->itemOpacityChanged(q);
             }
         }
         break;
+    }
     case QQuickItem::ItemActiveFocusHasChanged:
         q->itemChange(change, data);
         break;
-    case QQuickItem::ItemRotationHasChanged:
+    case QQuickItem::ItemRotationHasChanged: {
         q->itemChange(change, data);
-        for (int ii = 0; ii < changeListeners.count(); ++ii) {
-            const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+        const auto listeners = changeListeners;
+        for (const QQuickItemPrivate::ChangeListener &change : listeners) {
             if (change.types & QQuickItemPrivate::Rotation) {
                 change.listener->itemRotationChanged(q);
             }
         }
         break;
+    }
     case QQuickItem::ItemAntialiasingHasChanged:
         // fall through
     case QQuickItem::ItemDevicePixelRatioHasChanged:
@@ -6350,8 +6356,8 @@ void QQuickItem::resetWidth()
 void QQuickItemPrivate::implicitWidthChanged()
 {
     Q_Q(QQuickItem);
-    for (int ii = 0; ii < changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+    const auto listeners = changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::ImplicitWidth) {
             change.listener->itemImplicitWidthChanged(q);
         }
@@ -6514,8 +6520,8 @@ void QQuickItem::resetHeight()
 void QQuickItemPrivate::implicitHeightChanged()
 {
     Q_Q(QQuickItem);
-    for (int ii = 0; ii < changeListeners.count(); ++ii) {
-        const QQuickItemPrivate::ChangeListener &change = changeListeners.at(ii);
+    const auto listeners = changeListeners;
+    for (const QQuickItemPrivate::ChangeListener &change : listeners) {
         if (change.types & QQuickItemPrivate::ImplicitHeight) {
             change.listener->itemImplicitHeightChanged(q);
         }
diff --git a/src/quick/items/qquickitem_p.h b/src/quick/items/qquickitem_p.h
index ad649e5b5f..fed3e88b68 100644
--- a/src/quick/items/qquickitem_p.h
+++ b/src/quick/items/qquickitem_p.h
@@ -62,7 +62,6 @@
 #include <QtQuick/qsgnode.h>
 #include "qquickclipnode_p.h"
 
-#include <private/qpodvector_p.h>
 #include <QtQuick/private/qquickstate_p.h>
 #include <private/qqmlnullablevalue_p.h>
 #include <private/qqmlnotifier_p.h>
@@ -332,7 +331,7 @@ public:
     Q_DECLARE_FLAGS(GeometryChangeTypes, GeometryChangeType)
 
     struct ChangeListener {
-        ChangeListener(QQuickItemChangeListener *l, QQuickItemPrivate::ChangeTypes t) : listener(l), types(t), gTypes(GeometryChange) {}
+        ChangeListener(QQuickItemChangeListener *l = Q_NULLPTR, QQuickItemPrivate::ChangeTypes t = 0) : listener(l), types(t), gTypes(GeometryChange) {}
         ChangeListener(QQuickItemChangeListener *l, QQuickItemPrivate::GeometryChangeTypes gt) : listener(l), types(Geometry), gTypes(gt) {}
         QQuickItemChangeListener *listener;
         QQuickItemPrivate::ChangeTypes types;
@@ -386,7 +385,7 @@ public:
 
     inline Qt::MouseButtons acceptedMouseButtons() const;
 
-    QPODVector<QQuickItemPrivate::ChangeListener,4> changeListeners;
+    QVector<QQuickItemPrivate::ChangeListener> changeListeners;
 
     void addItemChangeListener(QQuickItemChangeListener *listener, ChangeTypes types);
     void removeItemChangeListener(QQuickItemChangeListener *, ChangeTypes types);
@@ -932,6 +931,7 @@ QSGNode *QQuickItemPrivate::childContainerNode()
 }
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(QQuickItemPrivate::ChangeTypes)
+Q_DECLARE_TYPEINFO(QQuickItemPrivate::ChangeListener, Q_PRIMITIVE_TYPE);
 
 QT_END_NAMESPACE