OWSaveBase: Keep relative path

janezd · janezd · commit cfea969b39ee · 2020-03-14T18:17:16.000+01:00
diff --git a/Orange/widgets/utils/save/owsavebase.py b/Orange/widgets/utils/save/owsavebase.py
@@ -48,9 +48,16 @@ class Error(widget.OWWidget.Error):
     want_main_area = False
     resizing_enabled = False
 
-    last_dir = Setting("")
-    filter = Setting("")  # default will be provided in __init__
-    filename = Setting("", schema_only=True)
+    filter = Setting("")  # Default is provided in __init__
+
+    # If the path is in the same directory as the workflow file or its
+    # subdirectory, it is stored as a relative path, otherwise as absolute.
+    # For the sake of security, we do not store relative paths from other
+    # directories, like home or cwd. After loading the widget from a schema,
+    # auto_save is set to off, unless the stored_path is relative (to the
+    # workflow).
+    stored_path = Setting("")
+    stored_name = Setting("", schema_only=True)  # File name, without path
     auto_save = Setting(False)
 
     filters = []
@@ -69,6 +76,8 @@ def __init__(self, start_row=0):
         """
         super().__init__()
         self.data = None
+        self._absolute_path = self._abs_path_from_setting()
+
         # This cannot be done outside because `filters` is defined by subclass
         if not self.filter:
             self.filter = next(iter(self.get_filters()))
@@ -90,6 +99,60 @@ def __init__(self, start_row=0):
         self.adjustSize()
         self.update_messages()
 
+    @property
+    def last_dir(self):
+        # Not the best name, but kept for compatibility
+        return self._absolute_path
+
+    @last_dir.setter
+    def last_dir(self, absolute_path):
+        """Store _absolute_path and update relative path (stored_path)"""
+        self._absolute_path = absolute_path
+
+        workflow_dir = self.workflowEnv().get("basedir", None)
+        if workflow_dir and absolute_path.startswith(workflow_dir.rstrip("/")):
+            self.stored_path = os.path.relpath(absolute_path, workflow_dir)
+        else:
+            self.stored_path = absolute_path
+
+    def _abs_path_from_setting(self):
+        """
+        Compute absolute path from `stored_path` from settings.
+
+        Auto save is disabled unless stored_path is relative and exists.
+        """
+        workflow_dir = self.workflowEnv().get("basedir")
+        if os.path.isabs(self.stored_path):
+            path = self.stored_path
+            self.auto_save = False
+        elif workflow_dir:
+            path = os.path.join(workflow_dir, self.stored_path)
+        else:
+            path = None
+
+        if path and os.path.exists(path):
+            return path
+        else:
+            self.stored_path = workflow_dir or _userhome
+            self.auto_save = False
+            return self.stored_path
+
+    @property
+    def filename(self):
+        if self.stored_name:
+            return os.path.join(self._absolute_path, self.stored_name)
+        else:
+            return ""
+
+    @filename.setter
+    def filename(self, value):
+        self.last_dir, self.stored_name = os.path.split(value)
+
+    def workflowEnvChanged(self, key, value, oldvalue):
+        # Trigger refresh of relative path, e.g. when saving the scheme
+        if key == "basedir":
+            self.last_dir = self._absolute_path
+
     @classmethod
     def get_filters(cls):
         return cls.filters
@@ -133,8 +196,7 @@ def save_file_as(self):
             return
         self.filename = filename
         self.filter = selected_filter
-        self.last_dir = os.path.split(self.filename)[0]
-        self.bt_save.setText(f"Save as {os.path.split(filename)[1]}")
+        self.bt_save.setText(f"Save as {self.stored_name}")
         self.update_messages()
         self._try_save()
 
@@ -243,6 +305,14 @@ def valid_filters(self):
     def default_valid_filter(self):
         return self.filter
 
+    @classmethod
+    def migrate_settings(cls, settings, version):
+        # We cannot use versions because they are overriden in derived classes
+        if "last_dir" in settings:
+            settings["stored_path"] = settings.pop("last_dir")
+        if "filename" in settings:
+            settings["stored_name"] = os.path.split(settings.pop("filename"))[1]
+
     # As of Qt 5.9, QFileDialog.setDefaultSuffix does not support double
     # suffixes, not even in non-native dialogs. We handle each OS separately.
     if sys.platform in ("darwin", "win32"):
diff --git a/Orange/widgets/utils/save/tests/test_owsavebase.py b/Orange/widgets/utils/save/tests/test_owsavebase.py
@@ -5,14 +5,15 @@
 
 # pylint: disable=missing-docstring, protected-access, unsubscriptable-object
 import unittest
-from unittest.mock import Mock
+from unittest.mock import Mock, patch
+import sys
 import os
 import collections
 
+from orangewidget.widget import Input
 from Orange.widgets.tests.base import WidgetTest
 from Orange.widgets.utils import getmembers
-from Orange.widgets.utils.save.owsavebase import OWSaveBase
-from orangewidget.widget import Input
+from Orange.widgets.utils.save.owsavebase import OWSaveBase, _userhome
 
 
 class SaveWidgetsTestBaseMixin:
@@ -46,18 +47,18 @@ def test_filters(self):
 class TestOWSaveBaseWithWriters(WidgetTest):
     # Tests for OWSaveBase methods that require filters to be dictionaries
     # with with writers as keys in `filters`.
-    def setUp(self):
-        class OWSaveMockWriter(OWSaveBase):
-            name = "Mock save"
-            writer = Mock()
-            writer.EXTENSIONS = [".csv"]
-            writer.SUPPORT_COMPRESSED = True
-            writer.SUPPORT_SPARSE_DATA = False
-            writer.OPTIONAL_TYPE_ANNOTATIONS = False
-            writers = [writer]
-            filters = {"csv (*.csv)": writer}
+    class OWSaveMockWriter(OWSaveBase):
+        name = "Mock save"
+        writer = Mock()
+        writer.EXTENSIONS = [".csv"]
+        writer.SUPPORT_COMPRESSED = True
+        writer.SUPPORT_SPARSE_DATA = False
+        writer.OPTIONAL_TYPE_ANNOTATIONS = False
+        writers = [writer]
+        filters = {"csv (*.csv)": writer}
 
-        self.widget = self.create_widget(OWSaveMockWriter)
+    def setUp(self):
+        self.widget = self.create_widget(self.OWSaveMockWriter)
 
     def test_no_data_no_save(self):
         widget = self.widget
@@ -96,6 +97,169 @@ def test_base_methods(self):
         self.assertIs(widget.valid_filters(), widget.get_filters())
         self.assertIs(widget.default_valid_filter(), widget.filter)
 
+    def assertPathEqual(self, a, b):
+        if sys.platform == "win32":
+            a = a.replace("\\", "/")
+            b = b.replace("\\", "/")
+        self.assertEqual(a.rstrip("/"), b.rstrip("/"))
+
+    @patch("os.path.exists", lambda name: name == "/home/u/orange/a/b")
+    def test_open_moved_workflow(self):
+        """Stored relative paths are properly changed on load"""
+        home = _userhome
+        home_c_foo = os.path.join(_userhome, "c.foo")
+        with patch("Orange.widgets.widget.OWWidget.workflowEnv",
+                   Mock(return_value={})):
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="a/b",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, home)
+            self.assertPathEqual(w.filename, home_c_foo)
+            self.assertFalse(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="/a/d",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, home)
+            self.assertPathEqual(w.filename, home_c_foo)
+            self.assertFalse(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path=".",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, home)
+            self.assertPathEqual(w.filename, home_c_foo)
+            self.assertFalse(w.auto_save)
+
+        with patch("Orange.widgets.widget.OWWidget.workflowEnv",
+                   Mock(return_value={"basedir": "/home/u/orange/"})):
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="a/b",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, "/home/u/orange/a/b")
+            self.assertPathEqual(w.filename, "/home/u/orange/a/b/c.foo")
+            self.assertTrue(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="a/b",
+                                     stored_name="c.foo",
+                                     auto_save=False))
+            self.assertPathEqual(w.last_dir, "/home/u/orange/a/b")
+            self.assertPathEqual(w.filename, "/home/u/orange/a/b/c.foo")
+            self.assertFalse(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="a/d",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, "/home/u/orange/")
+            self.assertPathEqual(w.filename, "/home/u/orange/c.foo")
+            self.assertFalse(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="/a/d",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, "/home/u/orange/")
+            self.assertPathEqual(w.filename, "/home/u/orange/c.foo")
+            self.assertFalse(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path=".",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, "/home/u/orange/")
+            self.assertPathEqual(w.filename, "/home/u/orange/c.foo")
+            self.assertFalse(w.auto_save)
+
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(stored_path="",
+                                     stored_name="c.foo",
+                                     auto_save=True))
+            self.assertPathEqual(w.last_dir, "/home/u/orange/")
+            self.assertPathEqual(w.filename, "/home/u/orange/c.foo")
+            self.assertFalse(w.auto_save)
+
+    def test_move_workflow(self):
+        """Widget correctly stores relative paths"""
+        w = self.widget
+        w._try_save = Mock()
+        w.update_messages = Mock()
+        env = {}
+
+        with patch("Orange.widgets.widget.OWWidget.workflowEnv",
+                   Mock(return_value=env)):
+            # File is save to subdirectory of workflow path
+            env["basedir"] = "/home/u/orange/"
+
+            w.get_save_filename = \
+                Mock(return_value=("/home/u/orange/a/b/c.foo", ""))
+            w.save_file_as()
+            self.assertPathEqual(w.last_dir, "/home/u/orange/a/b")
+            self.assertPathEqual(w.stored_path, "a/b/")
+            self.assertEqual(w.stored_name, "c.foo")
+
+            # Workflow path changes: relative path is changed to absolute
+            env["basedir"] = "/tmp/u/work/"
+            w.workflowEnvChanged("basedir", "/tmp/u/work", "/home/u/orange")
+            self.assertPathEqual(w.last_dir, "/home/u/orange/a/b/")
+            self.assertPathEqual(w.stored_path, "/home/u/orange/a/b/")
+            self.assertEqual(w.stored_name, "c.foo")
+
+            # Workflow path changes back: absolute path is again relative
+            env["basedir"] = "/home/u/orange/"
+            w.workflowEnvChanged("basedir", "/home/u/orange", "/tmp/u/work")
+            self.assertPathEqual(w.last_dir, "/home/u/orange/a/b")
+            self.assertPathEqual(w.stored_path, "a/b/")
+            self.assertEqual(w.stored_name, "c.foo")
+
+            # File is saved to an unrelated directory: path is absolute
+            w.get_save_filename = \
+                Mock(return_value=("/tmp/u/work/a/b/c.foo", ""))
+            w.save_file_as()
+            self.assertPathEqual(w.last_dir, "/tmp/u/work/a/b/")
+            self.assertPathEqual(w.stored_path, "/tmp/u/work/a/b/")
+            self.assertEqual(w.stored_name, "c.foo")
+
+            # File is saved to the workflow's directory: path is relative
+            w.get_save_filename = \
+                Mock(return_value=("/home/u/orange/c.foo", ""))
+            w.save_file_as()
+            self.assertPathEqual(w.last_dir, "/home/u/orange/")
+            self.assertPathEqual(w.stored_path, ".")
+            self.assertEqual(w.stored_name, "c.foo")
+
+    def test_migrate_pre_relative_settings(self):
+        with patch("os.path.exists", lambda name: name == "/a/b"):
+            w = self.create_widget(
+                self.OWSaveMockWriter,
+                stored_settings=dict(last_dir="/a/b", filename="/a/b/c.foo"))
+            self.assertPathEqual(w.last_dir, "/a/b")
+            self.assertPathEqual(w.filename, "/a/b/c.foo")
+            self.assertPathEqual(w.stored_path, "/a/b")
+            self.assertPathEqual(w.stored_name, "c.foo")
+
+        w = self.create_widget(
+            self.OWSaveMockWriter,
+            stored_settings=dict(last_dir="/a/b", filename="/a/b/c.foo"))
+        self.assertPathEqual(w.last_dir, _userhome)
+        self.assertPathEqual(w.filename, os.path.join(_userhome, "c.foo"))
+        self.assertPathEqual(w.stored_path, _userhome)
+        self.assertPathEqual(w.stored_name, "c.foo")
+
 
 class TestOWSaveBase(WidgetTest):
     # Tests for OWSaveBase methods with filters as list
@@ -142,7 +306,7 @@ class TestOWSaveUtils(unittest.TestCase):
     def test_replace_extension(self):
         class OWMockSaveBase(OWSaveBase):
             filters = ["Tab delimited (*.tab)",
-                       "Compressed tab delimitd (*.gz.tab)",
+                       "Compressed tab delimited (*.gz.tab)",
                        "Comma separated (*.csv)",
                        "Compressed comma separated (*.csv.gz)",
                        "Excel File (*.xlsx)"]
