From 212cd78c695567d05d8d12f1ce8e69f5717625c8 Mon Sep 17 00:00:00 2001
From: Laurent Monin <github@norz.org>
Date: Tue, 2 Apr 2019 13:24:47 +0200
Subject: [PATCH 1/2] Remove unused IntListOption

---
 picard/config.py | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/picard/config.py b/picard/config.py
index 9189524fd..78e787b36 100644
--- a/picard/config.py
+++ b/picard/config.py
@@ -260,11 +260,6 @@ def _convert_to_bool(value):
     return value is True or value == "true"
 
 
-@staticmethod
-def _convert_to_int_list(values):
-    return list(map(int, values))
-
-
 class TextOption(Option):
 
     convert = str
@@ -290,11 +285,6 @@ class ListOption(Option):
     convert = list
 
 
-class IntListOption(Option):
-
-    convert = _convert_to_int_list
-
-
 config = None
 setting = None
 persist = None

From a63cc6bdd1614476c63244e1dce92500dd1219f2 Mon Sep 17 00:00:00 2001
From: Laurent Monin <github@norz.org>
Date: Tue, 2 Apr 2019 13:40:10 +0200
Subject: [PATCH 2/2] Rework config and add tests for it

- get rid of useless ConfigSection.__config
- use name instead of full key (as section name is fixed)
- add support for qt type
- remove _convert_to_bool(), unneeded with qt type support
---
 picard/config.py    |  78 ++++-----
 test/test_config.py | 418 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 352 insertions(+), 144 deletions(-)

diff --git a/picard/config.py b/picard/config.py
index 78e787b36..8a6b49c37 100644
--- a/picard/config.py
+++ b/picard/config.py
@@ -29,7 +29,9 @@ from picard import (
     version_from_string,
     version_to_string,
 )
-from picard.util import LockableObject
+from picard.util import (
+    LockableObject,
+)
 
 
 class ConfigUpgradeError(Exception):
@@ -43,25 +45,17 @@ class ConfigSection(LockableObject):
     def __init__(self, config, name):
         super().__init__()
         self.__qt_config = config
-        self.__config = {}
         self.__name = name
-        self.__load_keys()
+        self.__prefix = self.__name + '/'
+        self.__prefix_len = len(self.__prefix)
 
-    def __qt_keys(self):
-        prefix = self.__name + '/'
-        return filter(lambda key: key.startswith(prefix),
-                      self.__qt_config.allKeys())
+    def key(self, name):
+        return self.__prefix + name
 
-    def __load_keys(self):
-        for key in self.__qt_keys():
-            try:
-                self.__config[key] = self.__qt_config.value(key)
-            except TypeError:
-                # Related to PICARD-1255, Unable to load the object into
-                # Python at all. Something weird with the way it is read and converted
-                # via the Qt C++ API. Simply ignore the key and it will be reset to
-                # default whenever the user opens Picard options
-                log.error('Unable to load config value: %s', key)
+    def _subkeys(self):
+        for key in self.__qt_config.allKeys():
+            if key[:self.__prefix_len] == self.__prefix:
+                yield key[self.__prefix_len:]
 
     def __getitem__(self, name):
         opt = Option.get(self.__name, name)
@@ -70,43 +64,41 @@ class ConfigSection(LockableObject):
         return self.value(name, opt, opt.default)
 
     def __setitem__(self, name, value):
-        key = self.__name + '/' + name
         self.lock_for_write()
         try:
-            self.__config[key] = value
-            self.__qt_config.setValue(key, value)
+            self.__qt_config.setValue(self.key(name), value)
         finally:
             self.unlock()
 
     def __contains__(self, name):
-        key = self.__name + '/' + name
-        return key in self.__config
+        return name in self._subkeys()
 
     def remove(self, name):
-        key = self.__name + '/' + name
         self.lock_for_write()
         try:
-            if key in self.__config:
-                self.__config.pop(key)
-                self.__qt_config.remove(key)
+            if name in self:
+                self.__qt_config.remove(self.key(name))
         finally:
             self.unlock()
 
