Perforce: Merge Settings and PerforceSettings classes

Structurally more similar to what the other plugins do.

Change-Id: Ic664277b089ebf60241fc8627353721e0e2f3002
Reviewed-by: Orgad Shaneh <orgads@gmail.com>

src/plugins/perforce/perforceplugin.cpp

@@ -395,7 +395,7 @@ PerforcePluginPrivate::PerforcePluginPrivate()
 
     dd = this;
 
-    m_settings.settings().readSettings(ICore::settings());
+    m_settings.readSettings(ICore::settings());
 
     const QString prefix = QLatin1String("p4");
     m_commandLocator = new CommandLocator("Perforce", prefix, prefix, this);
@@ -572,7 +572,7 @@ PerforcePluginPrivate::PerforcePluginPrivate()
     connect(m_filelogAction, &QAction::triggered, this, &PerforcePluginPrivate::filelogFile);
     perforceContainer->addAction(command);
 
-    QObject::connect(&m_settings.settings(), &AspectContainer::applied, [this] {
+    QObject::connect(&m_settings, &AspectContainer::applied, [this] {
         m_settings.clearTopLevel();
         applySettings();
     });
@@ -913,8 +913,8 @@ void PerforcePluginPrivate::filelog(const QString &workingDir, const QString &fi
     QTextCodec *codec = VcsBaseEditor::getCodec(workingDir, QStringList(fileName));
     QStringList args;
     args << QLatin1String("filelog") << QLatin1String("-li");
-    if (m_settings.logCount() > 0)
+    if (m_settings.logCount.value() > 0)
-        args << QLatin1String("-m") << QString::number(m_settings.logCount());
+        args << "-m" << QString::number(m_settings.logCount.value());
     if (!fileName.isEmpty())
         args.append(fileName);
     const PerforceResponse result = runP4Cmd(workingDir, args,
@@ -935,8 +935,8 @@ void PerforcePluginPrivate::changelists(const QString &workingDir, const QString
     QTextCodec *codec = VcsBaseEditor::getCodec(workingDir, QStringList(fileName));
     QStringList args;
     args << QLatin1String("changelists") << QLatin1String("-lit");
-    if (m_settings.logCount() > 0)
+    if (m_settings.logCount.value() > 0)
-        args << QLatin1String("-m") << QString::number(m_settings.logCount());
+        args << "-m" << QString::number(m_settings.logCount.value());
     if (!fileName.isEmpty())
         args.append(fileName);
     const PerforceResponse result = runP4Cmd(workingDir, args,
@@ -1132,7 +1132,7 @@ bool PerforcePluginPrivate::isVcsFileOrDirectory(const FilePath &fileName) const
 
 bool PerforcePluginPrivate::isConfigured() const
 {
-    const QString binary = m_settings.p4BinaryPath();
+    const QString binary = m_settings.p4BinaryPath.value();
     if (binary.isEmpty())
         return false;
     QFileInfo fi(binary);
@@ -1171,8 +1171,8 @@ bool PerforcePluginPrivate::vcsOpen(const QString &fileName)
 IVersionControl::SettingsFlags PerforcePluginPrivate::settingsFlags() const
 {
     SettingsFlags rc;
-    if (m_settings.autoOpen())
+    if (m_settings.autoOpen.value())
-        rc|= AutoOpen;
+        rc |= AutoOpen;
     return rc;
 }
 
@@ -1250,7 +1250,7 @@ PerforceResponse PerforcePluginPrivate::synchronousProcess(const QString &workin
     VcsOutputWindow *outputWindow = VcsOutputWindow::instance();
     // Run, connect stderr to the output window
     SynchronousProcess process;
-    const int timeOutS = (flags & LongTimeOut) ? m_settings.longTimeOutS() : m_settings.timeOutS();
+    const int timeOutS = (flags & LongTimeOut) ? m_settings.longTimeOutS() : m_settings.timeOutS.value();
     process.setTimeoutS(timeOutS);
     if (outputCodec)
         process.setCodec(outputCodec);
@@ -1282,7 +1282,7 @@ PerforceResponse PerforcePluginPrivate::synchronousProcess(const QString &workin
         }
     }
     process.setTimeOutMessageBoxEnabled(true);
-    const SynchronousProcessResponse sp_resp = process.run({m_settings.p4BinaryPath(), args});
+    const SynchronousProcessResponse sp_resp = process.run({m_settings.p4BinaryPath.value(), args});
 
     PerforceResponse response;
     response.error = true;
@@ -1301,7 +1301,7 @@ PerforceResponse PerforcePluginPrivate::synchronousProcess(const QString &workin
         response.message = msgCrash();
         break;
     case SynchronousProcessResponse::StartFailed:
-        response.message = msgNotStarted(m_settings.p4BinaryPath());
+        response.message = msgNotStarted(m_settings.p4BinaryPath.value());
         break;
     case SynchronousProcessResponse::Hang:
         response.message = msgCrash();
@@ -1325,13 +1325,13 @@ PerforceResponse PerforcePluginPrivate::fullySynchronousProcess(const QString &w
         process.setWorkingDirectory(workingDir);
 
     PerforceResponse response;
-    process.start(m_settings.p4BinaryPath(), args);
+    process.start(m_settings.p4BinaryPath.value(), args);
     if (stdInput.isEmpty())
         process.closeWriteChannel();
 
     if (!process.waitForStarted(3000)) {
         response.error = true;
-        response.message = msgNotStarted(m_settings.p4BinaryPath());
+        response.message = msgNotStarted(m_settings.p4BinaryPath.value());
         return response;
     }
     if (!stdInput.isEmpty()) {
@@ -1339,7 +1339,7 @@ PerforceResponse PerforcePluginPrivate::fullySynchronousProcess(const QString &w
             SynchronousProcess::stopProcess(process);
             response.error = true;
             response.message = tr("Unable to write input data to process %1: %2").
-                               arg(QDir::toNativeSeparators(m_settings.p4BinaryPath()),
+                               arg(QDir::toNativeSeparators(m_settings.p4BinaryPath.value()),
                                    process.errorString());
             return response;
         }
@@ -1348,7 +1348,7 @@ PerforceResponse PerforcePluginPrivate::fullySynchronousProcess(const QString &w
 
     QByteArray stdOut;
     QByteArray stdErr;
-    const int timeOutS = (flags & LongTimeOut) ? m_settings.longTimeOutS() : m_settings.timeOutS();
+    const int timeOutS = (flags & LongTimeOut) ? m_settings.longTimeOutS() : m_settings.timeOutS.value();
     if (!SynchronousProcess::readDataFromProcess(process, timeOutS, &stdOut, &stdErr, true)) {
         SynchronousProcess::stopProcess(process);
         response.error = true;
@@ -1404,7 +1404,7 @@ PerforceResponse PerforcePluginPrivate::runP4Cmd(const QString &workingDir,
     actualArgs.append(args);
 
     if (flags & CommandToWindow)
-        VcsOutputWindow::appendCommand(workingDir, {m_settings.p4BinaryPath(), actualArgs});
+        VcsOutputWindow::appendCommand(workingDir, {m_settings.p4BinaryPath.value(), actualArgs});
 
     if (flags & ShowBusyCursor)
         QApplication::setOverrideCursor(QCursor(Qt::WaitCursor));
@@ -1586,7 +1586,7 @@ bool PerforcePluginPrivate::submitEditorAboutToClose()
     QTC_ASSERT(editorDocument, return true);
     // Prompt the user. Force a prompt unless submit was actually invoked (that
     // is, the editor was closed or shutdown).
-    bool wantsPrompt = m_settings.promptToSubmit();
+    bool wantsPrompt = m_settings.promptToSubmit.value();
     const VcsBaseSubmitEditor::PromptSubmitResult answer =
             perforceEditor->promptSubmit(this, &wantsPrompt, !m_submitActionTriggered);
     m_submitActionTriggered = false;
@@ -1595,9 +1595,9 @@ bool PerforcePluginPrivate::submitEditorAboutToClose()
         return false;
 
     // Set without triggering the checking mechanism
-    if (wantsPrompt != m_settings.promptToSubmit()) {
+    if (wantsPrompt != m_settings.promptToSubmit.value()) {
-        m_settings.setPromptToSubmit(wantsPrompt);
+        m_settings.promptToSubmit.setValue(wantsPrompt);
-        m_settings.settings().writeSettings(ICore::settings());
+        m_settings.writeSettings(ICore::settings());
     }
     if (!DocumentManager::saveDocument(editorDocument))
         return false;
@@ -1724,7 +1724,7 @@ void PerforcePluginPrivate::setTopLevel(const QString &topLevel)
 
 void PerforcePluginPrivate::applySettings()
 {
-    m_settings.settings().writeSettings(ICore::settings());
+    m_settings.writeSettings(ICore::settings());
     m_managedDirectoryCache.clear();
     getTopLevel();
     emit configurationChanged();
@@ -1738,7 +1738,7 @@ void PerforcePluginPrivate::slotTopLevelFailed(const QString &errorMessage)
 void PerforcePluginPrivate::getTopLevel(const QString &workingDirectory, bool isSync)
 {
     // Run a new checker
-    if (m_settings.p4BinaryPath().isEmpty())
+    if (m_settings.p4BinaryPath.value().isEmpty())
         return;
     auto checker = new PerforceChecker(dd);
     connect(checker, &PerforceChecker::failed, dd, &PerforcePluginPrivate::slotTopLevelFailed);
@@ -1746,7 +1746,7 @@ void PerforcePluginPrivate::getTopLevel(const QString &workingDirectory, bool is
     connect(checker, &PerforceChecker::succeeded, dd, &PerforcePluginPrivate::setTopLevel);
     connect(checker, &PerforceChecker::succeeded,checker, &QObject::deleteLater);
 
-    checker->start(m_settings.p4BinaryPath(), workingDirectory,
+    checker->start(m_settings.p4BinaryPath.value(), workingDirectory,
                    m_settings.commonP4Arguments(QString()), 30000);
 
     if (isSync)

src/plugins/perforce/perforcesettings.cpp

@@ -52,7 +52,7 @@ static QString defaultCommand()
     return QLatin1String("p4" QTC_HOST_EXE_SUFFIX);
 }
 
-Settings::Settings()
+PerforceSettings::PerforceSettings()
 {
     setSettingsGroup("Perforce");
     setAutoApply(false);
@@ -114,7 +114,13 @@ Settings::Settings()
     autoOpen.setLabelText(tr("Automatically open files when editing"));
 }
 
-QStringList Settings::commonP4Arguments() const
+// --------------------PerforceSettings
+PerforceSettings::~PerforceSettings()
+{
+    delete m_topLevelDir;
+}
+
+QStringList PerforceSettings::commonP4Arguments() const
 {
     QStringList lst;
     if (customEnv.value()) {
@@ -128,60 +134,14 @@ QStringList Settings::commonP4Arguments() const
     return lst;
 }
 
-// --------------------PerforceSettings
-PerforceSettings::~PerforceSettings()
-{
-    delete m_topLevelDir;
-}
-
 bool PerforceSettings::isValid() const
 {
-    return !m_topLevel.isEmpty() && !m_settings.p4BinaryPath.value().isEmpty();
+    return !m_topLevel.isEmpty() && !p4BinaryPath.value().isEmpty();
-}
-
-QString PerforceSettings::p4BinaryPath() const
-{
-    return m_settings.p4BinaryPath.value();
-}
-
-QString PerforceSettings::p4Port() const
-{
-    return m_settings.p4Port.value();
-}
-
-QString PerforceSettings::p4Client() const
-{
-    return m_settings.p4Client.value();
-}
-
-QString PerforceSettings::p4User() const
-{
-    return m_settings.p4User.value();
 }
 
 bool PerforceSettings::defaultEnv() const
 {
-    return !m_settings.customEnv.value(); // Note: negated
+    return !customEnv.value(); // Note: negated
-}
-
-bool PerforceSettings::promptToSubmit() const
-{
-    return m_settings.promptToSubmit.value();
-}
-
-void PerforceSettings::setPromptToSubmit(bool p)
-{
-    m_settings.promptToSubmit.setValue(p);
-}
-
-bool PerforceSettings::autoOpen() const
-{
-    return m_settings.autoOpen.value();
-}
-
-void PerforceSettings::setAutoOpen(bool b)
-{
-    m_settings.autoOpen.setValue(b);
 }
 
 QString PerforceSettings::topLevel() const
@@ -256,7 +216,7 @@ QStringList PerforceSettings::commonP4Arguments(const QString &workingDir) const
         rc << QLatin1String("-d")
            << QDir::toNativeSeparators(mapPathRoot(workingDir, m_topLevelSymLinkTarget, m_topLevel));
     }
-    rc.append(m_settings.commonP4Arguments());
+    rc.append(commonP4Arguments());
     return rc;
 }
 
@@ -270,16 +230,16 @@ QString PerforceSettings::mapToFileSystem(const QString &perforceFilePath) const
 PerforceSettingsPage::PerforceSettingsPage(PerforceSettings *settings)
 {
     setId(VcsBase::Constants::VCS_ID_PERFORCE);
-    setDisplayName(Settings::tr("Perforce"));
+    setDisplayName(PerforceSettings::tr("Perforce"));
     setCategory(VcsBase::Constants::VCS_SETTINGS_CATEGORY);
-    setSettings(&settings->settings());
+    setSettings(settings);
 
     setLayouter([settings, this](QWidget *widget) {
-        Settings &s = settings->settings();
+        PerforceSettings &s = *settings;
         using namespace Layouting;
 
         auto errorLabel = new QLabel;
-        auto testButton = new QPushButton(Settings::tr("Test"));
+        auto testButton = new QPushButton(PerforceSettings::tr("Test"));
         connect(testButton, &QPushButton::clicked, this, [this, settings, errorLabel, testButton] {
             testButton->setEnabled(false);
             auto checker = new PerforceChecker(errorLabel);
@@ -300,23 +260,22 @@ PerforceSettingsPage::PerforceSettingsPage(PerforceSettings *settings)
             });
 
             errorLabel->setStyleSheet(QString());
-            errorLabel->setText(Settings::tr("Testing..."));
+            errorLabel->setText(PerforceSettings::tr("Testing..."));
-            const Settings &s = settings->settings();
+            checker->start(settings->p4BinaryPath.value(), QString(), settings->commonP4Arguments(), 10000);
-            checker->start(s.p4BinaryPath.value(), QString(), s.commonP4Arguments(), 10000);
         });
 
         Group config {
-            Title(Settings::tr("Configuration")),
+            Title(PerforceSettings::tr("Configuration")),
             Row { s.p4BinaryPath }
         };
 
         Group environment {
-            Title(Settings::tr("Environment Variables"), &s.customEnv),
+            Title(PerforceSettings::tr("Environment Variables"), &s.customEnv),
             Row { s.p4Port, s.p4Client, s.p4User }
         };
 
         Group misc {
-            Title(Settings::tr("Miscellaneous")),
+            Title(PerforceSettings::tr("Miscellaneous")),
             Row { s.logCount, s.timeOutS, Stretch() },
             s.promptToSubmit,
             s.autoOpen

src/plugins/perforce/perforcesettings.h

@@ -36,32 +36,7 @@ QT_END_NAMESPACE
 namespace Perforce {
 namespace Internal {
 
-class Settings : public Utils::AspectContainer
+/* PerforceSettings: Aggregates settings items and toplevel directory
-{
-    Q_DECLARE_TR_FUNCTIONS(Perforce::Internal::SettingsPage)
-
-public:
-    Settings();
-    QStringList commonP4Arguments() const;
-
-    // Checks. On success, errorMessage will contains the client root.
-    bool check(QString *repositoryRoot /* = 0*/, QString *errorMessage) const;
-    static bool doCheck(const QString &binary, const QStringList &basicArgs,
-                        QString *repositoryRoot /* = 0 */,
-                        QString *errorMessage);
-
-    Utils::StringAspect p4BinaryPath;
-    Utils::StringAspect p4Port;
-    Utils::StringAspect p4Client;
-    Utils::StringAspect p4User;
-    Utils::IntegerAspect logCount;
-    Utils::BoolAspect customEnv;
-    Utils::IntegerAspect timeOutS;
-    Utils::BoolAspect promptToSubmit;
-    Utils::BoolAspect autoOpen;
-};
-
-/* PerforceSettings: Aggregates settings struct and toplevel directory
  * which is determined externally by background checks and provides a convenience
  * for determining the common arguments.
  * Those must contain (apart from server connection settings) the working directory
@@ -75,22 +50,24 @@ public:
  * p4. This is why the client root portion of working directory must be mapped for the
  * "-d" option, so that running p4 in "/depot/dev/foo" results in "-d $HOME/dev/foo". */
 
-class PerforceSettings
+class PerforceSettings : public Utils::AspectContainer
 {
+    Q_DECLARE_TR_FUNCTIONS(Perforce::Internal::SettingsPage)
+
 public:
-    PerforceSettings() = default;
+    PerforceSettings();
     ~PerforceSettings();
-    PerforceSettings(const PerforceSettings &other) = delete;
 
     bool isValid() const;
 
-    const Settings &settings() const { return m_settings; }
+    // Checks. On success, errorMessage will contains the client root.
-    Settings &settings() { return m_settings; }
+    bool check(QString *repositoryRoot /* = 0*/, QString *errorMessage) const;
+    static bool doCheck(const QString &binary, const QStringList &basicArgs,
+                        QString *repositoryRoot /* = 0 */,
+                        QString *errorMessage);
 
-    int timeOutS() const { return m_settings.timeOutS.value();  }
+    int longTimeOutS() const { return timeOutS.value() * 10; }
-    int longTimeOutS() const { return m_settings.timeOutS.value() * 10; }
+    int timeOutMS() const { return timeOutS.value() * 1000;  }
-    int timeOutMS() const { return m_settings.timeOutS.value() * 1000;  }
-    int logCount() const { return m_settings.logCount.value(); }
 
     QString topLevel() const;
     QString topLevelSymLinkTarget() const;
@@ -107,25 +84,26 @@ public:
     // Map p4 path back to file system in case of a symlinked top-level
     QString mapToFileSystem(const QString &perforceFilePath) const;
 
-    QString p4BinaryPath() const;
-    QString p4Port() const;
-    QString p4Client() const;
-    QString p4User() const;
     bool defaultEnv() const;
-    bool promptToSubmit() const;
-    void setPromptToSubmit(bool p);
-    bool autoOpen() const;
-    void setAutoOpen(bool p);
 
     // Return basic arguments, including -d and server connection parameters.
+    QStringList commonP4Arguments() const;
     QStringList commonP4Arguments(const QString &workingDir) const;
 
     void clearTopLevel();
 
+    Utils::StringAspect p4BinaryPath;
+    Utils::StringAspect p4Port;
+    Utils::StringAspect p4Client;
+    Utils::StringAspect p4User;
+    Utils::IntegerAspect logCount;
+    Utils::BoolAspect customEnv;
+    Utils::IntegerAspect timeOutS;
+    Utils::BoolAspect promptToSubmit;
+    Utils::BoolAspect autoOpen;
+
 private:
     QStringList workingDirectoryArguments(const QString &workingDir) const;
-
-    Settings m_settings;
     QString m_topLevel;
     QString m_topLevelSymLinkTarget;
     QDir *m_topLevelDir = nullptr;