Merge pull request #2146 from jerneju/gh-2143-editdomain-duplicate

[FIX] Edit Domain: Prevent duplicate variable names

Author: lanzagar

Orange/widgets/data/oweditdomain.py

@@ -70,7 +70,7 @@ def variable_from_description(description, compute_value=None):
     module, type_name, name, kwargs, attrs = description
     try:
         constructor = get_qualified(module, type_name)
-    except (ImportError, AttributeError) as ex:
+    except (ImportError, AttributeError):
         raise ValueError("Invalid descriptor type '{}.{}"
                          "".format(module, type_name))
 
@@ -213,7 +213,7 @@ def set_data(self, var):
     def get_data(self):
         """Retrieve the modified variable.
         """
-        name = str(self.name_edit.text())
+        name = str(self.name_edit.text()).strip()
         labels = self.labels_model.get_dict()
 
         # Is the variable actually changed.
@@ -226,12 +226,15 @@ def get_data(self):
 
         return var
 
+    def is_legal(self):
+        name = str(self.name_edit.text()).strip()
+        return not len(name) == 0
+
     def is_same(self):
         """Is the current model state the same as the input.
         """
-        name = str(self.name_edit.text())
+        name = str(self.name_edit.text()).strip()
         labels = self.labels_model.get_dict()
-
         return (self.var is not None and name == self.var.name and
                 labels == self.var.attributes)
 
@@ -243,7 +246,7 @@ def clear(self):
         self.labels_model.set_dict({})
 
     def maybe_commit(self):
-        if not self.is_same():
+        if not self.is_same() and self.is_legal():
             self.commit()
 
     def commit(self):
@@ -317,7 +320,7 @@ def set_data(self, var):
     def get_data(self):
         """Retrieve the modified variable
         """
-        name = str(self.name_edit.text())
+        name = str(self.name_edit.text()).strip()
         labels = self.labels_model.get_dict()
         values = map(str, self.values_model)
 
@@ -402,6 +405,10 @@ def __init__(self):
 
         box.layout().addWidget(self.editor_stack)
 
+        self.Error.add_message(
+            "duplicate_var_name",
+            "A variable name is duplicated.")
+
     @check_sql_input
     def set_data(self, data):
         """Set input data set."""
@@ -514,7 +521,9 @@ def _on_variable_changed(self):
 
         # Replace the variable in the 'Domain Features' view/model
         old_var = self.input_vars[self.selected_index]
-        new_var = editor.get_data().copy(compute_value=Orange.preprocess.transformation.Identity(old_var))
+        new_var = editor.get_data().copy(
+            compute_value=Orange.preprocess.transformation.Identity(old_var)
+        )
         self.domain_model[self.selected_index] = new_var
 
 
@@ -530,17 +539,21 @@ def _invalidate(self):
     def commit(self):
         """Send the changed data to output."""
         new_data = None
+        var_names = [vn.name for vn in self.domain_model]
+        self.Error.duplicate_var_name.clear()
         if self.data is not None:
-            input_domain = self.data.domain
-            n_attrs = len(input_domain.attributes)
-            n_vars = len(input_domain.variables)
-            n_class_vars = len(input_domain.class_vars)
-            all_new_vars = list(self.domain_model)
-            attrs = all_new_vars[: n_attrs]
-            class_vars = all_new_vars[n_attrs: n_attrs + n_class_vars]
-            new_metas = all_new_vars[n_attrs + n_class_vars:]
-            new_domain = Orange.data.Domain(attrs, class_vars, new_metas)
-            new_data = self.data.from_table(new_domain, self.data)
+            if len(var_names) == len(set(var_names)):
+                input_domain = self.data.domain
+                n_attrs = len(input_domain.attributes)
+                n_class_vars = len(input_domain.class_vars)
+                all_new_vars = list(self.domain_model)
+                attrs = all_new_vars[: n_attrs]
+                class_vars = all_new_vars[n_attrs: n_attrs + n_class_vars]
+                new_metas = all_new_vars[n_attrs + n_class_vars:]
+                new_domain = Orange.data.Domain(attrs, class_vars, new_metas)
+                new_data = self.data.from_table(new_domain, self.data)
+            else:
+                self.Error.duplicate_var_name()
 
         self.send("Data", new_data)
 

Orange/widgets/data/tests/test_oweditdomain.py

@@ -140,6 +140,35 @@ def test_list_attributes_remain_lists(self):
         t2 = self.get_output("Data")
         self.assertEqual(t2.domain["a"].attributes["list"], [1, 2, 4])
 
+    def test_duplicate_names(self):
+        """
+        Tests if widget shows error when duplicate name is entered.
+        And tests if widget sends None data when error is shown.
+        GH-2143
+        GH-2146
+        """
+        table = Table("iris")
+        self.send_signal("Data", table)
+        self.assertFalse(self.widget.Error.duplicate_var_name.is_shown())
+
+        idx = self.widget.domain_view.model().index(0)
+        self.widget.domain_view.setCurrentIndex(idx)
+        editor = self.widget.editor_stack.findChild(ContinuousVariableEditor)
+
+        editor.name_edit.setText("iris")
+        editor.commit()
+        self.widget.commit()
+        self.assertTrue(self.widget.Error.duplicate_var_name.is_shown())
+        output = self.get_output("Data")
+        self.assertIsNone(output)
+
+        editor.name_edit.setText("sepal height")
+        editor.commit()
+        self.widget.commit()
+        self.assertFalse(self.widget.Error.duplicate_var_name.is_shown())
+        output = self.get_output("Data")
+        self.assertIsInstance(output, Table)
+
 
 class TestEditors(GuiTest):
     def test_variable_editor(self):