[FIX] migrate_context: widgets crash when migrating context without version

[VesnaT]

Issue

Most widgets that implement migrate_context crash upon undo.
To reproduce see #3600.

Description of changes

Add checks for required context values to all widget that implement migrate_context.

Includes

• Code changes
• Tests
• Documentation

[codecov]

Codecov Report

Merging #3603 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3603      +/-   ##
==========================================
- Coverage   84.07%   84.07%   -0.01%     
==========================================
  Files         370      370              
  Lines       67252    67253       +1     
==========================================
- Hits        56541    56540       -1     
- Misses      10711    10713       +2

[janezd]

I may be wrong, but this doesn't looks like a fix but just a workaround. As I understand, the problem is that version that is passed to the migration is wrong -- for instance, settings are version 3, while the argument says that they're 0. If migration cannot rely on being passed the correct version, it's the same as if settings didn't have versions and migration would have to guess.

[astaric]

This problem is caused by contexts being mutable. After widget settings are packed into node.properties, update_defaults is called. This one calls settings_from_widget, which replaces context's values with a new dict that does not contain the __version__ key.

Updating pack to copy contexts should solve the issue. A similar fix already exists in write_default_to_file

diff --git a/Orange/widgets/settings.py b/Orange/widgets/settings.py
diff --git a/Orange/widgets/settings.py b/Orange/widgets/settings.py
index 4204f2a25..cf7be9bc1 100644
--- a/Orange/widgets/settings.py
+++ b/Orange/widgets/settings.py
@@ -663,9 +663,10 @@ class ContextHandler(SettingsHandler):
         """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_settings = [copy.copy(context) for context in widget.context_settings]
+        for context in context_settings:
             context.values[VERSION_KEY] = self.widget_class.settings_version
-        data["context_settings"] = widget.context_settings
+        data["context_settings"] = context_settings
         return data

[janezd]

@astaric, kudos. I needed ten minutes of jumping around the code just to understand your comment -- and it's not because of the complexity of the comment but of the code. (Which is again not your fault; it used to be much worse and let's not talk about this. :))

Your patch copies contexts to prevent overwriting them with ill-formed, version-less contexts later on. But why are contexts without versions even allowed? Why is __version__ set in pack_data and not in settings_from_widget, for instance here?

[astaric]

Historical reasons. Version support was first added to pack_data methods and later to write_defaults_file. Each time, the same change was made to methods in SettingsHandler and ContextHandler. I agree that adding the version to context in settings_from_widget is a better place that covers both paths.

Returning a value from pack_data that can/will be changed in the future seemed wrong, this is why I suggested returning copies of contexts.

[janezd]

settings_from_widget is defined in ContextHandler. SettingsHandler does not have it yet, thus version cannot be set in settings_from_widget.

It would still be nice, though, if settings always had a version, but let's do it in another PR.