Fix bad rendering after text selection

In 9db23e0e04906cf9ea33e23fa41f34955e5e6fe0 we added a new call to
createTextNode(), but it turns out that it could be called again for the
same block, a few lines below (in code that was originally added in
dfdc4ce825e814b30449cfa1c85d3d2f47f8a845), if we have not reached the
"last node that needed replacing or last block of the frame". This
resulted in a memory leak: TransformNodes were created without parents,
which b616028dae933e7ff6142d41bdafee8ad445a8ec tried to fix by calling
addCurrentTextNodeToRoot(). That change made the code in "Update the
position of the subsequent text blocks" re-add offsets that had already
been added, which had the effect of moving down the blocks below the
selected ones by the height of one line; but the new call to
addCurrentTextNodeToRoot() was anyway just adding empty TransformNode
instances to the scene graph. Afterwards we can see that any
TransformNode that does not have a GeometryNode as a child is not
actually rendering any text, it's just wasting memory. Having a debug
operator for QQuickTextEditPrivate::Node, and using a QQuickTextEdit
subclass in the autotest that checks the nodes corresponding to blocks
rendered at the end of updatePaintNode(), makes this easier to see
and verify.

So now, if createTextNode() has already been called, we avoid calling
it again a few lines below; but we keep the checks in place to make
sure that every node which does not have a parent will get added to
the scene graph (regardless of which line of code it was created on),
so that there's no leak.

It remains to be seen if this could be cleaned up further.

Amends 9db23e0e04906cf9ea33e23fa41f34955e5e6fe0 and
b616028dae933e7ff6142d41bdafee8ad445a8ec

Task-number: QTBUG-103819
Fixes: QTBUG-105208
Fixes: QTBUG-105555
Fixes: QTBUG-105728
Change-Id: I321980c705887bfdb37825f7e6ed8da3d200654e
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io>
(cherry picked from commit b271755e6c9a5a939c3ad6dfca8da573779e06ee)
Reviewed-by: Shawn Rutledge <shawn.rutledge@qt.io>

4 files changed, 116 insertions, 2 deletions

diff --git a/src/quick/items/qquicktextedit.cpp b/src/quick/items/qquicktextedit.cpp
index 4752e3e299..0df71fc559 100644
--- a/src/quick/items/qquicktextedit.cpp
+++ b/src/quick/items/qquicktextedit.cpp
@@ -2255,11 +2255,13 @@ QSGNode *QQuickTextEdit::updatePaintNode(QSGNode *oldNode, UpdatePaintNodeData *
                         }
                     }
 
+                    bool createdNodeInView = false;
                     if (inView) {
                         if (!engine.hasContents()) {
                             if (node && !node->parent())
                                 d->addCurrentTextNodeToRoot(&engine, rootNode, node, nodeIterator, nodeStart);
                             node = d->createTextNode();
+                            createdNodeInView = true;
                             updateNodeTransform(node, nodeOffset);
                             nodeStart = block.position();
                         }
@@ -2269,13 +2271,13 @@ QSGNode *QQuickTextEdit::updatePaintNode(QSGNode *oldNode, UpdatePaintNodeData *
 
                     if ((it.atEnd()) || block.next().position() >= firstCleanNode.startPos())
                         break; // last node that needed replacing or last block of the frame
-
                     QList<int>::const_iterator lowerBound = std::lower_bound(frameBoundaries.constBegin(), frameBoundaries.constEnd(), block.next().position());
                     if (node && (currentNodeSize > nodeBreakingSize || lowerBound == frameBoundaries.constEnd() || *lowerBound > nodeStart)) {
                         currentNodeSize = 0;
                         if (!node->parent())
                             d->addCurrentTextNodeToRoot(&engine, rootNode, node, nodeIterator, nodeStart);
-                        node = d->createTextNode();
+                        if (!createdNodeInView)
+                            node = d->createTextNode();
                         resetEngine(&engine, d->color, d->selectedTextColor, d->selectionColor);
                         nodeStart = block.next().position();
                     }
@@ -3322,6 +3324,16 @@ void QQuickTextEdit::clear()
     d->control->clear();
 }
 
+#ifndef QT_NO_DEBUG_STREAM
+QDebug operator<<(QDebug debug, const QQuickTextEditPrivate::Node &n)
+{
+    QDebugStateSaver saver(debug);
+    debug.space();
+    debug << "Node(startPos:" << n.m_startPos << "dirty:" << n.m_dirty << n.m_node << ')';
+    return debug;
+}
+#endif
+
 QT_END_NAMESPACE
 
 #include "moc_qquicktextedit_p.cpp"
diff --git a/src/quick/items/qquicktextedit_p_p.h b/src/quick/items/qquicktextedit_p_p.h
index 4ec2752eb5..1f0099e00e 100644
--- a/src/quick/items/qquicktextedit_p_p.h
+++ b/src/quick/items/qquicktextedit_p_p.h
@@ -89,6 +89,10 @@ public:
         int m_startPos;
         QQuickTextNode* m_node;
         bool m_dirty;
+
+#ifndef QT_NO_DEBUG_STREAM
+        friend QDebug Q_QUICK_PRIVATE_EXPORT operator<<(QDebug, const Node &);
+#endif
     };
     typedef QList<Node>::iterator TextNodeIterator;
 
@@ -237,6 +241,10 @@ public:
     static const int largeTextSizeThreshold;
 };
 
+#ifndef QT_NO_DEBUG_STREAM
+QDebug Q_QUICK_PRIVATE_EXPORT operator<<(QDebug debug, const QQuickTextEditPrivate::Node &);
+#endif
+
 QT_END_NAMESPACE
 
 #endif // QQUICKTEXTEDIT_P_P_H
