From 21844350df530a65071e8679d5e047adf553e0f7 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Mon, 4 Nov 2019 13:12:03 +0100 Subject: [PATCH 1/4] QQmlVMEMetaObject: Scope MemberData for allocating write If we need to allocate in order to write a property of the object, we need to make sure that the member data is not garbage collected during that allocation. Change-Id: I885cdc547588c1b20450e1586765cd0266b4c4f0 Reviewed-by: Simon Hausmann Reviewed-by: Fabian Kosmale --- src/qml/qml/qqmlvmemetaobject.cpp | 55 +++++-------------------------- src/qml/qml/qqmlvmemetaobject_p.h | 18 ++++++---- 2 files changed, 21 insertions(+), 52 deletions(-) diff --git a/src/qml/qml/qqmlvmemetaobject.cpp b/src/qml/qml/qqmlvmemetaobject.cpp index 6bc469c836..15fb181516 100644 --- a/src/qml/qml/qqmlvmemetaobject.cpp +++ b/src/qml/qml/qqmlvmemetaobject.cpp @@ -388,57 +388,20 @@ void QQmlVMEMetaObject::writeProperty(int id, double v) void QQmlVMEMetaObject::writeProperty(int id, const QString& v) { QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newString(v)); -} - -void QQmlVMEMetaObject::writeProperty(int id, const QUrl& v) -{ - QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newVariantObject(QVariant::fromValue(v))); -} - -void QQmlVMEMetaObject::writeProperty(int id, const QDate& v) -{ - QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newVariantObject(QVariant::fromValue(v))); -} - -void QQmlVMEMetaObject::writeProperty(int id, const QDateTime& v) -{ - QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newVariantObject(QVariant::fromValue(v))); -} - -void QQmlVMEMetaObject::writeProperty(int id, const QPointF& v) -{ - QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newVariantObject(QVariant::fromValue(v))); -} - -void QQmlVMEMetaObject::writeProperty(int id, const QSizeF& v) -{ - QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newVariantObject(QVariant::fromValue(v))); -} - -void QQmlVMEMetaObject::writeProperty(int id, const QRectF& v) -{ - QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, engine->newVariantObject(QVariant::fromValue(v))); + if (md) { + QV4::Scope scope(engine); + QV4::Scoped(scope, md)->set(engine, id, engine->newString(v)); + } } void QQmlVMEMetaObject::writeProperty(int id, QObject* v) { QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); - if (md) - md->set(engine, id, QV4::Value::fromReturnedValue(QV4::QObjectWrapper::wrap(engine, v))); + if (md) { + QV4::Scope scope(engine); + QV4::Scoped(scope, md)->set(engine, id, QV4::Value::fromReturnedValue( + QV4::QObjectWrapper::wrap(engine, v))); + } QQmlVMEVariantQObjectPtr *guard = getQObjectGuardForProperty(id); if (v && !guard) { diff --git a/src/qml/qml/qqmlvmemetaobject_p.h b/src/qml/qml/qqmlvmemetaobject_p.h index dbcc9d2884..35bc35ce4b 100644 --- a/src/qml/qml/qqmlvmemetaobject_p.h +++ b/src/qml/qml/qqmlvmemetaobject_p.h @@ -196,12 +196,18 @@ public: void writeProperty(int id, bool v); void writeProperty(int id, double v); void writeProperty(int id, const QString& v); - void writeProperty(int id, const QPointF& v); - void writeProperty(int id, const QSizeF& v); - void writeProperty(int id, const QUrl& v); - void writeProperty(int id, const QDate& v); - void writeProperty(int id, const QDateTime& v); - void writeProperty(int id, const QRectF& v); + + template + void writeProperty(int id, const VariantCompatible &v) + { + QV4::MemberData *md = propertyAndMethodStorageAsMemberData(); + if (md) { + QV4::Scope scope(engine); + QV4::Scoped(scope, md)->set(engine, id, engine->newVariantObject( + QVariant::fromValue(v))); + } + } + void writeProperty(int id, QObject *v); void ensureQObjectWrapper(); From b802031e2d8b4b38267f1ec2c00507bfd8ed1f5f Mon Sep 17 00:00:00 2001 From: Mitch Curtis Date: Tue, 5 Nov 2019 15:49:23 +0100 Subject: [PATCH 2/4] QQuickAccessibleAttached: keep track of name being explicitly set This allows types to attach an accessible name to an item, so long as the user hasn't done so themselves. Task-number: QTBUG-66583 Change-Id: I04f26815ffeaf1198fee25dc414253de8b8dfabe Reviewed-by: Liang Qi --- src/quick/items/qquickaccessibleattached.cpp | 13 +++++++ src/quick/items/qquickaccessibleattached_p.h | 5 +++ .../qquickaccessible/tst_qquickaccessible.cpp | 34 +++++++++++++++++-- 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/quick/items/qquickaccessibleattached.cpp b/src/quick/items/qquickaccessibleattached.cpp index c150e4efa2..2da01e9151 100644 --- a/src/quick/items/qquickaccessibleattached.cpp +++ b/src/quick/items/qquickaccessibleattached.cpp @@ -433,6 +433,19 @@ void QQuickAccessibleAttached::setRole(QAccessible::Role role) } } +bool QQuickAccessibleAttached::wasNameExplicitlySet() const +{ + return m_nameExplicitlySet; +} + +// Allows types to attach an accessible name to an item as a convenience, +// so long as the user hasn't done so themselves. +void QQuickAccessibleAttached::setNameImplicitly(const QString &name) +{ + setName(name); + m_nameExplicitlySet = false; +} + QQuickAccessibleAttached *QQuickAccessibleAttached::qmlAttachedProperties(QObject *obj) { return new QQuickAccessibleAttached(obj); diff --git a/src/quick/items/qquickaccessibleattached_p.h b/src/quick/items/qquickaccessibleattached_p.h index f4194ef13d..87fb79ecc9 100644 --- a/src/quick/items/qquickaccessibleattached_p.h +++ b/src/quick/items/qquickaccessibleattached_p.h @@ -118,7 +118,10 @@ public: return QString(); return m_name; } + + bool wasNameExplicitlySet() const; void setName(const QString &name) { + m_nameExplicitlySet = true; if (name != m_name) { m_name = name; Q_EMIT nameChanged(); @@ -126,6 +129,7 @@ public: QAccessible::updateAccessibility(&ev); } } + void setNameImplicitly(const QString &name); QString description() const { return m_description; } void setDescription(const QString &description) @@ -216,6 +220,7 @@ private: QAccessible::State m_state; QAccessible::State m_stateExplicitlySet; QString m_name; + bool m_nameExplicitlySet = false; QString m_description; static QMetaMethod sigPress; diff --git a/tests/auto/quick/qquickaccessible/tst_qquickaccessible.cpp b/tests/auto/quick/qquickaccessible/tst_qquickaccessible.cpp index c5fdb6c1b9..2a1b65c392 100644 --- a/tests/auto/quick/qquickaccessible/tst_qquickaccessible.cpp +++ b/tests/auto/quick/qquickaccessible/tst_qquickaccessible.cpp @@ -185,7 +185,8 @@ void tst_QQuickAccessible::quickAttachedProperties() QObject *object = component.create(); QVERIFY(object != nullptr); - QObject *attachedObject = QQuickAccessibleAttached::attachedProperties(object); + const auto attachedObject = qobject_cast( + QQuickAccessibleAttached::attachedProperties(object)); QVERIFY(attachedObject); if (attachedObject) { QVariant p = attachedObject->property("role"); @@ -195,6 +196,7 @@ void tst_QQuickAccessible::quickAttachedProperties() QCOMPARE(p.isNull(), true); p = attachedObject->property("description"); QCOMPARE(p.isNull(), true); + QCOMPARE(attachedObject->wasNameExplicitlySet(), false); } delete object; } @@ -211,7 +213,8 @@ void tst_QQuickAccessible::quickAttachedProperties() QObject *object = component.create(); QVERIFY(object != nullptr); - QObject *attachedObject = QQuickAccessibleAttached::attachedProperties(object); + const auto attachedObject = qobject_cast( + QQuickAccessibleAttached::attachedProperties(object)); QVERIFY(attachedObject); if (attachedObject) { QVariant p = attachedObject->property("role"); @@ -223,6 +226,7 @@ void tst_QQuickAccessible::quickAttachedProperties() p = attachedObject->property("description"); QCOMPARE(p.isNull(), false); QCOMPARE(p.toString(), QLatin1String("Duck")); + QCOMPARE(attachedObject->wasNameExplicitlySet(), true); } delete object; } @@ -292,6 +296,32 @@ void tst_QQuickAccessible::quickAttachedProperties() } delete object; } + // Check that a name can be implicitly set. + { + QQmlEngine engine; + QQmlComponent component(&engine); + component.setData(R"( + import QtQuick 2.0 + Text { + Accessible.role: Accessible.Button + Accessible.description: "Text Button" + })", QUrl()); + QScopedPointer object(component.create()); + QVERIFY(object); + + const auto attachedObject = qobject_cast( + QQuickAccessibleAttached::attachedProperties(object.data())); + QVERIFY(attachedObject); + QVERIFY(!attachedObject->wasNameExplicitlySet()); + + attachedObject->setNameImplicitly(QLatin1String("Implicit")); + QCOMPARE(attachedObject->name(), QLatin1String("Implicit")); + QVERIFY(!attachedObject->wasNameExplicitlySet()); + + attachedObject->setName(QLatin1String("Explicit")); + QCOMPARE(attachedObject->name(), QLatin1String("Explicit")); + QVERIFY(attachedObject->wasNameExplicitlySet()); + } QTestAccessibility::clearEvents(); } From 63042da9c94fd8d04583631249a7bfa54ba2656f Mon Sep 17 00:00:00 2001 From: Mitch Curtis Date: Mon, 11 Nov 2019 11:18:15 +0100 Subject: [PATCH 3/4] Doc: add section on imperative vs declarative to best practices page Change-Id: I6ea16474e5e59f76f7b2c5806e381a1a4b05db20 Fixes: QTBUG-79903 Reviewed-by: Fabian Kosmale --- .../src/guidelines/qtquick-bestpractices.qdoc | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/quick/doc/src/guidelines/qtquick-bestpractices.qdoc b/src/quick/doc/src/guidelines/qtquick-bestpractices.qdoc index f4a1616943..abfff7cc11 100644 --- a/src/quick/doc/src/guidelines/qtquick-bestpractices.qdoc +++ b/src/quick/doc/src/guidelines/qtquick-bestpractices.qdoc @@ -402,6 +402,43 @@ property MyMenu optionsMenu For information on performance in QML and Qt Quick, see \l {Performance Considerations And Suggestions}. +\section1 Prefer Declarative Bindings Over Imperative Assignments + +In QML, it's possible to use imperative JavaScript code to perform tasks +such as responding to input events, send data over a network, and so on. +Imperative code has an important place in QML, but it's also important +to be aware of when not to use it. + +For example, consider the following imperative assignment: + +\code +Rectangle { + Component.onCompleted: color = "red" +} +\endcode + +This has the following disadvantages: + +\list +\li It's slow. The color property will first be evaluated with a + default-constructed value, and then again with "red" later on. +\li It delays errors that could be found at build time to run time, slowing + down the development process. +\li It overwrites any declarative binding that was in place. In most cases this + is intended, but sometimes it can be unintentional. + See \l {Debugging overwriting of bindings} for more information. +\li It interferes with tooling; Qt Quick Designer, for example, doesn't support + JavaScript. +\endlist + +The code can be rewritten to be a declarative binding instead: + +\code +Rectangle { + color: "red" +} +\endcode + \section1 Tools and Utilities For information on useful tools and utilies that make working with QML and From c89f7a221b7c31a0a4e1b0eed2e91d7633f4eab2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Arve=20S=C3=A6ther?= Date: Mon, 14 Oct 2019 14:39:48 +0200 Subject: [PATCH 4/4] Fix a layout bug caused by a delegate item that was moved by the user The position of the first item in the list of visualItems was used to know how to layout the rest of the visual items. However, this did not work if the first item was actually moved (e.g. due to a DnD operation). We therefore store the position of the first visual item after each time we arrange it, and use that as a basis on where to start layouting from. Task-number: QTBUG-78076 Change-Id: I837f5b7d61a13d98d23287685c6fd66817360906 Reviewed-by: Richard Moe Gustavsen --- src/quick/items/qquickitemview.cpp | 6 +++++- src/quick/items/qquickitemview_p_p.h | 6 ++++++ src/quick/items/qquicklistview.cpp | 1 + 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/quick/items/qquickitemview.cpp b/src/quick/items/qquickitemview.cpp index 120eeb13d5..370ecee01d 100644 --- a/src/quick/items/qquickitemview.cpp +++ b/src/quick/items/qquickitemview.cpp @@ -1784,6 +1784,7 @@ void QQuickItemViewPrivate::refill(qreal from, qreal to) if (prevCount != itemCount) emit q->countChanged(); } while (currentChanges.hasPendingChanges() || bufferedChanges.hasPendingChanges()); + storeFirstVisibleItemPosition(); } void QQuickItemViewPrivate::regenerate(bool orientationChanged) @@ -1870,6 +1871,7 @@ void QQuickItemViewPrivate::layout() updateSections(); layoutVisibleItems(); + storeFirstVisibleItemPosition(); int lastIndexInView = findLastIndexInView(); refill(); @@ -1954,7 +1956,7 @@ bool QQuickItemViewPrivate::applyModelChanges(ChangeResult *totalInsertionResult prevFirstItemInViewPos = prevFirstItemInView->position(); prevFirstItemInViewIndex = prevFirstItemInView->index; } - qreal prevVisibleItemsFirstPos = visibleItems.count() ? visibleItems.constFirst()->position() : 0.0; + qreal prevVisibleItemsFirstPos = visibleItems.count() ? firstVisibleItemPosition : 0.0; totalInsertionResult->visiblePos = prevFirstItemInViewPos; totalRemovalResult->visiblePos = prevFirstItemInViewPos; @@ -2000,6 +2002,7 @@ bool QQuickItemViewPrivate::applyModelChanges(ChangeResult *totalInsertionResult if (!insertions.isEmpty()) { repositionFirstItem(prevVisibleItemsFirst, prevVisibleItemsFirstPos, prevFirstItemInView, &insertionResult, &removalResult); layoutVisibleItems(removals.first().index); + storeFirstVisibleItemPosition(); } } @@ -2020,6 +2023,7 @@ bool QQuickItemViewPrivate::applyModelChanges(ChangeResult *totalInsertionResult if (i < insertions.count() - 1) { repositionFirstItem(prevVisibleItemsFirst, prevVisibleItemsFirstPos, prevFirstItemInView, &insertionResult, &removalResult); layoutVisibleItems(insertions[i].index); + storeFirstVisibleItemPosition(); } itemCount += insertions[i].count; } diff --git a/src/quick/items/qquickitemview_p_p.h b/src/quick/items/qquickitemview_p_p.h index 1f42c847b3..325b91d190 100644 --- a/src/quick/items/qquickitemview_p_p.h +++ b/src/quick/items/qquickitemview_p_p.h @@ -258,6 +258,12 @@ public: MovementReason moveReason; QList visibleItems; + qreal firstVisibleItemPosition = 0; + void storeFirstVisibleItemPosition() { + if (!visibleItems.isEmpty()) { + firstVisibleItemPosition = visibleItems.constFirst()->position(); + } + } int visibleIndex; int currentIndex; FxViewItem *currentItem; diff --git a/src/quick/items/qquicklistview.cpp b/src/quick/items/qquicklistview.cpp index 8b70ffd0d7..ebae4d14ea 100644 --- a/src/quick/items/qquicklistview.cpp +++ b/src/quick/items/qquicklistview.cpp @@ -805,6 +805,7 @@ void QQuickListViewPrivate::layoutVisibleItems(int fromModelIndex) FxViewItem *firstItem = *visibleItems.constBegin(); bool fixedCurrent = currentItem && firstItem->item == currentItem->item; + firstVisibleItemPosition = firstItem->position(); qreal sum = firstItem->size(); qreal pos = firstItem->position() + firstItem->size() + spacing; firstItem->setVisible(firstItem->endPosition() >= from && firstItem->position() <= to);