diff --git a/src/core/Group.cpp b/src/core/Group.cpp index eb293a935..2028c7828 100644 --- a/src/core/Group.cpp +++ b/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 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; diff --git a/src/gui/DatabaseTabWidget.cpp b/src/gui/DatabaseTabWidget.cpp index d4001501d..af6907001 100644 --- a/src/gui/DatabaseTabWidget.cpp +++ b/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(sender()); Database* oldDb = databaseFromDatabaseWidget(dbWidget); DatabaseManagerStruct dbStruct = m_dbList[oldDb]; + dbStruct.modified = unsavedChanges; m_dbList.remove(oldDb); m_dbList.insert(newDb, dbStruct); diff --git a/src/gui/DatabaseTabWidget.h b/src/gui/DatabaseTabWidget.h index 7d095b560..24bdbde2f 100644 --- a/src/gui/DatabaseTabWidget.h +++ b/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(); diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index bf620b6d5..89e7bf689 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -29,7 +29,6 @@ #include #include #include -#include #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 diff --git a/src/gui/DatabaseWidget.h b/src/gui/DatabaseWidget.h index 0f52ea08d..1031def44 100644 --- a/src/gui/DatabaseWidget.h +++ b/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 diff --git a/src/gui/SettingsWidgetGeneral.ui b/src/gui/SettingsWidgetGeneral.ui index 223e2b9af..dabc1eea4 100644 --- a/src/gui/SettingsWidgetGeneral.ui +++ b/src/gui/SettingsWidgetGeneral.ui @@ -7,7 +7,7 @@ 0 0 684 - 430 + 459 @@ -58,7 +58,7 @@ - Automatically reload when the database is expernally modified + Automatically reload the database when modified externally diff --git a/tests/data/NewDatabase.kdbx b/tests/data/NewDatabase.kdbx index f45929de2..4e77724c7 100644 Binary files a/tests/data/NewDatabase.kdbx and b/tests/data/NewDatabase.kdbx differ diff --git a/tests/gui/TestGui.cpp b/tests/gui/TestGui.cpp index b25a09fdc..829e669b6 100644 --- a/tests/gui/TestGui.cpp +++ b/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((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((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(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(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(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); diff --git a/tests/gui/TestGui.h b/tests/gui/TestGui.h index 82ffc1850..02e8da1d3 100644 --- a/tests/gui/TestGui.h +++ b/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;