Merge pull request #4532 from janezd/relative-save-paths

PrimozGodec · web-flow · commit 6fc07416190b · 2020-03-20T15:48:45.000+01:00
[ENH] Save widgets: Store paths relative to workflow directory
diff --git a/Orange/widgets/data/tests/test_owsave.py b/Orange/widgets/data/tests/test_owsave.py
@@ -79,7 +79,6 @@ def test_dataset(self):
 
     def test_initial_start_dir(self):
         widget = self.widget
-        widget.filename = _w("/usr/foo/bar.csv")
         self.assertEqual(widget.initial_start_dir(),
                          _w(os.path.expanduser("~/")))
 
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()))
@@ -81,7 +90,10 @@ def __init__(self, start_row=0):
                 callback=self.update_messages),
             start_row, 0, 1, 2)
         grid.setRowMinimumHeight(start_row + 1, 8)
-        self.bt_save = gui.button(None, self, "Save", callback=self.save_file)
+        self.bt_save = gui.button(
+            None, self,
+            label=f"Save as {self.stored_name}" if self.stored_name else "Save",
+            callback=self.save_file)
         grid.addWidget(self.bt_save, start_row + 2, 0)
         grid.addWidget(
             gui.button(None, self, "Save as ...", callback=self.save_file_as),
@@ -90,6 +102,61 @@ 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)
+
+    # pylint: disable=unused-argument
+    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 +200,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 +309,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,175 @@ 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")
+
+    def test_save_button_label(self):
+        w = self.create_widget(
+            self.OWSaveMockWriter,
+            stored_settings=dict(stored_path="", stored_name="c.foo"))
+        self.assertTrue(w.bt_save.text().endswith(" c.foo"))
+
 
 class TestOWSaveBase(WidgetTest):
     # Tests for OWSaveBase methods with filters as list
@@ -142,7 +312,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)"]
diff --git a/doc/visual-programming/source/widgets/data/save.md b/doc/visual-programming/source/widgets/data/save.md
@@ -11,6 +11,8 @@ The **Save Data** widget considers a dataset provided in the input channel and s
 
 The widget does not save the data every time it receives a new signal in the input as this would constantly (and, mostly, inadvertently) overwrite the file. Instead, the data is saved only after a new file name is set or the user pushes the *Save* button.
 
+If the file is saved to the same directory as the workflow or in the subtree of that directory, the widget remembers the relative path. Otherwise it will store an absolute path, but disable auto save for security reasons.
+
 ![](images/Save-stamped.png)
 
 1. Save by overwriting the existing file.
diff --git a/doc/visual-programming/source/widgets/model/savemodel.md b/doc/visual-programming/source/widgets/model/savemodel.md
@@ -3,6 +3,8 @@ Save Model
 
 Save a trained model to an output file.
 
+If the file is saved to the same directory as the workflow or in the subtree of that directory, the widget remembers the relative path. Otherwise it will store an absolute path, but disable auto save for security reasons.
+
 **Inputs**
 
 - Model: trained model
diff --git a/doc/visual-programming/source/widgets/unsupervised/savedistancematrix.md b/doc/visual-programming/source/widgets/unsupervised/savedistancematrix.md
@@ -3,6 +3,8 @@ Save Distance Matrix
 
 Saves a distance matrix.
 
+If the file is saved to the same directory as the workflow or in the subtree of that directory, the widget remembers the relative path. Otherwise it will store an absolute path, but disable auto save for security reasons.
+
 **Inputs**
 
 - Distances: distance matrix
