QQuickScrollView: respect the content size set on/by the flickable

If the flickable inside a scrollview has an explicit content
size assigned, or if the flickable was created by the
application, then don't fall back to calculate the content
size. The reason is that it should not try to change the content
size of a flickable that is controlled by the application (or by
the flickable itself, in the case of ListView, ScrollView and
TextArea). Sometimes e.g ListView will report a negative
contentWidth, which is not considered illegal.

From before we used to just check if the child flickable had
a negative content size, and if so, take that as a evidence
that the flickable was owned by the scrollview. But with this
patch, we instead add two extra variables that keeps explicit
track of whether or not we should read the content size from the
flickable, regardless of the values it might return.

With the two new variables, we also no longer need the
"ownsFlickable" property, as we can instead use the new
variables to check for the same.

Fixes: QTBUG-72536
Fixes: QTBUG-74000
Fixes: QTBUG-72940
Change-Id: Iec87cc953557bf8c1bdb32a3c11b721c607fc19a
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>

2 files changed, 128 insertions, 27 deletions

diff --git a/src/quicktemplates2/qquickscrollview.cpp b/src/quicktemplates2/qquickscrollview.cpp
index 7890a30a..c0f73692 100644
--- a/src/quicktemplates2/qquickscrollview.cpp
+++ b/src/quicktemplates2/qquickscrollview.cpp
@@ -129,8 +129,8 @@ public:
     QQuickFlickable *ensureFlickable(bool content);
     bool setFlickable(QQuickFlickable *flickable, bool content);
 
-    void updateContentWidth();
-    void updateContentHeight();
+    void flickableContentWidthChanged();
+    void flickableContentHeightChanged();
 
     qreal getContentWidth() const override;
     qreal getContentHeight() const override;
@@ -154,7 +154,8 @@ public:
 
     bool wasTouched = false;
     QQuickFlickable *flickable = nullptr;
-    bool ownsFlickable = false;
+    bool flickableHasExplicitContentWidth = true;
+    bool flickableHasExplicitContentHeight = true;
 };
 
 QList<QQuickItem *> QQuickScrollViewPrivate::contentChildItems() const
@@ -176,8 +177,9 @@ QQuickFlickable *QQuickScrollViewPrivate::ensureFlickable(bool content)
 {
     Q_Q(QQuickScrollView);
     if (!flickable) {
+        flickableHasExplicitContentWidth = false;
+        flickableHasExplicitContentHeight = false;
         setFlickable(new QQuickFlickable(q), content);
-        ownsFlickable = true;
     }
     return flickable;
 }
@@ -197,8 +199,8 @@ bool QQuickScrollViewPrivate::setFlickable(QQuickFlickable *item, bool content)
             QQuickScrollBarAttachedPrivate::get(attached)->setFlickable(nullptr);
 
         QObjectPrivate::disconnect(flickable->contentItem(), &QQuickItem::childrenChanged, this, &QQuickPanePrivate::contentChildrenChange);
-        QObjectPrivate::disconnect(flickable, &QQuickFlickable::contentWidthChanged, this, &QQuickScrollViewPrivate::updateContentWidth);
-        QObjectPrivate::disconnect(flickable, &QQuickFlickable::contentHeightChanged, this, &QQuickScrollViewPrivate::updateContentHeight);
+        QObjectPrivate::disconnect(flickable, &QQuickFlickable::contentWidthChanged, this, &QQuickScrollViewPrivate::flickableContentWidthChanged);
+        QObjectPrivate::disconnect(flickable, &QQuickFlickable::contentHeightChanged, this, &QQuickScrollViewPrivate::flickableContentHeightChanged);
     }
 
     flickable = item;
@@ -210,24 +212,24 @@ bool QQuickScrollViewPrivate::setFlickable(QQuickFlickable *item, bool content)
         if (hasContentWidth)
             flickable->setContentWidth(contentWidth);
         else
-            updateContentWidth();
+            flickableContentWidthChanged();
         if (hasContentHeight)
             flickable->setContentHeight(contentHeight);
         else
-            updateContentHeight();
+            flickableContentHeightChanged();
 
         if (attached)
             QQuickScrollBarAttachedPrivate::get(attached)->setFlickable(flickable);
 
         QObjectPrivate::connect(flickable->contentItem(), &QQuickItem::childrenChanged, this, &QQuickPanePrivate::contentChildrenChange);
