Cmake: Fix compile output glitch

Using a dedicated OutputLineParser ensures that we only ever see
complete lines and thus prevents "partially red" lines in the compile
output pane.

Fixes: QTCREATORBUG-24209
Change-Id: I12b3de70b81789afe727b66e366facdcc81f8ab8
Reviewed-by: Alessandro Portale <alessandro.portale@qt.io>
Reviewed-by: Cristian Adam <cristian.adam@qt.io>

3 files changed, 74 insertions, 59 deletions

diff --git a/src/libs/utils/outputformatter.cpp b/src/libs/utils/outputformatter.cpp
index 355fcb24ab..5650e1fa88 100644
--- a/src/libs/utils/outputformatter.cpp
+++ b/src/libs/utils/outputformatter.cpp
@@ -282,12 +282,24 @@ void OutputFormatter::overridePostPrintAction(const PostPrintAction &postPrintAc
 
 void OutputFormatter::doAppendMessage(const QString &text, OutputFormat format)
 {
-    const QTextCharFormat charFmt = charFormat(format);
-    const QList<FormattedText> formattedText = parseAnsi(text, charFmt);
+    QTextCharFormat charFmt = charFormat(format);
+    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; });
     QList<OutputLineParser *> involvedParsers;
     const OutputLineParser::Result res = handleMessage(cleanLine, format, involvedParsers);
