Get rid of OutputProxy in ShellCommand

Before we have used the OutputProxyFactory, which
was called in the non-gui thread. The factory, when run,
created the connection between the thread and outside
world (e.g. VersionControl output window). Instead of
setting the factory we provide a set of virtual functions
called directly from non-gui threads. We also provide overrides
for them in VcsCommand class. Their implementation
safely redirects the calls directly to the VcsOutputWindow
through the QMetaObject::invokeMethod() with auto connection
as a default.

Task-number: QTCREATORBUG-25744
Change-Id: I09f2da278003b71095e953a51499a5513cb8f03f
Reviewed-by: Eike Ziller <eike.ziller@qt.io>

src/libs/utils/shellcommand.cpp

@@ -85,7 +85,6 @@ public:
 
     ~ShellCommandPrivate() { delete m_progressParser; }
 
-    std::function<OutputProxy *()> m_proxyFactory = []() { return new OutputProxy; };
     QString m_displayName;
     const QString m_defaultWorkingDirectory;
     const Environment m_environment;
@@ -318,33 +317,30 @@ void ShellCommand::runCommand(SynchronousProcess &proc,
         return;
     }
 
-    QSharedPointer<OutputProxy> proxy(d->m_proxyFactory());
-
     if (!(d->m_flags & SuppressCommandLogging))
