Merge pull request #2432 from phw/fix-upgrade-hooks

Fix upgrade hooks re-declaring options

picard/config_upgrade.py

@@ -5,7 +5,7 @@
 # Copyright (C) 2013-2014 Michael Wiencek
 # Copyright (C) 2013-2016, 2018-2024 Laurent Monin
 # Copyright (C) 2014, 2017 Lukáš Lalinský
-# Copyright (C) 2014, 2018-2023 Philipp Wolfer
+# Copyright (C) 2014, 2018-2024 Philipp Wolfer
 # Copyright (C) 2015 Ohm Patel
 # Copyright (C) 2016 Suhas
 # Copyright (C) 2016-2017 Sambhav Kothari
@@ -43,8 +43,6 @@ from picard import (
 from picard.config import (
     BoolOption,
     IntOption,
-    ListOption,
-    Option,
     TextOption,
 )
 from picard.const import (
@@ -379,8 +377,7 @@ def upgrade_to_v2_6_0beta3(config):
     """Replace use_system_theme with ui_theme options"""
     from picard.ui.theme import UiTheme
     _s = config.setting
-    TextOption('setting', 'ui_theme', str(UiTheme.DEFAULT))
-    if _s['use_system_theme']:
+    if _s.value('use_system_theme', BoolOption):
         _s['ui_theme'] = str(UiTheme.SYSTEM)
     _s.remove('use_system_theme')
 
@@ -392,11 +389,10 @@ def upgrade_to_v2_7_0dev2(config):
         _p = config.persist
         splitter_dict = {}
         for (old_splitter_key, new_splitter_key) in key_map:
-            if _p.__contains__(old_splitter_key):
-                if _p[old_splitter_key] is not None:
-                    splitter_dict[new_splitter_key] = bytearray(_p[old_splitter_key])
+            if old_splitter_key in _p:
+                if v := _p.raw_value(old_splitter_key):
+                    splitter_dict[new_splitter_key] = v
                 _p.remove(old_splitter_key)
-        Option('persist', new_persist_key, {})
         _p[new_persist_key] = splitter_dict
 
     # MainWindow splitters
@@ -436,12 +432,8 @@ def upgrade_to_v2_7_0dev3(config):
         FileNamingScript,
         ScriptImportError,
     )
-    Option('setting', 'file_renaming_scripts', {})
-    ListOption('setting', 'file_naming_scripts', [])
-    TextOption('setting', 'file_naming_format', DEFAULT_FILE_NAMING_FORMAT)
-    TextOption('setting', 'selected_file_naming_script_id', '')
     scripts = {}
-    for item in config.setting['file_naming_scripts']:
+    for item in config.setting.raw_value('file_naming_scripts') or []:
         try:
             script_item = FileNamingScript().create_from_yaml(item, create_new_id=False)
             scripts[script_item['id']] = script_item.to_dict()
@@ -450,7 +442,7 @@ def upgrade_to_v2_7_0dev3(config):
     script_list = set(scripts.keys()) | set(map(lambda item: item['id'], get_file_naming_script_presets()))
     if config.setting['selected_file_naming_script_id'] not in script_list:
         script_item = FileNamingScript(
-            script=config.setting['file_naming_format'],
+            script=config.setting.value('file_naming_format', TextOption),
             title=_("Primary file naming script"),
             readonly=False,
             deletable=True,
@@ -465,22 +457,22 @@ def upgrade_to_v2_7_0dev3(config):
 def upgrade_to_v2_7_0dev4(config):
     """Replace artist_script_exception with artist_script_exceptions"""
     _s = config.setting
-    ListOption('setting', 'artist_script_exceptions', [])
-    if _s['artist_script_exception']:
-        _s['artist_script_exceptions'] = [_s['artist_script_exception']]
+    if script := _s.value('artist_script_exception', TextOption):
+        _s['artist_script_exceptions'] = [script]
     _s.remove('artist_script_exception')
-    ListOption('setting', 'artist_locales', ['en'])
-    if _s['artist_locale']:
-        _s['artist_locales'] = [_s['artist_locale']]
+    if locale := _s.value('artist_locale', TextOption):
+        _s['artist_locales'] = [locale]
     _s.remove('artist_locale')
 
 
 def upgrade_to_v2_7_0dev5(config):
     """Replace artist_script_exceptions with script_exceptions and remove artist_script_exception_weighting"""
     _s = config.setting
-    ListOption('setting', 'script_exceptions', [])
-    weighting = _s['artist_script_exception_weighting'] or 0
-    artist_script_exceptions = _s['artist_script_exceptions'] or []
+    weighting = _s.value('artist_script_exception_weighting', IntOption) or 0
+    if 'artist_script_exceptions' in _s:
+        artist_script_exceptions = _s.raw_value('artist_script_exceptions') or []
+    else:
+        artist_script_exceptions = []
     _s['script_exceptions'] = [(script_exception, weighting) for script_exception in artist_script_exceptions]
     _s.remove('artist_script_exceptions')
     _s.remove('artist_script_exception_weighting')

test/test_config_upgrade.py

@@ -3,7 +3,7 @@
 # Picard, the next-generation MusicBrainz tagger
 #
 # Copyright (C) 2019-2021 Laurent Monin
-# Copyright (C) 2019-2023 Philipp Wolfer
+# Copyright (C) 2019-2024 Philipp Wolfer
 # Copyright (C) 2021 Bob Swift
 # Copyright (C) 2021 Gabriel Ferreira
 #
@@ -60,6 +60,9 @@ from picard.config_upgrade import (
     upgrade_to_v2_6_0beta2,
     upgrade_to_v2_6_0beta3,
     upgrade_to_v2_6_0dev1,
+    upgrade_to_v2_7_0dev3,
+    upgrade_to_v2_7_0dev4,
+    upgrade_to_v2_7_0dev5,
     upgrade_to_v2_8_0dev2,
     upgrade_to_v3_0_0dev3,
 )
@@ -70,6 +73,8 @@ from picard.const import (
 from picard.util import unique_numbered_title
 from picard.version import Version
 
+from picard.ui.theme import UiTheme
+
 
 def _upgrade_hook_ok_1_2_3_dev_1(config):
     pass
@@ -423,14 +428,77 @@ class TestPicardConfigUpgrades(TestPicardConfigCommon):
         self.assertTrue(self.config.setting['save_only_one_front_image'])
 
     def test_upgrade_to_v2_6_0beta3(self):
-        from picard.ui.theme import UiTheme
+        # Legacy setting
         BoolOption('setting', 'use_system_theme', False)
         self.config.setting['use_system_theme'] = True
+        del Option.registry['setting', 'use_system_theme']
+        # New setting
+        TextOption('setting', 'ui_theme', str(UiTheme.DEFAULT))
         upgrade_to_v2_6_0beta3(self.config)
         self.assertNotIn('use_system_theme', self.config.setting)
         self.assertIn('ui_theme', self.config.setting)
         self.assertEqual(str(UiTheme.SYSTEM), self.config.setting['ui_theme'])
 
+    def test_upgrade_to_v2_7_0dev3(self):
+        # Legacy settings
+        ListOption('setting', 'file_naming_scripts', [])
+        self.config.setting['file_naming_scripts'] = [
+            '{"id": "766bb2ce-5170-45f1-900c-02e7f9bd41cb", "title": "Script 1", "script": "$noop(1)"}',
+            '{"id": "ab0abb63-797c-4a20-95a8-df1b9109f883", "title": "Script 2", "script": "$noop(2)"}',
+        ]
+        TextOption('setting', 'file_naming_format', DEFAULT_FILE_NAMING_FORMAT)
+        self.config.setting['file_naming_format'] = "%title%"
+        del Option.registry[('setting', 'file_naming_scripts')]
+        del Option.registry[('setting', 'file_naming_format')]
+        # New settings
+        Option('setting', 'file_renaming_scripts', {})
+        TextOption('setting', 'selected_file_naming_script_id', '')
+        upgrade_to_v2_7_0dev3(self.config)
+        new_scripts = self.config.setting['file_renaming_scripts']
+        selected_script_id = self.config.setting['selected_file_naming_script_id']
+        self.assertEqual(3, len(new_scripts))
+        self.assertIn(selected_script_id, new_scripts)
+        script1 = new_scripts['766bb2ce-5170-45f1-900c-02e7f9bd41cb']
+        self.assertEqual('Script 1', script1['title'])
+        self.assertEqual('$noop(1)', script1['script'])
+        script2 = new_scripts['ab0abb63-797c-4a20-95a8-df1b9109f883']
+        self.assertEqual('Script 2', script2['title'])
+        self.assertEqual('$noop(2)', script2['script'])
+        default_script = new_scripts[selected_script_id]
+        self.assertEqual('Primary file naming script', default_script['title'])
+        self.assertEqual('%title%', default_script['script'])
+
+    def test_upgrade_to_v2_7_0dev4(self):
+        # Legacy settings
+        TextOption('setting', 'artist_script_exception', '')
+        TextOption('setting', 'artist_locale', '')
+        self.config.setting['artist_script_exception'] = 'LATIN'
+        self.config.setting['artist_locale'] = 'en'
+        del Option.registry[('setting', 'artist_script_exception')]
+        del Option.registry[('setting', 'artist_locale')]
+        # New settings
+        ListOption('setting', 'artist_script_exceptions', [])
+        ListOption('setting', 'artist_locales', ['en'])
+        upgrade_to_v2_7_0dev4(self.config)
+        self.assertEqual(['LATIN'], self.config.setting['artist_script_exceptions'])
+        self.assertEqual(['en'], self.config.setting['artist_locales'])
+
+    def test_upgrade_to_v2_7_0dev5(self):
+        # Legacy settings
+        ListOption('setting', 'artist_script_exceptions', [])
+        IntOption('setting', 'artist_script_exception_weighting', 0)
+        self.config.setting['artist_script_exceptions'] = ['LATIN', 'HEBREW']
+        self.config.setting['artist_script_exception_weighting'] = 20
+        del Option.registry[('setting', 'artist_script_exceptions')]
+        del Option.registry[('setting', 'artist_script_exception_weighting')]
+        # New settings
+        ListOption('setting', 'script_exceptions', [])
+        upgrade_to_v2_7_0dev5(self.config)
+        self.assertEqual(self.config.setting['script_exceptions'], [
+            ('LATIN', 20),
+            ('HEBREW', 20),
+        ])
+
     def test_upgrade_to_v2_8_0dev2(self):
         ListOption('setting', 'toolbar_layout', [])
         self.config.setting['toolbar_layout'] = [