Implemented major autoreload functionality

* Ignore autoreload on save / save-as
* Consolidated db save code
* Corrected bug (crash) in merge entry code due to not cloning the entry
* Enhanced known modified status of database
* Implemented test cases for autoreload

src/core/Group.cpp

@@ -547,7 +547,7 @@ void Group::merge(const Group* other)
         if (!findEntry(entry->uuid())) {
             entry->clone(Entry::CloneNoFlags)->setGroup(this);
         } else {
-            resolveConflict(this->findEntry(entry->uuid()), entry);
+            resolveConflict(findEntry(entry->uuid()), entry);
         }
     }
 
@@ -555,8 +555,8 @@ void Group::merge(const Group* other)
     const QList<Group*> dbChildren = other->children();
     for (Group* group : dbChildren) {
         // groups are searched by name instead of uuid
-        if (this->findChildByName(group->name())) {
-            this->findChildByName(group->name())->merge(group);
+        if (findChildByName(group->name())) {
+            findChildByName(group->name())->merge(group);
         } else {
             group->setParent(this);
         }
@@ -765,24 +765,24 @@ void Group::resolveConflict(Entry* existingEntry, Entry* otherEntry)
 
     Entry* clonedEntry;
 
-    switch(this->mergeMode()) {
+    switch(mergeMode()) {
         case KeepBoth:
             // if one entry is newer, create a clone and add it to the group
             if (timeExisting > timeOther) {
                 clonedEntry = otherEntry->clone(Entry::CloneNoFlags);
                 clonedEntry->setGroup(this);
-                this->markOlderEntry(clonedEntry);
+                markOlderEntry(clonedEntry);
             } else if (timeExisting < timeOther) {
                 clonedEntry = otherEntry->clone(Entry::CloneNoFlags);
                 clonedEntry->setGroup(this);
-                this->markOlderEntry(existingEntry);
+                markOlderEntry(existingEntry);
             }
             break;
         case KeepNewer:
             if (timeExisting < timeOther) {
                 // only if other entry is newer, replace existing one
-                this->removeEntry(existingEntry);
-                this->addEntry(otherEntry);
+                removeEntry(existingEntry);
+                addEntry(otherEntry->clone(Entry::CloneNoFlags));
             }
 
             break;

src/gui/DatabaseTabWidget.cpp

@@ -221,6 +221,7 @@ void DatabaseTabWidget::importKeePass1Database()
     Database* db = new Database();
     DatabaseManagerStruct dbStruct;
     dbStruct.dbWidget = new DatabaseWidget(db, this);
+    dbStruct.dbWidget->databaseModified();
     dbStruct.modified = true;
 
     insertDatabase(db, dbStruct);
@@ -312,17 +313,28 @@ bool DatabaseTabWidget::closeAllDatabases()
 bool DatabaseTabWidget::saveDatabase(Database* db)
 {
     DatabaseManagerStruct& dbStruct = m_dbList[db];
+    // temporarily disable autoreload
+    dbStruct.dbWidget->ignoreNextAutoreload();
 
     if (dbStruct.saveToFilename) {
         QSaveFile saveFile(dbStruct.canonicalFilePath);
         if (saveFile.open(QIODevice::WriteOnly)) {
+            // write the database to the file
             m_writer.writeDatabase(&saveFile, db);
             if (m_writer.hasError()) {
                 MessageBox::critical(this, tr("Error"), tr("Writing the database failed.") + "\n\n"
                                      + m_writer.errorString());
                 return false;
             }
-            if (!saveFile.commit()) {
+
+            if (saveFile.commit()) {
+                // successfully saved database file
+                dbStruct.modified = false;
+                dbStruct.dbWidget->databaseSaved();
+                updateTabName(db);
+                return true;
+            }
+            else {
                 MessageBox::critical(this, tr("Error"), tr("Writing the database failed.") + "\n\n"
                                      + saveFile.errorString());
                 return false;
@@ -333,10 +345,6 @@ bool DatabaseTabWidget::saveDatabase(Database* db)
                                  + saveFile.errorString());
             return false;
         }
-
-        dbStruct.modified = false;
-        updateTabName(db);
-        return true;
     }
     else {
         return saveDatabaseAs(db);
@@ -390,22 +398,14 @@ bool DatabaseTabWidget::saveDatabaseAs(Database* db)
             }
         }
 
-        QSaveFile saveFile(fileName);
-        if (!saveFile.open(QIODevice::WriteOnly)) {
-            MessageBox::critical(this, tr("Error"), tr("Writing the database failed.") + "\n\n"
-                                 + saveFile.errorString());
-            return false;
-        }
+        // setup variables so saveDatabase succeeds
+        dbStruct.saveToFilename = true;
+        dbStruct.canonicalFilePath = fileName;
 
-        m_writer.writeDatabase(&saveFile, db);
-        if (m_writer.hasError()) {
-            MessageBox::critical(this, tr("Error"), tr("Writing the database failed.") + "\n\n"
-                                 + m_writer.errorString());
-            return false;
-        }
-        if (!saveFile.commit()) {
-            MessageBox::critical(this, tr("Error"), tr("Writing the database failed.") + "\n\n"
-                                 + saveFile.errorString());
+        if (!saveDatabase(db)) {
+            // failed to save, revert back
+            dbStruct.saveToFilename = false;
+            dbStruct.canonicalFilePath = oldFileName;
             return false;
         }
 
@@ -626,7 +626,7 @@ void DatabaseTabWidget::insertDatabase(Database* db, const DatabaseManagerStruct
     setCurrentIndex(index);
     connectDatabase(db);
     connect(dbStruct.dbWidget, SIGNAL(closeRequest()), SLOT(closeDatabaseFromSender()));
-    connect(dbStruct.dbWidget, SIGNAL(databaseChanged(Database*)), SLOT(changeDatabase(Database*)));
+    connect(dbStruct.dbWidget, SIGNAL(databaseChanged(Database*, bool)), SLOT(changeDatabase(Database*, bool)));
     connect(dbStruct.dbWidget, SIGNAL(unlockedDatabase()), SLOT(updateTabNameFromDbWidgetSender()));
     connect(dbStruct.dbWidget, SIGNAL(unlockedDatabase()), SLOT(emitDatabaseUnlockedFromDbWidgetSender()));
 }
@@ -744,6 +744,7 @@ void DatabaseTabWidget::modified()
 
     if (!dbStruct.modified) {
         dbStruct.modified = true;
+        dbStruct.dbWidget->databaseModified();
         updateTabName(db);
     }
 }
@@ -765,7 +766,7 @@ void DatabaseTabWidget::updateLastDatabases(const QString& filename)
     }
 }
 
-void DatabaseTabWidget::changeDatabase(Database* newDb)
+void DatabaseTabWidget::changeDatabase(Database* newDb, bool unsavedChanges)
 {
     Q_ASSERT(sender());
     Q_ASSERT(!m_dbList.contains(newDb));
@@ -773,6 +774,7 @@ void DatabaseTabWidget::changeDatabase(Database* newDb)
     DatabaseWidget* dbWidget = static_cast<DatabaseWidget*>(sender());
     Database* oldDb = databaseFromDatabaseWidget(dbWidget);
     DatabaseManagerStruct dbStruct = m_dbList[oldDb];
+    dbStruct.modified = unsavedChanges;
     m_dbList.remove(oldDb);
     m_dbList.insert(newDb, dbStruct);
 

src/gui/DatabaseTabWidget.h

@@ -91,7 +91,7 @@ private Q_SLOTS:
     void updateTabNameFromDbWidgetSender();
     void modified();
     void toggleTabbar();
-    void changeDatabase(Database* newDb);
+    void changeDatabase(Database* newDb, bool unsavedChanges);
     void emitActivateDatabaseChanged();
     void emitDatabaseUnlockedFromDbWidgetSender();
 

src/gui/DatabaseWidget.cpp

@@ -29,7 +29,6 @@
 #include <QProcess>
 #include <QHeaderView>
 #include <QApplication>
-#include <QtDebug>
 
 #include "autotype/AutoType.h"
 #include "core/Config.h"
@@ -162,9 +161,14 @@ DatabaseWidget::DatabaseWidget(Database* db, QWidget* parent)
 	connect(m_unlockDatabaseDialog, SIGNAL(unlockDone(bool)), SLOT(unlockDatabase(bool)));
     connect(&m_fileWatcher, SIGNAL(fileChanged(QString)), this, SLOT(onWatchedFileChanged()));
     connect(&m_fileWatchTimer, SIGNAL(timeout()), this, SLOT(reloadDatabaseFile()));
+    connect(&m_ignoreWatchTimer, SIGNAL(timeout()), this, SLOT(onWatchedFileChanged()));
     connect(this, SIGNAL(currentChanged(int)), this, SLOT(emitCurrentModeChanged()));
 
+    m_databaseModified = false;
+
     m_fileWatchTimer.setSingleShot(true);
+    m_ignoreWatchTimer.setSingleShot(true);
+    m_ignoreNextAutoreload = false;
 
     m_searchCaseSensitive = false;
     m_searchCurrentGroup = false;
@@ -298,7 +302,7 @@ void DatabaseWidget::replaceDatabase(Database* db)
     Database* oldDb = m_db;
     m_db = db;
     m_groupView->changeDatabase(m_db);
-    Q_EMIT databaseChanged(m_db);
+    Q_EMIT databaseChanged(m_db, m_databaseModified);
     delete oldDb;
 }
 
@@ -818,6 +822,16 @@ void DatabaseWidget::switchToImportKeepass1(const QString& fileName)
     setCurrentWidget(m_keepass1OpenWidget);
 }
 
+void DatabaseWidget::databaseModified()
+{
+    m_databaseModified = true;
+}
+
+void DatabaseWidget::databaseSaved()
+{
+    m_databaseModified = false;
+}
+
 void DatabaseWidget::search(const QString& searchtext)
 {
     if (searchtext.isEmpty())
@@ -956,15 +970,34 @@ void DatabaseWidget::lock()
 
 void DatabaseWidget::updateFilename(const QString& fileName)
 {
+    if (! m_filename.isEmpty()) {
+        m_fileWatcher.removePath(m_filename);
+    }
+
+    m_fileWatcher.addPath(fileName);
     m_filename = fileName;
 }
 
+void DatabaseWidget::ignoreNextAutoreload()
+{
+    m_ignoreNextAutoreload = true;
+    m_ignoreWatchTimer.start(100);
+}
+
 void DatabaseWidget::onWatchedFileChanged()
 {
-    if (m_fileWatchTimer.isActive())
-        return;
+    if (m_ignoreNextAutoreload) {
+        // Reset the watch
+        m_ignoreNextAutoreload = false;
+        m_ignoreWatchTimer.stop();
+        m_fileWatcher.addPath(m_filename);
+    }
+    else {
+        if (m_fileWatchTimer.isActive())
+            return;
 
-    m_fileWatchTimer.start(500);
+        m_fileWatchTimer.start(500);
+    }
 }
 
 void DatabaseWidget::reloadDatabaseFile()
@@ -972,16 +1005,16 @@ void DatabaseWidget::reloadDatabaseFile()
     if (m_db == nullptr)
         return;
 
-    // TODO: Also check if db is currently modified before reloading
     if (! config()->get("AutoReloadOnChange").toBool()) {
         // Ask if we want to reload the db
-        QMessageBox::StandardButton mb = MessageBox::question(this, tr("Reload database file"),
+        QMessageBox::StandardButton mb = MessageBox::question(this, tr("Autoreload Request"),
                              tr("The database file has changed. Do you want to load the changes?"),
                              QMessageBox::Yes | QMessageBox::No);
 
         if (mb == QMessageBox::No) {
-            // TODO: taint database
-
+            // Notify everyone the database does not match the file
+            emit m_db->modified();
+            m_databaseModified = true;
             // Rewatch the database file
             m_fileWatcher.addPath(m_filename);
             return;
@@ -993,14 +1026,37 @@ void DatabaseWidget::reloadDatabaseFile()
     if (file.open(QIODevice::ReadOnly)) {
         Database* db = reader.readDatabase(&file, database()->key());
         if (db != nullptr) {
+            if (m_databaseModified) {
+                // Ask if we want to merge changes into new database
+                QMessageBox::StandardButton mb = MessageBox::question(this, tr("Merge Request"),
+                                     tr("The database file has changed and you have unsaved changes."
+                                        "Do you want to merge your changes?"),
+                                     QMessageBox::Yes | QMessageBox::No);
+
+                if (mb == QMessageBox::Yes) {
+                    // Merge the old database into the new one
+                    m_db->setEmitModified(false);
+                    db->merge(m_db);
+                }
+                else {
+                    // Since we are accepting the new file as-is, internally mark as unmodified
+                    // TODO: when saving is moved out of DatabaseTabWidget, this should be replaced
+                    m_databaseModified = false;
+                }
+            }
+
             replaceDatabase(db);
         }
         else {
-            // TODO: error message for failure to read the new db
+            MessageBox::critical(this, tr("Autoreload Failed"),
+                                 tr("Could not parse or unlock the new database file while attempting"
+                                    "to autoreload this database."));
         }
     }
     else {
-        // TODO: error message for failure to open db file
+        MessageBox::critical(this, tr("Autoreload Failed"),
+                             tr("Could not open the new database file while attempting to autoreload"
+                                "this database."));
     }
 
     // Rewatch the database file

src/gui/DatabaseWidget.h

@@ -92,13 +92,14 @@ public:
     EntryView* entryView();
     void showUnlockDialog();
     void closeUnlockDialog();
+    void ignoreNextAutoreload();
 
 Q_SIGNALS:
     void closeRequest();
     void currentModeChanged(DatabaseWidget::Mode mode);
     void groupChanged();
     void entrySelectionChanged();
-    void databaseChanged(Database* newDb);
+    void databaseChanged(Database* newDb, bool unsavedChanges);
     void databaseMerged(Database* mergedDb);
     void groupContextMenuRequested(const QPoint& globalPos);
     void entryContextMenuRequested(const QPoint& globalPos);
@@ -136,6 +137,8 @@ public Q_SLOTS:
     void switchToOpenMergeDatabase(const QString& fileName);
     void switchToOpenMergeDatabase(const QString& fileName, const QString& password, const QString& keyFile);
     void switchToImportKeepass1(const QString& fileName);
+    void databaseModified();
+    void databaseSaved();
     // Search related slots
     void search(const QString& searchtext);
     void setSearchCaseSensitive(bool state);
@@ -194,8 +197,12 @@ private:
     bool m_searchCaseSensitive;
     bool m_searchCurrentGroup;
 
+    // Autoreload
     QFileSystemWatcher m_fileWatcher;
     QTimer m_fileWatchTimer;
+    bool m_ignoreNextAutoreload;
+    QTimer m_ignoreWatchTimer;
+    bool m_databaseModified;
 };
 
 #endif // KEEPASSX_DATABASEWIDGET_H

src/gui/SettingsWidgetGeneral.ui

@@ -7,7 +7,7 @@
     <x>0</x>
     <y>0</y>
     <width>684</width>
-    <height>430</height>
+    <height>459</height>
    </rect>
   </property>
   <layout class="QFormLayout" name="formLayout">
@@ -58,7 +58,7 @@
    <item row="5" column="0">
     <widget class="QCheckBox" name="autoReloadOnChangeCheckBox">
      <property name="text">
-      <string>Automatically reload when the database is expernally modified</string>
+      <string>Automatically reload the database when modified externally</string>
      </property>
     </widget>
    </item>

tests/gui/TestGui.cpp

@@ -65,20 +65,20 @@ void TestGui::initTestCase()
     QByteArray tmpData;
     QFile sourceDbFile(QString(KEEPASSX_TEST_DATA_DIR).append("/NewDatabase.kdbx"));
     QVERIFY(sourceDbFile.open(QIODevice::ReadOnly));
-    QVERIFY(Tools::readAllFromDevice(&sourceDbFile, tmpData));
+    QVERIFY(Tools::readAllFromDevice(&sourceDbFile, m_dbData));
     sourceDbFile.close();
-
-    // Write the temp storage to a temp database file for use in our tests
-    QVERIFY(m_dbFile.open());
-    QCOMPARE(m_dbFile.write(tmpData), static_cast<qint64>((tmpData.size())));
-    m_dbFile.close();
-
-    m_dbFileName = QFileInfo(m_dbFile).fileName();
 }
 
 // Every test starts with opening the temp database
 void TestGui::init()
 {
+    // Write the temp storage to a temp database file for use in our tests
+    QVERIFY(m_dbFile.open());
+    QCOMPARE(m_dbFile.write(m_dbData), static_cast<qint64>((m_dbData.size())));
+    m_dbFile.close();
+
+    m_dbFileName = QFileInfo(m_dbFile).fileName();
+
     fileDialog()->setNextFileName(m_dbFile.fileName());
     triggerAction("actionDatabaseOpen");
 
@@ -110,8 +110,8 @@ void TestGui::cleanup()
 
 void TestGui::testMergeDatabase()
 {
-    // this triggers a warning. Perhaps similar to https://bugreports.qt.io/browse/QTBUG-49623 ?
-    QSignalSpy dbMergeSpy(m_tabWidget->currentWidget(), SIGNAL(databaseMerged(Database*)));
+    // It is safe to ignore the warning this line produces
+    QSignalSpy dbMergeSpy(m_dbWidget, SIGNAL(databaseMerged(Database*)));
 
     // set file to merge from
     fileDialog()->setNextFileName(QString(KEEPASSX_TEST_DATA_DIR).append("/MergeDatabase.kdbx"));
@@ -139,6 +139,74 @@ void TestGui::testMergeDatabase()
     QCOMPARE(m_db->rootGroup()->findChildByName("General")->entries().size(), 1);
 }
 
+void TestGui::testAutoreloadDatabase()
+{
+    config()->set("AutoReloadOnChange", false);
+
+    // Load the MergeDatabase.kdbx file into temporary storage
+    QByteArray tmpData;
+    QFile mergeDbFile(QString(KEEPASSX_TEST_DATA_DIR).append("/MergeDatabase.kdbx"));
+    QVERIFY(mergeDbFile.open(QIODevice::ReadOnly));
+    QVERIFY(Tools::readAllFromDevice(&mergeDbFile, tmpData));
+    mergeDbFile.close();
+
+    // Test accepting new file in autoreload
+    MessageBox::setNextAnswer(QMessageBox::Yes);
+    // Overwrite the current database with the temp data
+    QVERIFY(m_dbFile.open());
+    QVERIFY(m_dbFile.write(tmpData, static_cast<qint64>(tmpData.size())));
+    m_dbFile.close();
+    Tools::wait(1500);
+
+    m_db = m_dbWidget->database();
+
+    // the General group contains one entry from the new db data
+    QCOMPARE(m_db->rootGroup()->findChildByName("General")->entries().size(), 1);
+    QVERIFY(! m_tabWidget->tabText(m_tabWidget->currentIndex()).endsWith("*"));
+
+    // Reset the state
+    cleanup();
+    init();
+
+    // Test rejecting new file in autoreload
+    MessageBox::setNextAnswer(QMessageBox::No);
+    // Overwrite the current temp database with a new file
+    m_dbFile.open();
+    QVERIFY(m_dbFile.write(tmpData, static_cast<qint64>(tmpData.size())));
+    m_dbFile.close();
+    Tools::wait(1500);
+
+    m_db = m_dbWidget->database();
+
+    // Ensure the merge did not take place
+    QCOMPARE(m_db->rootGroup()->findChildByName("General")->entries().size(), 0);
+    QVERIFY(m_tabWidget->tabText(m_tabWidget->currentIndex()).endsWith("*"));
+
+    // Reset the state
+    cleanup();
+    init();
+
+     // Test accepting a merge of edits into autoreload
+    // Turn on autoload so we only get one messagebox (for the merge)
+    config()->set("AutoReloadOnChange", true);
+
+    // Modify some entries
+    testEditEntry();
+
+    // This is saying yes to merging the entries
+    MessageBox::setNextAnswer(QMessageBox::Yes);
+    // Overwrite the current database with the temp data
+    QVERIFY(m_dbFile.open());
+    QVERIFY(m_dbFile.write(tmpData, static_cast<qint64>(tmpData.size())));
+    m_dbFile.close();
+    Tools::wait(1500);
+
+    m_db = m_dbWidget->database();
+
+    QCOMPARE(m_db->rootGroup()->findChildByName("General")->entries().size(), 1);
+    QVERIFY(m_tabWidget->tabText(m_tabWidget->currentIndex()).endsWith("*"));
+}
+
 void TestGui::testTabs()
 {
     QCOMPARE(m_tabWidget->count(), 1);

tests/gui/TestGui.h

@@ -39,6 +39,7 @@ private Q_SLOTS:
     void cleanupTestCase();
 
     void testMergeDatabase();
+    void testAutoreloadDatabase();
     void testTabs();
     void testEditEntry();
     void testAddEntry();
@@ -64,6 +65,7 @@ private:
     MainWindow* m_mainWindow;
     DatabaseTabWidget* m_tabWidget;
     DatabaseWidget* m_dbWidget;
+    QByteArray m_dbData;
     QTemporaryFile m_dbFile;
     QString m_dbFileName;
     Database* m_db;