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

@@ -58,6 +58,7 @@ static const QHash<Config::ConfigKey, ConfigDirective> 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<Config::ConfigKey, ConfigDirective> 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<QString, Config::ConfigKey> 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},

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,

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()

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);

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)

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)

src/gui/ApplicationSettingsWidgetGeneral.ui

@@ -253,14 +253,14 @@
               <item>
                <widget class="QCheckBox" name="autoSaveOnExitCheckBox">
                 <property name="text">
-                 <string>Automatically save on exit</string>
+                 <string>Automatically save when locking database</string>
                 </property>
                </widget>
               </item>
               <item>
-               <widget class="QCheckBox" name="trackNonDataChangesCheckBox">
+               <widget class="QCheckBox" name="autoSaveNonDataChangesCheckBox">
                 <property name="text">
-                 <string>Mark database as modified for non-data changes (e.g., expanding groups)</string>
+                 <string>Automatically save non-data changes when locking database</string>
                 </property>
                </widget>
               </item>
@@ -1037,7 +1037,7 @@
   <tabstop>checkForUpdatesIncludeBetasCheckBox</tabstop>
   <tabstop>autoSaveAfterEveryChangeCheckBox</tabstop>
   <tabstop>autoSaveOnExitCheckBox</tabstop>
-  <tabstop>trackNonDataChangesCheckBox</tabstop>
+  <tabstop>autoSaveNonDataChangesCheckBox</tabstop>
   <tabstop>backupBeforeSaveCheckBox</tabstop>
   <tabstop>autoReloadOnChangeCheckBox</tabstop>
   <tabstop>useAtomicSavesCheckBox</tabstop>

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<Database>::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<QWidget> 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<QWidget> 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<QWidget> 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.
  *

src/gui/DatabaseWidget.h

@@ -260,6 +260,7 @@ private:
     void openDatabaseFromEntry(const Entry* entry, bool inBackground = true);
     bool confirmDeleteEntries(QList<Entry*> entries, bool permanent);
     void performIconDownloads(const QList<Entry*>& entries, bool force = false);
+    bool performSave(QString& errorMessage, const QString& fileName = {});
     Entry* currentSelectedEntry();
 
     QSharedPointer<Database> m_db;

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());