diff --git a/tests/auto/quick/qquicktextedit/data/threeLines.qml b/tests/auto/quick/qquicktextedit/data/threeLines.qml
new file mode 100644
index 0000000000..cee03bfa15
--- /dev/null
+++ b/tests/auto/quick/qquicktextedit/data/threeLines.qml
@@ -0,0 +1,7 @@
+import QtQuick
+import Qt.test 1.0
+
+NodeCheckerTextEdit {
+    width: 200; height: 100
+    text: "Line 1\nLine 2\nLine 3\n"
+}
diff --git a/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp b/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp
index fbbd36e18a..66d1d29ae6 100644
--- a/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp
+++ b/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp
@@ -101,6 +101,8 @@ public:
     tst_qquicktextedit();
 
 private slots:
+    void initTestCase() override;
+
     void cleanup();
     void text();
     void width();
@@ -182,6 +184,7 @@ private slots:
     void implicitSizeBinding();
     void largeTextObservesViewport_data();
     void largeTextObservesViewport();
+    void renderingAroundSelection();
 
     void signal_editingfinished();
 
@@ -369,6 +372,56 @@ tst_qquicktextedit::tst_qquicktextedit()
                  //
 }
 
+class NodeCheckerTextEdit : public QQuickTextEdit
+{
+public:
+    NodeCheckerTextEdit(QQuickItem *parent = nullptr) : QQuickTextEdit(parent) {}
+
+    void populateLinePositions(QSGNode *node)
+    {
+        linePositions.clear();
+        lastLinePosition = 0;
+        QSGNode *ch = node->firstChild();
+        while (ch != node->lastChild()) {
+            QCOMPARE(ch->type(), QSGNode::TransformNodeType);
+            QSGTransformNode *tn = static_cast<QSGTransformNode *>(ch);
+            int y = 0;
+            if (!tn->matrix().isIdentity())
+                y = tn->matrix().column(3).y();
+            if (tn->childCount() == 0) {
+                // A TransformNode with no children is a waste of memory.
+                // So far, QQuickTextEdit still creates a couple of extras.
+                qCDebug(lcTests) << "ignoring leaf TransformNode" << tn << "@ y" << y;
+            } else {
+                qCDebug(lcTests) << "child" << tn << "@ y" << y << "has children" << tn->childCount();
+                if (!linePositions.contains(y)) {
+                    linePositions.append(y);
+                    lastLinePosition = qMax(lastLinePosition, y);
+                }
+            }
+            ch = ch->nextSibling();
+        }
+        std::sort(linePositions.begin(), linePositions.end());
+    }
+
+    QSGNode *updatePaintNode(QSGNode *node, UpdatePaintNodeData *data) override
+    {
+       QSGNode *ret = QQuickTextEdit::updatePaintNode(node, data);
+       qCDebug(lcTests) << "updated root node" << ret;
+       populateLinePositions(ret);
+       return ret;
+    }
+
+    QList<int> linePositions;
+    int lastLinePosition;
+};
+
+void tst_qquicktextedit::initTestCase()
+{
+    QQmlDataTest::initTestCase();
+    qmlRegisterType<NodeCheckerTextEdit>("Qt.test", 1, 0, "NodeCheckerTextEdit");
+}
+
 void tst_qquicktextedit::cleanup()
 {
     // ensure not even skipped tests with custom input context leave it dangling
@@ -3824,6 +3877,40 @@ void tst_qquicktextedit::largeTextObservesViewport()
     QCOMPARE(textPriv->cursorItem->isVisible(), textPriv->renderedRegion.intersects(textItem->cursorRectangle()));
 }
 
+void tst_qquicktextedit::renderingAroundSelection()
+{
+    QQuickView window;
+    QVERIFY(QQuickTest::showView(window, testFileUrl("threeLines.qml")));
+    NodeCheckerTextEdit *textItem = qmlobject_cast<NodeCheckerTextEdit*>(window.rootObject());
+    QVERIFY(textItem);
+    QTRY_VERIFY(textItem->linePositions.count() > 0);
+    const auto linePositions = textItem->linePositions;
+    const int lastLinePosition = textItem->lastLinePosition;
+    QQuickTextEditPrivate *textPriv = QQuickTextEditPrivate::get(textItem);
+    QSignalSpy renderSpy(&window, &QQuickWindow::afterRendering);
+
+    if (lcTests().isDebugEnabled())
+        QTest::qWait(500); // for visual check; not needed in CI
+
+    const int renderCount = renderSpy.count();
+    QPoint p1 = textItem->mapToScene(textItem->positionToRectangle(8).center()).toPoint();
+    QPoint p2 = textItem->mapToScene(textItem->positionToRectangle(10).center()).toPoint();
+    qCDebug(lcTests) << "drag from" << p1 << "to" << p2;
+    QTest::mousePress(&window, Qt::LeftButton, Qt::NoModifier, p1);
+    QTest::mouseMove(&window, p2);
+    QTest::mouseRelease(&window, Qt::LeftButton, Qt::NoModifier, p2);
+    // ensure that QQuickTextEdit::updatePaintNode() has a chance to run
+    QTRY_VERIFY(renderSpy.count() > renderCount);
+
+    if (lcTests().isDebugEnabled())
+        QTest::qWait(500); // for visual check; not needed in CI
+
+    qCDebug(lcTests) << "TextEdit's nodes" << textPriv->textNodeMap;
+    qCDebug(lcTests) << "font" << textItem->font() << "line positions" << textItem->linePositions << "should be" << linePositions;
+    QCOMPARE(textItem->lastLinePosition, lastLinePosition);
+    QTRY_COMPARE(textItem->linePositions, linePositions);
+}
+
 void tst_qquicktextedit::signal_editingfinished()
 {
     QQuickView *window = new QQuickView(nullptr);