[Config] Refactor config class

* Refactored duplication with setting config key and logging
* Simplified lazy importing reactor for callLater. This lazy importing
is required for testing and also prevents Gtk UI lockup if reactor
imported in Config.
* Fixed saving config to file when setting a key that doesn't exist yet.
This was due to returning early in the set_item method.
* Added a `default` arg to set_item to prevent saving to file when only
setting a default value for a key in init.
* Moved casting value to existing key type from set_item to dedicated
function.

2 files changed, 45 insertions, 46 deletions

diff --git a/deluge/config.py b/deluge/config.py
index 59225bc01..e8577dc35 100644
--- a/deluge/config.py
+++ b/deluge/config.py
@@ -49,7 +49,6 @@ from tempfile import NamedTemporaryFile
 from deluge.common import JSON_FORMAT, get_default_config_dir
 
 log = logging.getLogger(__name__)
-callLater = None  # noqa: N816 Necessary for the config tests
 
 
 def prop(func):
@@ -100,6 +99,21 @@ def find_json_objects(text, decoder=json.JSONDecoder()):
     return objects
 
 
+def cast_to_existing_type(value, old_value):
+    """Attempt to convert new value type to match old value type"""
+    types_match = isinstance(old_value, (type(None), type(value)))
+    if value is not None and not types_match:
+        old_type = type(old_value)
+        # Skip convert to bytes since requires knowledge of encoding and value should
+        # be unicode anyway.
+        if old_type is bytes:
+            return value
+
+        return old_type(value)
+
+    return value
+
+
 class Config:
     """This class is used to access/create/modify config files.
 
@@ -127,7 +141,7 @@ class Config:
 
         if defaults:
             for key, value in defaults.items():
-                self.set_item(key, value)
+                self.set_item(key, value, default=True)
 
         # Load the config from file in the config_dir
         if config_dir:
@@ -137,6 +151,12 @@ class Config:
 
         self.load()
 
+    def callLater(self, period, func):  # noqa: N802 ignore camelCase
+        """Wrapper around reactor.callLater for test purpose."""
+        from twisted.internet import reactor
+
+        return reactor.callLater(period, func)
+
     def __contains__(self, item):
         return item in self.__config
 
@@ -145,7 +165,7 @@ class Config:
 
         return self.set_item(key, value)
 
-    def set_item(self, key, value):
+    def set_item(self, key, value, default=False):
         """Sets item 'key' to 'value' in the config dictionary.
 
         Does not allow changing the item's type unless it is None.
@@ -157,6 +177,8 @@ class Config:
             key (str): Item to change to change.
             value (any): The value to change item to, must be same type as what is
                 currently in the config.
+            default (optional, bool): When setting a default value skip func or save
+                callbacks.
 
         Raises:
             ValueError: Raised when the type of value is not the same as what is
@@ -169,58 +191,43 @@ class Config:
             5
 
         """
-        if key not in self.__config:
-            self.__config[key] = value
-            log.debug('Setting key "%s" to: %s (of type: %s)', key, value, type(value))
-            return
-
-        if self.__config[key] == value:
-            return
+        if isinstance(value, bytes):
+            value = value.decode()
 
-        # Change the value type if it is not None and does not match.
-        type_match = isinstance(self.__config[key], (type(None), type(value)))
-        if value is not None and not type_match:
+        if key in self.__config:
             try:
-                oldtype = type(self.__config[key])
-                # Don't convert to bytes as requires encoding and value will
-                # be decoded anyway.
-                if oldtype is not bytes:
-                    value = oldtype(value)
+                value = cast_to_existing_type(value, self.__config[key])
             except ValueError:
                 log.warning('Value Type "%s" invalid for key: %s', type(value), key)
                 raise
-
-        if isinstance(value, bytes):
-            value = value.decode('utf8')
+            else:
+                if self.__config[key] == value:
+                    return
 
         log.debug('Setting key "%s" to: %s (of type: %s)', key, value, type(value))
         self.__config[key] = value
 
-        global callLater
-        if callLater is None:
-            # Must import here and not at the top or it will throw ReactorAlreadyInstalledError
-            from twisted.internet.reactor import (  # pylint: disable=redefined-outer-name
-                callLater,
-            )
+        # Skip save or func callbacks if setting default value for keys
+        if default:
+            return
+
         # Run the set_function for this key if any
-        try:
-            for func in self.__set_functions[key]:
-                callLater(0, func, key, value)
-        except KeyError:
-            pass
+        for func in self.__set_functions.get(key, []):
+            self.callLater(0, func, key, value)
+
         try:
 
             def do_change_callbacks(key, value):
                 for func in self.__change_callbacks:
                     func(key, value)
 
-            callLater(0, do_change_callbacks, key, value)
+            self.callLater(0, do_change_callbacks, key, value)
         except Exception:
             pass
 
         # We set the save_timer for 5 seconds if not already set
         if not self._save_timer or not self._save_timer.active():
-            self._save_timer = callLater(5, self.save)
+            self._save_timer = self.callLater(5, self.save)
 
     def __getitem__(self, key):
         """See get_item """
@@ -296,16 +303,9 @@ class Config:
 
         del self.__config[key]
 
-        global callLater
-        if callLater is None:
-            # Must import here and not at the top or it will throw ReactorAlreadyInstalledError
-            from twisted.internet.reactor import (  # pylint: disable=redefined-outer-name
-                callLater,
-            )
-
         # We set the save_timer for 5 seconds if not already set
         if not self._save_timer or not self._save_timer.active():
-            self._save_timer = callLater(5, self.save)
+            self._save_timer = self.callLater(5, self.save)
 
     def register_change_callback(self, callback):
         """Registers a callback function for any changed value.
diff --git a/deluge/tests/test_config.py b/deluge/tests/test_config.py
index 3ee6b9f33..79568809c 100644
--- a/deluge/tests/test_config.py
+++ b/deluge/tests/test_config.py
@@ -10,7 +10,6 @@ from codecs import getwriter
 import pytest
 from twisted.internet import task
 
-import deluge.config
 from deluge.common import JSON_FORMAT
 from deluge.config import Config
 
@@ -136,16 +135,16 @@ class TestConfig:
         assert config['int'] == 2
 
     def test_save_timer(self):
-        self.clock = task.Clock()
-        deluge.config.callLater = self.clock.callLater
+        clock = task.Clock()
 
         config = Config('test.conf', defaults=DEFAULTS, config_dir=self.config_dir)
+        config.callLater = clock.callLater
         config['string'] = 'baz'
         config['int'] = 2
         assert config._save_timer.active()
 
         # Timeout set for 5 seconds in config, so lets move clock by 5 seconds
-        self.clock.advance(5)
+        clock.advance(5)
 
         def check_config(config):
             assert not config._save_timer.active()