-    def raw_value(self, name):
+    def raw_value(self, name, qtype=None):
         """Return an option value without any type conversion."""
-        value = self.__config[self.__name + '/' + name]
-        return value
+        key = self.key(name)
+        if qtype is not None:
+            return self.__qt_config.value(key, type=qtype)
+        else:
+            return self.__qt_config.value(key)
 
     def value(self, name, option_type, default=None):
         """Return an option value converted to the given Option type."""
-        key = self.__name + '/' + name
         self.lock_for_read()
         try:
-            if key in self.__config:
-                return option_type.convert(self.raw_value(name))
+            if name in self:
+                value = self.raw_value(name, qtype=option_type.qtype)
+                return option_type.convert(value)
             return default
-        except Exception:
-            log.error('Error reading option value', exc_info=True)
+        except Exception as why:
+            log.error('Cannot read %s value: %s', self.key(name), why, exc_info=True)
             return default
         finally:
             self.unlock()
@@ -201,7 +193,7 @@ class Config(QtCore.QSettings):
                                    version_to_string(self._version),
                                    version_to_string(version),
                                    hook['func'].__doc__.strip()))
-                    hook['func'](*hook['args'])
+                    hook['func'](self, *hook['args'])
                 except BaseException:
                     import traceback
                     raise ConfigUpgradeError(
@@ -235,6 +227,7 @@ class Option(QtCore.QObject):
     """Generic option."""
 
     registry = {}
+    qtype = None
 
     def __init__(self, section, name, default):
         super().__init__()
@@ -250,24 +243,16 @@ class Option(QtCore.QObject):
         return cls.registry.get((section, name))
 
 
-@staticmethod
-def _convert_to_bool(value):
-    # The QSettings IniFormat saves boolean values as the strings "true"
-    # and "false". Thus, explicit boolean and string comparisons are used
-    # to determine the value. NOTE: In PyQt >= 4.8.3, QSettings.value has
-    # an optional "type" parameter that avoids this. But we still support
-    # PyQt >= 4.5, so that is not used.
-    return value is True or value == "true"
-
-
 class TextOption(Option):
 
     convert = str
+    qtype = 'QString'
 
 
 class BoolOption(Option):
 
-    convert = _convert_to_bool
+    convert = bool
+    qtype = bool
 
 
 class IntOption(Option):
@@ -283,6 +268,7 @@ class FloatOption(Option):
 class ListOption(Option):
 
     convert = list
+    qtype = 'QVariantList'
 
 
 config = None
diff --git a/test/test_config.py b/test/test_config.py
index a29ce7042..91177daf8 100644
--- a/test/test_config.py
+++ b/test/test_config.py
@@ -1,138 +1,360 @@
+# -*- coding: utf-8 -*-
+#
+# Picard, the next-generation MusicBrainz tagger
+# Copyright (C) 2019 Laurent Monin
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+import logging
+import os
+import shutil
+from tempfile import mkdtemp
+
 from test.picardtestcase import PicardTestCase
 
-from picard import config
+from picard.config import (
+    BoolOption,
+    Config,
+    FloatOption,
+    IntOption,
+    ListOption,
+    Option,
+    TextOption,
+)
+
+class TestPicardConfigCommon(PicardTestCase):
+
+    def setUp(self):
+        super().setUp()
+        self.tmp_directory = mkdtemp()
+        self.configpath = os.path.join(self.tmp_directory, 'test.ini')
+        self.config = Config.from_file(None, self.configpath)
+        self.config.application["version"] = "testing"
+        logging.disable(logging.ERROR)
+        Option.registry = {}
+
+    def tearDown(self):
+        shutil.rmtree(self.tmp_directory)
 
 
-class CommonTests:
+class TestPicardConfig(TestPicardConfigCommon):
 
-    class CommonOptionTest(PicardTestCase):
-        opt_type = config.Option
-        default_value = 'somevalue'
+    def test_remove(self):
+        TextOption("setting", "text_option", "abc")
 
-        def setUp(self):
-            self.opt_type.registry = {}
+        self.config.setting["text_option"] = "def"
+        self.assertEqual(self.config.setting["text_option"], "def")
 
-        def test_constructor(self):
-            opt = self.opt_type('test', 'option1', self.default_value)
-            self.assertEqual('test', opt.section)
-            self.assertEqual('option1', opt.name)
-            self.assertEqual(self.default_value, opt.default)
-
-        def test_registry(self):
-            opt = self.opt_type('test', 'option1', self.default_value)
-            self.assertEqual(opt, self.opt_type.get('test', 'option1'))
+        self.config.setting.remove("text_option")
+        self.assertEqual(self.config.setting["text_option"], "abc")
 
 
-class OptionTest(CommonTests.CommonOptionTest):
+class TestPicardConfigTextOption(TestPicardConfigCommon):
 
-    def test_default_convert(self):
-        for default in ['somevalue', True, [], tuple(), 42]:
-            opt = config.Option('test', 'option1', default)
-            self.assertEqual(type(default), opt.convert)
-            self.assertEqual(default, opt.convert(default))
+    ### TextOption
+    def test_text_opt_convert(self):
+        opt = TextOption("setting", "text_option", "abc")
+        self.assertEqual(opt.convert(123), "123")
+
+    def test_text_opt_no_config(self):
+        TextOption("setting", "text_option", "abc")
+
+        # test default, nothing in config yet
+        self.assertEqual(self.config.setting["text_option"], "abc")
+        self.assertIs(type(self.config.setting["text_option"]), str)
+
+    def test_text_opt_set_read_back(self):
+        TextOption("setting", "text_option", "abc")
+
+        # set option to "def", and read back
+        self.config.setting["text_option"] = "def"
+        self.assertEqual(self.config.setting["text_option"], "def")
+        self.assertIs(type(self.config.setting["text_option"]), str)
+
+    def test_text_opt_set_none(self):
+        TextOption("setting", "text_option", "abc")
+
+        # set option to None
+        self.config.setting["text_option"] = None
+        self.assertEqual(self.config.setting["text_option"], "")
+
+    def test_text_opt_set_empty(self):
+        TextOption("setting", "text_option", "abc")
+
+        # set option to ""
+        self.config.setting["text_option"] = ""
+        self.assertEqual(self.config.setting["text_option"], "")
+
+    def test_text_opt_invalid_value(self):
+        TextOption("setting", "text_option", "abc")
+
+        # store invalid value in config file directly
+        self.config.setValue('setting/text_option', object)
+        self.assertEqual(self.config.setting["text_option"], 'abc')
 
 
-class TextOptionTest(CommonTests.CommonOptionTest):
-    opt_type = config.TextOption
-    default_value = 'test'
+class TestPicardConfigBoolOption(TestPicardConfigCommon):
 
-    def _test_convert(self, obj):
-        self.assertEqual('', obj.convert(''))
-        self.assertEqual('test', obj.convert('test'))
-        self.assertEqual('42', obj.convert(42))
+    ### BoolOption
+    def test_bool_opt_convert(self):
+        opt = BoolOption("setting", "bool_option", False)
+        self.assertEqual(opt.convert(1), True)
 
-    def test_convert_instance(self):
-        opt = self.opt_type('test', 'option1', self.default_value)
-        self._test_convert(opt)
+    def test_bool_opt_no_config(self):
+        BoolOption("setting", "bool_option", True)
 
-    def test_convert_static(self):
-        self._test_convert(self.opt_type)
+        # test default, nothing in config yet
+        self.assertEqual(self.config.setting["bool_option"], True)
+        self.assertIs(type(self.config.setting["bool_option"]), bool)
+
+    def test_bool_opt_set_read_back(self):
+        BoolOption("setting", "bool_option", True)
+
+        # set option and read back
+        self.config.setting["bool_option"] = False
+        self.assertEqual(self.config.setting["bool_option"], False)
+        self.assertIs(type(self.config.setting["bool_option"]), bool)
+
+    def test_bool_opt_set_str(self):
+        BoolOption("setting", "bool_option", False)
+
+        # set option to invalid value
+        self.config.setting["bool_option"] = 'yes'
+        self.assertEqual(self.config.setting["bool_option"], True)
+
+    def test_bool_opt_set_empty_str(self):
+        BoolOption("setting", "bool_option", True)
+
+        # set option to empty string
+        self.config.setting["bool_option"] = ''
+        self.assertEqual(self.config.setting["bool_option"], False)
+
+    def test_bool_opt_set_none(self):
+        BoolOption("setting", "bool_option", True)
+
+        # set option to None value
+        self.config.setting["bool_option"] = None
+        self.assertEqual(self.config.setting["bool_option"], False)
+
+    def test_bool_opt_set_direct_str(self):
+        BoolOption("setting", "bool_option", False)
+
+        # store invalid bool value in config file directly
+        self.config.setValue('setting/bool_option', 'yes')
+        self.assertEqual(self.config.setting["bool_option"], True)
+
+    def test_bool_opt_set_direct_str_true(self):
+        BoolOption("setting", "bool_option", False)
+
+        # store 'true' directly, it should be ok, due to conversion
+        self.config.setValue('setting/bool_option', 'true')
+        self.assertEqual(self.config.setting["bool_option"], True)
 
 
-class BoolOptionTest(CommonTests.CommonOptionTest):
-    opt_type = config.BoolOption
-    default_value = False
+class TestPicardConfigIntOption(TestPicardConfigCommon):
 
-    def _test_convert(self, obj):
-        self.assertTrue(obj.convert(True))
-        self.assertTrue(obj.convert('true'))
-        self.assertFalse(obj.convert(False))
-        self.assertFalse(obj.convert(None))
-        self.assertFalse(obj.convert('unknown'))
-        self.assertFalse(obj.convert('false'))
+    ### IntOption
+    def test_int_opt_convert(self):
+        opt = IntOption("setting", "int_option", 666)
+        self.assertEqual(opt.convert("123"), 123)
 
-    def test_convert_instance(self):
-        opt = self.opt_type('test', 'option1', self.default_value)
-        self._test_convert(opt)
+    def test_int_opt_no_config(self):
+        IntOption("setting", "int_option", 666)
 
-    def test_convert_static(self):
-        self._test_convert(self.opt_type)
+        # test default, nothing in config yet
+        self.assertEqual(self.config.setting["int_option"], 666)
+        self.assertIs(type(self.config.setting["int_option"]), int)
+
+    def test_int_opt_set_read_back(self):
+        IntOption("setting", "int_option", 666)
+
+        # set option and read back
+        self.config.setting["int_option"] = 333
+        self.assertEqual(self.config.setting["int_option"], 333)
+        self.assertIs(type(self.config.setting["int_option"]), int)
+
+    def test_int_opt_not_int(self):
+        IntOption("setting", "int_option", 666)
+
+        # set option to invalid value
+        self.config.setting["int_option"] = 'invalid'
+        self.assertEqual(self.config.setting["int_option"], 666)
+
+    def test_int_opt_set_none(self):
+        IntOption("setting", "int_option", 666)
+
+        # set option to None
+        self.config.setting["int_option"] = None
+        self.assertEqual(self.config.setting["int_option"], 666)
+
+    def test_int_opt_direct_invalid(self):
+        IntOption("setting", "int_option", 666)
+
+        # store invalid int value in config file directly
+        self.config.setValue('setting/int_option', 'x333')
+        self.assertEqual(self.config.setting["int_option"], 666)
+
+    def test_int_opt_direct_validstr(self):
+        IntOption("setting", "int_option", 666)
+
+        # store int as string directly, it should be ok, due to conversion
+        self.config.setValue('setting/int_option', '333')
+        self.assertEqual(self.config.setting["int_option"], 333)
 
 
-class IntOptionTest(CommonTests.CommonOptionTest):
-    opt_type = config.IntOption
-    default_value = 42
+class TestPicardConfigFloatOption(TestPicardConfigCommon):
 
-    def _test_convert(self, obj):
-        self.assertEqual(42, obj.convert(42))
-        self.assertEqual(42, obj.convert('42'))
-        self.assertEqual(0, obj.convert(False))
-        self.assertEqual(1, obj.convert(True))
-        with self.assertRaises(ValueError):
-            obj.convert('notanumber')
+    # FloatOption
+    def test_float_opt_convert(self):
+        opt = FloatOption("setting", "float_option", 666.6)
+        self.assertEqual(opt.convert("333.3"), 333.3)
 
-    def test_convert_instance(self):
-        opt = self.opt_type('test', 'option1', self.default_value)
-        self._test_convert(opt)
+    def test_float_opt_no_config(self):
+        FloatOption("setting", "float_option", 666.6)
 
-    def test_convert_static(self):
-        self._test_convert(self.opt_type)
+        # test default, nothing in config yet
+        self.assertEqual(self.config.setting["float_option"], 666.6)
+        self.assertIs(type(self.config.setting["float_option"]), float)
+
+    def test_float_opt_set_read_back(self):
+        FloatOption("setting", "float_option", 666.6)
+
+        # set option and read back
+        self.config.setting["float_option"] = 333.3
+        self.assertEqual(self.config.setting["float_option"], 333.3)
+        self.assertIs(type(self.config.setting["float_option"]), float)
+
+    def test_float_opt_not_float(self):
+        FloatOption("setting", "float_option", 666.6)
+
+        # set option to invalid value
+        self.config.setting["float_option"] = 'invalid'
+        self.assertEqual(self.config.setting["float_option"], 666.6)
+
+    def test_float_opt_set_none(self):
+        FloatOption("setting", "float_option", 666.6)
+
+        # set option to None
+        self.config.setting["float_option"] = None
+        self.assertEqual(self.config.setting["float_option"], 666.6)
+
+    def test_float_opt_direct_invalid(self):
+        FloatOption("setting", "float_option", 666.6)
+
+        # store invalid float value in config file directly
+        self.config.setValue('setting/float_option', '333.3x')
+        self.assertEqual(self.config.setting["float_option"], 666.6)
+
+    def test_float_opt_direct_validstr(self):
+        FloatOption("setting", "float_option", 666.6)
+
+        # store float as string directly, it should be ok, due to conversion
+        self.config.setValue('setting/float_option', '333.3')
+        self.assertEqual(self.config.setting["float_option"], 333.3)
 
 
-class FloatOptionTest(CommonTests.CommonOptionTest):
-    opt_type = config.FloatOption
-    default_value = 42.5
+class TestPicardConfigListOption(TestPicardConfigCommon):
 
-    def _test_convert(self, obj):
-        self.assertEqual(42.5, obj.convert(42.5))
-        self.assertEqual(42.5, obj.convert('42.5'))
+    ### ListOption
+    def test_list_opt_convert(self):
+        opt = ListOption("setting", "list_option", [])
+        self.assertEqual(opt.convert("123"), ['1', '2', '3'])
 
-    def test_convert_instance(self):
-        opt = self.opt_type('test', 'option1', self.default_value)
-        self._test_convert(opt)
+    def test_list_opt_no_config(self):
+        ListOption("setting", "list_option", ["a", "b"])
 
-    def test_convert_static(self):
-        self._test_convert(self.opt_type)
+        # test default, nothing in config yet
+        self.assertEqual(self.config.setting["list_option"], ["a", "b"])
+        self.assertIs(type(self.config.setting["list_option"]), list)
+
+    def test_list_opt_set_read_back(self):
+        ListOption("setting", "list_option", ["a", "b"])
+
+        # set option and read back
+        self.config.setting["list_option"] = ["c", "d"]
+        self.assertEqual(self.config.setting["list_option"], ["c", "d"])
+        self.assertIs(type(self.config.setting["list_option"]), list)
+
+    def test_list_opt_not_list(self):
+        ListOption("setting", "list_option", ["a", "b"])
+
+        # set option to invalid value
+        self.config.setting["list_option"] = 'invalid'
+        self.assertEqual(self.config.setting["list_option"], ["a", "b"])
+
+    def test_list_opt_set_none(self):
+        ListOption("setting", "list_option", ["a", "b"])
+
+        # set option to None
+        self.config.setting["list_option"] = None
+        self.assertEqual(self.config.setting["list_option"], [])
+
+    def test_list_opt_set_empty(self):
+        ListOption("setting", "list_option", ["a", "b"])
+
+        # set option to empty list
+        self.config.setting["list_option"] = []
+        self.assertEqual(self.config.setting["list_option"], [])
+
+    def test_list_opt_direct_invalid(self):
+        ListOption("setting", "list_option", ["a", "b"])
+
+        # store invalid list value in config file directly
+        self.config.setValue('setting/list_option', 'efg')
+        self.assertEqual(self.config.setting["list_option"], ["a", "b"])
 
 
-class ListOptionTest(CommonTests.CommonOptionTest):
-    opt_type = config.ListOption
-    default_value = ['somevalue']
+class TestPicardConfigVarOption(TestPicardConfigCommon):
 
-    def _test_convert(self, obj):
-        self.assertEqual([], obj.convert([]))
-        self.assertEqual(['a', 'b', 'c'], obj.convert(('a', 'b', 'c')))
-        self.assertEqual(['a', 'b', 'c'], obj.convert('abc'))
+    ### Option
+    def test_var_opt_convert(self):
+        opt = Option("setting", "var_option", set())
+        self.assertEqual(opt.convert(["a", "b", "a"]), {"a", "b"})
 
-    def test_convert_instance(self):
-        opt = self.opt_type('test', 'option1', self.default_value)
-        self._test_convert(opt)
+    def test_var_opt_no_config(self):
+        Option("setting", "var_option", set(["a", "b"]))
 
-    def test_convert_static(self):
-        self._test_convert(self.opt_type)
+        # test default, nothing in config yet
+        self.assertEqual(self.config.setting["var_option"], set(["a", "b"]))
+        self.assertIs(type(self.config.setting["var_option"]), set)
 
+    def test_var_opt_set_read_back(self):
+        Option("setting", "var_option", set(["a", "b"]))
 
-class IntListOptionTest(CommonTests.CommonOptionTest):
-    opt_type = config.IntListOption
-    default_value = [1, 2, 3]
+        # set option to "def", and read back
+        self.config.setting["var_option"] = set(["c", "d"])
+        self.assertEqual(self.config.setting["var_option"], set(["c", "d"]))
+        self.assertIs(type(self.config.setting["var_option"]), set)
 
-    def _test_convert(self, obj):
-        self.assertEqual([], obj.convert([]))
-        self.assertEqual([1, 2, 3], obj.convert(('1', '2', '3')))
+    def test_var_opt_set_none(self):
+        Option("setting", "var_option", set(["a", "b"]))
 
-    def test_convert_instance(self):
-        opt = self.opt_type('test', 'option1', self.default_value)
-        self._test_convert(opt)
+        # set option to None
+        self.config.setting["var_option"] = None
+        self.assertEqual(self.config.setting["var_option"], set(["a", "b"]))
 
-    def test_convert_static(self):
-        self._test_convert(self.opt_type)
+    def test_var_opt_set_empty(self):
+        Option("setting", "var_option", set(["a", "b"]))
+
+        # set option to ""
+        self.config.setting["var_option"] = set()
+        self.assertEqual(self.config.setting["var_option"], set())
+
+    def test_var_opt_invalid_value(self):
+        Option("setting", "var_option", set(["a", "b"]))
+
+        # store invalid value in config file directly
+        self.config.setValue('setting/var_option', object)
+        self.assertEqual(self.config.setting["var_option"], set(["a", "b"]))