-        QObjectPrivate::connect(flickable, &QQuickFlickable::contentWidthChanged, this, &QQuickScrollViewPrivate::updateContentWidth);
-        QObjectPrivate::connect(flickable, &QQuickFlickable::contentHeightChanged, this, &QQuickScrollViewPrivate::updateContentHeight);
+        QObjectPrivate::connect(flickable, &QQuickFlickable::contentWidthChanged, this, &QQuickScrollViewPrivate::flickableContentWidthChanged);
+        QObjectPrivate::connect(flickable, &QQuickFlickable::contentHeightChanged, this, &QQuickScrollViewPrivate::flickableContentHeightChanged);
     }
 
     return true;
 }
 
-void QQuickScrollViewPrivate::updateContentWidth()
+void QQuickScrollViewPrivate::flickableContentWidthChanged()
 {
     Q_Q(QQuickScrollView);
     if (!flickable || !componentComplete)
@@ -237,11 +239,12 @@ void QQuickScrollViewPrivate::updateContentWidth()
     if (qFuzzyCompare(cw, implicitContentWidth))
         return;
 
+    flickableHasExplicitContentWidth = true;
     implicitContentWidth = cw;
     emit q->implicitContentWidthChanged();
 }
 
-void QQuickScrollViewPrivate::updateContentHeight()
+void QQuickScrollViewPrivate::flickableContentHeightChanged()
 {
     Q_Q(QQuickScrollView);
     if (!flickable || !componentComplete)
@@ -251,29 +254,32 @@ void QQuickScrollViewPrivate::updateContentHeight()
     if (qFuzzyCompare(ch, implicitContentHeight))
         return;
 
+    flickableHasExplicitContentHeight = true;
     implicitContentHeight = ch;
     emit q->implicitContentHeightChanged();
 }
 
 qreal QQuickScrollViewPrivate::getContentWidth() const
 {
-    if (flickable) {
-        const qreal cw = flickable->contentWidth();
-        if (cw > 0)
-            return cw;
-    }
+    if (flickable && flickableHasExplicitContentWidth)
+        return flickable->contentWidth();
 
+    // The scrollview wraps a flickable created by us, and nobody searched for it and
+    // modified its contentWidth. In that case, since the application does not control
+    // this flickable, we fall back to calculate the content width based on the child
+    // items inside it.
     return QQuickPanePrivate::getContentWidth();
 }
 
 qreal QQuickScrollViewPrivate::getContentHeight() const
 {
-    if (flickable) {
-        const qreal ch = flickable->contentHeight();
-        if (ch > 0)
-            return ch;
-    }
+    if (flickable && flickableHasExplicitContentHeight)
+        return flickable->contentHeight();
 
+    // The scrollview wraps a flickable created by us, and nobody searched for it and
+    // modified its contentHeight. In that case, since the application does not control
+    // this flickable, we fall back to calculate the content height based on the child
+    // items inside it.
     return QQuickPanePrivate::getContentHeight();
 }
 
@@ -557,7 +563,13 @@ void QQuickScrollView::componentComplete()
 void QQuickScrollView::contentItemChange(QQuickItem *newItem, QQuickItem *oldItem)
 {
     Q_D(QQuickScrollView);
-    d->setFlickable(qobject_cast<QQuickFlickable *>(newItem), false);
+    if (newItem != d->flickable) {
+        // The new flickable was not created by us. In that case, we always
+        // assume/require that it has an explicit content size assigned.
+        d->flickableHasExplicitContentWidth = true;
+        d->flickableHasExplicitContentHeight = true;
+        d->setFlickable(qobject_cast<QQuickFlickable *>(newItem), false);
+    }
     QQuickPane::contentItemChange(newItem, oldItem);
 }
 
