AbstractSettings: Introduce VersionUpdater

Uniform version number parsing inside AbstractSettings.
Get rid of updateVersion() virtual method and add
protected setVersionRegExp() instead.

If the user calls version() we wait for version process
to finish if it was running. This makes sure that we
don't return old / invalid version number while version
process is still running.

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

src/plugins/beautifier/abstractsettings.cpp

@@ -34,11 +34,16 @@
 #include <utils/fileutils.h>
 #include <utils/genericconstants.h>
 #include <utils/mimeutils.h>
+#include <utils/qtcprocess.h>
 
 #include <QFile>
 #include <QFileInfo>
+#include <QRegularExpression>
+#include <QVersionNumber>
 #include <QXmlStreamReader>
 
+using namespace Utils;
+
 namespace Beautifier {
 namespace Internal {
 
@@ -47,12 +52,65 @@ const char COMMAND[]        = "command";
 const char SUPPORTED_MIME[] = "supportedMime";
 }
 
+class VersionUpdater
+{
+public:
+    VersionUpdater()
+    {
+        QObject::connect(&m_process, &QtcProcess::done, [this] {
+            if (m_process.result() != ProcessResult::FinishedWithSuccess)
+                return;
+
+            m_versionNumber = parseVersion(m_process.cleanedStdOut());
+            if (m_versionNumber.isNull())
+                m_versionNumber = parseVersion(m_process.cleanedStdErr());
+        });
+    }
+
+    void setVersionRegExp(const QRegularExpression &versionRegExp)
+    {
+        m_versionRegExp = versionRegExp;
+    }
+
+    void update(const FilePath &executable)
+    {
+        m_versionNumber = {};
+        if (m_versionRegExp.pattern().isEmpty())
+            return;
+        m_process.close();
+        m_process.setCommand({executable, {"--version"}});
+        m_process.start();
+    }
+
+    QVersionNumber version() const
+    {
+        if (m_process.state() != QProcess::NotRunning)
+            m_process.waitForFinished(-1);
+        return m_versionNumber;
+    }
+
+private:
+    QVersionNumber parseVersion(const QString &text) const
+    {
+        const QRegularExpressionMatch match = m_versionRegExp.match(text);
+        if (!match.hasMatch())
+            return {};
+
+        return {match.captured(1).toInt(), match.captured(2).toInt()};
+    }
+
+    QRegularExpression m_versionRegExp;
+    mutable QtcProcess m_process;
+    QVersionNumber m_versionNumber;
+};
+
 AbstractSettings::AbstractSettings(const QString &name, const QString &ending)
     : m_ending(ending)
     , m_styleDir(Core::ICore::userResourcePath(Beautifier::Constants::SETTINGS_DIRNAME)
                      .pathAppended(name)
                      .toString())
     , m_name(name)
+    , m_versionUpdater(new VersionUpdater)
 {
 }
 
@@ -122,29 +180,28 @@ QString AbstractSettings::styleFileName(const QString &key) const
     return m_styleDir.absoluteFilePath(key + m_ending);
 }
 
-Utils::FilePath AbstractSettings::command() const
+FilePath AbstractSettings::command() const
 {
-    return Utils::FilePath::fromString(m_command);
+    return FilePath::fromString(m_command);
 }
 
-void AbstractSettings::setCommand(const QString &command)
+void AbstractSettings::setCommand(const QString &cmd)
 {
-    if (command == m_command)
+    if (cmd == m_command)
         return;
 
-    m_command = command;
+    m_command = cmd;
-    updateVersion();
+    m_versionUpdater->update(command());
 }
 
-int AbstractSettings::version() const
+QVersionNumber AbstractSettings::version() const
 {
-    return m_version;
+    return m_versionUpdater->version();
 }
 
-void AbstractSettings::updateVersion()
+void AbstractSettings::setVersionRegExp(const QRegularExpression &versionRegExp)
 {
-    // If a beautifier needs to know the current tool's version, reimplement and store the version
+    m_versionUpdater->setVersionRegExp(versionRegExp);
-    // in m_version.
 }
 
 QString AbstractSettings::supportedMimeTypesAsString() const
