Some refactoring of the spawn code and to make waitForApplication get the right error code (usually)

A note: It is possible for the executed program to completely exit before MO attempts to get hold of the pid from the job handle,
in which case strangeness will happen (this has always been an issue)

7 files changed, 103 insertions, 112 deletions

diff --git a/src/iuserinterface.h b/src/iuserinterface.h
index e03bcde2..540839c6 100644
--- a/src/iuserinterface.h
+++ b/src/iuserinterface.h
@@ -34,6 +34,7 @@ public:
   virtual void lock() = 0;

   virtual void unlock() = 0;

   virtual bool unlockClicked() = 0;

+  virtual void setProcessName(QString const &) = 0;

 

 

 };

diff --git a/src/lockeddialog.cpp b/src/lockeddialog.cpp
index 907e3c0a..519abd5b 100644
--- a/src/lockeddialog.cpp
+++ b/src/lockeddialog.cpp
@@ -19,8 +19,11 @@ along with Mod Organizer.  If not, see <http://www.gnu.org/licenses/>.
 

 #include "lockeddialog.h"

 #include "ui_lockeddialog.h"

-#include <QResizeEvent>

 

+#include <QPoint>

+#include <QResizeEvent>

+#include <QWidget>

+#include <Qt>                 // for Qt::FramelessWindowHint, etc

 

 LockedDialog::LockedDialog(QWidget *parent, const QString &text, bool unlockButton)

   : QDialog(parent)

diff --git a/src/lockeddialog.h b/src/lockeddialog.h
index 60af425d..29ac459b 100644
--- a/src/lockeddialog.h
+++ b/src/lockeddialog.h
@@ -20,7 +20,12 @@ along with Mod Organizer.  If not, see <http://www.gnu.org/licenses/>.
 #ifndef LOCKEDDIALOG_H

 #define LOCKEDDIALOG_H

 

-#include <QDialog>

+#include <QDialog>  // for QDialog

+#include <QObject>  // for Q_OBJECT, slots

+#include <QString>  // for QString

+

+class QResizeEvent;

+class QWidget;

 

 namespace Ui {

     class LockedDialog;

@@ -49,6 +54,10 @@ public:
    **/

   bool unlockClicked() const { return m_UnlockClicked; }

 

+  /**

+   * @brief set the name of the process being run

+   * @param name of process

+   */

   void setProcessName(const QString &name);

 

 protected:

diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp
index 096ce94b..702e661d 100644
--- a/src/mainwindow.cpp
+++ b/src/mainwindow.cpp
@@ -1608,18 +1608,31 @@ void MainWindow::storeSettings(QSettings &settings)
 

 void MainWindow::lock()

 {

+  if (m_LockDialog != nullptr) {

+    ++m_LockCount;

+    return;

+  }

   m_LockDialog = new LockedDialog(qApp->activeWindow());

   m_LockDialog->show();

   setEnabled(false);

+  m_LockDialog->setEnabled(true); //What's the point otherwise?

+  ++m_LockCount;

 }

 

 void MainWindow::unlock()

 {

-  if (m_LockDialog != nullptr) {

+  //If you come through here with a null lock pointer, it's a bug!

+  if (m_LockDialog == nullptr) {

+    qDebug("Unlocking main window when already unlocked");

+    return;

+  }

+  --m_LockCount;

+  if (m_LockCount == 0) {

     m_LockDialog->hide();

     m_LockDialog->deleteLater();

+    m_LockDialog = nullptr;

+    setEnabled(true);

   }

-  setEnabled(true);

 }

 

 bool MainWindow::unlockClicked()

@@ -1631,6 +1644,13 @@ bool MainWindow::unlockClicked()
   }

 }

 

+void MainWindow::setProcessName(QString const &name)

+{

+  if (m_LockDialog != nullptr) {

+    m_LockDialog->setProcessName(name);

+  }

+}

