Do not store default values in user's QgsSettings

The new behavior is to store a value in user's QSettings (that overrides the global settings) only if the the value has changed from the default reported by QgsSettings. If a value was changed and it is changed back to the default the override must be removed from the user settings. The rationale is that global settings should be the ultimate source of default values, unless the user override the default with a different value. Fixes #21049
qgis · Feb 28, 2019 · cd5fedf · cd5fedf
1 parent 34a0650
commit cd5fedf
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 2 deletions.
diff --git a/src/core/qgssettings.cpp b/src/core/qgssettings.cpp
@@ -281,7 +281,17 @@ void QgsSettings::setArrayIndex( int i )
 void QgsSettings::setValue( const QString &key, const QVariant &value, const QgsSettings::Section section )
 {
   // TODO: add valueChanged signal
-  mUserSettings->setValue( prefixedKey( key, section ), value );
+  // Do not store if it hasn't changed from default value
+  QVariant v = QgsSettings::value( prefixedKey( key, section ) );
+  QVariant currentValue { QgsSettings::value( prefixedKey( key, section ) ) };
+  if ( ( currentValue.isValid() || value.isValid() ) && ( currentValue != value ) )
+  {
+    mUserSettings->setValue( prefixedKey( key, section ), value );
+  }
+  else if ( mGlobalSettings->value( prefixedKey( key, section ) ) == currentValue )
+  {
+    mUserSettings->remove( prefixedKey( key, section ) );
+  }
 }
 
 // To lower case and clean the path

diff --git a/tests/src/python/test_qgssettings.py b/tests/src/python/test_qgssettings.py
@@ -14,7 +14,8 @@
 import tempfile
 from qgis.core import QgsSettings, QgsTolerance, QgsMapLayerProxyModel
 from qgis.testing import start_app, unittest
-from qgis.PyQt.QtCore import QSettings
+from qgis.PyQt.QtCore import QSettings, QVariant
+from pathlib import Path
 
 __author__ = 'Alessandro Pasotti'
 __date__ = '02/02/2017'
@@ -33,9 +34,12 @@ class TestQgsSettings(unittest.TestCase):
     def setUp(self):
         self.cnt += 1
         h, path = tempfile.mkstemp('.ini')
+        Path(path).touch()
         assert QgsSettings.setGlobalSettingsPath(path)
         self.settings = QgsSettings('testqgissettings', 'testqgissettings%s' % self.cnt)
         self.globalsettings = QSettings(self.settings.globalSettingsPath(), QSettings.IniFormat)
+        self.globalsettings.sync()
+        assert os.path.exists(self.globalsettings.fileName())
 
     def tearDown(self):
         settings_file = self.settings.fileName()
@@ -329,6 +333,7 @@ def test_section_getters_setters(self):
         self.settings.setValue('key1', 'provider1', section=QgsSettings.Providers)
         self.settings.setValue('key2', 'provider2', section=QgsSettings.Providers)
 
+        # This is an overwrite of previous setting and it is intentional
         self.settings.setValue('key1', 'auth1', section=QgsSettings.Auth)
         self.settings.setValue('key2', 'auth2', section=QgsSettings.Auth)
 
@@ -422,6 +427,46 @@ def test_flagValue(self):
         self.assertEqual(self.settings.flagValue('flag', pointAndLine), pointAndLine)
         self.assertEqual(type(self.settings.flagValue('enum', pointAndLine)), QgsMapLayerProxyModel.Filters)
 
+    def test_overwriteDefaultValues(self):
+        """Test that unchanged values are not stored"""
+        self.globalsettings.setValue('a_value_with_default', 'a value')
+        self.globalsettings.setValue('an_invalid_value', QVariant())
+
+        self.assertEqual(self.settings.value('a_value_with_default'), 'a value')
+        self.assertEqual(self.settings.value('an_invalid_value'), QVariant())
+
+        # Now, set them with the same current value
+        self.settings.setValue('a_value_with_default', 'a value')
+        self.settings.setValue('an_invalid_value', QVariant())
+
+        # Check
+        pure_settings = QSettings(self.settings.fileName(), QSettings.IniFormat)
+        self.assertFalse('a_value_with_default' in pure_settings.allKeys())
+        self.assertFalse('an_invalid_value' in pure_settings.allKeys())
+
+        # Set a changed value
+        self.settings.setValue('a_value_with_default', 'a new value')
+        self.settings.setValue('an_invalid_value', 'valid value')
+
+        # Check
+        self.assertTrue('a_value_with_default' in pure_settings.allKeys())
+        self.assertTrue('an_invalid_value' in pure_settings.allKeys())
+
+        self.assertEqual(self.settings.value('a_value_with_default'), 'a new value')
+        self.assertEqual(self.settings.value('an_invalid_value'), 'valid value')
+
+        # Re-set to original values
+        self.settings.setValue('a_value_with_default', 'a value')
+        self.settings.setValue('an_invalid_value', QVariant())
+
+        self.assertEqual(self.settings.value('a_value_with_default'), 'a value')
+        self.assertEqual(self.settings.value('an_invalid_value'), QVariant())
+
+        # Check if they are gone
+        pure_settings = QSettings(self.settings.fileName(), QSettings.IniFormat)
+        self.assertFalse('a_value_with_default' not in pure_settings.allKeys())
+        self.assertFalse('an_invalid_value' not in pure_settings.allKeys())
+
 
 if __name__ == '__main__':
     unittest.main()