From bdb7bc0543abe6870f3d24887b5dd0d94930d2d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kaj=20Gr=C3=B6nholm?= Date: Wed, 5 Apr 2023 11:19:31 +0300 Subject: [PATCH] QQuickScrollView and QQuickTextEdit: Fix binding loops If the content item of a ScrollArea is a height <-> width dependent item, like a TextEdit, a change in the height of the content, may trigger the vertical scrollbar to become visible, (if the content height is bigger than the ScrollArea's height). This in turn causes the right padding of the ScrollArea to change, to make place for the scrollbar, causing in this way a content resize again, giving us a binding loop on the Scrollbar's visible property and the ScrollView's 'implicitHeight'. To get rid of the binding loop on the 'implicitHeight' property, check if the TextEdit's updateSize() has already been called, and if so, don't emit its contentSizeChanged() signal again. In order to get rid of the binding loop on the Scrollbars' 'visible' property, introduce two new properties: effectiveScrollBarWidth and effectiveScrollBarHeight which represent the actual width/height of the scrollbars. Set this property dependent on the scrollbar's policy, visibility, and the scrollview's content size and make sure to update whenever any of these changes. Also, follow the same pattern as above and don't emit the signal twice if on a recursive call. [ChangeLog][Controls][ScrollView] Added effectiveScrollBarWidth and effectiveScrollBarHeight which hold the effective sizes of scrollbars. Fixes: QTBUG-97974 Change-Id: I3d915eae53881d000769de1824c3908c7acd5c6b Reviewed-by: Volker Hilsheimer Reviewed-by: Doris Verria Reviewed-by: Richard Moe Gustavsen --- src/quick/items/qquicktextedit.cpp | 7 +- src/quick/items/qquicktextedit_p_p.h | 3 +- src/quickcontrols/macos/ScrollView.qml | 4 +- src/quickcontrols/windows/ScrollView.qml | 4 +- src/quicktemplates/qquickscrollview.cpp | 130 +++++++++++++++++- src/quicktemplates/qquickscrollview_p.h | 9 ++ .../controls/data/tst_scrollview.qml | 12 ++ 7 files changed, 157 insertions(+), 12 deletions(-) diff --git a/src/quick/items/qquicktextedit.cpp b/src/quick/items/qquicktextedit.cpp index a23e662b62..2b58489e44 100644 --- a/src/quick/items/qquicktextedit.cpp +++ b/src/quick/items/qquicktextedit.cpp @@ -2707,7 +2707,12 @@ void QQuickTextEdit::updateSize() QSizeF size(newWidth, newHeight); if (d->contentSize != size) { d->contentSize = size; - emit contentSizeChanged(); + // Note: inResize is a bitfield so QScopedValueRollback can't be used here + const bool wasInResize = d->inResize; + d->inResize = true; + if (!wasInResize) + emit contentSizeChanged(); + d->inResize = wasInResize; updateTotalLines(); } } diff --git a/src/quick/items/qquicktextedit_p_p.h b/src/quick/items/qquicktextedit_p_p.h index db07462a5a..087a18734a 100644 --- a/src/quick/items/qquicktextedit_p_p.h +++ b/src/quick/items/qquicktextedit_p_p.h @@ -95,7 +95,7 @@ public: , focusOnPress(true), persistentSelection(false), requireImplicitWidth(false) , selectByMouse(true), canPaste(false), canPasteValid(false), hAlignImplicit(true) , textCached(true), inLayout(false), selectByKeyboard(false), selectByKeyboardSet(false) - , hadSelection(false), markdownText(false) + , hadSelection(false), markdownText(false), inResize(false) { } @@ -202,6 +202,7 @@ public: bool selectByKeyboardSet:1; bool hadSelection : 1; bool markdownText : 1; + bool inResize : 1; static const int largeTextSizeThreshold; }; diff --git a/src/quickcontrols/macos/ScrollView.qml b/src/quickcontrols/macos/ScrollView.qml index 4557da1c0c..86e6bc1a15 100644 --- a/src/quickcontrols/macos/ScrollView.qml +++ b/src/quickcontrols/macos/ScrollView.qml @@ -14,8 +14,8 @@ T.ScrollView { implicitHeight: Math.max(implicitBackgroundHeight + topInset + bottomInset, contentHeight + topPadding + bottomPadding) - rightPadding: ScrollBar.vertical.visible ? ScrollBar.vertical.width : 0 - bottomPadding: ScrollBar.horizontal.visible ? ScrollBar.horizontal.height : 0 + rightPadding: effectiveScrollBarWidth + bottomPadding: effectiveScrollBarHeight // Don't set __notCustomizable here, because it would require special-casing // setFlickable's call to setContentItem. diff --git a/src/quickcontrols/windows/ScrollView.qml b/src/quickcontrols/windows/ScrollView.qml index 28f2d8e0fc..65d65899ca 100644 --- a/src/quickcontrols/windows/ScrollView.qml +++ b/src/quickcontrols/windows/ScrollView.qml @@ -13,8 +13,8 @@ T.ScrollView { implicitHeight: Math.max(implicitBackgroundHeight + topInset + bottomInset, contentHeight + topPadding + bottomPadding) - rightPadding: ScrollBar.vertical.visible ? ScrollBar.vertical.width : 0 - bottomPadding: ScrollBar.horizontal.visible ? ScrollBar.horizontal.height : 0 + rightPadding: effectiveScrollBarWidth + bottomPadding: effectiveScrollBarHeight // Don't set __notCustomizable here, because it would require special-casing // setFlickable's call to setContentItem. diff --git a/src/quicktemplates/qquickscrollview.cpp b/src/quicktemplates/qquickscrollview.cpp index 978e695710..e14e8e89d2 100644 --- a/src/quicktemplates/qquickscrollview.cpp +++ b/src/quicktemplates/qquickscrollview.cpp @@ -132,10 +132,17 @@ public: void itemImplicitWidthChanged(QQuickItem *item) override; + void updateScrollBarWidth(); + void updateScrollBarHeight(); + + void disconnectScrollBarSignals(QQuickScrollBarAttachedPrivate *scrollBar); bool wasTouched = false; QQuickFlickable *flickable = nullptr; bool flickableHasExplicitContentWidth = true; bool flickableHasExplicitContentHeight = true; + bool isUpdatingScrollBar = false; + qreal effectiveScrollBarWidth = 0; + qreal effectiveScrollBarHeight = 0; }; QList QQuickScrollViewPrivate::contentChildItems() const @@ -176,6 +183,58 @@ QQuickFlickable *QQuickScrollViewPrivate::ensureFlickable(ContentItemFlag conten return flickable; } +void QQuickScrollViewPrivate::updateScrollBarWidth() +{ + Q_Q(QQuickScrollView); + qreal oldEffectiveScrollBarWidth = effectiveScrollBarWidth; + if (auto *vBar = verticalScrollBar()) { + if (vBar->policy() == QQuickScrollBar::AlwaysOff || !vBar->isVisible()) + effectiveScrollBarWidth = 0; + else + effectiveScrollBarWidth = vBar->width(); + } + if (effectiveScrollBarWidth != oldEffectiveScrollBarWidth) { + if (!isUpdatingScrollBar) { + QScopedValueRollback rollback(isUpdatingScrollBar, true); + emit q->effectiveScrollBarWidthChanged(); + } + } +} + +void QQuickScrollViewPrivate::updateScrollBarHeight() +{ + Q_Q(QQuickScrollView); + qreal oldEffectiveScrollBarHeight = effectiveScrollBarHeight; + if (auto *hBar = horizontalScrollBar()) { + if (hBar->policy() == QQuickScrollBar::AlwaysOff || !hBar->isVisible()) + effectiveScrollBarHeight = 0; + else + effectiveScrollBarHeight = hBar->height(); + } + if (effectiveScrollBarHeight != oldEffectiveScrollBarHeight) { + if (!isUpdatingScrollBar) { + QScopedValueRollback rollback(isUpdatingScrollBar, true); + emit q->effectiveScrollBarHeightChanged(); + } + + } +} + +void QQuickScrollViewPrivate::disconnectScrollBarSignals(QQuickScrollBarAttachedPrivate *scrollBar) +{ + if (!scrollBar) + return; + + if (scrollBar->vertical) { + QObjectPrivate::disconnect(scrollBar->vertical, &QQuickScrollBar::policyChanged, this, &QQuickScrollViewPrivate::updateScrollBarWidth); + QObjectPrivate::disconnect(scrollBar->vertical, &QQuickScrollBar::visibleChanged, this, &QQuickScrollViewPrivate::updateScrollBarWidth); + } + if (scrollBar->horizontal) { + QObjectPrivate::disconnect(scrollBar->horizontal, &QQuickScrollBar::policyChanged, this, &QQuickScrollViewPrivate::updateScrollBarHeight); + QObjectPrivate::disconnect(scrollBar->horizontal, &QQuickScrollBar::visibleChanged, this, &QQuickScrollViewPrivate::updateScrollBarHeight); + } +} + bool QQuickScrollViewPrivate::setFlickable(QQuickFlickable *item, ContentItemFlag contentItemFlag) { Q_Q(QQuickScrollView); @@ -187,8 +246,11 @@ bool QQuickScrollViewPrivate::setFlickable(QQuickFlickable *item, ContentItemFla if (flickable) { flickable->removeEventFilter(q); - if (attached) - QQuickScrollBarAttachedPrivate::get(attached)->setFlickable(nullptr); + if (attached) { + auto *scrollBar = QQuickScrollBarAttachedPrivate::get(attached); + scrollBar->setFlickable(nullptr); + disconnectScrollBarSignals(scrollBar); + } QObjectPrivate::disconnect(flickable->contentItem(), &QQuickItem::childrenChanged, this, &QQuickPanePrivate::contentChildrenChange); QObjectPrivate::disconnect(flickable, &QQuickFlickable::contentWidthChanged, this, &QQuickScrollViewPrivate::flickableContentWidthChanged); @@ -210,8 +272,18 @@ bool QQuickScrollViewPrivate::setFlickable(QQuickFlickable *item, ContentItemFla else flickableContentHeightChanged(); - if (attached) - QQuickScrollBarAttachedPrivate::get(attached)->setFlickable(flickable); + if (attached) { + auto *scrollBar = QQuickScrollBarAttachedPrivate::get(attached); + scrollBar->setFlickable(flickable); + if (scrollBar->vertical) { + QObjectPrivate::connect(scrollBar->vertical, &QQuickScrollBar::policyChanged, this, &QQuickScrollViewPrivate::updateScrollBarWidth); + QObjectPrivate::connect(scrollBar->vertical, &QQuickScrollBar::visibleChanged, this, &QQuickScrollViewPrivate::updateScrollBarWidth); + } + if (scrollBar->horizontal) { + QObjectPrivate::connect(scrollBar->horizontal, &QQuickScrollBar::policyChanged, this, &QQuickScrollViewPrivate::updateScrollBarHeight); + QObjectPrivate::connect(scrollBar->horizontal, &QQuickScrollBar::visibleChanged, this, &QQuickScrollViewPrivate::updateScrollBarHeight); + } + } QObjectPrivate::connect(flickable->contentItem(), &QQuickItem::childrenChanged, this, &QQuickPanePrivate::contentChildrenChange); QObjectPrivate::connect(flickable, &QQuickFlickable::contentWidthChanged, this, &QQuickScrollViewPrivate::flickableContentWidthChanged); @@ -418,6 +490,48 @@ QQuickScrollView::QQuickScrollView(QQuickItem *parent) setWheelEnabled(true); } +QQuickScrollView::~QQuickScrollView() +{ + Q_D(QQuickScrollView); + QQuickScrollBarAttached *attached = qobject_cast(qmlAttachedPropertiesObject(this, false)); + if (attached) { + auto *scrollBar = QQuickScrollBarAttachedPrivate::get(attached); + d->disconnectScrollBarSignals(scrollBar); + } +} + +/*! + \qmlproperty real QtQuick.Controls::ScrollView::effectiveScrollBarWidth + \since 6.6 + + This property holds the effective width of the vertical scrollbar. + When the scrollbar policy is \c QQuickScrollBar::AlwaysOff or the scrollbar + is not visible, this property is \c 0. + + \sa {ScrollBar::policy} +*/ +qreal QQuickScrollView::effectiveScrollBarWidth() +{ + Q_D(QQuickScrollView); + return d->effectiveScrollBarWidth; +} + +/*! + \qmlproperty real QtQuick.Controls::ScrollView::effectiveScrollBarHeight + \since 6.6 + + This property holds the effective height of the horizontal scrollbar. + When the scrollbar policy is \c QQuickScrollBar::AlwaysOff or the scrollbar + is not visible, this property is \c 0. + + \sa {ScrollBar::policy} +*/ +qreal QQuickScrollView::effectiveScrollBarHeight() +{ + Q_D(QQuickScrollView); + return d->effectiveScrollBarHeight; +} + /*! \qmlproperty list QtQuick.Controls::ScrollView::contentData \qmldefault @@ -603,10 +717,14 @@ void QQuickScrollView::contentSizeChange(const QSizeF &newSize, const QSizeF &ol // 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->flickableHasExplicitContentWidth) + if (d->hasContentWidth || !d->flickableHasExplicitContentWidth) { d->flickable->setContentWidth(newSize.width()); - if (d->hasContentHeight || !d->flickableHasExplicitContentHeight) + d->updateScrollBarWidth(); + } + if (d->hasContentHeight || !d->flickableHasExplicitContentHeight) { d->flickable->setContentHeight(newSize.height()); + d->updateScrollBarHeight(); + } } } diff --git a/src/quicktemplates/qquickscrollview_p.h b/src/quicktemplates/qquickscrollview_p.h index 927de4365c..ea4e1a0a78 100644 --- a/src/quicktemplates/qquickscrollview_p.h +++ b/src/quicktemplates/qquickscrollview_p.h @@ -27,9 +27,14 @@ class Q_QUICKTEMPLATES2_PRIVATE_EXPORT QQuickScrollView : public QQuickPane Q_OBJECT QML_NAMED_ELEMENT(ScrollView) QML_ADDED_IN_VERSION(2, 2) + Q_PROPERTY(qreal effectiveScrollBarWidth READ effectiveScrollBarWidth NOTIFY effectiveScrollBarWidthChanged FINAL REVISION(6, 6)) + Q_PROPERTY(qreal effectiveScrollBarHeight READ effectiveScrollBarHeight NOTIFY effectiveScrollBarHeightChanged FINAL REVISION(6, 6)) public: explicit QQuickScrollView(QQuickItem *parent = nullptr); + ~QQuickScrollView(); + qreal effectiveScrollBarWidth(); + qreal effectiveScrollBarHeight(); protected: bool childMouseEventFilter(QQuickItem *item, QEvent *event) override; @@ -44,6 +49,10 @@ protected: QAccessible::Role accessibleRole() const override; #endif +Q_SIGNALS: + Q_REVISION(6, 6) void effectiveScrollBarWidthChanged(); + Q_REVISION(6, 6) void effectiveScrollBarHeightChanged(); + private: Q_DISABLE_COPY(QQuickScrollView) Q_DECLARE_PRIVATE(QQuickScrollView) diff --git a/tests/auto/quickcontrols/controls/data/tst_scrollview.qml b/tests/auto/quickcontrols/controls/data/tst_scrollview.qml index 5c9c6f1184..50322ce9d6 100644 --- a/tests/auto/quickcontrols/controls/data/tst_scrollview.qml +++ b/tests/auto/quickcontrols/controls/data/tst_scrollview.qml @@ -188,6 +188,18 @@ TestCase { horizontal.increase() verify(horizontal.position > 0) compare(control.contentItem.visibleArea.xPosition, horizontal.position) + + vertical.policy = ScrollBar.AlwaysOn + horizontal.policy = ScrollBar.AlwaysOn + + verify(control.effectiveScrollBarWidth > 0) + verify(control.effectiveScrollBarHeight > 0) + + vertical.policy = ScrollBar.AlwaysOff + horizontal.policy = ScrollBar.AlwaysOff + + compare(control.effectiveScrollBarWidth, 0) + compare(control.effectiveScrollBarHeight, 0) } function test_oneChild_data() {