From 89a75828f2996be23a7169f72ed98480e14d1bdc Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 25 Apr 2024 09:37:24 +0200 Subject: [PATCH 1/2] Fix upgrade hooks re-declaring options Options must only be registered once. This includes upgrade hooks, which must only access declared options or for removed options access their raw value. --- picard/config_upgrade.py | 45 +++++++++++------------ test/test_config_upgrade.py | 72 +++++++++++++++++++++++++++++++++++-- 2 files changed, 90 insertions(+), 27 deletions(-) diff --git a/picard/config_upgrade.py b/picard/config_upgrade.py index 1dfbf9237..fdafae651 100644 --- a/picard/config_upgrade.py +++ b/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 @@ -34,7 +34,10 @@ from inspect import ( import re import sys -from PyQt6 import QtWidgets +from PyQt6 import ( + QtCore, + QtWidgets, +) from picard import ( PICARD_VERSION, @@ -43,8 +46,6 @@ from picard import ( from picard.config import ( BoolOption, IntOption, - ListOption, - Option, TextOption, ) from picard.const import ( @@ -379,8 +380,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 +392,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, qtype=QtCore.QByteArray): + 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 +435,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 +445,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 +460,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') diff --git a/test/test_config_upgrade.py b/test/test_config_upgrade.py index 01690882d..35171bc72 100644 --- a/test/test_config_upgrade.py +++ b/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'] = [ From e2758240441506dfbbf74b7999e10a3ce001d86e Mon Sep 17 00:00:00 2001 From: Philipp Wolfer Date: Thu, 25 Apr 2024 18:24:34 +0200 Subject: [PATCH 2/2] Fix upgrade_to_v2_7_0dev2 upgrade hook Copy the splitter config value as is instead of attempting a conversion. --- picard/config_upgrade.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/picard/config_upgrade.py b/picard/config_upgrade.py index fdafae651..348875c7a 100644 --- a/picard/config_upgrade.py +++ b/picard/config_upgrade.py @@ -34,10 +34,7 @@ from inspect import ( import re import sys -from PyQt6 import ( - QtCore, - QtWidgets, -) +from PyQt6 import QtWidgets from picard import ( PICARD_VERSION, @@ -393,7 +390,7 @@ def upgrade_to_v2_7_0dev2(config): splitter_dict = {} for (old_splitter_key, new_splitter_key) in key_map: if old_splitter_key in _p: - if v := _p.raw_value(old_splitter_key, qtype=QtCore.QByteArray): + if v := _p.raw_value(old_splitter_key): splitter_dict[new_splitter_key] = v _p.remove(old_splitter_key) _p[new_persist_key] = splitter_dict