Merge pull request #748 from GriffinRichards/fix-file-watcher

Condense file watcher warning, reduce resource usage

Author: GriffinRichards

include/project.h

@@ -61,7 +61,6 @@ class Project : public QObject
     QMap<QString, uint32_t> metatileBehaviorMap;
     QMap<uint32_t, QString> metatileBehaviorMapInverse;
     ParseUtil parser;
-    QFileSystemWatcher fileWatcher;
     QSet<QString> modifiedFiles;
     bool usingAsmTilesets;
     QSet<QString> disabledSettingsNames;
@@ -263,6 +262,7 @@ class Project : public QObject
     static QString getMapGroupPrefix();
 
 private:
+    QPointer<QFileSystemWatcher> fileWatcher;
     QMap<QString, qint64> modifiedFileTimestamps;
     QMap<QString, QString> facingDirections;
     QHash<QString, QString> speciesToIconPath;
@@ -332,6 +332,8 @@ class Project : public QObject
     void ignoreWatchedFilesTemporarily(const QStringList &filepaths);
     void recordFileChange(const QString &filepath);
     void resetFileCache();
+    void resetFileWatcher();
+    void logFileWatchStatus();
 
     bool saveMapLayouts();
     bool saveMapGroups();

include/ui/preferenceeditor.h

@@ -24,6 +24,7 @@ class PreferenceEditor : public QMainWindow
     void preferencesSaved();
     void themeChanged(const QString &theme);
     void scriptSettingsChanged(bool on);
+    void reloadProjectRequested();
 
 private:
     Ui::PreferenceEditor *ui;

src/core/map.cpp

@@ -163,17 +163,19 @@ QStringList Map::getScriptLabels(Event::Group group) {
             m_loggedScriptsFileError = true;
         }
 