+

 void MainWindow::on_btnRefreshData_clicked()

 {

   m_OrganizerCore.refreshDirectoryStructure();

diff --git a/src/mainwindow.h b/src/mainwindow.h
index 177048b4..8e63a14f 100644
--- a/src/mainwindow.h
+++ b/src/mainwindow.h
@@ -118,6 +118,7 @@ public:
   virtual void lock() override;

   virtual void unlock() override;

   virtual bool unlockClicked() override;

+  virtual void setProcessName(QString const &name) override;

 

   bool addProfile();

   void updateBSAList(const QStringList &defaultArchives, const QStringList &activeArchives);

@@ -350,6 +351,7 @@ private:
   bool m_DidUpdateMasterList;

 

   LockedDialog *m_LockDialog { nullptr };

+  uint64_t m_LockCount { 0 };

 

   MOBase::DelayedFileWriter m_ArchiveListWriter;

 

diff --git a/src/organizercore.cpp b/src/organizercore.cpp
index cee367b0..4267573b 100644
--- a/src/organizercore.cpp
+++ b/src/organizercore.cpp
@@ -1,13 +1,19 @@
 #include "organizercore.h"

 

+#include "delayedfilewriter.h"

+#include "guessedvalue.h"

 #include "imodinterface.h"

+#include "imoinfo.h"

 #include "iplugingame.h"

 #include "iuserinterface.h"

 #include "loadmechanism.h"

 #include "messagedialog.h"

 #include "modlistsortproxy.h"

+#include "modrepositoryfileinfo.h"

+#include "nexusinterface.h"

 #include "plugincontainer.h"

 #include "pluginlistsortproxy.h"

+#include "profile.h"

 #include "logbuffer.h"

 #include "credentialsdialog.h"

 #include "filedialogmemory.h"

@@ -26,21 +32,32 @@
 #include <questionboxmemory.h>

 

 #include <QApplication>

+#include <QCoreApplication>

+#include <QDialog>

 #include <QDialogButtonBox>

 #include <QMessageBox>

 #include <QNetworkInterface>

 #include <QProcess>

 #include <QTimer>

+#include <QUrl>

 #include <QWidget>

 

 #include <QtDebug>

+#include <QtGlobal> // for qPrintable, etc

 

 #include <Psapi.h>

+#include <tchar.h> // for _tcsicmp

+

+#include <limits.h>

+#include <stddef.h>

+#include <string.h> // for memset, wcsrchr

 

 #include <exception>

 #include <functional>

 #include <memory>

 #include <set>

+#include <string> //for wstring

+#include <tuple>

 #include <utility>

 

 

@@ -892,66 +909,22 @@ QStringList OrganizerCore::modsSortedByProfilePriority() const
 

 void OrganizerCore::spawnBinary(const QFileInfo &binary, const QString &arguments, const QDir &currentDirectory, bool closeAfterStart, const QString &steamAppID)

 {

-  LockedDialog *dialog = new LockedDialog(qApp->activeWindow());

-  dialog->show();

-  ON_BLOCK_EXIT([&] () { dialog->hide(); dialog->deleteLater(); });

+  if (m_UserInterface != nullptr) {

+    m_UserInterface->lock();

+  }

+  ON_BLOCK_EXIT([&] () {

+    if (m_UserInterface != nullptr) { m_UserInterface->unlock(); }

+  });

 

   HANDLE processHandle = spawnBinaryDirect(binary, arguments, m_CurrentProfile->name(), currentDirectory, steamAppID);

   if (processHandle != INVALID_HANDLE_VALUE) {

-    if (closeAfterStart && (m_UserInterface != nullptr)) {

+    if (closeAfterStart && m_UserInterface != nullptr) {

       m_UserInterface->closeWindow();

     } else {

-      if (m_UserInterface != nullptr) {

-        m_UserInterface->setWindowEnabled(false);

-      }

-      // re-enable the locked dialog because what'd be the point otherwise?

-      dialog->setEnabled(true);

-

-      QCoreApplication::processEvents();

 

       DWORD processExitCode;

-      DWORD retLen;

-      JOBOBJECT_BASIC_PROCESS_ID_LIST info;

+      (void)waitForProcessCompletion(processHandle, &processExitCode);

 

-      {

-        DWORD currentProcess = 0UL;

-        bool isJobHandle = true;

-

-        DWORD res = ::MsgWaitForMultipleObjects(1, &processHandle, false, 1000, QS_KEY | QS_MOUSE);

-        while ((res != WAIT_FAILED) && (res != WAIT_OBJECT_0) && !dialog->unlockClicked()) {

-          if (isJobHandle) {

-            if (::QueryInformationJobObject(processHandle, JobObjectBasicProcessIdList, &info, sizeof(info), &retLen) > 0) {

-              if (info.NumberOfProcessIdsInList == 0) {

-                break;

-              } else {

-                if (info.ProcessIdList[0] != currentProcess) {

-                  currentProcess = info.ProcessIdList[0];

-                  dialog->setProcessName(ToQString(getProcessName(currentProcess)));

-                }

-              }

-            } else {

-              // the info-object I passed only provides space for 1 process id. but since this code only cares about whether there

-              // is more than one that's good enough. ERROR_MORE_DATA simply signals there are at least two processes running.

-              // any other error probably means the handle is a regular process handle, probably caused by running MO in a job without

-              // the right to break out.

-              if (::GetLastError() != ERROR_MORE_DATA) {

-                isJobHandle = false;

-              }

-            }

-          }

-

-          // keep processing events so the app doesn't appear dead

-          QCoreApplication::processEvents();

-

-          res = ::MsgWaitForMultipleObjects(1, &processHandle, false, 1000, QS_KEY | QS_MOUSE);

-        }

-        ::GetExitCodeProcess(processHandle, &processExitCode);

-      }

-      ::CloseHandle(processHandle);

-

-      if (m_UserInterface != nullptr) {

-        m_UserInterface->setWindowEnabled(true);

-      }

       refreshDirectoryStructure();

       // need to remove our stored load order because it may be outdated if a foreign tool changed the

       // file time. After removing that file, refreshESPList will use the file time as the order

@@ -965,6 +938,7 @@ void OrganizerCore::spawnBinary(const QFileInfo &binary, const QString &argument
         savePluginList();

       }

 

+      //These callbacks should not fiddle with directoy structure and ESPs.

       m_FinishedRun(binary.absoluteFilePath(), processExitCode);

     }

   }

@@ -1081,22 +1055,30 @@ bool OrganizerCore::waitForApplication(HANDLE handle, LPDWORD exitCode)
 {

   if (m_UserInterface != nullptr) {

     m_UserInterface->lock();

-    ON_BLOCK_EXIT([&] () { m_UserInterface->unlock(); });

   }

 

-  DWORD retLen;

-  JOBOBJECT_BASIC_PROCESS_ID_LIST info;

+  ON_BLOCK_EXIT([&] () {

+    if (m_UserInterface != nullptr) {

+      m_UserInterface->unlock();

+    } });

+  return waitForProcessCompletion(handle, exitCode);

+}

 

+bool OrganizerCore::waitForProcessCompletion(HANDLE handle, LPDWORD exitCode)

+{

   bool isJobHandle = true;

 

   ULONG lastProcessID = ULONG_MAX;

   HANDLE processHandle = handle;

 

-  DWORD res = ::MsgWaitForMultipleObjects(1, &handle, false, 500, QS_KEY | QS_MOUSE);

-  while ((res != WAIT_FAILED)

-         && (res != WAIT_OBJECT_0)

-         && ((m_UserInterface == nullptr) || !m_UserInterface->unlockClicked())) {

+  DWORD res;

+  //Wait for a an event on the handle, a key press, mouse click or timeout

+  while (res = ::MsgWaitForMultipleObjects(1, &handle, false, 500, QS_KEY | QS_MOUSE),

+         (res != WAIT_FAILED && res != WAIT_OBJECT_0

+          && (m_UserInterface == nullptr || !m_UserInterface->unlockClicked()))) {

     if (isJobHandle) {

+      DWORD retLen;

+      JOBOBJECT_BASIC_PROCESS_ID_LIST info;

       if (::QueryInformationJobObject(handle, JobObjectBasicProcessIdList, &info, sizeof(info), &retLen) > 0) {

         if (info.NumberOfProcessIdsInList == 0) {

           // fake signaled state

@@ -1106,6 +1088,9 @@ bool OrganizerCore::waitForApplication(HANDLE handle, LPDWORD exitCode)
           // this is indeed a job handle. Figure out one of the process handles as well.

           if (lastProcessID != info.ProcessIdList[0]) {

             lastProcessID = info.ProcessIdList[0];

+            if (m_UserInterface != nullptr) {

+              m_UserInterface->setProcessName(ToQString(getProcessName(lastProcessID)));

+            }

             if (processHandle != handle) {

               ::CloseHandle(processHandle);

             }

@@ -1125,14 +1110,22 @@ bool OrganizerCore::waitForApplication(HANDLE handle, LPDWORD exitCode)
 

     // keep processing events so the app doesn't appear dead

     QCoreApplication::processEvents();

-

-    res = ::MsgWaitForMultipleObjects(1, &handle, false, 500, QS_KEY | QS_MOUSE);

   }

 

   if (exitCode != nullptr) {

-    ::GetExitCodeProcess(processHandle, exitCode);

+    //This is actually wrong if the process we started finished before we

+    //got the event and so we end up with a job handle.

+    if (! ::GetExitCodeProcess(processHandle, exitCode))

+    {

+      DWORD error = ::GetLastError();

+      qDebug() << "Failed to get process exit code: Error " << error;

+    }

   }

+

   ::CloseHandle(processHandle);

+  if (handle != processHandle) {

+    ::CloseHandle(handle);

+  }

 

   return res == WAIT_OBJECT_0;

 }

@@ -1599,43 +1592,3 @@ void OrganizerCore::prepareStart() {
   storeSettings();

 }

 

-/*

-std::vector<std::pair<QString, QString>> OrganizerCore::fileMapping()

-{

-  return fileMapping(managedGame()->dataDirectory().absolutePath(),

-                     directoryStructure(),

-                     directoryStructure());

-}

-

-

-std::vector<std::pair<QString, QString>> OrganizerCore::fileMapping(

-    const QString &dataPath,

-    const DirectoryEntry *base,

-    const DirectoryEntry *directoryEntry)

-{

-  std::vector<std::pair<QString, QString>> result;

-

-  for (FileEntry::Ptr current : directoryEntry->getFiles()) {

-    bool isArchive = false;

-    int origin = current->getOrigin(isArchive);

-    if (isArchive || (origin == 0)) {

-      continue;

-    }

-

-    QString fileName = ToQString(current->getRelativePath());

-    QString source = ToQString(base->getOriginByID(origin).getPath()) + fileName;

-    QString target = QDir::toNativeSeparators(dataPath) + fileName;

-    result.push_back(std::make_pair(source, target));

-  }

-

-  // recurse into subdirectories

-  std::vector<DirectoryEntry*>::const_iterator current, end;

-  directoryEntry->getSubDirectories(current, end);

-  for (; current != end; ++current) {

-    std::vector<std::pair<QString, QString>> subRes = fileMapping(dataPath, base, *current);

-    result.insert(result.end(), subRes.begin(), subRes.end());

-  }

-  return result;

-}

-

-*/

diff --git a/src/organizercore.h b/src/organizercore.h
index b50b3e9f..120e83fa 100644
--- a/src/organizercore.h
+++ b/src/organizercore.h
@@ -21,21 +21,28 @@
 class ModListSortProxy;

 class PluginListSortProxy;

 class Profile;

-namespace MOBase { template <typename T> class GuessedValue; }

+namespace MOBase {

+  template <typename T> class GuessedValue;

+  class IModInterface;

+}

 namespace MOShared { class DirectoryEntry; }

 

 #include <QDir>

+#include <QFileInfo>

 #include <QList>

 #include <QObject>

 #include <QSettings>

 #include <QString>

 #include <QStringList>

 #include <QThread>

+#include <QVariant>

 

 class QNetworkReply;

 class QUrl;

 class QWidget;

 

+#include <Windows.h> //for HANDLE, LPDWORD

+

 #include <functional>

 #include <vector>

 

@@ -222,11 +229,7 @@ private:
 

   bool testForSteam();

 

-  /*

-   * std::vector<std::pair<QString, QString>> fileMapping(const QString &dataPath,

-                                                       const MOShared::DirectoryEntry *base,

-                                                       const MOShared::DirectoryEntry *directoryEntry);

-*/

+  bool waitForProcessCompletion(HANDLE handle, LPDWORD exitCode);

 

 private slots: