From c538f0b907aaae5b8033b9bd370e7d9ea58488de Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Sat, 1 Aug 2020 18:00:47 -0400 Subject: [PATCH] Fixup saving non-data changes on database lock * Fix #5107 * Change setting for non-data changes to Auto save on database lock (or not) instead of marking modified. * When enabled, database will be auto-saved if there are only non-data changes, but will not prompt the user if saving has failed. * When disabled, database will not auto-save if there are only non-data changes (same behavior as 2.5 and below) and will not mark the database dirty. --- src/core/Config.cpp | 3 +- src/core/Config.h | 2 +- src/core/Database.cpp | 9 ++- src/core/Database.h | 3 +- src/core/Group.cpp | 18 +---- src/gui/ApplicationSettingsWidget.cpp | 10 ++- src/gui/ApplicationSettingsWidgetGeneral.ui | 8 +- src/gui/DatabaseWidget.cpp | 85 ++++++++++----------- src/gui/DatabaseWidget.h | 1 + tests/TestConfig.cpp | 1 - 10 files changed, 66 insertions(+), 74 deletions(-) diff --git a/src/core/Config.cpp b/src/core/Config.cpp index 6d55df8c7..afb71f534 100644 --- a/src/core/Config.cpp +++ b/src/core/Config.cpp @@ -58,6 +58,7 @@ static const QHash configStrings = { {Config::AutoSaveAfterEveryChange,{QS("AutoSaveAfterEveryChange"), Roaming, true}}, {Config::AutoReloadOnChange,{QS("AutoReloadOnChange"), Roaming, true}}, {Config::AutoSaveOnExit,{QS("AutoSaveOnExit"), Roaming, true}}, + {Config::AutoSaveNonDataChanges,{QS("AutoSaveNonDataChanges"), Roaming, true}}, {Config::BackupBeforeSave,{QS("BackupBeforeSave"), Roaming, false}}, {Config::UseAtomicSaves,{QS("UseAtomicSaves"), Roaming, true}}, {Config::SearchLimitGroup,{QS("SearchLimitGroup"), Roaming, false}}, @@ -73,7 +74,6 @@ static const QHash configStrings = { {Config::AutoTypeStartDelay,{QS("AutoTypeStartDelay"), Roaming, 500}}, {Config::GlobalAutoTypeKey,{QS("GlobalAutoTypeKey"), Roaming, 0}}, {Config::GlobalAutoTypeModifiers,{QS("GlobalAutoTypeModifiers"), Roaming, 0}}, - {Config::TrackNonDataChanges,{QS("TrackNonDataChanges"), Roaming, false}}, {Config::FaviconDownloadTimeout,{QS("FaviconDownloadTimeout"), Roaming, 10}}, {Config::UpdateCheckMessageShown,{QS("UpdateCheckMessageShown"), Roaming, false}}, {Config::UseTouchID,{QS("UseTouchID"), Roaming, false}}, @@ -297,7 +297,6 @@ static const QHash deprecationMap = { {QS("security/IconDownloadFallbackToGoogle"), Config::Security_IconDownloadFallback}, // 2.6.0 - {QS("IgnoreGroupExpansion"), Config::TrackNonDataChanges}, {QS("security/autotypeask"), Config::Security_AutoTypeAsk}, {QS("security/clearclipboard"), Config::Security_ClearClipboard}, {QS("security/clearclipboardtimeout"), Config::Security_ClearClipboardTimeout}, diff --git a/src/core/Config.h b/src/core/Config.h index 9363873c9..64e8c945a 100644 --- a/src/core/Config.h +++ b/src/core/Config.h @@ -42,6 +42,7 @@ public: AutoSaveAfterEveryChange, AutoReloadOnChange, AutoSaveOnExit, + AutoSaveNonDataChanges, BackupBeforeSave, UseAtomicSaves, SearchLimitGroup, @@ -57,7 +58,6 @@ public: AutoTypeStartDelay, GlobalAutoTypeKey, GlobalAutoTypeModifiers, - TrackNonDataChanges, FaviconDownloadTimeout, UpdateCheckMessageShown, UseTouchID, diff --git a/src/core/Database.cpp b/src/core/Database.cpp index a315adf96..ccd0ffb21 100644 --- a/src/core/Database.cpp +++ b/src/core/Database.cpp @@ -848,9 +848,14 @@ void Database::setEmitModified(bool value) m_emitModified = value; } -bool Database::isModified(bool includeNonDataChanges) const +bool Database::isModified() const { - return m_modified || (includeNonDataChanges && m_hasNonDataChange); + return m_modified; +} + +bool Database::hasNonDataChanges() const +{ + return m_hasNonDataChange; } void Database::markAsModified() diff --git a/src/core/Database.h b/src/core/Database.h index 86d7e0898..88a096473 100644 --- a/src/core/Database.h +++ b/src/core/Database.h @@ -81,7 +81,8 @@ public: void releaseData(); bool isInitialized() const; - bool isModified(bool includeNonDataChanges = false) const; + bool isModified() const; + bool hasNonDataChanges() const; void setEmitModified(bool value); bool isReadOnly() const; void setReadOnly(bool readOnly); diff --git a/src/core/Group.cpp b/src/core/Group.cpp index d9782a7cc..8be8b5f95 100644 --- a/src/core/Group.cpp +++ b/src/core/Group.cpp @@ -362,11 +362,7 @@ void Group::setExpanded(bool expanded) { if (m_data.isExpanded != expanded) { m_data.isExpanded = expanded; - if (config()->get(Config::TrackNonDataChanges).toBool()) { - emit groupModified(); - } else { - emit groupNonDataChange(); - } + emit groupNonDataChange(); } } @@ -972,11 +968,7 @@ void Group::moveEntryUp(Entry* entry) emit entryAboutToMoveUp(row); m_entries.move(row, row - 1); emit entryMovedUp(); - if (config()->get(Config::TrackNonDataChanges).toBool()) { - emit groupModified(); - } else { - emit groupNonDataChange(); - } + emit groupNonDataChange(); } void Group::moveEntryDown(Entry* entry) @@ -989,11 +981,7 @@ void Group::moveEntryDown(Entry* entry) emit entryAboutToMoveDown(row); m_entries.move(row, row + 1); emit entryMovedDown(); - if (config()->get(Config::TrackNonDataChanges).toBool()) { - emit groupModified(); - } else { - emit groupNonDataChange(); - } + emit groupNonDataChange(); } void Group::connectDatabaseSignalsRecursive(Database* db) diff --git a/src/gui/ApplicationSettingsWidget.cpp b/src/gui/ApplicationSettingsWidget.cpp index 3f84b7654..cf09df39b 100644 --- a/src/gui/ApplicationSettingsWidget.cpp +++ b/src/gui/ApplicationSettingsWidget.cpp @@ -184,6 +184,7 @@ void ApplicationSettingsWidget::loadSettings() config()->get(Config::OpenPreviousDatabasesOnStartup).toBool()); m_generalUi->autoSaveAfterEveryChangeCheckBox->setChecked(config()->get(Config::AutoSaveAfterEveryChange).toBool()); m_generalUi->autoSaveOnExitCheckBox->setChecked(config()->get(Config::AutoSaveOnExit).toBool()); + m_generalUi->autoSaveNonDataChangesCheckBox->setChecked(config()->get(Config::AutoSaveNonDataChanges).toBool()); m_generalUi->backupBeforeSaveCheckBox->setChecked(config()->get(Config::BackupBeforeSave).toBool()); m_generalUi->useAtomicSavesCheckBox->setChecked(config()->get(Config::UseAtomicSaves).toBool()); m_generalUi->autoReloadOnChangeCheckBox->setChecked(config()->get(Config::AutoReloadOnChange).toBool()); @@ -197,7 +198,6 @@ void ApplicationSettingsWidget::loadSettings() config()->get(Config::UseGroupIconOnEntryCreation).toBool()); m_generalUi->autoTypeEntryTitleMatchCheckBox->setChecked(config()->get(Config::AutoTypeEntryTitleMatch).toBool()); m_generalUi->autoTypeEntryURLMatchCheckBox->setChecked(config()->get(Config::AutoTypeEntryURLMatch).toBool()); - m_generalUi->trackNonDataChangesCheckBox->setChecked(config()->get(Config::TrackNonDataChanges).toBool()); m_generalUi->faviconTimeoutSpinBox->setValue(config()->get(Config::FaviconDownloadTimeout).toInt()); m_generalUi->languageComboBox->clear(); @@ -312,6 +312,7 @@ void ApplicationSettingsWidget::saveSettings() m_generalUi->openPreviousDatabasesOnStartupCheckBox->isChecked()); config()->set(Config::AutoSaveAfterEveryChange, m_generalUi->autoSaveAfterEveryChangeCheckBox->isChecked()); config()->set(Config::AutoSaveOnExit, m_generalUi->autoSaveOnExitCheckBox->isChecked()); + config()->set(Config::AutoSaveNonDataChanges, m_generalUi->autoSaveNonDataChangesCheckBox->isChecked()); config()->set(Config::BackupBeforeSave, m_generalUi->backupBeforeSaveCheckBox->isChecked()); config()->set(Config::UseAtomicSaves, m_generalUi->useAtomicSavesCheckBox->isChecked()); config()->set(Config::AutoReloadOnChange, m_generalUi->autoReloadOnChangeCheckBox->isChecked()); @@ -321,7 +322,6 @@ void ApplicationSettingsWidget::saveSettings() config()->set(Config::MinimizeOnCopy, m_generalUi->minimizeOnCopyRadioButton->isChecked()); config()->set(Config::DropToBackgroundOnCopy, m_generalUi->dropToBackgroundOnCopyRadioButton->isChecked()); config()->set(Config::UseGroupIconOnEntryCreation, m_generalUi->useGroupIconOnEntryCreationCheckBox->isChecked()); - config()->set(Config::TrackNonDataChanges, m_generalUi->trackNonDataChangesCheckBox->isChecked()); config()->set(Config::AutoTypeEntryTitleMatch, m_generalUi->autoTypeEntryTitleMatchCheckBox->isChecked()); config()->set(Config::AutoTypeEntryURLMatch, m_generalUi->autoTypeEntryURLMatchCheckBox->isChecked()); config()->set(Config::FaviconDownloadTimeout, m_generalUi->faviconTimeoutSpinBox->value()); @@ -451,11 +451,13 @@ void ApplicationSettingsWidget::reject() void ApplicationSettingsWidget::autoSaveToggled(bool checked) { - // Explicitly enable auto-save on exit if it wasn't already - if (checked && !m_generalUi->autoSaveOnExitCheckBox->isChecked()) { + // Explicitly enable other auto-save options + if (checked) { m_generalUi->autoSaveOnExitCheckBox->setChecked(true); + m_generalUi->autoSaveNonDataChangesCheckBox->setChecked(true); } m_generalUi->autoSaveOnExitCheckBox->setEnabled(!checked); + m_generalUi->autoSaveNonDataChangesCheckBox->setEnabled(!checked); } void ApplicationSettingsWidget::hideWindowOnCopyCheckBoxToggled(bool checked) diff --git a/src/gui/ApplicationSettingsWidgetGeneral.ui b/src/gui/ApplicationSettingsWidgetGeneral.ui index 8d5ba8ad7..197d25b20 100644 --- a/src/gui/ApplicationSettingsWidgetGeneral.ui +++ b/src/gui/ApplicationSettingsWidgetGeneral.ui @@ -253,14 +253,14 @@ - Automatically save on exit + Automatically save when locking database - + - Mark database as modified for non-data changes (e.g., expanding groups) + Automatically save non-data changes when locking database @@ -1037,7 +1037,7 @@ checkForUpdatesIncludeBetasCheckBox autoSaveAfterEveryChangeCheckBox autoSaveOnExitCheckBox - trackNonDataChangesCheckBox + autoSaveNonDataChangesCheckBox backupBeforeSaveCheckBox autoReloadOnChangeCheckBox useAtomicSavesCheckBox diff --git a/src/gui/DatabaseWidget.cpp b/src/gui/DatabaseWidget.cpp index 8aca27940..90f464422 100644 --- a/src/gui/DatabaseWidget.cpp +++ b/src/gui/DatabaseWidget.cpp @@ -1562,7 +1562,7 @@ bool DatabaseWidget::lock() } } - if (m_db->isModified(true)) { + if (m_db->isModified()) { bool saved = false; // Attempt to save on exit, but don't block locking if it fails if (config()->get(Config::AutoSaveOnExit).toBool() @@ -1590,6 +1590,10 @@ bool DatabaseWidget::lock() return false; } } + } else if (m_db->hasNonDataChanges() && config()->get(Config::AutoSaveNonDataChanges).toBool()) { + // Silently auto-save non-data changes, ignore errors + QString errorMessage; + performSave(errorMessage); } if (m_groupView->currentGroup()) { @@ -1646,7 +1650,7 @@ void DatabaseWidget::reloadDatabaseFile() QString error; auto db = QSharedPointer::create(m_db->filePath()); if (db->open(database()->key(), &error)) { - if (m_db->isModified(true)) { + if (m_db->isModified() || db->hasNonDataChanges()) { // Ask if we want to merge changes into new database auto result = MessageBox::question( this, @@ -1839,33 +1843,14 @@ bool DatabaseWidget::save() m_blockAutoSave = true; ++m_saveAttempts; - QPointer focusWidget(qApp->focusWidget()); - - // TODO: Make this async - // Lock out interactions - m_entryView->setDisabled(true); - m_groupView->setDisabled(true); - QApplication::processEvents(); - - bool useAtomicSaves = config()->get(Config::UseAtomicSaves).toBool(); QString errorMessage; - bool ok = m_db->save(&errorMessage, useAtomicSaves, config()->get(Config::BackupBeforeSave).toBool()); - - // Return control - m_entryView->setDisabled(false); - m_groupView->setDisabled(false); - - if (focusWidget) { - focusWidget->setFocus(); - } - - if (ok) { + if (performSave(errorMessage)) { m_saveAttempts = 0; m_blockAutoSave = false; return true; } - if (m_saveAttempts > 2 && useAtomicSaves) { + if (m_saveAttempts > 2 && config()->get(Config::UseAtomicSaves).toBool()) { // Saving failed 3 times, issue a warning and attempt to resolve auto result = MessageBox::question(this, tr("Disable safe saves?"), @@ -1913,28 +1898,8 @@ bool DatabaseWidget::saveAs() bool ok = false; if (!newFilePath.isEmpty()) { - QPointer focusWidget(qApp->focusWidget()); - - // Lock out interactions - m_entryView->setDisabled(true); - m_groupView->setDisabled(true); - QApplication::processEvents(); - QString errorMessage; - ok = m_db->saveAs(newFilePath, - &errorMessage, - config()->get(Config::UseAtomicSaves).toBool(), - config()->get(Config::BackupBeforeSave).toBool()); - - // Return control - m_entryView->setDisabled(false); - m_groupView->setDisabled(false); - - if (focusWidget) { - focusWidget->setFocus(); - } - - if (!ok) { + if (!performSave(errorMessage, newFilePath)) { showMessage(tr("Writing the database failed: %1").arg(errorMessage), MessageWidget::Error, true, @@ -1945,6 +1910,38 @@ bool DatabaseWidget::saveAs() return ok; } +bool DatabaseWidget::performSave(QString& errorMessage, const QString& fileName) +{ + QPointer focusWidget(qApp->focusWidget()); + + // Lock out interactions + m_entryView->setDisabled(true); + m_groupView->setDisabled(true); + QApplication::processEvents(); + + bool ok; + if (fileName.isEmpty()) { + ok = m_db->save(&errorMessage, + config()->get(Config::UseAtomicSaves).toBool(), + config()->get(Config::BackupBeforeSave).toBool()); + } else { + ok = m_db->saveAs(fileName, + &errorMessage, + config()->get(Config::UseAtomicSaves).toBool(), + config()->get(Config::BackupBeforeSave).toBool()); + } + + // Return control + m_entryView->setDisabled(false); + m_groupView->setDisabled(false); + + if (focusWidget) { + focusWidget->setFocus(); + } + + return ok; +} + /** * Save copy of database under a new user-selected filename. * diff --git a/src/gui/DatabaseWidget.h b/src/gui/DatabaseWidget.h index 2564977dc..ae660bf88 100644 --- a/src/gui/DatabaseWidget.h +++ b/src/gui/DatabaseWidget.h @@ -260,6 +260,7 @@ private: void openDatabaseFromEntry(const Entry* entry, bool inBackground = true); bool confirmDeleteEntries(QList entries, bool permanent); void performIconDownloads(const QList& entries, bool force = false); + bool performSave(QString& errorMessage, const QString& fileName = {}); Entry* currentSelectedEntry(); QSharedPointer m_db; diff --git a/tests/TestConfig.cpp b/tests/TestConfig.cpp index 23661b046..1ce6c14ec 100644 --- a/tests/TestConfig.cpp +++ b/tests/TestConfig.cpp @@ -37,7 +37,6 @@ void TestConfig::testUpgrade() Config::createConfigFromFile(tempFile.fileName()); // value of new setting should be opposite the value of deprecated setting - QVERIFY(config()->get(Config::TrackNonDataChanges).toBool()); QVERIFY(!config()->get(Config::Security_PasswordsRepeatVisible).toBool()); QVERIFY(!config()->get(Config::Security_PasswordsHidden).toBool()); QVERIFY(config()->get(Config::Security_PasswordEmptyPlaceholder).toBool());