Fixed p4 plugin.

Introduce settings struct again. Add an errorString() to
the settings class. Change checkP4Command to checkP4Configuration
and make it report errors when appropriate. Make errorString show
up in settings page and add "Test" button.
Build options correctly (omit empty settings) and use basic
options for the test (which was the bug causing p4 to be
disabled if options were actually specified).

7 files changed, 261 insertions, 123 deletions

diff --git a/src/plugins/perforce/perforceplugin.cpp b/src/plugins/perforce/perforceplugin.cpp
index 2bbbb705ae..39e6460d34 100644
--- a/src/plugins/perforce/perforceplugin.cpp
+++ b/src/plugins/perforce/perforceplugin.cpp
@@ -488,8 +488,9 @@ void PerforcePlugin::submit()
     if (VCSBase::VCSBaseSubmitEditor::raiseSubmitEditor())
         return;
 
-    if (!checkP4Command()) {
-        showOutput(tr("No p4 executable specified!"), true);
+    QString errorMessage;
+    if (!checkP4Configuration(&errorMessage)) {
+        showOutput(errorMessage, true);
         return;
     }
 
@@ -660,7 +661,7 @@ void PerforcePlugin::updateActions()
 
 bool PerforcePlugin::managesDirectory(const QString &directory) const
 {
-    if (!checkP4Command())
+    if (!checkP4Configuration())
         return false;
     const QString p4Path = directory + QLatin1String("/...");
     QStringList args;
@@ -727,8 +728,7 @@ PerforceResponse PerforcePlugin::runP4Cmd(const QStringList &args,
         qDebug() << "PerforcePlugin::runP4Cmd" << args << extraArgs << debugCodec(outputCodec);
     PerforceResponse response;
     response.error = true;
-    if (!checkP4Command()) {
-        response.message = tr("No p4 executable specified!");
+    if (!checkP4Configuration(&response.message)) {
         m_perforceOutputWindow->append(response.message, true);
         return response;
     }
@@ -959,8 +959,9 @@ bool PerforcePlugin::editorAboutToClose(Core::IEditor *editor)
         if (answer == VCSBase::VCSBaseSubmitEditor::SubmitConfirmed) {
             m_changeTmpFile->seek(0);
             QByteArray change = m_changeTmpFile->readAll();
-            if (!checkP4Command()) {
-                showOutput(tr("No p4 executable specified!"), true);
+            QString errorMessage;
+            if (!checkP4Configuration(&errorMessage)) {
+                showOutput(errorMessage, true);
                 return false;
             }
 
@@ -1005,7 +1006,7 @@ void PerforcePlugin::openFiles(const QStringList &files)
 
 QString PerforcePlugin::clientFilePath(const QString &serverFilePath)
 {
-    if (!checkP4Command())
+    if (!checkP4Configuration())
         return QString();
 
     QApplication::setOverrideCursor(Qt::WaitCursor);
@@ -1040,15 +1041,19 @@ QString PerforcePlugin::currentFileName()
     return fileName;
 }
 
-bool PerforcePlugin::checkP4Command() const
+bool PerforcePlugin::checkP4Configuration(QString *errorMessage /* = 0 */) const
 {
-    return m_settings.isValid();
+    if (m_settings.isValid())
+        return true;
+    if (errorMessage)
+        *errorMessage = tr("Invalid configuration: %1").arg(m_settings.errorString());
+    return false;
 }
 
 QString PerforcePlugin::pendingChangesData()
 {
     QString data;
-    if (!checkP4Command())
+    if (!checkP4Configuration())
         return data;
 
     QString user;
@@ -1124,18 +1129,10 @@ const PerforceSettings& PerforcePlugin::settings() const
     return m_settings;
 }
 
-void PerforcePlugin::setSettings(const QString &p4Command, const QString &p4Port, const QString &p4Client, const QString p4User, bool defaultEnv)
+void PerforcePlugin::setSettings(const Settings &newSettings)
 {
-
-    if (m_settings.p4Command() == p4Command
-        && m_settings.p4Port() == p4Port
-        && m_settings.p4Client() == p4Client
-        && m_settings.p4User() == p4User
-        && m_settings.defaultEnv() == defaultEnv)
-    {
-        // Nothing to do
-    } else {
-        m_settings.setSettings(p4Command, p4Port, p4Client, p4User, defaultEnv);
+    if (newSettings != m_settings.settings()) {
+        m_settings.setSettings(newSettings);
         m_settings.toSettings(Core::ICore::instance()->settings());
     }
 }
diff --git a/src/plugins/perforce/perforceplugin.h b/src/plugins/perforce/perforceplugin.h
index 042736a2dd..193594b62d 100644
--- a/src/plugins/perforce/perforceplugin.h
+++ b/src/plugins/perforce/perforceplugin.h
@@ -112,7 +112,7 @@ public:
     static PerforcePlugin *perforcePluginInstance();
 
     const PerforceSettings& settings() const;
-    void setSettings(const QString &p4Command, const QString &p4Port, const QString &p4Client, const QString p4User, bool defaultEnv);
+    void setSettings(const Settings &s);
 
     // Map a perforce name "//xx" to its real name in the file system
     QString fileNameFromPerforceName(const QString& perforceName, QString *errorMessage) const;
@@ -162,7 +162,7 @@ private:
 
     QString clientFilePath(const QString &serverFilePath);
     QString currentFileName();
-    bool checkP4Command() const;
+    bool checkP4Configuration(QString *errorMessage = 0) const;
     void showOutput(const QString &output, bool popup = false) const;
     void annotate(const QString &fileName);
     void filelog(const QString &fileName);
diff --git a/src/plugins/perforce/perforcesettings.cpp b/src/plugins/perforce/perforcesettings.cpp
index cb90713ca9..c49e7dedc0 100644
--- a/src/plugins/perforce/perforcesettings.cpp
+++ b/src/plugins/perforce/perforcesettings.cpp
@@ -33,8 +33,11 @@
 #include <QtCore/QtConcurrentRun>
 #include <QtCore/QSettings>
 #include <QtCore/QStringList>
+#include <QtCore/QCoreApplication>
 #include <QtCore/QProcess>
 
+enum { debug = 0 };
+
 static const char *groupC = "Perforce";
 static const char *commandKeyC = "Command";
 static const char *defaultKeyC = "Default";
@@ -55,6 +58,85 @@ static QString defaultCommand()
 namespace Perforce {
 namespace Internal {
 
+Settings::Settings() :
+    defaultEnv(true)
+{
+}
+
+bool Settings::equals(const Settings &rhs) const
+{
+    return defaultEnv == rhs.defaultEnv
+            && p4Command == rhs.p4Command && p4Port == rhs.p4Port
+            && p4Client == rhs.p4Client && p4User == rhs.p4User;
+};
+
+QStringList Settings::basicP4Args() const
+{
+    if (defaultEnv)
+        return QStringList();
+    QStringList lst;
+    if (!p4Client.isEmpty())
+        lst << QLatin1String("-c") << p4Client;
+    if (!p4Port.isEmpty())
+        lst << QLatin1String("-p") << p4Port;
+    if (!p4User.isEmpty())
+        lst << QLatin1String("-u") << p4User;
+    return lst;
+}
+
+bool Settings::check(QString *errorMessage) const
+{
+    return doCheck(p4Command, basicP4Args(), errorMessage);
+}
+
+// Check on a p4 view by grepping "view -o" for mapped files
+bool Settings::doCheck(const QString &binary, const QStringList &basicArgs, QString *errorMessage)
+{
+    errorMessage->clear();
+    if (binary.isEmpty()) {
+        *errorMessage = QCoreApplication::translate("Perforce::Internal",  "No executable specified");
+        return false;
+    }
+    // List the client state and check for mapped files
+    QProcess p4;
+    QStringList args = basicArgs;
+    args << QLatin1String("client") << QLatin1String("-o");
+    if (debug)
+        qDebug() << binary << args;
+    p4.start(binary, args);
+    if (!p4.waitForStarted()) {
+        *errorMessage = QCoreApplication::translate("Perforce::Internal", "Unable to launch \"%1\": %2").arg(binary, p4.errorString());
+        return false;
+    }
+    p4.closeWriteChannel();
+    const int timeOutMS = 5000;
+    if (!p4.waitForFinished(timeOutMS)) {
+        p4.kill();
+        p4.waitForFinished();
+        *errorMessage = QCoreApplication::translate("Perforce::Internal", "\"%1\" timed out after %2ms.").arg(binary).arg(timeOutMS);
+        return false;
+    }
+    if (p4.exitStatus() != QProcess::NormalExit) {
+        *errorMessage = QCoreApplication::translate("Perforce::Internal", "\"%1\" crashed.").arg(binary);
+        return false;
+    }
+    const QString stdErr = QString::fromLocal8Bit(p4.readAllStandardError());
+    if (p4.exitCode()) {
+        *errorMessage = QCoreApplication::translate("Perforce::Internal", "\"%1\" terminated with exit code %2: %3").
+                        arg(binary).arg(p4.exitCode()).arg(stdErr);
+        return false;
+
+    }
+    // List the client state and check for "View"
+    const QString response = QString::fromLocal8Bit(p4.readAllStandardOutput());
+    if (!response.contains(QLatin1String("View:")) && !response.contains(QLatin1String("//depot/"))) {
+        *errorMessage = QCoreApplication::translate("Perforce::Internal", "The client does not seem to contain any mapped files.");
+        return false;
+    }
+    return true;
+}
+
+// --------------------PerforceSettings
 PerforceSettings::PerforceSettings()
     : m_valid(false)
 {
@@ -79,29 +161,20 @@ bool PerforceSettings::isValid() const
 void PerforceSettings::run(QFutureInterface<void> &fi)
 {
     m_mutex.lock();
-    QString executable = m_p4Command;
-    QStringList arguments = basicP4Args();
+    const QString executable = m_settings.p4Command;
+    const QStringList arguments = basicP4Args();
     m_mutex.unlock();
 
-    // TODO actually check
-    bool valid = true;
-
-    QProcess p4;
-    p4.start(m_p4Command, QStringList() << "client"<<"-o");
-    p4.waitForFinished(2000);
-    if (p4.state() != QProcess::NotRunning) {
-        p4.kill();
-        p4.waitForFinished();
-        valid = false;
-    } else {
-        QString response = p4.readAllStandardOutput();
-        if (!response.contains("View:"))
-            valid = false;
-    }
+    QString errorString;
+    const bool isValid = Settings::doCheck(executable, arguments, &errorString);
+    if (debug)
+        qDebug() << isValid << errorString;
 
     m_mutex.lock();
-    if (executable == m_p4Command && arguments == basicP4Args()) // Check that those settings weren't changed in between
-        m_valid = valid;
+    if (executable == m_settings.p4Command && arguments == basicP4Args()) { // Check that those settings weren't changed in between
+        m_errorString = errorString;
+        m_valid = isValid;
+    }
     m_mutex.unlock();
     fi.reportFinished();
 }
@@ -110,11 +183,11 @@ void PerforceSettings::fromSettings(QSettings *settings)
 {
     m_mutex.lock();
     settings->beginGroup(QLatin1String(groupC));
-    m_p4Command = settings->value(QLatin1String(commandKeyC), defaultCommand()).toString();
-    m_defaultEnv = settings->value(QLatin1String(defaultKeyC), true).toBool();
-    m_p4Port = settings->value(QLatin1String(portKeyC), QString()).toString();
-    m_p4Client = settings->value(QLatin1String(clientKeyC), QString()).toString();
-    m_p4User = settings->value(QLatin1String(userKeyC), QString()).toString();
+    m_settings.p4Command = settings->value(QLatin1String(commandKeyC), defaultCommand()).toString();
+    m_settings.defaultEnv = settings->value(QLatin1String(defaultKeyC), true).toBool();
+    m_settings.p4Port = settings->value(QLatin1String(portKeyC), QString()).toString();
+    m_settings.p4Client = settings->value(QLatin1String(clientKeyC), QString()).toString();
+    m_settings.p4User = settings->value(QLatin1String(userKeyC), QString()).toString();
     settings->endGroup();
     m_mutex.unlock();
 
@@ -125,64 +198,69 @@ void PerforceSettings::toSettings(QSettings *settings) const
 {
     m_mutex.lock();
     settings->beginGroup(QLatin1String(groupC));
-    settings->setValue(commandKeyC, m_p4Command);
-    settings->setValue(defaultKeyC, m_defaultEnv);
-    settings->setValue(portKeyC, m_p4Port);
-    settings->setValue(clientKeyC, m_p4Client);
-    settings->setValue(userKeyC, m_p4User);
+    settings->setValue(commandKeyC, m_settings.p4Command);
+    settings->setValue(defaultKeyC, m_settings.defaultEnv);
+    settings->setValue(portKeyC, m_settings.p4Port);
+    settings->setValue(clientKeyC, m_settings.p4Client);
+    settings->setValue(userKeyC, m_settings.p4User);
     settings->endGroup();
     m_mutex.unlock();
 }
 
-void PerforceSettings::setSettings(const QString &p4Command, const QString &p4Port, const QString &p4Client, const QString p4User, bool defaultEnv)
+void PerforceSettings::setSettings(const Settings &newSettings)
+{    
+    if (newSettings != m_settings) {
+        // trigger check
+        m_settings = newSettings;
+        m_mutex.lock();
+        m_valid = false;
+        m_mutex.unlock();
+        m_future = QtConcurrent::run(&PerforceSettings::run, this);
+    }
+}
+
+Settings PerforceSettings::settings() const
 {
-    m_mutex.lock();
-    m_p4Command = p4Command;
-    m_p4Port = p4Port;
-    m_p4Client = p4Client;
-    m_p4User = p4User;
-    m_defaultEnv = defaultEnv;
-    m_valid = false;
-    m_mutex.unlock();
-    m_future = QtConcurrent::run(&PerforceSettings::run, this);
+    return m_settings;
 }
 
 QString PerforceSettings::p4Command() const
 {
-    return m_p4Command;
+    return m_settings.p4Command;
 }
 
 QString PerforceSettings::p4Port() const
 {
-    return m_p4Port;
+    return m_settings.p4Port;
 }
 
 QString PerforceSettings::p4Client() const
 {
-    return m_p4Client;
+    return m_settings.p4Client;
 }
 
 QString PerforceSettings::p4User() const
 {
-    return m_p4User;
+    return m_settings.p4User;
 }
 
 bool PerforceSettings::defaultEnv() const
 {
-    return m_defaultEnv;
+    return m_settings.defaultEnv;
 }
 
-QStringList PerforceSettings::basicP4Args() const
+QString PerforceSettings::errorString() const
 {
-    QStringList lst;
-    if (!m_defaultEnv) {
-        lst << QLatin1String("-c") << m_p4Client;
-        lst << QLatin1String("-p") << m_p4Port;
-        lst << QLatin1String("-u") << m_p4User;
-    }
-    return lst;
+    m_mutex.lock();
+    const QString rc = m_errorString;
+    m_mutex.unlock();
+    return rc;
 }
 
+QStringList PerforceSettings::basicP4Args() const
+{
+    return m_settings.basicP4Args();
+}
 
 } // Internal
 } // Perforce
diff --git a/src/plugins/perforce/perforcesettings.h b/src/plugins/perforce/perforcesettings.h
index 7cf86aeebb..c3138693a3 100644
--- a/src/plugins/perforce/perforcesettings.h
+++ b/src/plugins/perforce/perforcesettings.h
@@ -40,13 +40,40 @@ QT_END_NAMESPACE
 namespace Perforce {
 namespace Internal {
 
+struct Settings {
+    Settings();
+    bool equals(const Settings &s) const;
+    QStringList basicP4Args() const;
+
+    bool check(QString *errorMessage) const;
+    static bool doCheck(const QString &binary, const QStringList &basicArgs, QString *errorMessage);
+
+    QString p4Command;
+    QString p4Port;
+    QString p4Client;
+    QString p4User;
+    QString errorString;
+    bool defaultEnv;
+};
+
+inline bool operator==(const Settings &s1, const Settings &s2) { return s1.equals(s2); }
+inline bool operator!=(const Settings &s1, const Settings &s2) { return !s1.equals(s2); }
+
+// PerforceSettings: Aggregates settings struct and contains a sophisticated
+// background check invoked on setSettings() to figure out whether the p4
+// configuration is actually valid (disabling it when invalid to save time
+// when updating actions. etc.)
+
 class PerforceSettings {
 public:
     PerforceSettings();
     ~PerforceSettings();
     void fromSettings(QSettings *settings);
     void toSettings(QSettings *) const;
-    void setSettings(const QString &p4Command, const QString &p4Port, const QString &p4Client, const QString p4User, bool defaultEnv);
+
+    void setSettings(const Settings &s);
+    Settings settings() const;
+
     bool isValid() const;
 
     QString p4Command() const;
@@ -55,16 +82,18 @@ public:
     QString p4User() const;
     bool defaultEnv() const;
     QStringList basicP4Args() const;
+
+    // Error code of last check
+    QString errorString() const;
+
 private:
     void run(QFutureInterface<void> &fi);
+
     mutable QFuture<void> m_future;
     mutable QMutex m_mutex;
 
-    QString m_p4Command;
-    QString m_p4Port;
-    QString m_p4Client;
-    QString m_p4User;
-    bool m_defaultEnv;
+    Settings m_settings;
+    QString m_errorString;
     bool m_valid;
     Q_DISABLE_COPY(PerforceSettings);
 };
diff --git a/src/plugins/perforce/settingspage.cpp b/src/plugins/perforce/settingspage.cpp
index 024c2ac01f..4004c81da3 100644
--- a/src/plugins/perforce/settingspage.cpp
+++ b/src/plugins/perforce/settingspage.cpp
@@ -33,7 +33,7 @@
 
 #include <vcsbase/vcsbaseconstants.h>
 
-#include <QtCore/QCoreApplication>
+#include <QtGui/QApplication>
 #include <QtGui/QLineEdit>
 #include <QtGui/QFileDialog>
 
@@ -46,31 +46,31 @@ SettingsPageWidget::SettingsPageWidget(QWidget *parent) :
     m_ui.setupUi(this);
     m_ui.pathChooser->setPromptDialogTitle(tr("Perforce Command"));
     m_ui.pathChooser->setExpectedKind(PathChooser::Command);
+    connect(m_ui.testPushButton, SIGNAL(clicked()), this, SLOT(slotTest()));
 }
 
-QString SettingsPageWidget::p4Command() const
+void SettingsPageWidget::slotTest()
 {
-    return m_ui.pathChooser->path();
+    QString message;
+    QApplication::setOverrideCursor(Qt::BusyCursor);
+    setStatusText(true, tr("Testing..."));
+    QApplication::processEvents(QEventLoop::ExcludeUserInputEvents);
+    const bool ok = settings().check(&message);
+    QApplication::restoreOverrideCursor();
+    if (ok)
+        message = tr("Test succeeded.");
+    setStatusText(ok, message);
 }
 
-bool SettingsPageWidget::defaultEnv() const
+Settings SettingsPageWidget::settings() const
 {
-    return m_ui.defaultCheckBox->isChecked();
-}
-
-QString SettingsPageWidget::p4Port() const
-{
-    return m_ui.portLineEdit->text();
-}
-
-QString SettingsPageWidget::p4User() const
-{
-    return m_ui.userLineEdit->text();
-}
-
-QString SettingsPageWidget::p4Client() const
-{
-    return m_ui.clientLineEdit->text();
+    Settings  settings;
+    settings.p4Command = m_ui.pathChooser->path();
+    settings.defaultEnv = m_ui.defaultCheckBox->isChecked();
+    settings.p4Port = m_ui.portLineEdit->text();
+    settings.p4User = m_ui.userLineEdit->text();
+    settings.p4Client=  m_ui.clientLineEdit->text();
+    return settings;
 }
 
 void SettingsPageWidget::setSettings(const PerforceSettings &s)
@@ -80,6 +80,14 @@ void SettingsPageWidget::setSettings(const PerforceSettings &s)
     m_ui.portLineEdit->setText(s.p4Port());
     m_ui.clientLineEdit->setText(s.p4Client());
     m_ui.userLineEdit->setText(s.p4User());
+    const QString errorString = s.errorString();
+    setStatusText(errorString.isEmpty(), errorString);
+}
+
+void SettingsPageWidget::setStatusText(bool ok, const QString &t)
+{
+    m_ui.errorLabel->setStyleSheet(ok ? QString() : QString(QLatin1String("background-color: red")));
+    m_ui.errorLabel->setText(t);
 }
 
 SettingsPage::SettingsPage()
@@ -115,5 +123,5 @@ QWidget *SettingsPage::createPage(QWidget *parent)
 
 void SettingsPage::apply()
 {
-    PerforcePlugin::perforcePluginInstance()->setSettings(m_widget->p4Command(), m_widget->p4Port(), m_widget->p4Client(), m_widget->p4User(), m_widget->defaultEnv());
+    PerforcePlugin::perforcePluginInstance()->setSettings(m_widget->settings());
 }
diff --git a/src/plugins/perforce/settingspage.h b/src/plugins/perforce/settingspage.h
index f4c6d028af..1b284abe74 100644
--- a/src/plugins/perforce/settingspage.h
+++ b/src/plugins/perforce/settingspage.h
@@ -41,21 +41,22 @@ namespace Perforce {
 namespace Internal {
 
 class PerforceSettings;
+struct Settings;
 
 class SettingsPageWidget : public QWidget {
     Q_OBJECT
 public:
     explicit SettingsPageWidget(QWidget *parent);
 
-    QString p4Command() const;
-    bool defaultEnv() const;
-    QString p4Port() const;
-    QString p4User() const;
-    QString p4Client() const;
-
     void setSettings(const PerforceSettings &);
+    Settings settings() const;
+
+private slots:
+    void slotTest();
 
 private:
+    void setStatusText(bool ok, const QString &);
+
     Ui::SettingsPage m_ui;
 };
 
diff --git a/src/plugins/perforce/settingspage.ui b/src/plugins/perforce/settingspage.ui
index 5af25e5f81..3842a2aa04 100644
--- a/src/plugins/perforce/settingspage.ui
+++ b/src/plugins/perforce/settingspage.ui
@@ -6,17 +6,11 @@
    <rect>
     <x>0</x>
     <y>0</y>
-    <width>276</width>
-    <height>198</height>
+    <width>408</width>
+    <height>463</height>
    </rect>
   </property>
-  <layout class="QVBoxLayout">
-   <property name="spacing">
-    <number>6</number>
-   </property>
-   <property name="margin">
-    <number>9</number>
-   </property>
+  <layout class="QVBoxLayout" name="verticalLayout">
    <item>
     <layout class="QHBoxLayout">
      <property name="spacing">
@@ -26,7 +20,7 @@
       <number>0</number>
      </property>
      <item>
-      <widget class="QLabel" name="label_4">
+      <widget class="QLabel" name="commandLabel">
        <property name="text">
         <string>P4 Command:</string>
        </property>
@@ -66,21 +60,21 @@
        <widget class="QLineEdit" name="clientLineEdit"/>
       </item>
       <item row="1" column="0">
-       <widget class="QLabel" name="label_2">
+       <widget class="QLabel" name="clientLabel">
         <property name="text">
          <string>P4 Client:</string>
         </property>
        </widget>
       </item>
       <item row="2" column="0">
-       <widget class="QLabel" name="label_3">
+       <widget class="QLabel" name="userLabel">
         <property name="text">
          <string>P4 User:</string>
         </property>
        </widget>
       </item>
       <item row="0" column="0">
-       <widget class="QLabel" name="label">
+       <widget class="QLabel" name="portLabel">
         <property name="text">
          <string>P4 Port:</string>
         </property>
@@ -96,7 +90,31 @@
     </widget>
    </item>
    <item>
-    <spacer>
+    <layout class="QHBoxLayout" name="horizontalLayout">
+     <item>
+      <spacer name="horizontalSpacer">
+       <property name="orientation">
+        <enum>Qt::Horizontal</enum>
+       </property>
+       <property name="sizeHint" stdset="0">
+        <size>
+         <width>40</width>
+         <height>20</height>
+        </size>
+       </property>
+      </spacer>
+     </item>
+     <item>
+      <widget class="QPushButton" name="testPushButton">
+       <property name="text">
+        <string>Test</string>
+       </property>
+      </widget>
+     </item>
+    </layout>
+   </item>
+   <item>
+    <spacer name="verticalSpacer">
      <property name="orientation">
       <enum>Qt::Vertical</enum>
      </property>
@@ -108,6 +126,13 @@
      </property>
     </spacer>
    </item>
+   <item>
+    <widget class="QLabel" name="errorLabel">
+     <property name="text">
+      <string>TextLabel</string>
+     </property>
+    </widget>
+   </item>
   </layout>
  </widget>
  <customwidgets>