@@ -566,15 +578,15 @@ void QQuickScrollView::contentSizeChange(const QSizeF &newSize, const QSizeF &ol
     Q_D(QQuickScrollView);
     QQuickPane::contentSizeChange(newSize, oldSize);
     if (d->flickable) {
-        // Only set the content size on the flickable if we created the
-        // flickable ourselves. Otherwise we can end up overwriting
+        // Only set the content size on the flickable if the flickable doesn't
+        // have an explicit assignment from before. Otherwise we can end up overwriting
         // assignments done to those properties by the application. The
         // exception is if the application has assigned a content size
         // directly to the scrollview, which will then win even if the
         // application has assigned something else to the flickable.
-        if (d->hasContentWidth || d->ownsFlickable)
+        if (d->hasContentWidth || !d->flickableHasExplicitContentWidth)
             d->flickable->setContentWidth(newSize.width());
-        if (d->hasContentHeight || d->ownsFlickable)
+        if (d->hasContentHeight || !d->flickableHasExplicitContentHeight)
             d->flickable->setContentHeight(newSize.height());
     }
 }
diff --git a/tests/auto/controls/data/tst_scrollview.qml b/tests/auto/controls/data/tst_scrollview.qml
index 4d5b1e5b..87c39509 100644
--- a/tests/auto/controls/data/tst_scrollview.qml
+++ b/tests/auto/controls/data/tst_scrollview.qml
@@ -146,6 +146,40 @@ TestCase {
     }
 
     Component {
+        id: scrollableFlickable
+        ScrollView {
+            Flickable {
+                Item {
+                    width: 100
+                    height: 100
+                }
+            }
+        }
+    }
+
+    Component {
+        id: scrollableWithContentSize
+        ScrollView {
+            contentWidth: 1000
+            contentHeight: 1000
+            Flickable {
+            }
+        }
+    }
+
+    Component {
+        id: scrollableAndFlicableWithContentSize
+        ScrollView {
+            contentWidth: 1000
+            contentHeight: 1000
+            Flickable {
+                contentWidth: 200
+                contentHeight: 200
+            }
+        }
+    }
+
+    Component {
         id: scrollableTextArea
         ScrollView {
             TextArea {
@@ -257,6 +291,61 @@ TestCase {
         compare(control.contentHeight, listview.contentHeight)
     }
 
+    function test_scrollableFlickable() {
+        // Check that if the application adds a flickable as a child of a
+        // scrollview, the scrollview doesn't try to calculate and change
+        // the flickables contentWidth/Height based on the flickables
+        // children, even if the flickable has an empty or negative content
+        // size. Some flickables (e.g ListView) sets a negative
+        // contentWidth on purpose, which should be respected.
+        var scrollview = createTemporaryObject(scrollableFlickable, testCase)
+        verify(scrollview)
+
+        var flickable = scrollview.contentItem
+        verify(flickable.hasOwnProperty("contentX"))
+        verify(flickable.hasOwnProperty("contentY"))
+
+        compare(flickable.contentWidth, -1)
+        compare(flickable.contentHeight, -1)
+        compare(scrollview.contentWidth, -1)
+        compare(scrollview.contentHeight, -1)
+    }
+
+    function test_scrollableWithContentSize() {
+        // Check that if the scrollview has contentWidth/Height set, but
+        // not the flickable, then those values will be forwarded and used
+        // by the flickable (rather than trying to calculate the content size
+        // based on the flickables children).
+        var scrollview = createTemporaryObject(scrollableWithContentSize, testCase)
+        verify(scrollview)
+
+        var flickable = scrollview.contentItem
+        verify(flickable.hasOwnProperty("contentX"))
+        verify(flickable.hasOwnProperty("contentY"))
+
+        compare(flickable.contentWidth, 1000)
+        compare(flickable.contentHeight, 1000)
+        compare(scrollview.contentWidth, 1000)
+        compare(scrollview.contentHeight, 1000)
+    }
+
+    function test_scrollableAndFlickableWithContentSize() {
+        // Check that if both the scrollview and the flickable has
+        // contentWidth/Height set (which is an inconsistency/fault
+        // by the app), the content size of the scrollview wins.
+        var scrollview = createTemporaryObject(scrollableAndFlicableWithContentSize, testCase)
+        verify(scrollview)
+
+        var flickable = scrollview.contentItem
+        verify(flickable.hasOwnProperty("contentX"))
+        verify(flickable.hasOwnProperty("contentY"))
+
+        compare(flickable.contentWidth, 1000)
+        compare(flickable.contentHeight, 1000)
+        compare(scrollview.contentWidth, 1000)
+        compare(scrollview.contentHeight, 1000)
+    }
+
     function test_flickableWithExplicitContentSize() {
         var control = createTemporaryObject(emptyFlickable, testCase)
         verify(control)