ImageViewer: Fix and improve setting default attributes

Author: janezd

orangecontrib/imageanalytics/widgets/owimageviewer.py

@@ -41,7 +41,7 @@
     QNetworkRequest,
 )
 from AnyQt.QtWidgets import QApplication, QShortcut
-from Orange.data import DiscreteVariable, Domain, StringVariable, Table, Variable
+from Orange.data import DiscreteVariable, StringVariable, Table, Variable
 from Orange.widgets import gui, settings
 from Orange.widgets.utils.annotated_data import create_annotated_table
 from Orange.widgets.utils.itemmodels import DomainModel
@@ -50,10 +50,7 @@
 from orangewidget.utils.itemmodels import PyListModel
 from orangewidget.widget import Message, Msg
 
-from orangecontrib.imageanalytics.utils.image_utils import (
-    extract_paths,
-    filter_image_attributes,
-)
+from orangecontrib.imageanalytics.utils.image_utils import extract_paths
 from orangecontrib.imageanalytics.widgets.utils.imagepreview import Preview
 from orangecontrib.imageanalytics.widgets.utils.thumbnailview import (
     IconView as _IconView,
@@ -206,11 +203,6 @@ def _intersectSet(self, rect: QRect) -> List[QModelIndex]:
         return indices
 
 
-class MetaDomainModel(DomainModel):
-    def set_domain(self, domain: Domain):
-        super().set_domain(None if domain is None else Domain([], metas=domain.metas))
-
-
 class OWImageViewer(OWWidget):
     name = "Image Viewer"
     description = "View images referred to in the data."
@@ -231,8 +223,13 @@ class Error(OWWidget.Error):
             "Unable to display images! Please ensure that the chosen "
             "Image Filename Attribute store the correct paths to the images."
         )
+        no_image_attr = Msg(
+            "Data does not contain any variables with image names.\n"
+            "Numeric variables cannot store image names."
+        )
 
     settingsHandler = settings.DomainContextHandler()
+    settings_version = 2
 
     image_attr: Optional[Variable] = settings.ContextSetting(None)
     title_attr: Optional[Variable] = settings.ContextSetting(None)
@@ -257,9 +254,7 @@ def __init__(self):
         self._errcount = 0
         self._successcount = 0
 
