OutputFormatter: Do all formatting centrally

Instead of working directly on the text edit, the specialized
OutputFormatter classes now simply ask the base class to do it for them.
In practice, the request currently always is "turn this part of the text
into a link", but the interface can be extended to other types of
formatting, should that ever be required.
This is a win/win situation: Derived classes no longer have to fiddle
with QTextCursor & friends (nor do they have to call any base class
functions), while the base class can make strong assumptions about what
the derived class does to the text edit (i.e.: nothing).

Change-Id: Icc4bc52d4001b0359247563e39a206fa274833d7
Reviewed-by: hjk <hjk@qt.io>

src/libs/utils/outputformatter.cpp

@@ -28,12 +28,13 @@
 #include "qtcassert.h"
 #include "synchronousprocess.h"
 #include "theme/theme.h"
-#include "utils/optional.h"
 
 #include <QPair>
 #include <QPlainTextEdit>
 #include <QTextCursor>
 
+#include <numeric>
+
 namespace Utils {
 
 namespace Internal {
@@ -78,27 +79,29 @@ void OutputFormatter::setPlainTextEdit(QPlainTextEdit *plainText)
 
 void OutputFormatter::doAppendMessage(const QString &text, OutputFormat format)
 {
-    if (handleMessage(text, format) == Status::NotHandled)
+    const QTextCharFormat charFmt = charFormat(format);
-        appendMessageDefault(text, format);
+    const QList<FormattedText> formattedText = parseAnsi(text, charFmt);
+    const QString cleanLine = std::accumulate(formattedText.begin(), formattedText.end(), QString(),
+            [](const FormattedText &t1, const FormattedText &t2) { return t1.text + t2.text; });
+    const Result res = handleMessage(cleanLine, format);
+    if (res.newContent) {
+        append(res.newContent.value(), charFmt);
+        return;
+    }
+    for (const FormattedText &output : linkifiedText(formattedText, res.linkSpecs))
+        append(output.text, output.format);
 }
 
-OutputFormatter::Status OutputFormatter::handleMessage(const QString &text, OutputFormat format)
+OutputFormatter::Result OutputFormatter::handleMessage(const QString &text, OutputFormat format)
 {
     Q_UNUSED(text);
     Q_UNUSED(format);
     return Status::NotHandled;
 }
 
-void OutputFormatter::doAppendMessage(const QString &text, const QTextCharFormat &format)
-{
-    const QList<FormattedText> formattedTextList = parseAnsi(text, format);
-    for (const FormattedText &output : formattedTextList)
-        append(output.text, output.format);
-}
-
 QTextCharFormat OutputFormatter::charFormat(OutputFormat format) const
 {
-    return d->formats[format];
+    return d->formatOverride ? d->formatOverride.value() : d->formats[format];
 }
 
 QList<FormattedText> OutputFormatter::parseAnsi(const QString &text, const QTextCharFormat &format)
@@ -106,6 +109,59 @@ QList<FormattedText> OutputFormatter::parseAnsi(const QString &text, const QText
     return d->escapeCodeHandler.parseText(FormattedText(text, format));
 }
 
+const QList<FormattedText> OutputFormatter::linkifiedText(
+        const QList<FormattedText> &text, const OutputFormatter::LinkSpecs &linkSpecs)
+{
+    if (linkSpecs.isEmpty())
+        return text;
+
+    QList<FormattedText> linkified;
+    int totalTextLengthSoFar = 0;
+    int nextLinkSpecIndex = 0;
+
+    for (const FormattedText &t : text) {
+
+        // There is no more linkification work to be done. Just copy the text as-is.
+        if (nextLinkSpecIndex >= linkSpecs.size()) {
+            linkified << t;
+            continue;
+        }
+
+        for (int nextLocalTextPos = 0; nextLocalTextPos < t.text.size(); ) {
+
+            // There are no more links in this part, so copy the rest of the text as-is.
+            if (nextLinkSpecIndex >= linkSpecs.size()) {
+                linkified << FormattedText(t.text.mid(nextLocalTextPos), t.format);
+                totalTextLengthSoFar += t.text.length() - nextLocalTextPos;
+                break;
+            }
+
+            const LinkSpec &linkSpec = linkSpecs.at(nextLinkSpecIndex);
+            const int localLinkStartPos = linkSpec.startPos - totalTextLengthSoFar;
+            ++nextLinkSpecIndex;
+
+            // We ignore links that would cross format boundaries.
+            if (localLinkStartPos < nextLocalTextPos
+                    || localLinkStartPos + linkSpec.length > t.text.length()) {
+                linkified << FormattedText(t.text.mid(nextLocalTextPos), t.format);
+                totalTextLengthSoFar += t.text.length() - nextLocalTextPos;
+                break;
+            }
+
+            // Now we know we have a link that is fully inside this part of the text.
+            // Split the text so that the link part gets the appropriate format.
+            const int prefixLength = localLinkStartPos - nextLocalTextPos;
+            const QString textBeforeLink = t.text.mid(nextLocalTextPos, prefixLength);
+            linkified << FormattedText(textBeforeLink, t.format);
+            const QString linkedText = t.text.mid(localLinkStartPos, linkSpec.length);
+            linkified << FormattedText(linkedText, linkFormat(t.format, linkSpec.target));
+            nextLocalTextPos = localLinkStartPos + linkSpec.length;
+            totalTextLengthSoFar += prefixLength + linkSpec.length;
+        }
+    }
+    return linkified;
+}
+
 void OutputFormatter::append(const QString &text, const QTextCharFormat &format)
 {
     int startPos = 0;
@@ -120,11 +176,6 @@ void OutputFormatter::append(const QString &text, const QTextCharFormat &format)
         d->cursor.insertText(text.mid(startPos), format);
 }
 
-QTextCursor &OutputFormatter::cursor() const
-{
-    return d->cursor;
-}
-
 QTextCharFormat OutputFormatter::linkFormat(const QTextCharFormat &inputFormat, const QString &href)
 {
     QTextCharFormat result = inputFormat;
@@ -140,11 +191,6 @@ void OutputFormatter::overrideTextCharFormat(const QTextCharFormat &fmt)
     d->formatOverride = fmt;
 }
 
-void OutputFormatter::appendMessageDefault(const QString &text, OutputFormat format)
-{
-    doAppendMessage(text, d->formatOverride ? d->formatOverride.value() : d->formats[format]);
-}
-
 void OutputFormatter::clearLastLine()
 {
     // Note that this approach will fail if the text edit is not read-only and users
@@ -282,30 +328,32 @@ void AggregatingOutputFormatter::setFormatters(const QList<OutputFormatter *> &f
     d->nextFormatter = nullptr;
 }
 
-OutputFormatter::Status AggregatingOutputFormatter::handleMessage(const QString &text,
+OutputFormatter::Result AggregatingOutputFormatter::handleMessage(const QString &text,
                                                                   OutputFormat format)
 {
     if (d->nextFormatter) {
-        switch (d->nextFormatter->handleMessage(text, format)) {
+        const Result res = d->nextFormatter->handleMessage(text, format);
+        switch (res.status) {
         case Status::Done:
             d->nextFormatter = nullptr;
-            return Status::Done;
+            return res;
         case Status::InProgress:
-            return Status::InProgress;
+            return res;
         case Status::NotHandled:
-            QTC_CHECK(false);
+            QTC_CHECK(false); // TODO: This case will be legal after the merge
             d->nextFormatter = nullptr;
-            return Status::NotHandled;
+            return res;
         }
     }
     QTC_CHECK(!d->nextFormatter);
     for (OutputFormatter * const formatter : qAsConst(d->formatters)) {
-        switch (formatter->handleMessage(text, format)) {
+        const Result res = formatter->handleMessage(text, format);
+        switch (res.status) {
         case Status::Done:
-            return Status::Done;
+            return res;
         case Status::InProgress:
             d->nextFormatter = formatter;
-            return Status::InProgress;
+            return res;
         case Status::NotHandled:
             break;
         }

src/libs/utils/outputformatter.h

@@ -26,6 +26,7 @@
 #pragma once
 
 #include "utils_global.h"
+#include "optional.h"
 #include "outputformat.h"
 
 #include <QObject>
@@ -61,33 +62,49 @@ public:
     virtual bool handleLink(const QString &href);
     void clear();
     void setBoldFontEnabled(bool enabled);
-    static QTextCharFormat linkFormat(const QTextCharFormat &inputFormat, const QString &href);
 
     // For unit testing only
     void overrideTextCharFormat(const QTextCharFormat &fmt);
 
 protected:
-    enum class Status { Done, InProgress, NotHandled };
-
-    void appendMessageDefault(const QString &text, OutputFormat format);
-    void clearLastLine();
     QTextCharFormat charFormat(OutputFormat format) const;
-    QList<FormattedText> parseAnsi(const QString &text, const QTextCharFormat &format);
+    static QTextCharFormat linkFormat(const QTextCharFormat &inputFormat, const QString &href);
-    QTextCursor &cursor() const;
+
+    enum class Status { Done, InProgress, NotHandled };
+    class LinkSpec {
+    public:
+        LinkSpec() = default;
+        LinkSpec(int sp, int l, const QString &t) : startPos(sp), length(l), target(t) {}
+        int startPos = -1;
+        int length = -1;
+        QString target;
+    };
+    using LinkSpecs = QList<LinkSpec>;
+    class Result {
+    public:
+        Result(Status s, const LinkSpecs &l = {}, const optional<QString> &c = {})
+            : status(s), linkSpecs(l), newContent(c) {}
+        Status status;
+        LinkSpecs linkSpecs;
+        optional<QString> newContent; // Hard content override. Only to be used in extreme cases.
+    };
 
 private:
     // text contains at most one line feed character, and if it does occur, it's the last character.
     // Either way, the input is to be considered "complete" for formatting purposes.
     void doAppendMessage(const QString &text, OutputFormat format);
 
-    virtual Status handleMessage(const QString &text, OutputFormat format);
+    virtual Result handleMessage(const QString &text, OutputFormat format);
     virtual void reset() {}
 
-    void doAppendMessage(const QString &text, const QTextCharFormat &format);
     void append(const QString &text, const QTextCharFormat &format);
     void initFormats();
     void flushIncompleteLine();
     void dumpIncompleteLine(const QString &line, OutputFormat format);
+    void clearLastLine();
+    QList<FormattedText> parseAnsi(const QString &text, const QTextCharFormat &format);
+    const QList<Utils::FormattedText> linkifiedText(const QList<FormattedText> &text,
+                                                    const LinkSpecs &linkSpecs);
 
     Internal::OutputFormatterPrivate *d;
 };
@@ -102,7 +119,7 @@ public:
     bool handleLink(const QString &href) override;
 
 private:
-    Status handleMessage(const QString &text, OutputFormat format) override;
+    Result handleMessage(const QString &text, OutputFormat format) override;
 
     class Private;
     Private * const d;

src/plugins/coreplugin/outputwindow.cpp

@@ -506,20 +506,19 @@ void OutputWindow::setWordWrapEnabled(bool wrap)
 class TestFormatterA : public OutputFormatter
 {
 private:
-    Status handleMessage(const QString &text, OutputFormat format) override
+    Result handleMessage(const QString &text, OutputFormat) override
     {
+        static const QString replacement = "handled by A\n";
         if (m_handling) {
-            appendMessageDefault("handled by A\n", format);
             if (text.startsWith("A")) {
                 m_handling = false;
-                return Status::Done;
+                return {Status::Done, {}, replacement};
             }
-            return Status::InProgress;
+            return {Status::InProgress, {}, replacement};
         }
         if (text.startsWith("A")) {
             m_handling = true;
-            appendMessageDefault("handled by A\n", format);
+            return {Status::InProgress, {}, replacement};
-            return Status::InProgress;
         }
         return Status::NotHandled;
     }
@@ -533,12 +532,10 @@ private:
 class TestFormatterB : public OutputFormatter
 {
 private:
-    Status handleMessage(const QString &text, OutputFormat format) override
+    Result handleMessage(const QString &text, OutputFormat) override
     {
-        if (text.startsWith("B")) {
+        if (text.startsWith("B"))
-            appendMessageDefault("handled by B\n", format);
+            return {Status::Done, {}, QString("handled by B\n")};
-            return Status::Done;
-        }
         return Status::NotHandled;
     }
 };

src/plugins/python/pythonrunconfiguration.cpp

@@ -71,30 +71,24 @@ public:
     }
 
 private:
-    Status handleMessage(const QString &text, OutputFormat format) final
+    Result handleMessage(const QString &text, OutputFormat format) final
     {
         if (!m_inTraceBack) {
             m_inTraceBack = format == StdErrFormat
                     && text.startsWith("Traceback (most recent call last):");
-            if (m_inTraceBack) {
+            if (m_inTraceBack)
-                OutputFormatter::appendMessageDefault(text, format);
                 return Status::InProgress;
-            }
             return Status::NotHandled;
         }
 
         const Core::Id category(PythonErrorTaskCategory);
         const QRegularExpressionMatch match = filePattern.match(text);
         if (match.hasMatch()) {
-            QTextCursor tc = plainTextEdit()->textCursor();
+            const LinkSpec link(match.capturedStart(2), match.capturedLength(2), match.captured(2));
-            tc.movePosition(QTextCursor::End, QTextCursor::MoveAnchor);
-            tc.insertText(match.captured(1));
-            tc.insertText(match.captured(2), linkFormat(charFormat(format), match.captured(2)));
-
             const auto fileName = FilePath::fromString(match.captured(3));
             const int lineNumber = match.capturedRef(4).toInt();
             m_tasks.append({Task::Warning, QString(), fileName, lineNumber, category});
-            return Status::InProgress;
+            return {Status::InProgress, {link}};
         }
 
         Status status = Status::InProgress;
@@ -118,7 +112,6 @@ private:
             m_inTraceBack = false;
             status = Status::Done;
         }
-        OutputFormatter::appendMessageDefault(text, format);
         return status;
     }
 

src/plugins/qtsupport/qtoutputformatter.cpp

@@ -53,13 +53,6 @@ using namespace Utils;
 namespace QtSupport {
 namespace Internal {
 
-struct LinkResult
-{
-    int start = -1;
-    int end = -1;
-    QString href;
-};
-
 class QtOutputFormatterPrivate
 {
 public:
@@ -84,7 +77,6 @@ public:
     const QRegularExpression qtTestFailWin;
     QPointer<Project> project;
     FileInProjectFinder projectFinder;
-    QTextCursor cursor;
 };
 
 class QtOutputFormatter : public OutputFormatter
@@ -97,13 +89,11 @@ protected:
     virtual void openEditor(const QString &fileName, int line, int column = -1);
 
 private:
-    Status handleMessage(const QString &text, Utils::OutputFormat format) override;
+    Result handleMessage(const QString &text, Utils::OutputFormat format) override;
     bool handleLink(const QString &href) override;
 
     void updateProjectFileList();
-    LinkResult matchLine(const QString &line) const;
+    LinkSpec matchLine(const QString &line) const;
-    void appendLine(const LinkResult &lr, const QString &line, const QTextCharFormat &format);
-    Status doAppendMessage(const QString &txt, const QTextCharFormat &format);
 
     QtOutputFormatterPrivate *d;
     friend class QtSupportPlugin; // for testing
@@ -130,18 +120,18 @@ QtOutputFormatter::~QtOutputFormatter()
     delete d;
 }
 
-LinkResult QtOutputFormatter::matchLine(const QString &line) const
+OutputFormatter::LinkSpec QtOutputFormatter::matchLine(const QString &line) const
 {
-    LinkResult lr;
+    LinkSpec lr;
 
     auto hasMatch = [&lr, line](const QRegularExpression &regex) {
         const QRegularExpressionMatch match = regex.match(line);
         if (!match.hasMatch())
             return false;
 
-        lr.href = match.captured(1);
+        lr.target = match.captured(1);
-        lr.start = match.capturedStart(1);
+        lr.startPos = match.capturedStart(1);
-        lr.end = lr.start + lr.href.length();
+        lr.length = lr.target.length();
         return true;
     };
 
@@ -161,47 +151,13 @@ LinkResult QtOutputFormatter::matchLine(const QString &line) const
     return lr;
 }
 
-OutputFormatter::Status QtOutputFormatter::doAppendMessage(const QString &txt,
+OutputFormatter::Result QtOutputFormatter::handleMessage(const QString &txt, OutputFormat format)
-                                                           const QTextCharFormat &format)
 {
-    // FIXME: We'll do the ANSI parsing twice if there is no match.
+    Q_UNUSED(format);
-    //        Ideally, we'd (optionally) pre-process ANSI escape codes in the
+    const LinkSpec lr = matchLine(txt);
-    //        base class before passing the text here, but then we can no longer
+    if (!lr.target.isEmpty())
-    //        pass complete lines...
+        return Result(Status::Done, {lr});
-    const QList<FormattedText> ansiTextList = parseAnsi(txt, format);
+    return Status::NotHandled;
-    QList<std::tuple<QString, QTextCharFormat, LinkResult>> parts;
-    bool hasMatches = false;
-    for (const FormattedText &output : ansiTextList) {
-        const LinkResult lr = matchLine(output.text);
-        if (!lr.href.isEmpty())
-            hasMatches = true;
-        parts << std::make_tuple(output.text, output.format, lr);
-    }
-    if (!hasMatches)
-        return Status::NotHandled;
-    for (const auto &part : parts) {
-        const LinkResult &lr = std::get<2>(part);
-        const QString &text = std::get<0>(part);
-        const QTextCharFormat &fmt = std::get<1>(part);
-        if (!lr.href.isEmpty())
-            appendLine(lr, text, fmt);
-        else
-            cursor().insertText(text, fmt);
-    }
-    return Status::Done;
-}
-
-QtOutputFormatter::Status QtOutputFormatter::handleMessage(const QString &txt, OutputFormat format)
-{
-    return doAppendMessage(txt, charFormat(format));
-}
-
-void QtOutputFormatter::appendLine(const LinkResult &lr, const QString &line,
-                                   const QTextCharFormat &format)
-{
-    cursor().insertText(line.left(lr.start), format);
-    cursor().insertText(line.mid(lr.start, lr.end - lr.start), linkFormat(format, lr.href));
-    cursor().insertText(line.mid(lr.end), format);
 }
 
 bool QtOutputFormatter::handleLink(const QString &href)
@@ -347,7 +303,7 @@ void QtSupportPlugin::testQtOutputFormatter_data()
 
     QTest::newRow("pass through")
             << "Pass through plain text."
-            << -1 << -1 << QString()
+            << -1 << -2 << QString()
             << QString() << -1 << -1;
 
     QTest::newRow("qrc:/main.qml:20")
@@ -455,12 +411,12 @@ void QtSupportPlugin::testQtOutputFormatter()
 
     TestQtOutputFormatter formatter;
 
-    LinkResult result = formatter.matchLine(input);
+    QtOutputFormatter::LinkSpec result = formatter.matchLine(input);
-    formatter.handleLink(result.href);
+    formatter.handleLink(result.target);
 
-    QCOMPARE(result.start, linkStart);
+    QCOMPARE(result.startPos, linkStart);
-    QCOMPARE(result.end, linkEnd);
+    QCOMPARE(result.startPos + result.length, linkEnd);
-    QCOMPARE(result.href, href);
+    QCOMPARE(result.target, href);
 
     QCOMPARE(formatter.fileName, file);
     QCOMPARE(formatter.line, line);
@@ -497,7 +453,7 @@ void QtSupportPlugin::testQtOutputFormatter_appendMessage_data()
             << "Object::Test in test.cpp:123"
             << "Object::Test in test.cpp:123"
             << QTextCharFormat()
-            << OutputFormatter::linkFormat(QTextCharFormat(), "test.cpp:123");
+            << QtOutputFormatter::linkFormat(QTextCharFormat(), "test.cpp:123");
     QTest::newRow("colored")
             << "blue da ba dee"
             << "blue da ba dee"
@@ -547,7 +503,7 @@ void QtSupportPlugin::testQtOutputFormatter_appendMixedAssertAndAnsi()
                 "file://test.cpp:123 "
                 "Blue\n";
 
-    formatter.doAppendMessage(inputText, QTextCharFormat());
+    formatter.appendMessage(inputText, DebugFormat);
 
     QCOMPARE(edit.toPlainText(), outputText);
 
@@ -556,7 +512,7 @@ void QtSupportPlugin::testQtOutputFormatter_appendMixedAssertAndAnsi()
 
     edit.moveCursor(QTextCursor::WordRight);
     edit.moveCursor(QTextCursor::Right);
-    QCOMPARE(edit.currentCharFormat(), OutputFormatter::linkFormat(QTextCharFormat(), "file://test.cpp:123"));
+    QCOMPARE(edit.currentCharFormat(), QtOutputFormatter::linkFormat(QTextCharFormat(), "file://test.cpp:123"));
 
     edit.moveCursor(QTextCursor::End);
     QCOMPARE(edit.currentCharFormat(), blueFormat());

src/plugins/vcsbase/vcsoutputformatter.cpp

@@ -43,30 +43,23 @@ VcsOutputFormatter::VcsOutputFormatter() :
 {
 }
 
-VcsOutputFormatter::Status VcsOutputFormatter::handleMessage(const QString &text,
+Utils::OutputFormatter::Result VcsOutputFormatter::handleMessage(const QString &text,
-                                                             Utils::OutputFormat format)
+                                                                 Utils::OutputFormat format)
 {
+    Q_UNUSED(format);
     QRegularExpressionMatchIterator it = m_regexp.globalMatch(text);
     if (!it.hasNext())
         return Status::NotHandled;
-    int begin = 0;
+    LinkSpecs linkSpecs;
     while (it.hasNext()) {
         const QRegularExpressionMatch match = it.next();
-        const QTextCharFormat normalFormat = charFormat(format);
+        const int startPos = match.capturedStart();
-        appendMessageDefault(text.mid(begin, match.capturedStart() - begin), format);
-        QTextCursor tc = plainTextEdit()->textCursor();
         QStringView url = match.capturedView();
-        begin = match.capturedEnd();
+        while (url.rbegin()->isPunct())
-        while (url.rbegin()->isPunct()) {
             url.chop(1);
-            --begin;
+        linkSpecs << LinkSpec(startPos, url.length(), url.toString());
-        }
-        tc.movePosition(QTextCursor::End);
-        tc.insertText(url.toString(), linkFormat(normalFormat, url.toString()));
-        tc.movePosition(QTextCursor::End);
     }
-    appendMessageDefault(text.mid(begin), format);
+    return {Status::Done, linkSpecs};
-    return Status::Done;
 }
 
 bool VcsOutputFormatter::handleLink(const QString &href)

src/plugins/vcsbase/vcsoutputformatter.h

@@ -44,7 +44,7 @@ signals:
     void referenceClicked(const QString &reference);
 
 private:
-    Status handleMessage(const QString &text, Utils::OutputFormat format) override;
+    Result handleMessage(const QString &text, Utils::OutputFormat format) override;
 
     const QRegularExpression m_regexp;
 };