Merge pull request #6415 from janezd/edit-domain-noncontext

[FIX] Edit Domain no longer forgets its settings

Orange/widgets/data/oweditdomain.py

@@ -42,7 +42,8 @@
 )
 from Orange.misc.collections import DictMissingConst
 from Orange.util import frompyfunc
-from Orange.widgets import widget, gui, settings
+from Orange.widgets import widget, gui
+from Orange.widgets.settings import Setting
 from Orange.widgets.utils import itemmodels, ftry, disconnected
 from Orange.widgets.utils.buttons import FixedSizeButton
 from Orange.widgets.utils.itemmodels import signal_blocking
@@ -56,6 +57,8 @@
 V = TypeVar("V", bound=Orange.data.Variable)  # pylint: disable=invalid-name
 H = TypeVar("H", bound=Hashable)  # pylint: disable=invalid-name
 
+MAX_HINTS = 1000
+
 
 def unique(sequence: Iterable[H]) -> Iterable[H]:
     """
@@ -1888,13 +1891,11 @@ class Outputs:
     class Error(widget.OWWidget.Error):
         duplicate_var_name = widget.Msg("A variable name is duplicated.")
 
-    settingsHandler = settings.DomainContextHandler()
-    settings_version = 2
+    settings_version = 3
 
-    _domain_change_store = settings.ContextSetting({})
-    _selected_item = settings.ContextSetting(None)  # type: Optional[Tuple[str, int]]
-    _merge_dialog_settings = settings.ContextSetting({})
-    output_table_name = settings.ContextSetting("")
+    _domain_change_hints = Setting({}, schema_only=True)
+    _merge_dialog_settings = Setting({}, schema_only=True)
+    output_table_name = Setting("", schema_only=True)
 
     want_main_area = False
 
@@ -1903,6 +1904,7 @@ def __init__(self):
         self.data = None  # type: Optional[Orange.data.Table]
         #: The current selected variable index
         self.selected_index = -1
+        self._selected_item = None
         self._invalidated = False
         self.typeindex = 0
 
@@ -1960,14 +1962,12 @@ def __init__(self):
     @Inputs.data
     def set_data(self, data):
         """Set input dataset."""
-        self.closeContext()
         self.clear()
         self.data = data
 
         if self.data is not None:
             self.setup_model(data)
             self.le_output_name.setPlaceholderText(data.name)
-            self.openContext(self.data)
             self._editor.set_merge_context(self._merge_dialog_settings)
             self._restore()
         else:
@@ -1983,8 +1983,6 @@ def clear(self):
         assert self.selected_index == -1
         self.selected_index = -1
 
-        self._selected_item = None
-        self._domain_change_store = {}
         self._merge_dialog_settings = {}
 
     def reset_selected(self):
@@ -2006,7 +2004,6 @@ def reset_selected(self):
 
     def reset_all(self):
         """Reset all variables to their original state."""
-        self._domain_change_store = {}
         if self.data is not None:
             model = self.variables_model
             for i in range(model.rowCount()):
@@ -2049,19 +2046,30 @@ def _restore(self, ):
         Restore the edit transform from saved state.
         """
         model = self.variables_model
+        hints = self._domain_change_hints
+        first_key = None
         for i in range(model.rowCount()):
             midx = model.index(i, 0)
             coldesc = model.data(midx, Qt.EditRole)  # type: DataVector
-            tr = self._restore_transform(coldesc.vtype)
+            tr, key = self._restore_transform(coldesc.vtype)
             if tr:
                 model.setData(midx, tr, TransformRole)
+                if first_key is None:
+                    first_key = key
+        # Reduce the number of hints to MAX_HINTS, but keep all current hints
+        # Current hints start with `first_key`.
+        while len(hints) > MAX_HINTS and \
+                (key := next(iter(hints))) is not first_key:
+            del hints[key]  # pylint: disable=unsupported-delete-operation
 
         # Restore the current variable selection
         i = -1
         if self._selected_item is not None:
             for i, vec in enumerate(model):
                 if vec.vtype.name_type() == self._selected_item:
                     break
+            else:
+                self._selected_item = None
         if i == -1 and model.rowCount():
             i = 0
 
@@ -2114,13 +2122,20 @@ def _on_variable_changed(self):
         self._store_transform(var, transform)
         self._invalidate()
 
-    def _store_transform(self, var, transform):
+    def _store_transform(self, var, transform, deconvar=None):
         # type: (Variable, List[Transform]) -> None
-        self._domain_change_store[deconstruct(var)] = [deconstruct(t) for t in transform]
+        deconvar = deconvar or deconstruct(var)
+        # Remove the existing key (if any) to put the new one at the end,
+        # to make sure it comes after the sentinel
+        self._domain_change_hints.pop(deconvar, None)
+        # pylint: disable=unsupported-assignment-operation
+        self._domain_change_hints[deconvar] = \
+            [deconstruct(t) for t in transform]
 
     def _restore_transform(self, var):
         # type: (Variable) -> List[Transform]