+
+    // If the line was recognized by a parser and a redirection was detected for that parser,
+    // then our formatting should reflect that redirection as well, i.e. print in red
+    // even if the nominal format is stdout.
+    if (!involvedParsers.isEmpty()) {
+        const OutputFormat formatForParser = outputTypeForParser(involvedParsers.last(), format);
+        if (formatForParser != format && cleanLine == text && formattedText.length() == 1) {
+            charFmt = charFormat(formatForParser);
+            formattedText.first().format = charFmt;
+        }
+    }
+
     if (res.newContent) {
         append(res.newContent.value(), charFmt);
         return;
diff --git a/src/plugins/cmakeprojectmanager/cmakebuildstep.cpp b/src/plugins/cmakeprojectmanager/cmakebuildstep.cpp
index 0f99ad4fce..5f0aed9011 100644
--- a/src/plugins/cmakeprojectmanager/cmakebuildstep.cpp
+++ b/src/plugins/cmakeprojectmanager/cmakebuildstep.cpp
@@ -47,6 +47,7 @@
 #include <QFormLayout>
 #include <QLineEdit>
 #include <QListWidget>
+#include <QRegularExpression>
 
 using namespace ProjectExplorer;
 
@@ -59,6 +60,53 @@ const char TOOL_ARGUMENTS_KEY[] = "CMakeProjectManager.MakeStep.AdditionalArgume
 const char ADD_RUNCONFIGURATION_ARGUMENT_KEY[] = "CMakeProjectManager.MakeStep.AddRunConfigurationArgument";
 const char ADD_RUNCONFIGURATION_TEXT[] = "Current executable";
 
+class CmakeProgressParser : public Utils::OutputLineParser
+{
+    Q_OBJECT
+
+signals:
+    void progress(int percentage);
+
+private:
+    Result handleLine(const QString &line, Utils::OutputFormat format) override
+    {
+        if (format != Utils::StdOutFormat)
+            return Status::NotHandled;
+
+        static const QRegularExpression percentProgress("^\\[\\s*(\\d*)%\\]");
+        static const QRegularExpression ninjaProgress("^\\[\\s*(\\d*)/\\s*(\\d*)");
+
+        QRegularExpressionMatch match = percentProgress.match(line);
+        if (match.hasMatch()) {
+            bool ok = false;
+            const int percent = match.captured(1).toInt(&ok);
+            if (ok)
+                emit progress(percent);
+            return Status::Done;
+        }
+        match = ninjaProgress.match(line);
+        if (match.hasMatch()) {
+            m_useNinja = true;
+            bool ok = false;
+            const int done = match.captured(1).toInt(&ok);
+            if (ok) {
+                const int all = match.captured(2).toInt(&ok);
+                if (ok && all != 0) {
+                    const int percent = static_cast<int>(100.0 * done / all);
+                    emit progress(percent);
+                }
+            }
+            return Status::Done;
+        }
+        return Status::NotHandled;
+    }
+    bool hasDetectedRedirection() const override { return m_useNinja; }
+
+    // TODO: Shouldn't we know the backend in advance? Then we could merge this class
+    //       with CmakeParser.
+    bool m_useNinja = false;
+};
+
 class CMakeBuildStepConfigWidget : public BuildStepConfigWidget
 {
     Q_DECLARE_TR_FUNCTIONS(CMakeProjectManager::Internal::CMakeBuildStepConfigWidget)
@@ -88,8 +136,6 @@ static bool isCurrentExecutableTarget(const QString &target)
 CMakeBuildStep::CMakeBuildStep(BuildStepList *bsl, Utils::Id id) :
     AbstractProcessStep(bsl, id)
 {
-    m_percentProgress = QRegExp("^\\[\\s*(\\d*)%\\]");
-    m_ninjaProgress = QRegExp("^\\[\\s*(\\d*)/\\s*(\\d*)");
     m_ninjaProgressString = "[%f/%t "; // ninja: [33/100
     //: Default display name for the cmake make step.
     setDefaultDisplayName(tr("CMake Build"));
@@ -211,9 +257,18 @@ bool CMakeBuildStep::init()
 void CMakeBuildStep::setupOutputFormatter(Utils::OutputFormatter *formatter)
 {
     CMakeParser *cmakeParser = new CMakeParser;
+    CmakeProgressParser * const progressParser = new CmakeProgressParser;
+    connect(progressParser, &CmakeProgressParser::progress, this, [this](int percent) {
+        emit progress(percent, {});
+    });
+    formatter->addLineParser(progressParser);
     cmakeParser->setSourceDirectory(project()->projectDirectory().toString());
     formatter->addLineParsers({cmakeParser, new GnuMakeParser});
-    formatter->addLineParsers(target()->kit()->createOutputParsers());
+    const QList<Utils::OutputLineParser *> additionalParsers
+            = target()->kit()->createOutputParsers();
+    for (Utils::OutputLineParser * const p : additionalParsers)
+        p->setRedirectionDetector(progressParser);
+    formatter->addLineParsers(additionalParsers);
     formatter->addSearchDir(processParameters()->effectiveWorkingDirectory());
     AbstractProcessStep::setupOutputFormatter(formatter);
 }
@@ -276,47 +331,6 @@ QString CMakeBuildStep::defaultBuildTarget() const
     return allTarget();
 }
 
-void CMakeBuildStep::stdOutput(const QString &output)
-{
-    int offset = 0;
-    while (offset != -1) {
-        const int newlinePos = output.indexOf('\n', offset);
-        QString line;
-        if (newlinePos == -1) {
-            line = output.mid(offset);
-            offset = -1;
-        } else {
-            line = output.mid(offset, newlinePos - offset + 1);
-            offset = newlinePos + 1;
-        }
-        if (m_percentProgress.indexIn(line) != -1) {
-            AbstractProcessStep::stdOutput(line);
-            bool ok = false;
-            int percent = m_percentProgress.cap(1).toInt(&ok);
-            if (ok)
-                emit progress(percent, QString());
-            continue;
-        } else if (m_ninjaProgress.indexIn(line) != -1) {
-            AbstractProcessStep::stdOutput(line);
-            m_useNinja = true;
-            bool ok = false;
-            int done = m_ninjaProgress.cap(1).toInt(&ok);
-            if (ok) {
-                int all = m_ninjaProgress.cap(2).toInt(&ok);
-                if (ok && all != 0) {
-                    const int percent = static_cast<int>(100.0 * done/all);
-                    emit progress(percent, QString());
-                }
-            }
-            continue;
-        }
-        if (m_useNinja)
-            AbstractProcessStep::stdError(line);
-        else
-            AbstractProcessStep::stdOutput(line);
-    }
-}
-
 QStringList CMakeBuildStep::buildTargets() const
 {
     return m_buildTargets;
@@ -573,12 +587,6 @@ CMakeBuildStepFactory::CMakeBuildStepFactory()
     setSupportedProjectType(Constants::CMAKE_PROJECT_ID);
 }
 
-void CMakeBuildStep::processStarted()
-{
-    m_useNinja = false;
-    AbstractProcessStep::processStarted();
-}
-
 void CMakeBuildStep::processFinished(int exitCode, QProcess::ExitStatus status)
 {
     AbstractProcessStep::processFinished(exitCode, status);
@@ -587,3 +595,5 @@ void CMakeBuildStep::processFinished(int exitCode, QProcess::ExitStatus status)
 
 } // Internal
 } // CMakeProjectManager
+
+#include <cmakebuildstep.moc>
diff --git a/src/plugins/cmakeprojectmanager/cmakebuildstep.h b/src/plugins/cmakeprojectmanager/cmakebuildstep.h
index ffab03c180..e3e3e0ec19 100644
--- a/src/plugins/cmakeprojectmanager/cmakebuildstep.h
+++ b/src/plugins/cmakeprojectmanager/cmakebuildstep.h
@@ -73,14 +73,10 @@ signals:
     void buildTargetsChanged();
 
 protected:
-    void processStarted() override;
     void processFinished(int exitCode, QProcess::ExitStatus status) override;
 
     bool fromMap(const QVariantMap &map) override;
 
-    // For parsing [ 76%]
-    void stdOutput(const QString &output) override;
-
 private:
     void ctor(ProjectExplorer::BuildStepList *bsl);
 
@@ -98,13 +94,10 @@ private:
 
     QMetaObject::Connection m_runTrigger;
 
-    QRegExp m_percentProgress;
-    QRegExp m_ninjaProgress;
     QString m_ninjaProgressString;
     QStringList m_buildTargets;
     QString m_cmakeArguments;
     QString m_toolArguments;
-    bool m_useNinja = false;
     bool m_waiting = false;
 };