@@ -157,7 +214,7 @@ void AbstractSettings::setSupportedMimeTypes(const QString &mimes)
     const QStringList stringTypes = mimes.split(';');
     QStringList types;
     for (const QString &type : stringTypes) {
-        const Utils::MimeType mime = Utils::mimeTypeForName(type.trimmed());
+        const MimeType mime = mimeTypeForName(type.trimmed());
         if (!mime.isValid())
             continue;
         const QString canonicalName = mime.name();
@@ -179,8 +236,8 @@ bool AbstractSettings::isApplicable(const Core::IDocument *document) const
     if (m_supportedMimeTypes.isEmpty())
         return true;
 
-    const Utils::MimeType documentMimeType = Utils::mimeTypeForName(document->mimeType());
+    const MimeType documentMimeType = mimeTypeForName(document->mimeType());
-    return Utils::anyOf(m_supportedMimeTypes, [&documentMimeType](const QString &mime) {
+    return anyOf(m_supportedMimeTypes, [&documentMimeType](const QString &mime) {
         return documentMimeType.inherits(mime);
     });
 }
@@ -246,7 +303,7 @@ void AbstractSettings::save()
             continue;
         }
 
-        Utils::FileSaver saver(Utils::FilePath::fromUserInput(fi.absoluteFilePath()));
+        FileSaver saver(FilePath::fromUserInput(fi.absoluteFilePath()));
         if (saver.hasError()) {
             BeautifierPlugin::showError(tr("Cannot open file \"%1\": %2.")
                                         .arg(saver.filePath().toUserOutput())

src/plugins/beautifier/abstractsettings.h

@@ -35,12 +35,19 @@
 #include <QStringList>
 #include <QVector>
 
+QT_BEGIN_NAMESPACE
+class QRegularExpression;
+class QVersionNumber;
+QT_END_NAMESPACE
+
 namespace Core { class IDocument; }
 namespace Utils { class FilePath; }
 
 namespace Beautifier {
 namespace Internal {
 
+class VersionUpdater;
+
 class AbstractSettings : public QObject
 {
     Q_OBJECT
@@ -66,9 +73,8 @@ public:
     virtual QString styleFileName(const QString &key) const;
 
     Utils::FilePath command() const;
-    void setCommand(const QString &command);
+    void setCommand(const QString &cmd);
-    int version() const;
+    QVersionNumber version() const;
-    virtual void updateVersion();
 
     QString supportedMimeTypesAsString() const;
     void setSupportedMimeTypes(const QString &mimes);
@@ -81,9 +87,10 @@ signals:
     void supportedMimeTypesChanged();
 
 protected:
+    void setVersionRegExp(const QRegularExpression &versionRegExp);
+
     QMap<QString, QString> m_styles;
     QMap<QString, QVariant> m_settings;
-    int m_version = 0;
     QString m_ending;
     QDir m_styleDir;
 
@@ -92,6 +99,7 @@ protected:
 
 private:
     QString m_name;
+    std::unique_ptr<VersionUpdater> m_versionUpdater;
     QStringList m_stylesToRemove;
     QSet<QString> m_changedStyles;
     QString m_command;

src/plugins/beautifier/artisticstyle/artisticstyle.cpp

@@ -53,6 +53,7 @@
 
 #include <QAction>
 #include <QMenu>
+#include <QVersionNumber>
 
 using namespace TextEditor;
 
@@ -150,10 +151,10 @@ Command ArtisticStyle::command(const QString &cfgFile) const
     command.addOption("-q");
     command.addOption("--options=" + cfgFile);
 
-    const int version = m_settings.version();
+    const QVersionNumber version = m_settings.version();
-    if (version > ArtisticStyleSettings::Version_2_03) {
+    if (version > QVersionNumber(2, 3)) {
         command.setProcessing(Command::PipeProcessing);
-        if (version == ArtisticStyleSettings::Version_2_04)
+        if (version == QVersionNumber(2, 4))
             command.setPipeAddsNewline(true);
         command.setReturnsCRLF(Utils::HostOsInfo::isWindowsHost());
         command.addOption("-z2");

src/plugins/beautifier/artisticstyle/artisticstylesettings.cpp

@@ -57,9 +57,7 @@ const char SETTINGS_NAME[]            = "artisticstyle";
 ArtisticStyleSettings::ArtisticStyleSettings() :
     AbstractSettings(SETTINGS_NAME, ".astyle")
 {
-    connect(&m_versionWatcher, &QFutureWatcherBase::finished,
+    setVersionRegExp(QRegularExpression("([2-9]{1})\\.([0-9]{1,2})(\\.[1-9]{1})?$"));
-            this, &ArtisticStyleSettings::helperSetVersion);
-
     setCommand("astyle");
     m_settings.insert(USE_OTHER_FILES, QVariant(true));
     m_settings.insert(USE_SPECIFIC_CONFIG_FILE, QVariant(false));
@@ -70,48 +68,6 @@ ArtisticStyleSettings::ArtisticStyleSettings() :
     read();
 }
 
-static int parseVersion(const QString &text)
-{
-    // The version in Artistic Style is printed like "Artistic Style Version 2.04"
-    const QRegularExpression rx("([2-9]{1})\\.([0-9]{1,2})(\\.[1-9]{1})?$");
-    const QRegularExpressionMatch match = rx.match(text);
-    if (match.hasMatch()) {
-        const int major = match.captured(1).toInt() * 100;
-        const int minor = match.captured(2).toInt();
-        return major + minor;
-    }
-    return 0;
-}
-
-static int updateVersionHelper(const FilePath &command)
-{
-    QtcProcess process;
-    process.setCommand({command, {"--version"}});
-    process.runBlocking();
-    if (process.result() != ProcessResult::FinishedWithSuccess)
-        return 0;
-
-    // Astyle prints the version on stdout or stderr, depending on platform
-    const int version = parseVersion(process.cleanedStdOut().trimmed());
-    if (version != 0)
-        return version;
-    return parseVersion(process.cleanedStdErr().trimmed());
-}
-
-void ArtisticStyleSettings::updateVersion()
-{
-    if (m_versionFuture.isRunning())
-        m_versionFuture.cancel();
-
-    m_versionFuture = runAsync(updateVersionHelper, command());
-    m_versionWatcher.setFuture(m_versionFuture);
-}
-
-void ArtisticStyleSettings::helperSetVersion()
-{
-    m_version = m_versionWatcher.result();
-}
-
 bool ArtisticStyleSettings::useOtherFiles() const
 {
     return m_settings.value(USE_OTHER_FILES).toBool();
@@ -132,12 +88,12 @@ void ArtisticStyleSettings::setUseSpecificConfigFile(bool useSpecificConfigFile)
     m_settings.insert(USE_SPECIFIC_CONFIG_FILE, QVariant(useSpecificConfigFile));
 }
 
-Utils::FilePath ArtisticStyleSettings::specificConfigFile() const
+FilePath ArtisticStyleSettings::specificConfigFile() const
 {
-    return Utils::FilePath::fromString(m_settings.value(SPECIFIC_CONFIG_FILE).toString());
+    return FilePath::fromString(m_settings.value(SPECIFIC_CONFIG_FILE).toString());
 }
 
-void ArtisticStyleSettings::setSpecificConfigFile(const Utils::FilePath &specificConfigFile)
+void ArtisticStyleSettings::setSpecificConfigFile(const FilePath &specificConfigFile)
 {
     m_settings.insert(SPECIFIC_CONFIG_FILE, QVariant(specificConfigFile.toString()));
 }
@@ -182,7 +138,7 @@ QString ArtisticStyleSettings::documentationFilePath() const
 
 void ArtisticStyleSettings::createDocumentationFile() const
 {
-    Utils::QtcProcess process;
+    QtcProcess process;
     process.setTimeoutS(2);
     process.setCommand({command(), {"-h"}});
     process.runBlocking();

src/plugins/beautifier/artisticstyle/artisticstylesettings.h

@@ -29,9 +29,6 @@
 
 #include <utils/fileutils.h>
 
-#include <QFuture>
-#include <QFutureWatcher>
-
 namespace Beautifier {
 namespace Internal {
 
@@ -40,15 +37,8 @@ class ArtisticStyleSettings : public AbstractSettings
     Q_OBJECT
 
 public:
-    enum ArtisticStyleVersion {
-        Version_2_03 = 203,
-        Version_2_04 = 204
-    };
-
     ArtisticStyleSettings();
 
-    void updateVersion() override;
-
     bool useOtherFiles() const;
     void setUseOtherFiles(bool useOtherFiles);
 
@@ -69,11 +59,6 @@ public:
 
     QString documentationFilePath() const override;
     void createDocumentationFile() const override;
-
-private:
-    void helperSetVersion();
-    QFuture<int> m_versionFuture;
-    QFutureWatcher<int> m_versionWatcher;
 };
 
 } // namespace Internal

src/plugins/beautifier/uncrustify/uncrustify.cpp

@@ -49,6 +49,7 @@
 
 #include <QAction>
 #include <QMenu>
+#include <QVersionNumber>
 
 using namespace TextEditor;
 
@@ -180,7 +181,7 @@ Command Uncrustify::command(const QString &cfgFile, bool fragment) const
     Command command;
     command.setExecutable(m_settings.command().toString());
     command.setProcessing(Command::PipeProcessing);
-    if (m_settings.version() >= 62) {
+    if (m_settings.version() >= QVersionNumber(0, 62)) {
         command.addOption("--assume");
         command.addOption("%file");
     } else {

src/plugins/beautifier/uncrustify/uncrustifysettings.cpp

@@ -30,7 +30,6 @@
 #include "../beautifierconstants.h"
 
 #include <coreplugin/icore.h>
-
 #include <utils/qtcprocess.h>
 
 #include <QDateTime>
@@ -56,6 +55,7 @@ const char SETTINGS_NAME[]                 = "uncrustify";
 UncrustifySettings::UncrustifySettings() :
     AbstractSettings(SETTINGS_NAME, ".cfg")
 {
+    setVersionRegExp(QRegularExpression("([0-9]{1})\\.([0-9]{2})"));
     setCommand("uncrustify");
     m_settings.insert(USE_OTHER_FILES, QVariant(true));
     m_settings.insert(USE_HOME_FILE, QVariant(false));
@@ -210,33 +210,5 @@ void UncrustifySettings::createDocumentationFile() const
     }
 }
 
-static bool parseVersion(const QString &text, int &version)
-{
-    // The version in Uncrustify is printed like "uncrustify 0.62"
-    const QRegularExpression rx("([0-9]{1})\\.([0-9]{2})");
-    const QRegularExpressionMatch match = rx.match(text);
-    if (!match.hasMatch())
-        return false;
-
-    const int major = match.captured(1).toInt() * 100;
-    const int minor = match.captured(2).toInt();
-    version = major + minor;
-    return true;
-}
-
-void UncrustifySettings::updateVersion()
-{
-    m_versionProcess.reset(new QtcProcess);
-    connect(m_versionProcess.get(), &QtcProcess::finished, this, [this] {
-        if (m_versionProcess->exitStatus() == QProcess::NormalExit) {
-            if (!parseVersion(QString::fromUtf8(m_versionProcess->readAllStandardOutput()), m_version))
-                parseVersion(QString::fromUtf8(m_versionProcess->readAllStandardError()), m_version);
-        }
-        m_versionProcess.release()->deleteLater();
-    });
-    m_versionProcess->setCommand({ command(), { "--version" } });
-    m_versionProcess->start();
-}
-
 } // namespace Internal
 } // namespace Beautifier

src/plugins/beautifier/uncrustify/uncrustifysettings.h

@@ -26,9 +26,6 @@
 #pragma once
 
 #include "../abstractsettings.h"
-#include <utils/fileutils.h>
-
-namespace Utils { class QtcProcess; }
 
 namespace Beautifier {
 namespace Internal {
@@ -59,16 +56,11 @@ public:
     QString documentationFilePath() const override;
     void createDocumentationFile() const override;
 
-    void updateVersion() override;
-
     Utils::FilePath specificConfigFile() const;
     void setSpecificConfigFile(const Utils::FilePath &filePath);
 
     bool useSpecificConfigFile() const;
     void setUseSpecificConfigFile(bool useConfigFile);
-
-private:
-    std::unique_ptr<Utils::QtcProcess> m_versionProcess;
 };
 
 } // namespace Internal