-        emit proxy->appendCommand(dir, command);
+        appendCommand(dir, command);
 
     proc.setCommand(command);
     if ((d->m_flags & FullySynchronously)
             || (!(d->m_flags & NoFullySync)
                 && QThread::currentThread() == QCoreApplication::instance()->thread())) {
-        runFullySynchronous(proc, proxy, dir);
+        runFullySynchronous(proc, dir);
     } else {
-        runSynchronous(proc, proxy, dir);
+        runSynchronous(proc, dir);
     }
 
     if (!d->m_aborted) {
         // Success/Fail message in appropriate window?
         if (proc.result() == QtcProcess::FinishedWithSuccess) {
             if (d->m_flags & ShowSuccessMessage)
-                emit proxy->appendMessage(proc.exitMessage());
+                appendMessage(proc.exitMessage());
         } else if (!(d->m_flags & SuppressFailMessage)) {
-            emit proxy->appendError(proc.exitMessage());
+            appendError(proc.exitMessage());
         }
     }
 }
 
 void ShellCommand::runFullySynchronous(SynchronousProcess &process,
-                                       QSharedPointer<OutputProxy> proxy,
                                        const QString &workingDirectory)
 {
     // Set up process
@@ -364,20 +360,19 @@ void ShellCommand::runFullySynchronous(SynchronousProcess &process,
     if (!d->m_aborted) {
         const QString stdErr = process.stdErr();
         if (!stdErr.isEmpty() && !(d->m_flags & SuppressStdErr))
-            emit proxy->append(stdErr);
+            append(stdErr);
 
         const QString stdOut = process.stdOut();
         if (!stdOut.isEmpty() && d->m_flags & ShowStdOut) {
             if (d->m_flags & SilentOutput)
-                emit proxy->appendSilently(stdOut);
+                appendSilently(stdOut);
             else
-                emit proxy->append(stdOut);
+                append(stdOut);
         }
     }
 }
 
 void ShellCommand::runSynchronous(SynchronousProcess &process,
-                                  QSharedPointer<OutputProxy> proxy,
                                   const QString &workingDirectory)
 {
     connect(this, &ShellCommand::terminate, &process, &SynchronousProcess::stopProcess);
@@ -393,11 +388,11 @@ void ShellCommand::runSynchronous(SynchronousProcess &process,
     if (d->m_flags & MergeOutputChannels) {
         process.setProcessChannelMode(QProcess::MergedChannels);
     } else if (d->m_progressiveOutput || !(d->m_flags & SuppressStdErr)) {
-        process.setStdErrCallback([this, proxy](const QString &text) {
+        process.setStdErrCallback([this](const QString &text) {
             if (d->m_progressParser)
                 d->m_progressParser->parseProgress(text);
             if (!(d->m_flags & SuppressStdErr))
-                emit proxy->appendError(text);
+                appendError(text);
             if (d->m_progressiveOutput)
                 emit stdErrText(text);
         });
@@ -405,11 +400,11 @@ void ShellCommand::runSynchronous(SynchronousProcess &process,
 
     // connect stdout to the output window if desired
     if (d->m_progressParser || d->m_progressiveOutput || (d->m_flags & ShowStdOut)) {
-        process.setStdOutCallback([this, proxy](const QString &text) {
+        process.setStdOutCallback([this](const QString &text) {
             if (d->m_progressParser)
                 d->m_progressParser->parseProgress(text);
             if (d->m_flags & ShowStdOut)
-                emit proxy->append(text);
+                append(text);
             if (d->m_progressiveOutput) {
                 emit stdOutText(text);
                 d->m_hadOutput = true;
@@ -463,11 +458,6 @@ void ShellCommand::setProgressiveOutput(bool progressive)
     d->m_progressiveOutput = progressive;
 }
 
-void ShellCommand::setOutputProxyFactory(const std::function<OutputProxy *()> &factory)
-{
-    d->m_proxyFactory = factory;
-}
-
 void ShellCommand::setDisableUnixTerminal()
 {
     d->m_disableUnixTerminal = true;

src/libs/utils/shellcommand.h

@@ -29,8 +29,6 @@
 
 #include "qtcprocess.h"
 
-#include <functional>
-
 QT_BEGIN_NAMESPACE
 class QMutex;
 class QVariant;
@@ -62,22 +60,6 @@ private:
     friend class ShellCommand;
 };
 
-// Users of this class can either be in the GUI thread or in other threads.
-// Use Qt::AutoConnection to always append in the GUI thread (directly or queued)
-class QTCREATOR_UTILS_EXPORT OutputProxy : public QObject
-{
-    Q_OBJECT
-
-    friend class ShellCommand;
-
-signals:
-    void append(const QString &text);
-    void appendSilently(const QString &text);
-    void appendError(const QString &text);
-    void appendCommand(const QString &workingDirectory, const Utils::CommandLine &command);
-    void appendMessage(const QString &text);
-};
-
 class QTCREATOR_UTILS_EXPORT ShellCommand : public QObject
 {
     Q_OBJECT
@@ -136,7 +118,6 @@ public:
     bool hasProgressParser() const;
     void setProgressiveOutput(bool progressive);
 
-    void setOutputProxyFactory(const std::function<OutputProxy *()> &factory);
     void setDisableUnixTerminal();
 
     // This is called once per job in a thread.
@@ -162,16 +143,24 @@ protected:
     int timeoutS() const;
     QString workDirectory(const QString &wd) const;
 
+    // Below methods are called directly from other threads
+    virtual void append(const QString &text) { Q_UNUSED(text) }
+    virtual void appendSilently(const QString &text) { Q_UNUSED(text) }
+    virtual void appendError(const QString &text) { Q_UNUSED(text) }
+    virtual void appendCommand(const QString &workingDirectory, const Utils::CommandLine &command) {
+        Q_UNUSED(workingDirectory)
+        Q_UNUSED(command)
+    }
+    virtual void appendMessage(const QString &text) { Q_UNUSED(text) }
+
 private:
     void run(QFutureInterface<void> &future);
 
     // Run without a event loop in fully blocking mode. No signals will be delivered.
     void runFullySynchronous(SynchronousProcess &proc,
-                             QSharedPointer<OutputProxy> proxy,
                              const QString &workingDirectory);
     // Run with an event loop. Signals will be delivered.
     void runSynchronous(SynchronousProcess &proc,
-                        QSharedPointer<OutputProxy> proxy,
                         const QString &workingDirectory);
 
     class Internal::ShellCommandPrivate *const d;

src/plugins/vcsbase/vcscommand.cpp

@@ -42,23 +42,8 @@ VcsCommand::VcsCommand(const QString &workingDirectory, const Environment &envir
 {
     VcsOutputWindow::setRepository(workingDirectory);
     setDisableUnixTerminal();
-    setOutputProxyFactory([this] {
+    m_outputWindow = VcsOutputWindow::instance();
-        auto proxy = new OutputProxy;
-        VcsOutputWindow *outputWindow = VcsOutputWindow::instance();
 
-        connect(proxy, &OutputProxy::append,
-                outputWindow, [](const QString &txt) { VcsOutputWindow::append(txt); });
-        connect(proxy, &OutputProxy::appendSilently,
-                outputWindow, &VcsOutputWindow::appendSilently);
-        connect(proxy, &OutputProxy::appendError,
-                outputWindow, &VcsOutputWindow::appendError);
-        connect(proxy, &OutputProxy::appendCommand,
-                outputWindow, &VcsOutputWindow::appendCommand);
-        connect(proxy, &OutputProxy::appendMessage,
-                outputWindow, &VcsOutputWindow::appendMessage);
-
-        return proxy;
-    });
     connect(this, &VcsCommand::started, this, [this] {
         if (flags() & ExpectRepoChanges)
             Utils::GlobalFileChangeBlocker::instance()->forceBlocked(true);
@@ -84,6 +69,41 @@ void VcsCommand::runCommand(SynchronousProcess &proc,
     emitRepositoryChanged(workingDirectory);
 }
 
+void VcsCommand::append(const QString &text)
+{
+    QMetaObject::invokeMethod(m_outputWindow, [this, text] {
+        m_outputWindow->append(text);
+    });
+}
+
+void VcsCommand::appendSilently(const QString &text)
+{
+    QMetaObject::invokeMethod(m_outputWindow, [this, text] {
+        m_outputWindow->appendSilently(text);
+    });
+}
+
+void VcsCommand::appendError(const QString &text)
+{
+    QMetaObject::invokeMethod(m_outputWindow, [this, text] {
+        m_outputWindow->appendError(text);
+    });
+}
+
+void VcsCommand::appendCommand(const QString &workingDirectory, const Utils::CommandLine &command)
+{
+    QMetaObject::invokeMethod(m_outputWindow, [this, workingDirectory, command] {
+        m_outputWindow->appendCommand(workingDirectory, command);
+    });
+}
+
+void VcsCommand::appendMessage(const QString &text)
+{
+    QMetaObject::invokeMethod(m_outputWindow, [this, text] {
+        m_outputWindow->appendMessage(text);
+    });
+}
+
 void VcsCommand::emitRepositoryChanged(const QString &workingDirectory)
 {
     if (m_preventRepositoryChanged || !(flags() & VcsCommand::ExpectRepoChanges))

src/plugins/vcsbase/vcscommand.h

@@ -31,6 +31,8 @@
 
 namespace VcsBase {
 
+class VcsOutputWindow;
+
 class VCSBASE_EXPORT VcsCommand : public Core::ShellCommand
 {
     Q_OBJECT
@@ -49,12 +51,20 @@ public:
                     const Utils::CommandLine &command,
                     const QString &workDirectory = {}) override;
 
+protected:
+    void append(const QString &text) override;
+    void appendSilently(const QString &text) override;
+    void appendError(const QString &text) override;
+    void appendCommand(const QString &workingDirectory, const Utils::CommandLine &command) override;
+    void appendMessage(const QString &text) override;
+
 private:
     void emitRepositoryChanged(const QString &workingDirectory);
 
     void coreAboutToClose() override;
 
     bool m_preventRepositoryChanged;
+    VcsOutputWindow *m_outputWindow = nullptr;
 };
 
 } // namespace VcsBase