ListView: allow binding on x and y in footer and header delegates

This is generally a bad idea, but it's technically a regression that was
introduced in f03a9839b689a3a1810846bb8ec08e02a9bf23b5.

The reason why it's not recommended to bind directly on x and y in
delegates, is because the delegate item positions are determined by the
item views layout, which are in conflict with the positional bindings.

However, it should be possible to partially support positional bindings
in the header and footer delegates, in ListView.
The reason for using the word "partially", is because a ListView with
horizontal orientation, should always place the header first (with x
typically being 0). But the y axis, can in theory be set directly, since
it won't cause overlap with the remaining delegate items.

Same idea for vertical ListViews. Except that the y axis become the
"primary" axis, when the orientation changes to vertical, meaning that
the x axis will then be safe to modify.

The model-view-delegate doc page is also updated, to discourage people
from binding directly on x and y, in delegates that are meant to be used
for item views.

Fixes: QTBUG-122665
Pick-to:  6.7
Change-Id: I78363e159f14827a28dba7c72d1ecef45b0c6d2a
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>

5 files changed, 119 insertions, 17 deletions

diff --git a/src/quick/doc/src/concepts/modelviewsdata/modelview.qdoc b/src/quick/doc/src/concepts/modelviewsdata/modelview.qdoc
index 7552017ea4..3aaf40f199 100644
--- a/src/quick/doc/src/concepts/modelviewsdata/modelview.qdoc
+++ b/src/quick/doc/src/concepts/modelviewsdata/modelview.qdoc
@@ -117,6 +117,14 @@ To visualize data, bind the view's \c model property to a model and the
     \snippet qml/listview.qml delegate
     \image listview-setup.png
 
+    \section2 Positioning of View Delegates
+
+    The type of view will determine how the items are positioned. \l {ListView}
+    will position the items in a straight line, depending on the \l {ListView::}{orientation},
+    while a \l {GridView} can lay them out in a 2 dimentional grid. It's \b {not} recommended
+    to bind directly on \l {Item::x}{x} and \l {Item::y}{y}, since the view's layouting
+    behavior will always take precedence over any positional binding.
+
     \section2 Accessing Views and Models from Delegates
 
     The list view to which the delegate is bound is accessible from the delegate
diff --git a/src/quick/items/qquicklistview.cpp b/src/quick/items/qquicklistview.cpp
index b67ded3d4c..3fde95e213 100644
--- a/src/quick/items/qquicklistview.cpp
+++ b/src/quick/items/qquicklistview.cpp
@@ -289,7 +289,8 @@ public:
                     : itemX() + itemWidth());
         }
     }