-        self.image_model = MetaDomainModel(
-            valid_types=(StringVariable, DiscreteVariable)
-        )
+        self.image_model = DomainModel(valid_types=StringVariable)
         gui.comboBox(
             self.controlArea,
             self,
@@ -311,38 +306,59 @@ def sizeHint(self):
 
     @Inputs.data
     def setData(self, data):
-        self.closeContext()
+        if self.image_attr is not None:
+            # Don't store invalid contexts because they will match anything
+            # and crash the widget when they're used.
+            self.closeContext()
         self.clear()
-        self.data = data
-
-        if data is not None:
-            self.image_model.set_domain(data.domain)
-            self.title_model.set_domain(data.domain)
-            im_attr = filter_image_attributes(self.data)
-            self.image_attr = im_attr[0] if im_attr else None
-            self.title_attr = self.title_model[0] if self.title_model else None
+        if data is None:
+            self.commit.now()
+            return
 
-            self.openContext(data)
+        self.image_model.set_domain(data.domain)
+        self.title_model.set_domain(data.domain)
+        if not self.image_model:
+            self.Error.no_image_attr()
+            self.commit.now()
+            return
 
-            if self.image_model:
-                self.setupModel()
+        self.data = data
+        self._propose_image_and_title_attr()
+        self.openContext(data)
+        self.setupModel()
         self.commit.now()
 
-    def __select_image_attr(self):
-        for attr in self.image_model:
-            if attr.attributes.get("type").lower() == "image":
-                return attr
-        # check if function already exist
-        return self.image_model[0] if self.image_model else None
+    def _propose_image_and_title_attr(self):
+        self.image_attr = max(
+            self.image_model,
+            key=lambda attr: attr.attributes.get("type", "").lower() == "image"
+        )
+        # Use class variable if it exists. Otherwise,
+        # prefer string variables (there will be at least one, otherwise
+        # image_model is empty and widget reports an error,
+        # but avoid those marked as "image" and in particular the one used
+        # for image_attr
+        self.title_attr = self.data.domain.class_var or max(
+            # exclude separators
+            (attr for attr in self.title_model if isinstance(attr, Variable)),
+            key=lambda attr:
+                isinstance(attr, StringVariable)
+                and (3
+                     - (attr.attributes.get("type", "").lower() == "image")
+                     - (attr is self.image_attr))
+        )
 
     def clear(self):
         self.data = None
         self.Error.no_images_shown.clear()
+        self.Error.no_image_attr.clear()
         if self.__watcher is not None:
             self.__watcher.finishedAt.disconnect(self.__on_load_finished)
             self.__watcher = None
         self._cancelAllTasks()
         self.clearModel()
+        self.image_attr = None
+        self.title_attr = None
         self.image_model.set_domain(None)
         self.title_model.set_domain(None)
         self.selected_items = set()
@@ -377,11 +393,11 @@ def setupModel(self):
             self.thumbnailView.selectionModel().selectionChanged.connect(
                 self.onSelectionChanged
             )
-        self.__watcher = FutureSetWatcher()
-        self.__watcher.setFutures([it.future for it in self.items])
-        self.__watcher.finishedAt.connect(self.__on_load_finished)
-        self.__set_selected_items()
-        self._updateStatus()
+            self.__watcher = FutureSetWatcher()
+            self.__watcher.setFutures([it.future for it in self.items])
+            self.__watcher.finishedAt.connect(self.__on_load_finished)
+            self.__set_selected_items()
+            self._updateStatus()
 
     def __set_selected_items(self):
         model = self.thumbnailView.model()
@@ -469,6 +485,17 @@ def onDeleteWidget(self):
         self.clear()
         super().onDeleteWidget()
 
+    @classmethod
+    def migrate_context(cls, context, version):
+        if version < 2:
+            # Remove contexts in which image_attr is None because they match
+            # anything and crash the widget.
+            # Also remove context in which image_attr is not a string variable
+            # because widget now requires a string variable.
+            image_attr = context.values.get("image_attr")
+            if image_attr is None or image_attr[1] != 103:
+                raise settings.IncompatibleContext
+
 
 def column_data_as_qurl(
     table: Table, var: [StringVariable, DiscreteVariable]

orangecontrib/imageanalytics/widgets/tests/test_owimageviewer.py

@@ -1,11 +1,13 @@
-from AnyQt.QtCore import QItemSelection, QItemSelectionModel
-from typing import Set
-
 import os
+from typing import Set
 import unittest
+from unittest.mock import Mock
+
 import numpy as np
 from numpy.testing import assert_array_equal
 
+from AnyQt.QtCore import QItemSelection, QItemSelectionModel
+
 from Orange.data import (
     Table,
     StringVariable,
@@ -106,7 +108,7 @@ def test_selection(self):
         d = self.image_data
         str_var.attributes["origin"] = d.domain["Image"].attributes["origin"]
         new_data = Table(
-            Domain([], metas=(str_var,) + d.domain.metas[1:]),
+            Domain([], metas=(str_var,) + d.domain.metas[:-1]),
             np.empty((len(d), 0)),
             metas=d.metas,
         )
@@ -132,44 +134,98 @@ def test_selection_schema(self):
         assert_array_equal(self.image_data[2:3].metas, output.metas)
 
     def test_settings_schema(self):
-        data = Table("zoo")
-        data = data.transform(
-            Domain([], metas=data.domain.metas + data.domain.attributes)
-        )
+        domain = Domain([], None, [StringVariable(n) for n in "abc"])
+        data = Table.from_list(domain, [list("abc")] * 3)
         self.send_signal(self.widget.Inputs.data, data)
 
-        simulate.combobox_activate_item(self.widget.controls.image_attr, "toothed")
-        simulate.combobox_activate_item(self.widget.controls.title_attr, "toothed")
+        simulate.combobox_activate_item(self.widget.controls.image_attr, "b")
+        simulate.combobox_activate_item(self.widget.controls.title_attr, "c")
 
         settings = self.widget.settingsHandler.pack_data(self.widget)
         widget = self.create_widget(OWImageViewer, stored_settings=settings)
         self.send_signal(widget.Inputs.data, data, widget=widget)
 
-        self.assertEqual(data.domain["toothed"], self.widget.image_attr)
-        self.assertEqual(data.domain["toothed"], self.widget.title_attr)
+        self.assertEqual(data.domain["b"], self.widget.image_attr)
+        self.assertEqual(data.domain["c"], self.widget.title_attr)
 
     def test_set_attributes(self):
         data = self.image_data
         # by default - image attribute is one with type image
         self.send_signal(self.widget.Inputs.data, data)
         self.assertEqual(data.domain["Image"], self.widget.image_attr)
-        self.assertEqual(data.domain["b"], self.widget.title_attr)
+        self.assertEqual(data.domain["Image"], self.widget.title_attr)
+        self.__select_images({"afternoon-4175917_640.jpg", "atomium-4179270_640.jpg"})
+        self.assertIsNotNone(self.get_output(self.widget.Outputs.selected_data))
+        self.assertIsNotNone(self.get_output(self.widget.Outputs.data))
 
-        # none of attributes have type image select first non-continuous from meta
+        # no suitable attributes
         data = data.transform(
             Domain(data.domain.attributes, metas=data.domain.metas[:2])
         )
         self.send_signal(self.widget.Inputs.data, data)
-        self.assertEqual(data.domain["c"], self.widget.image_attr)
-        self.assertEqual(data.domain["b"], self.widget.title_attr)
+        self.assertEqual(None, self.widget.image_attr)
+        self.assertEqual(None, self.widget.title_attr)
+        self.assertTrue(self.widget.Error.no_image_attr.is_shown())
+        self.assertIsNone(self.get_output(self.widget.Outputs.selected_data))
+        self.assertIsNone(self.get_output(self.widget.Outputs.data))
 
-        # no suitable attributes - image_attr is None
-        data = data.transform(
-            Domain(data.domain.attributes, metas=data.domain.metas[:1])
-        )
-        self.send_signal(self.widget.Inputs.data, data)
-        self.assertIsNone(self.widget.image_attr)
-        self.assertEqual(data.domain["b"], self.widget.title_attr)
+        self.send_signal(self.widget.Inputs.data, None)
+        self.assertFalse(self.widget.Error.no_image_attr.is_shown())
+
+    def test_default_attr_priority(self):
+        w = self.widget
+        w.data = Mock()
+        w.data.domain.class_var = None
+
+        attrs = [
+            DiscreteVariable("a", values=["a", "b", "c"]),
+            ContinuousVariable("b")
+        ]
+        class_var = DiscreteVariable("c", values=["a", "b", "c"])
+        metas = [
+            ContinuousVariable("d"),
+            DiscreteVariable("e", values=["a", "b", "c"])
+        ] + [StringVariable(f"s{i}") for i in range(4)]
+        *_, s0, s1, s2, s3 = metas
+        s1.attributes = s2.attributes = {"type": "image"}
+
+        domain = Domain(attrs, class_var, metas)
+        w.image_model.set_domain(domain)
+        w.title_model.set_domain(domain)
+        w._propose_image_and_title_attr()
+        self.assertIs(s1, w.image_attr)
+        self.assertIs(s0, w.title_attr)
+
+        w.data.domain.class_var = class_var
+        w.image_attr = w.title_attr = None
+        w._propose_image_and_title_attr()
+        self.assertIs(s1, w.image_attr)
+        self.assertIs(class_var, w.title_attr)
+
+        w.data.domain.class_var = None
+        domain = Domain(attrs, class_var, metas[3:])
+        w.image_model.set_domain(domain)
+        w.title_model.set_domain(domain)
+        w.image_attr = w.title_attr = None
+        w._propose_image_and_title_attr()
+        self.assertIs(s1, w.image_attr)
+        self.assertIs(s3, w.title_attr)
+
+        domain = Domain(attrs, class_var, metas[3:-1])
+        w.image_model.set_domain(domain)
+        w.title_model.set_domain(domain)
+        w.image_attr = w.title_attr = None
+        w._propose_image_and_title_attr()
+        self.assertIs(s1, w.image_attr)
+        self.assertIs(s2, w.title_attr)
+
+        domain = Domain(attrs, class_var, metas[3:4])
+        w.image_model.set_domain(domain)
+        w.title_model.set_domain(domain)
+        w.image_attr = w.title_attr = None
+        w._propose_image_and_title_attr()
+        self.assertIs(s1, w.image_attr)
+        self.assertIs(s1, w.title_attr)
 
 
 if __name__ == "__main__":