-        if (!m_scriptFileWatcher) {
-            // Only create the file watcher when it's first needed (even an empty QFileSystemWatcher will consume system resources).
-            // The other option would be for Porymap to have a single global QFileSystemWatcher, but that has complications of its own.
-            m_scriptFileWatcher = new QFileSystemWatcher(this);
-            connect(m_scriptFileWatcher, &QFileSystemWatcher::fileChanged, this, &Map::invalidateScripts);
-        }
-        if (!m_scriptFileWatcher->files().contains(scriptsFilepath) && !m_scriptFileWatcher->addPath(scriptsFilepath) && !m_loggedScriptsFileError) {
-            logWarn(QString("Failed to add scripts file '%1' to file watcher for %2.")
-                            .arg(Util::stripPrefix(scriptsFilepath, projectConfig.projectDir() + "/"))
-                            .arg(m_name));
-            m_loggedScriptsFileError = true;
+        if (porymapConfig.monitorFiles) {
+            if (!m_scriptFileWatcher) {
+                // Only create the file watcher when it's first needed (even an empty QFileSystemWatcher will consume system resources).
+                // The other option would be for Porymap to have a single global QFileSystemWatcher, but that has complications of its own.
+                m_scriptFileWatcher = new QFileSystemWatcher(this);
+                connect(m_scriptFileWatcher, &QFileSystemWatcher::fileChanged, this, &Map::invalidateScripts);
+            }
+            if (!m_scriptFileWatcher->files().contains(scriptsFilepath) && !m_scriptFileWatcher->addPath(scriptsFilepath) && !m_loggedScriptsFileError) {
+                logWarn(QString("Failed to add scripts file '%1' to file watcher for %2.")
+                                .arg(Util::stripPrefix(scriptsFilepath, projectConfig.projectDir() + "/"))
+                                .arg(m_name));
+                m_loggedScriptsFileError = true;
+            }
         }
 
         m_scriptsLoaded = true;

src/mainwindow.cpp

@@ -2938,6 +2938,7 @@ void MainWindow::on_actionPreferences_triggered() {
         // require us to repopulate the EventFrames and redraw event pixmaps, respectively.
         connect(preferenceEditor, &PreferenceEditor::preferencesSaved, editor, &Editor::updateEvents);
         connect(preferenceEditor, &PreferenceEditor::scriptSettingsChanged, editor->project, &Project::readEventScriptLabels);
+        connect(preferenceEditor, &PreferenceEditor::reloadProjectRequested, this, &MainWindow::on_action_Reload_Project_triggered);
     }
 
     openSubWindow(preferenceEditor);

src/project.cpp

@@ -32,9 +32,7 @@ int Project::num_pals_total = 13;
 
 Project::Project(QObject *parent) :
     QObject(parent)
-{
-    QObject::connect(&this->fileWatcher, &QFileSystemWatcher::fileChanged, this, &Project::recordFileChange);
-}
+{ }
 
 Project::~Project()
 {
@@ -186,6 +184,7 @@ int Project::getSupportedMajorVersion(QString *errorOut) {
 
 bool Project::load() {
     this->parser.setUpdatesSplashScreen(true);
+    resetFileWatcher();
     resetFileCache();
     this->disabledSettingsNames.clear();
     bool success = readGlobalConstants()
@@ -225,14 +224,14 @@ bool Project::load() {
         initNewLayoutSettings();
         initNewMapSettings();
         applyParsedLimits();
+        logFileWatchStatus();
     }
     this->parser.setUpdatesSplashScreen(false);
     return success;
 }
 
 void Project::resetFileCache() {
     this->parser.clearFileCache();
-    this->failedFileWatchPaths.clear();
 
     const QSet<QString> filepaths = {
         // Whenever we load a tileset we'll need to parse some data from these files, so we cache them to avoid the overhead of opening the files.
@@ -749,16 +748,21 @@ bool Project::saveMapLayouts() {
 }
 
 bool Project::watchFile(const QString &filename) {
+    if (!porymapConfig.monitorFiles)
+        return true;
+
+    if (!this->fileWatcher) {
+        // Only create the file watcher when it's first needed (even an empty QFileSystemWatcher will consume system resources).
+        this->fileWatcher = new QFileSystemWatcher(this);
+        QObject::connect(this->fileWatcher, &QFileSystemWatcher::fileChanged, this, &Project::recordFileChange);
+    }
+
     QString filepath = filename.startsWith(this->root) ? filename : QString("%1/%2").arg(this->root).arg(filename);
-    if (!this->fileWatcher.addPath(filepath) && !this->fileWatcher.files().contains(filepath)) {
+    if (!this->fileWatcher->addPath(filepath) && !this->fileWatcher->files().contains(filepath)) {
         // We failed to watch the file, and this wasn't a file we were already watching.
-        // Log a warning, but only if A. we actually care that we failed, because 'monitor files' is enabled,
-        // B. we haven't logged a warning for this file yet, and C. we would have otherwise been able to watch it, because the file exists.
-        if (porymapConfig.monitorFiles && !this->failedFileWatchPaths.contains(filepath) && QFileInfo::exists(filepath)) {
+        // Record the filepath for logging later, assuming we should have been able to watch the file.
+        if (QFileInfo::exists(filepath)) {
             this->failedFileWatchPaths.insert(filepath);
-            logWarn(QString("Failed to add '%1' to file watcher. Currently watching %2 files.")
-                            .arg(Util::stripPrefix(filepath, this->root))
-                            .arg(this->fileWatcher.files().length()));
         }
         return false;
     }
@@ -774,8 +778,11 @@ bool Project::watchFiles(const QStringList &filenames) {
 }
 
 bool Project::stopFileWatch(const QString &filename) {
+    if (!this->fileWatcher)
+        return true;
+
     QString filepath = filename.startsWith(this->root) ? filename : QString("%1/%2").arg(this->root).arg(filename);
-    return this->fileWatcher.removePath(filepath);
+    return this->fileWatcher->removePath(filepath);
 }
 
 void Project::ignoreWatchedFileTemporarily(const QString &filepath) {
@@ -794,8 +801,8 @@ void Project::recordFileChange(const QString &filepath) {
     // Note: As a safety measure, many applications save an open file by writing a new file and then deleting the old one.
     //       In your slot function, you can check watcher.files().contains(path).
     //       If it returns false, check whether the file still exists and then call addPath() to continue watching it.
-    if (!this->fileWatcher.files().contains(filepath) && QFileInfo::exists(filepath)) {
-        this->fileWatcher.addPath(filepath);
+    if (this->fileWatcher && !this->fileWatcher->files().contains(filepath) && QFileInfo::exists(filepath)) {
+        this->fileWatcher->addPath(filepath);
     }
 
     if (this->modifiedFiles.contains(filepath)) {
@@ -815,6 +822,38 @@ void Project::recordFileChange(const QString &filepath) {
     emit fileChanged(filepath);
 }
 
+// When calling 'watchFile' we record failures rather than log them immediately.
+// We do this primarily to condense the warning if we fail to monitor any files.
+void Project::logFileWatchStatus() {
+    if (!this->fileWatcher)
+        return;
+
+    int numSuccessful = this->fileWatcher->files().length();
+    int numAttempted = numSuccessful + this->failedFileWatchPaths.count();
+    if (numAttempted == 0)
+        return;
+
+    if (numSuccessful == 0) {
+        // We failed to watch every file we tried. As of writing this happens if Porymap is running
+        // on Windows and the project files are in WSL2. Rather than filling the log by
+        // outputting a warning for every file, just log that we failed to monitor any of them.
+        logWarn(QString("Failed to monitor project files"));
+        return;
+    } else {
+        logInfo(QString("Successfully monitoring %1/%2 project files").arg(numSuccessful).arg(numAttempted));
+    }
+
+    for (const auto &failedPath : this->failedFileWatchPaths) {
+        logWarn(QString("Failed to monitor project file '%1'").arg(failedPath));
+    }
+}
+
+void Project::resetFileWatcher() {
+    this->failedFileWatchPaths.clear();
+    delete this->fileWatcher;
+    this->fileWatcher = nullptr;
+}
+
 bool Project::saveMapGroups() {
     QString mapGroupsFilepath = QString("%1/%2").arg(root).arg(projectConfig.getFilePath(ProjectFilePath::json_map_groups));
     QFile mapGroupsFile(mapGroupsFilepath);

src/ui/preferenceeditor.cpp

@@ -2,6 +2,7 @@
 #include "ui_preferenceeditor.h"
 #include "config.h"
 #include "noscrollcombobox.h"
+#include "message.h"
 
 #include <QAbstractButton>
 #include <QRegularExpression>
@@ -87,6 +88,8 @@ void PreferenceEditor::updateFields() {
 }
 
 void PreferenceEditor::saveFields() {
+    bool needsProjectReload = false;
+
     bool changedTheme = false;
     if (themeSelector->currentText() != porymapConfig.theme) {
         porymapConfig.theme = themeSelector->currentText();
@@ -100,7 +103,6 @@ void PreferenceEditor::saveFields() {
     porymapConfig.eventSelectionShapeMode = ui->radioButton_OnSprite->isChecked() ? QGraphicsPixmapItem::MaskShape : QGraphicsPixmapItem::BoundingRectShape;
     porymapConfig.textEditorOpenFolder = ui->lineEdit_TextEditorOpenFolder->text();
     porymapConfig.textEditorGotoLine = ui->lineEdit_TextEditorGotoLine->text();
-    porymapConfig.monitorFiles = ui->checkBox_MonitorProjectFiles->isChecked();
     porymapConfig.reopenOnLaunch = ui->checkBox_OpenRecentProject->isChecked();
     porymapConfig.checkForUpdates = ui->checkBox_CheckForUpdates->isChecked();
     porymapConfig.eventDeleteWarningDisabled = ui->checkBox_DisableEventWarning->isChecked();
@@ -110,6 +112,11 @@ void PreferenceEditor::saveFields() {
     if (ui->checkBox_StatusWarnings->isChecked()) porymapConfig.statusBarLogTypes.insert(LogType::LOG_WARN);
     if (ui->checkBox_StatusInformation->isChecked()) porymapConfig.statusBarLogTypes.insert(LogType::LOG_INFO);
 
+    if (porymapConfig.monitorFiles != ui->checkBox_MonitorProjectFiles->isChecked()) {
+        porymapConfig.monitorFiles = ui->checkBox_MonitorProjectFiles->isChecked();
+        needsProjectReload = true;
+    }
+
     if (porymapConfig.applicationFont != this->applicationFont) {
         porymapConfig.applicationFont = this->applicationFont;
         changedTheme = true;
@@ -119,12 +126,18 @@ void PreferenceEditor::saveFields() {
         changedTheme = true;
     }
 
-    porymapConfig.save();
+    if (changedTheme) {
+        emit themeChanged(porymapConfig.theme);
+    }
 
+    porymapConfig.save();
     emit preferencesSaved();
 
-    if (changedTheme) {
-        emit themeChanged(porymapConfig.theme);
+    if (needsProjectReload) {
+        auto message = QStringLiteral("Some changes will only take effect after reloading the project. Reload the project now?");
+        if (QuestionMessage::show(message, this) == QMessageBox::Yes) {
+            emit reloadProjectRequested();
+        }
     }
 }