From cd5fedfd5645582fbb523c117f268746bc3ae6c1 Mon Sep 17 00:00:00 2001
From: Alessandro Pasotti <elpaso@itopen.it>
Date: Thu, 28 Feb 2019 09:36:00 +0100
Subject: [PATCH] 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
---
 src/core/qgssettings.cpp             | 12 ++++++-
 tests/src/python/test_qgssettings.py | 47 +++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/src/core/qgssettings.cpp b/src/core/qgssettings.cpp
index ad224947134..742dfe1fdc8 100644
--- 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
index b61b9b97bb2..51f8e09c065 100644
--- a/tests/src/python/test_qgssettings.py
+++ b/tests/src/python/test_qgssettings.py
@@ -14,7 +14,8 @@ import os
 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 @@ class TestQgsSettings(unittest.TestCase):
         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 @@ class TestQgsSettings(unittest.TestCase):
         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()