diff --git a/Orange/widgets/settings.py b/Orange/widgets/settings.py index f132b5a8d87..5163156a6c2 100644 --- a/Orange/widgets/settings.py +++ b/Orange/widgets/settings.py @@ -47,6 +47,8 @@ _IMMUTABLES = (str, int, bytes, bool, float, tuple) +VERSION_KEY = "__version__" + class Setting: """Description of a setting. @@ -386,6 +388,7 @@ def read_defaults_file(self, settings_file): for key, value in defaults.items() if not isinstance(value, Setting) } + self._migrate_settings(self.defaults) def write_defaults(self): """Write (global) defaults for this widget class to a file. @@ -434,12 +437,18 @@ def initialize(self, instance, data=None): if isinstance(data, bytes): data = pickle.loads(data) + self._migrate_settings(data) if provider is self.provider: data = self._add_defaults(data) provider.initialize(instance, data) + def _migrate_settings(self, settings): + """Ask widget to migrate settings to the latest version.""" + if settings: + self.widget_class.migrate_settings(settings, settings.pop(VERSION_KEY, None)) + def _select_provider(self, instance): provider = self.provider.get_provider(instance.__class__) if provider is None: @@ -473,7 +482,9 @@ def pack_data(self, widget): ---------- widget : OWWidget """ - return self.provider.pack(widget) + packed_settings = self.provider.pack(widget) + packed_settings[VERSION_KEY] = self.widget_class.settings_version + return packed_settings def update_defaults(self, widget): """ @@ -588,6 +599,7 @@ def initialize(self, instance, data=None): super().initialize(instance, data) if data and "context_settings" in data: instance.context_settings = data["context_settings"] + self._migrate_contexts(instance.context_settings) else: instance.context_settings = [] @@ -596,6 +608,11 @@ def read_defaults_file(self, settings_file): pickle.""" super().read_defaults_file(settings_file) self.global_contexts = pickle.load(settings_file) + self._migrate_contexts(self.global_contexts) + + def _migrate_contexts(self, contexts): + for context in contexts: + self.widget_class.migrate_context(context, context.values.pop(VERSION_KEY, None)) def write_defaults_file(self, settings_file): """Call the inherited method, then add global context to the pickle.""" @@ -603,9 +620,11 @@ def write_defaults_file(self, settings_file): pickle.dump(self.global_contexts, settings_file, -1) def pack_data(self, widget): - """Call the inherited method, then add local contexts to the pickle.""" + """Call the inherited method, then add local contexts to the dict.""" data = super().pack_data(widget) self.settings_from_widget(widget) + for context in widget.context_settings: + context.values[VERSION_KEY] = self.widget_class.settings_version data["context_settings"] = widget.context_settings return data diff --git a/Orange/widgets/tests/test_context_handler.py b/Orange/widgets/tests/test_context_handler.py index 1e9b415b803..88bd900b305 100644 --- a/Orange/widgets/tests/test_context_handler.py +++ b/Orange/widgets/tests/test_context_handler.py @@ -1,27 +1,85 @@ -from copy import copy +import pickle +from copy import copy, deepcopy +from io import BytesIO from unittest import TestCase -from unittest.mock import Mock -from Orange.widgets.settings import ContextHandler, Setting, ContextSetting, Context +from unittest.mock import Mock, patch, call +from Orange.widgets.settings import ContextHandler, Setting, ContextSetting, Context, VERSION_KEY __author__ = 'anze' class SimpleWidget: + settings_version = 1 + setting = Setting(42) context_setting = ContextSetting(42) + migrate_settings = Mock() + migrate_context = Mock() + + +class DummyContext(Context): + id = 0 + + def __init__(self, version=None): + super().__init__() + DummyContext.id += 1 + self.id = DummyContext.id + if version: + self.values[VERSION_KEY] = version + + def __repr__(self): + return "Context(id={})".format(self.id) + __str__ = __repr__ + + def __eq__(self, other): + if not isinstance(other, DummyContext): + return False + return self.id == other.id + + +def create_defaults_file(contexts): + b = BytesIO() + pickle.dump({"x": 5}, b) + pickle.dump(contexts, b) + b.seek(0) + return b + class TestContextHandler(TestCase): + def test_read_defaults(self): + contexts = [DummyContext() for _ in range(3)] + + handler = ContextHandler() + handler.widget_class = SimpleWidget + + # Old settings without version + migrate_context = Mock() + with patch.object(SimpleWidget, "migrate_context", migrate_context): + handler.read_defaults_file(create_defaults_file(contexts)) + self.assertSequenceEqual(handler.global_contexts, contexts) + migrate_context.assert_has_calls([call(c, None) for c in contexts]) + + # Settings with version + contexts = [DummyContext(version=i) for i in range(1, 4)] + migrate_context.reset_mock() + with patch.object(SimpleWidget, "migrate_context", migrate_context): + handler.read_defaults_file(create_defaults_file(contexts)) + self.assertSequenceEqual(handler.global_contexts, contexts) + migrate_context.assert_has_calls([call(c, c.values[VERSION_KEY]) for c in contexts]) + def test_initialize(self): handler = ContextHandler() handler.provider = Mock() + handler.widget_class = SimpleWidget # Context settings from data widget = SimpleWidget() - handler.initialize(widget, {'context_settings': 5}) + context_settings = [DummyContext()] + handler.initialize(widget, {'context_settings': context_settings}) self.assertTrue(hasattr(widget, 'context_settings')) - self.assertEqual(widget.context_settings, 5) + self.assertEqual(widget.context_settings, context_settings) # Default (global) context settings widget = SimpleWidget() @@ -29,6 +87,26 @@ def test_initialize(self): self.assertTrue(hasattr(widget, 'context_settings')) self.assertEqual(widget.context_settings, handler.global_contexts) + def test_initialize_migrates_contexts(self): + handler = ContextHandler() + handler.bind(SimpleWidget) + + widget = SimpleWidget() + + # Old settings without version + contexts = [DummyContext() for _ in range(3)] + migrate_context = Mock() + with patch.object(SimpleWidget, "migrate_context", migrate_context): + handler.initialize(widget, dict(context_settings=contexts)) + migrate_context.assert_has_calls([call(c, None) for c in contexts]) + + # Settings with version + contexts = [DummyContext(version=i) for i in range(1, 4)] + migrate_context = Mock() + with patch.object(SimpleWidget, "migrate_context", migrate_context): + handler.initialize(widget, dict(context_settings=deepcopy(contexts))) + migrate_context.assert_has_calls([call(c, c.values[VERSION_KEY]) for c in contexts]) + def test_fast_save(self): handler = ContextHandler() handler.bind(SimpleWidget) @@ -68,3 +146,16 @@ def test_find_or_create_context(self): self.assertEqual(context.i, 5) self.assertEqual([c.i for c in widget.context_settings], [5, 2]) self.assertEqual([c.i for c in handler.global_contexts], [3, 7]) + + def test_pack_settings_stores_version(self): + handler = ContextHandler() + handler.bind(SimpleWidget) + + widget = SimpleWidget() + handler.initialize(widget) + widget.context_setting = [DummyContext() for _ in range(3)] + + settings = handler.pack_data(widget) + self.assertIn("context_settings", settings) + for c in settings["context_settings"]: + self.assertIn(VERSION_KEY, c.values) diff --git a/Orange/widgets/tests/test_settings_handler.py b/Orange/widgets/tests/test_settings_handler.py index 9ad63e0d4f8..ecb95b9a166 100644 --- a/Orange/widgets/tests/test_settings_handler.py +++ b/Orange/widgets/tests/test_settings_handler.py @@ -1,11 +1,11 @@ -from io import BytesIO +from contextlib import contextmanager import os import pickle from tempfile import mkstemp import unittest -from unittest.mock import patch, Mock +from unittest.mock import patch, Mock, mock_open import warnings -from Orange.widgets.settings import SettingsHandler, Setting, SettingProvider +from Orange.widgets.settings import SettingsHandler, Setting, SettingProvider, VERSION_KEY class SettingHandlerTestCase(unittest.TestCase): @@ -13,7 +13,8 @@ class SettingHandlerTestCase(unittest.TestCase): def test_create(self, SettingProvider): """:type SettingProvider: unittest.mock.Mock""" - with patch.object(SettingsHandler, 'read_defaults'): + mock_read_defaults = Mock() + with patch.object(SettingsHandler, 'read_defaults', mock_read_defaults): handler = SettingsHandler.create(SimpleWidget) self.assertEqual(handler.widget_class, SimpleWidget) @@ -21,14 +22,14 @@ def test_create(self, SettingProvider): # the widget definition and collects all settings and read # all settings and for widget class SettingProvider.assert_called_once_with(SimpleWidget) - SettingsHandler.read_defaults.assert_called_once_with() + mock_read_defaults.assert_called_once_with() def test_create_uses_template_if_provided(self): template = SettingsHandler() - template.read_defaults = lambda: None template.a = 'a' template.b = 'b' - handler = SettingsHandler.create(SimpleWidget, template) + with self.override_default_settings(SimpleWidget): + handler = SettingsHandler.create(SimpleWidget, template) self.assertEqual(handler.a, 'a') self.assertEqual(handler.b, 'b') @@ -37,19 +38,14 @@ def test_create_uses_template_if_provided(self): self.assertEqual(template.b, 'b') def test_read_defaults(self): - default_settings = {'a': 5, 'b': {1: 5}} - fd, settings_file = mkstemp(suffix='.ini') - with open(settings_file, 'wb') as f: - pickle.dump(default_settings, f) - os.close(fd) - handler = SettingsHandler() - handler._get_settings_filename = lambda: settings_file - handler.read_defaults() + handler.widget_class = SimpleWidget - self.assertEqual(handler.defaults, default_settings) + defaults = {'a': 5, 'b': {1: 5}} + with self.override_default_settings(SimpleWidget, defaults): + handler.read_defaults() - os.remove(settings_file) + self.assertEqual(handler.defaults, defaults) def test_write_defaults(self): fd, settings_file = mkstemp(suffix='.ini') @@ -71,6 +67,7 @@ def test_initialize_widget(self): handler = SettingsHandler() handler.defaults = {'default': 42, 'setting': 1} handler.provider = provider = Mock() + handler.widget_class = SimpleWidget provider.get_provider.return_value = provider widget = SimpleWidget() @@ -100,6 +97,7 @@ def test_initialize_component(self): handler = SettingsHandler() handler.defaults = {'default': 42} provider = Mock() + handler.widget_class = SimpleWidget handler.provider = Mock(get_provider=Mock(return_value=provider)) widget = SimpleWidget() @@ -122,6 +120,7 @@ def test_initialize_with_no_provider(self, SettingProvider): """:type SettingProvider: unittest.mock.Mock""" handler = SettingsHandler() handler.provider = Mock(get_provider=Mock(return_value=None)) + handler.widget_class = SimpleWidget provider = Mock() SettingProvider.return_value = provider widget = SimpleWidget() @@ -137,8 +136,9 @@ def test_initialize_with_no_provider(self, SettingProvider): def test_fast_save(self): handler = SettingsHandler() - handler.read_defaults = lambda: None - handler.bind(SimpleWidget) + + with self.override_default_settings(SimpleWidget): + handler.bind(SimpleWidget) widget = SimpleWidget() @@ -153,8 +153,8 @@ def test_fast_save(self): def test_fast_save_siblings_spill(self): handler_mk1 = SettingsHandler() - handler_mk1.read_defaults = lambda: None - handler_mk1.bind(SimpleWidgetMk1) + with self.override_default_settings(SimpleWidgetMk1): + handler_mk1.bind(SimpleWidgetMk1) widget_mk1 = SimpleWidgetMk1() @@ -174,8 +174,8 @@ def test_fast_save_siblings_spill(self): self.assertEqual(widget_mk1.component.int_setting, 1) handler_mk2 = SettingsHandler() - handler_mk2.read_defaults = lambda: None - handler_mk2.bind(SimpleWidgetMk2) + with self.override_default_settings(SimpleWidgetMk2): + handler_mk2.bind(SimpleWidgetMk2) widget_mk2 = SimpleWidgetMk2() @@ -193,8 +193,8 @@ def test_fast_save_siblings_spill(self): def test_schema_only_settings(self): handler = SettingsHandler() - handler.read_defaults = lambda: None - handler.bind(SimpleWidget) + with self.override_default_settings(SimpleWidget): + handler.bind(SimpleWidget) # fast_save should not update defaults widget = SimpleWidget() @@ -213,12 +213,82 @@ def test_schema_only_settings(self): data = handler.pack_data(widget) self.assertEqual(data['schema_only_setting'], 5) + def test_read_defaults_migrates_settings(self): + handler = SettingsHandler() + handler.widget_class = SimpleWidget + + migrate_settings = Mock() + with patch.object(SimpleWidget, "migrate_settings", migrate_settings): + # Old settings without version + settings = {"value": 5} + with self.override_default_settings(SimpleWidget, settings): + handler.read_defaults() + migrate_settings.assert_called_with(settings, None) + + migrate_settings.reset() + # Settings with version + settings_with_version = dict(settings) + settings_with_version[VERSION_KEY] = 1 + with self.override_default_settings(SimpleWidget, settings_with_version): + handler.read_defaults() + migrate_settings.assert_called_with(settings, 1) + + def test_initialize_migrates_settings(self): + handler = SettingsHandler() + with self.override_default_settings(SimpleWidget): + handler.bind(SimpleWidget) + + widget = SimpleWidget() + + migrate_settings = Mock() + with patch.object(SimpleWidget, "migrate_settings", migrate_settings): + # Old settings without version + settings = {"value": 5} + + handler.initialize(widget, settings) + migrate_settings.assert_called_with(settings, None) + + migrate_settings.reset_mock() + # Settings with version + + settings_with_version = dict(settings) + settings_with_version[VERSION_KEY] = 1 + handler.initialize(widget, settings_with_version) + migrate_settings.assert_called_with(settings, 1) + + def test_pack_settings_stores_version(self): + handler = SettingsHandler() + handler.bind(SimpleWidget) + + widget = SimpleWidget() + + settings = handler.pack_data(widget) + self.assertIn(VERSION_KEY, settings) + + @contextmanager + def override_default_settings(self, widget, defaults=None): + if defaults is None: + defaults = {} + + h = SettingsHandler() + h.widget_class = widget + filename = h._get_settings_filename() + with open(filename, "wb") as f: + pickle.dump(defaults, f) + + yield + + if os.path.isfile(filename): + os.remove(filename) + class Component: int_setting = Setting(42) class SimpleWidget: + settings_version = 1 + setting = Setting(42) schema_only_setting = Setting(None, schema_only=True) non_setting = 5 @@ -228,6 +298,9 @@ class SimpleWidget: def __init__(self): self.component = Component() + migrate_settings = Mock() + migrate_context = Mock() + class SimpleWidgetMk1(SimpleWidget): pass diff --git a/Orange/widgets/widget.py b/Orange/widgets/widget.py index bce95533f58..7e5f1136431 100644 --- a/Orange/widgets/widget.py +++ b/Orange/widgets/widget.py @@ -154,6 +154,13 @@ class OWWidget(QDialog, Report, ProgressBarMixin, WidgetMessagesMixin, settingsHandler = None """:type: SettingsHandler""" + #: Version of the settings representation + #: Subclasses should increase this number when they make breaking + #: changes to settings representation (a settings that used to store + #: int now stores string) and handle migrations in migrate and + #: migrate_context settings. + settings_version = 1 + savedWidgetGeometry = settings.Setting(None) #: A list of advice messages (:class:`Message`) to display to the user. @@ -738,6 +745,32 @@ def _userconfirmed(): self.__msgwidget.accepted.connect(_userconfirmed) + @classmethod + def migrate_settings(cls, settings, version): + """Fix settings to work with the current version of widgets + + Parameters + ---------- + settings : dict + dict of name - value mappings + version : Optional[int] + version of the saved settings + or None if settings were created before migrations + """ + + @classmethod + def migrate_context(cls, context, version): + """Fix contexts to work with the current version of widgets + + Parameters + ---------- + context : Context + Context object + version : Optional[int] + version of the saved context + or None if context was created before migrations + """ + class Message(object): """ diff --git a/doc/development/source/tutorial-settings.rst b/doc/development/source/tutorial-settings.rst index ed3487657c9..f5dedbfb046 100644 --- a/doc/development/source/tutorial-settings.rst +++ b/doc/development/source/tutorial-settings.rst @@ -227,3 +227,50 @@ its class definition ``settingsHandler = DomainContextHandler()`` declares that Scatter plot uses :class:`DomainContextHandler`. The :obj:`attr_x` and :obj:`attr_y` are declared as :class:`~Orange.widgets.settings.ContextSetting`. + + +Migrations +********** + +At the beginning of this section of the tutorial, we created a widget with +a setting called proportion, which contains a value between 0 and 100. But +imagine that for some reason, we are not satisfied with the value any more +and decide that the setting should hold a value between 0 and 1. + +We update the setting's default value, modify the appropriate code and we +are done. That is, until someone that has already used the old version of +the widget open the new one and the widget crashes. Remember, when the +widget is opened, it has the same settings that were used the last time. + +Is there anything we can do, as settings are replaced with saved before +the __init__ function is called? There is! Migrations to the rescue. + +Widget has a special attribute called `settings_version`. All widgets start +with a settings_version of 1. When incompatible changes are done to the +widget's settings, its settings_version should be increased. But increasing +the version by it self does not solve our problems. While the widget now +knows that it uses different settings, the old ones are still broken and +need to be updated before they can be used with the new widget. This can be +accomplished by reimplementing widget's methods `migrate_settings` +(for ordinary settings) and `migrate_context` (for context settings). +Both method are called with the old object and the version of the settings +stored with it. + +If we bumped settings_version from 1 to 2 when we did the above mentioned +change, our migrate_settings method would look like this: + +.. code-block:: python + + def migrate_settings(settings, version): + if version < 2: + if "proportion" in settings: + settings["proportion"] = settings["proportion"] / 100 + +Your migration rules can be simple or complex, but try to avoid simply +forgetting the values, as settings are also used in saved workflows. +Imagine opening a complex workflow you have designed a year ago with the +new version of Orange and finding out that all the settings are back to +default. Not fun! + +So take some time, write the migrations and do not forget to bump the +settings_version when you do breaking changes.