-        tr_ = self._domain_change_store.get(deconstruct(var), [])
+        key = deconstruct(var)
+        tr_ = self._domain_change_hints.get(key, [])
         tr = []
 
         for t in tr_:
@@ -2131,7 +2146,11 @@ def _restore_transform(self, var):
                     "Failed to restore transform: {}, {!r}".format(t, err),
                     UserWarning, stacklevel=2
                 )
-        return tr
+        if tr:
+            self._store_transform(var, tr, key)
+        else:
+            key = None
+        return tr, key
 
     def _invalidate(self):
         self._set_modified(True)
@@ -2295,6 +2314,29 @@ def migrate_context(cls, context, version):
                 store.append((deconstruct(src), [deconstruct(tr) for tr in trs]))
             context.values["_domain_change_store"] = (dict(store), -2)
 
+    @classmethod
+    def migrate_settings(cls, settings, version):
+        if version == 2 and "context_settings" in settings:
+            contexts = settings["context_settings"]
+            valuess = []
+            for context in contexts:
+                cls.migrate_context(context, context.values["__version__"])
+                valuess.append(context.values)
+            # Fix the order of keys
+            hints = dict.fromkeys(
+                chain(*(values["_domain_change_store"][0]
+                        for values in reversed(valuess)))
+            )
+            settings["output_table_name"] = ""
+            for values in valuess:
+                hints.update(values["_domain_change_store"][0])
+                new_name, _ = values.pop("output_table_name", ("", -2))
+                if new_name:
+                    settings["output_table_name"] = new_name
+            while len(hints) > MAX_HINTS:
+                del hints[next(iter(hints))]
+            settings["_domain_change_hints"] = hints
+            del settings["context_settings"]
 
 def enumerate_columns(
         table: Orange.data.Table

Orange/widgets/data/tests/test_oweditdomain.py

@@ -16,6 +16,7 @@
     QStyleOptionViewItem, QDialog, QMenu, QToolTip, QListView
 from AnyQt.QtTest import QTest, QSignalSpy
 
+from orangewidget.settings import Context
 from orangewidget.tests.utils import simulate
 
 from Orange.data import (
@@ -325,7 +326,7 @@ def test_restore(self):
         w = self.widget
 
         def restore(state):
-            w._domain_change_store = state
+            w._domain_change_hints = state
             w._restore()
 
         model = w.variables_model
@@ -338,6 +339,177 @@ def restore(state):
         tr = model.data(model.index(4), TransformRole)
         self.assertEqual(tr, [AsString(), Rename("Z")])
 
+    def test_hint_keeping(self):
+        editor: ContinuousVariableEditor = self.widget.findChild(ContinuousVariableEditor)
+        name_edit = editor.name_edit
+        model = self.widget.domain_view.model()
+
+        def rename(fr, to):
+            for idx in range(fr, to):
+                self.widget.domain_view.setCurrentIndex(model.index(idx))
+                cur_text = name_edit.text()
+                if cur_text[0] != "x":
+                    name_edit.setText("x" + cur_text)
+                editor.on_name_changed()
+
+        def data(fr, to):
+            return Table.from_numpy(Domain(vars[fr:to]),
+                                    np.zeros((1, to - fr)))
+
+
+        vars = [ContinuousVariable(f"v{i}") for i in range(1020)]
+        self.send_signal(data(0, 5))
+        rename(2, 4)
+
+        self.send_signal(None)
+        self.assertIsNone(self.get_output())
+
+        self.send_signal(data(3, 7))
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes],
+                         ["xv3", "v4", "v5", "v6"])
+
+        self.send_signal(data(0, 5))
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes],
+                         ["v0", "v1", "xv2", "xv3", "v4"])
+
+        self.send_signal(data(3, 7))
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes],
+                         ["xv3", "v4", "v5", "v6"])
+
+        # This is too large: widget should retain just hints related to
+        # the current data
+        self.send_signal(data(3, 1020))
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes[:4]],
+                         ["xv3", "v4", "v5", "v6"])
+        rename(5, 1017)
+        self.widget.commit()
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes[-3:]],
+                         ["xv1017", "xv1018", "xv1019"])
+
+        self.send_signal(None)
+        self.assertIsNone(self.get_output())
+
+        # Tests that hints for the current data are kept
+        # - including the earliest (v3) and latest (v1019)
+        self.send_signal(data(3, 1020))
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes[:4]],
+                         ["xv3", "v4", "v5", "v6"])
+        self.assertEqual([var.name for var in outp.domain.attributes[-3:]],
+                         ["xv1017", "xv1018", "xv1019"])
+
+        # Tests that older hints are dropped: v2 should be lost
+        self.send_signal(data(0, 5))
+        outp = self.get_output()
+        self.assertEqual([var.name for var in outp.domain.attributes],
+                         ["v0", "v1", "v2", "xv3", "v4"])
+
+    def test_migrate_settings_hints_2_to_3(self):
+        settings = {
+            '__version__': 2,
+            'context_settings':
+                [Context(values={
+                    '_domain_change_store': (
+                        {('Categorical', ('a', ('mir1', 'mir4', 'mir2'), (), False)):
+                            [('Rename', ('disease mir',))],
+                         ('Categorical', ('b', ('mir4', 'mir1', 'mir2'), (), False)):
+                             [('Rename', ('disease mirs',))]
+                         },
+                        -2),
+                    '_merge_dialog_settings': ({}, -4),
+                    '_selected_item': (('1', 0), -2),
+                    'output_table_name': ('boo', -2),
+                    '__version__': 2}),
+                 Context(values={
+                     '_domain_change_store': (
+                         {('Categorical', ('b', ('mir4', 'mir1', 'mir2'), (), False)):
+                             [('Rename', ('disease bmir',))],
+                          ('Categorical', ('c', ('mir4', 'mir1', 'mir2'), (), False)):
+                              [('Rename', ('disease mirs',))]
+                         },
+                         -2),
+                      '_merge_dialog_settings': ({}, -4),
+                      '_selected_item': (('1', 0), -2),
+                      'output_table_name': ('far', -2),
+                      '__version__': 2}),
+                ]}
+        migrated_hints = {
+            ('Categorical', ('b', ('mir4', 'mir1', 'mir2'), (), False)):
+                [('Rename', ('disease bmir',))],
+            ('Categorical', ('c', ('mir4', 'mir1', 'mir2'), (), False)):
+                [('Rename', ('disease mirs',))],
+            ('Categorical', ('a', ('mir1', 'mir4', 'mir2'), (), False)):
+                 [('Rename', ('disease mir',))],
+        }
+        widget = self.create_widget(OWEditDomain, stored_settings=settings)
+        self.assertEqual(widget._domain_change_hints, migrated_hints)
+        # order matters
+        self.assertEqual(list(widget._domain_change_hints), list(migrated_hints))
+        self.assertEqual(widget.output_table_name, "far")
+
+    def test_migrate_settings_2_to_3_realworld(self):
+        settings = {
+            'controlAreaVisible': True,
+            '__version__': 2,
+            'context_settings': [Context(
+                values={
+                    '_domain_change_store':
+                        ({('Real', ('sepal length', (1, 'f'), (), False)):
+                              [('AsString', ())],
+                          ('Real', ('sepal width', (1, 'f'), (), False)):
+                              [('AsTime', ()), ('StrpTime', ('Detect automatically', None, 1, 1))],
+                          ('Real', ('petal width', (1, 'f'), (), False)):
+                              [('Annotate', ((('a', 'b'),),))]}, -2),
+                    '_merge_dialog_settings': ({}, -4),
+                    '_selected_item': (('petal width', 2), -2),
+                    'output_table_name': ('', -2),
+                    '__version__': 2},
+                attributes={'sepal length': 2, 'sepal width': 2,
+                            'petal length': 2, 'petal width': 2, 'iris': 1},
+                metas={}
+            )]
+        }
+        widget = self.create_widget(OWEditDomain, stored_settings=settings)
+        self.assertEqual(
+            widget._domain_change_hints,
+            {('Real', ('sepal length', (1, 'f'), (), False)):
+                 [('AsString', ())],
+             ('Real', ('sepal width', (1, 'f'), (), False)):
+                 [('AsTime', ()),
+                  ('StrpTime', ('Detect automatically', None, 1, 1))],
+             ('Real', ('petal width', (1, 'f'), (), False)):
+                 [('Annotate', ((('a', 'b'),),))]}
+        )
+
+    def test_migrate_settings_name_2_to_3(self):
+        settings = {
+            '__version__': 2,
+            'context_settings':
+                [Context(values={
+                    '_domain_change_store': ({}, -2),
+                     'output_table_name': ('boo', -2),
+                     '__version__': 2}),
+                 Context(values={
+                     '_domain_change_store': ({}, -2),
+                     'output_table_name': ('far', -2),
+                     '__version__': 2}),
+                 Context(values={
+                     '_domain_change_store': ({}, -2),
+                     'output_table_name': ('', -2),
+                     '__version__': 2}),
+                 Context(values={
+                     '_domain_change_store': ({}, -2),
+                     '__version__': 2})
+                ]
+        }
+        widget = self.create_widget(OWEditDomain, stored_settings=settings)
+        self.assertEqual(widget.output_table_name, "far")
+
 
 class TestEditors(GuiTest):
     def test_variable_editor(self):