-    void setPosition(qreal pos, bool immediate = false) {
+
+    void setPosition(qreal pos, bool immediate = false, bool resetInactiveAxis = true) {
         // position the section immediately even if there is a transition
         if (section()) {
             if (view->orientation() == QQuickListView::Vertical) {
@@ -304,8 +305,9 @@ public:
                     section()->setX(pos);
             }
         }
-        moveTo(pointForPosition(pos), immediate);
+        moveTo(pointForPosition(pos, resetInactiveAxis), immediate);
     }
+
     void setSize(qreal size) {
         if (view->orientation() == QQuickListView::Vertical)
             item->setHeight(size);
@@ -320,26 +322,26 @@ public:
     QQuickListView *view;
 
 private:
-    QPointF pointForPosition(qreal pos) const {
+    QPointF pointForPosition(qreal pos, bool resetInactiveAxis) const {
         if (view->orientation() == QQuickListView::Vertical) {
             if (view->verticalLayoutDirection() == QQuickItemView::BottomToTop) {
                 if (section())
                     pos += section()->height();
-                return QPointF(0, -itemHeight() - pos);
+                return QPointF(resetInactiveAxis ? 0 : itemX(), -itemHeight() - pos);
             } else {
                 if (section())
                     pos += section()->height();
-                return QPointF(0, pos);
+                return QPointF(resetInactiveAxis ? 0 : itemX(), pos);
             }
         } else {
             if (view->effectiveLayoutDirection() == Qt::RightToLeft) {
                 if (section())
                     pos += section()->width();
-                return QPointF(-itemWidth() - pos, 0);
+                return QPointF(-itemWidth() - pos, resetInactiveAxis ? 0 : itemY());
             } else {
                 if (section())
                     pos += section()->width();
-                return QPointF(pos, 0);
+                return QPointF(pos, resetInactiveAxis ? 0 : itemY());
             }
         }
     }
@@ -1469,26 +1471,26 @@ void QQuickListViewPrivate::updateFooter()
 
     FxListItemSG *listItem = static_cast<FxListItemSG*>(footer);
     if (footerPositioning == QQuickListView::OverlayFooter) {
-        listItem->setPosition(isContentFlowReversed() ? -position() - footerSize() : position() + size() - footerSize());
+        listItem->setPosition(isContentFlowReversed() ? -position() - footerSize() : position() + size() - footerSize(), false, false);
     } else if (visibleItems.size()) {
         if (footerPositioning == QQuickListView::PullBackFooter) {
             qreal viewPos = isContentFlowReversed() ? -position() : position() + size();
             // using qBound() would throw an assert here, because max < min is a valid case
             // here, if the list's delegates do not fill the whole view
             qreal clampedPos = qMax(originPosition() - footerSize() + size(), qMin(listItem->position(), lastPosition()));
-            listItem->setPosition(qBound(viewPos - footerSize(), clampedPos, viewPos));
+            listItem->setPosition(qBound(viewPos - footerSize(), clampedPos, viewPos), false, false);
         } else {
             qreal endPos = lastPosition();
             if (findLastVisibleIndex() == model->count()-1) {
-                listItem->setPosition(endPos);
+                listItem->setPosition(endPos, false, false);
             } else {
                 qreal visiblePos = position() + q->height();
                 if (endPos <= visiblePos || listItem->position() < endPos)
-                    listItem->setPosition(endPos);
+                    listItem->setPosition(endPos, false, false);
             }
         }
     } else {
-        listItem->setPosition(visiblePos);
+        listItem->setPosition(visiblePos, false, false);
     }
 
     if (created)
@@ -1535,7 +1537,7 @@ void QQuickListViewPrivate::updateHeader()
 
     FxListItemSG *listItem = static_cast<FxListItemSG*>(header);
     if (headerPositioning == QQuickListView::OverlayHeader) {
-        listItem->setPosition(isContentFlowReversed() ? -position() - size() : position());
+        listItem->setPosition(isContentFlowReversed() ? -position() - size() : position(), false, false);
     } else if (visibleItems.size()) {
         const bool fixingUp = (orient == QQuickListView::Vertical ? vData : hData).fixingUp;
         if (headerPositioning == QQuickListView::PullBackHeader) {
@@ -1547,18 +1549,18 @@ void QQuickListViewPrivate::updateHeader()
             // using qBound() would throw an assert here, because max < min is a valid case
             // here, if the list's delegates do not fill the whole view
             qreal clampedPos = qMax(originPosition() - headerSize(), qMin(headerPosition, lastPosition() - size()));
-            listItem->setPosition(qBound(viewPos - headerSize(), clampedPos, viewPos));
+            listItem->setPosition(qBound(viewPos - headerSize(), clampedPos, viewPos), false, false);
         } else {
             qreal startPos = originPosition();
             if (visibleIndex == 0) {
-                listItem->setPosition(startPos - headerSize());
+                listItem->setPosition(startPos - headerSize(), false, false);
             } else {
                 if (position() <= startPos || listItem->position() > startPos - headerSize())
-                    listItem->setPosition(startPos - headerSize());
+                    listItem->setPosition(startPos - headerSize(), false, false);
             }
         }
     } else {
-        listItem->setPosition(-headerSize());
+        listItem->setPosition(-headerSize(), false, false);
     }
 
     if (created)
diff --git a/tests/auto/quick/qquicklistview2/data/bindOnHeaderAndFooterXPosition.qml b/tests/auto/quick/qquicklistview2/data/bindOnHeaderAndFooterXPosition.qml
new file mode 100644
index 0000000000..69431fb525
--- /dev/null
+++ b/tests/auto/quick/qquicklistview2/data/bindOnHeaderAndFooterXPosition.qml
@@ -0,0 +1,29 @@
+import QtQuick
+
+ListView {
+    width: 200
+    height: 300
+    spacing: 20
+    orientation: ListView.Vertical
+
+    header: Rectangle {
+        x: (ListView.view.width - width) / 2
+        color: 'tomato'
+        width: 50
+        height: 50
+    }
+
+    footer: Rectangle {
+        x: (ListView.view.width - width) / 2
+        color: 'lime'
+        width: 50
+        height: 50
+    }
+
+    model: 3
+    delegate: Text {
+        text: 'Foobar'
+        horizontalAlignment: Text.AlignHCenter
+        width: ListView.view.width
+    }
+}
diff --git a/tests/auto/quick/qquicklistview2/data/bindOnHeaderAndFooterYPosition.qml b/tests/auto/quick/qquicklistview2/data/bindOnHeaderAndFooterYPosition.qml
new file mode 100644
index 0000000000..a484a154a7
--- /dev/null
+++ b/tests/auto/quick/qquicklistview2/data/bindOnHeaderAndFooterYPosition.qml
@@ -0,0 +1,29 @@
+import QtQuick
+
+ListView {
+    width: 300
+    height: 200
+    spacing: 20
+    orientation: ListView.Horizontal
+
+    header: Rectangle {
+        y: (ListView.view.height - height) / 2
+        color: 'tomato'
+        width: 50
+        height: 50
+    }
+
+    footer: Rectangle {
+        y: (ListView.view.height - height) / 2
+        color: 'lime'
+        width: 50
+        height: 50
+    }
+
+    model: 3
+    delegate: Text {
+        text: 'Foobar'
+        verticalAlignment: Text.AlignVCenter
+        height: ListView.view.height
+    }
+}
diff --git a/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp b/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
index 105237c6c1..931d785d84 100644
--- a/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
+++ b/tests/auto/quick/qquicklistview2/tst_qquicklistview2.cpp
@@ -62,6 +62,8 @@ private slots:
 
     void changingOrientationResetsPreviousAxisValues_data();
     void changingOrientationResetsPreviousAxisValues();
+    void bindingDirectlyOnPositionInHeaderAndFooterDelegates_data();
+    void bindingDirectlyOnPositionInHeaderAndFooterDelegates();
 
     void clearObjectListModel();
 
@@ -1155,6 +1157,38 @@ void tst_QQuickListView2::changingOrientationResetsPreviousAxisValues() // QTBUG
     QVERIFY(!listView->property("isYReset").toBool());
 }
 
+void tst_QQuickListView2::bindingDirectlyOnPositionInHeaderAndFooterDelegates_data()
+{
+    QTest::addColumn<QByteArray>("sourceFile");
+    QTest::addColumn<qreal(QQuickItem::*)()const>("pos");
+    QTest::addColumn<qreal(QQuickItem::*)()const>("size");
+    QTest::newRow("XPosition") << QByteArray("bindOnHeaderAndFooterXPosition.qml") << &QQuickItem::x << &QQuickItem::width;
+    QTest::newRow("YPosition") << QByteArray("bindOnHeaderAndFooterYPosition.qml") << &QQuickItem::y << &QQuickItem::height;
+}
+void tst_QQuickListView2::bindingDirectlyOnPositionInHeaderAndFooterDelegates()
+{
+
+    typedef qreal (QQuickItem::*position_func_t)() const;
+    QFETCH(QByteArray, sourceFile);
+    QFETCH(position_func_t, pos);
+    QFETCH(position_func_t, size);
+
+    QQuickView window;
+    QVERIFY(QQuickTest::showView(window, testFileUrl(QString::fromLatin1(sourceFile))));
+    auto *listView = qobject_cast<QQuickListView *>(window.rootObject());
+    QVERIFY(listView);
+
+    const qreal widthOrHeight = (listView->*size)();
+
+    QCOMPARE((listView->headerItem()->*pos)(), (widthOrHeight - 50) / 2);
+    QCOMPARE((listView->footerItem()->*pos)(), (widthOrHeight - 50) / 2);
+
+    // Verify that the "regular" delegate items, don't honor x and y bindings.
+    // This should only be allowed for header and footer delegates.
+    for (int i = 0; i < listView->count(); ++i)
+        QCOMPARE((listView->itemAtIndex(i)->*pos)(), 0);
+}
+
 void tst_QQuickListView2::clearObjectListModel()
 {
     QQmlEngine engine;