Merge pull request #2814 from markotoplak/refactor_owsave

Refactor OWSave

Author: janezd

Orange/widgets/data/owsave.py

@@ -25,9 +25,11 @@ class Inputs:
     last_filter = Setting("")
     auto_save = Setting(False)
 
-    writers = FileFormat.writers
-    sparse_writers = {ext: w for ext, w in FileFormat.writers.items()
-                      if getattr(w, 'SUPPORT_SPARSE_DATA', False)}
+    @classmethod
+    def get_writers(cls, sparse):
+        return [f for f in FileFormat.formats
+                if getattr(f, 'write_file', None) and getattr(f, "EXTENSIONS", None)
+                and (not sparse or getattr(f, 'SUPPORT_SPARSE_DATA', False))]
 
     def __init__(self):
         super().__init__()
@@ -63,9 +65,8 @@ def save_file_as(self):
         file_name = self.filename or \
             os.path.join(self.last_dir or os.path.expanduser("~"),
                          getattr(self.data, 'name', ''))
-        filename, writer, filter = filedialogs.get_file_name(
-            file_name, self.last_filter,
-            self.sparse_writers if self.data.is_sparse() else self.writers)
+        filename, writer, filter = filedialogs.open_filename_dialog_save(
+            file_name, self.last_filter, self.get_writers(self.data.is_sparse()))
         if not filename:
             return
         self.filename = filename

Orange/widgets/data/tests/test_owfile.py

@@ -17,6 +17,7 @@
 import Orange
 from Orange.data import FileFormat, dataset_dirs, StringVariable, Table, \
     Domain, DiscreteVariable
+from Orange.util import OrangeDeprecationWarning
 from Orange.data.io import TabReader
 from Orange.tests import named_file
 from Orange.widgets.data.owfile import OWFile
@@ -27,14 +28,6 @@
 TITANIC_PATH = path.join(path.dirname(Orange.__file__), 'datasets', 'titanic.tab')
 
 
-class AddedFormat(FileFormat):
-    EXTENSIONS = ('.123',)
-    DESCRIPTION = "Test if a dialog format works after reading OWFile"
-
-    def read(self):
-        pass
-
-
 class FailedSheetsFormat(FileFormat):
     EXTENSIONS = ('.failed_sheet',)
     DESCRIPTION = "Make a sheet function that fails"
@@ -338,10 +331,22 @@ def test_drop_data_when_everything_skipped(self):
         data = self.get_output(self.widget.Outputs.data)
         self.assertIsNone(data)
 
+    def test_call_deprecated_dialog_formats(self):
+        with self.assertWarns(OrangeDeprecationWarning):
+            self.assertIn("Tab", dialog_formats())
+
     def test_add_new_format(self):
         # test adding file formats after registering the widget
-        formats = dialog_formats()
-        self.assertTrue(".123" in formats)
+        called = False
+        with named_file("", suffix=".tab") as filename:
+            def test_format(sd, sf, ff, **kwargs):
+                nonlocal called
+                called = True
+                self.assertIn(FailedSheetsFormat, ff)
+                return filename, TabReader, ""
+            with patch("Orange.widgets.data.owfile.open_filename_dialog", test_format):
+                self.widget.browse_file()
+        self.assertTrue(called)
 
     def test_domain_editor_conversions(self):
         dat = """V0\tV1\tV2\tV3\tV4\tV5\tV6

Orange/widgets/data/tests/test_owsave.py

@@ -0,0 +1,75 @@
+# Test methods with long descriptive names can omit docstrings
+# pylint: disable=missing-docstring
+from unittest.mock import patch
+
+from Orange.data import Table
+from Orange.data.io import TabReader, PickleReader, FileFormat
+from Orange.tests import named_file
+from Orange.widgets.tests.base import WidgetTest
+from Orange.widgets.utils.filedialogs import format_filter, fix_extension, open_filename_dialog_save
+from Orange.widgets.data.owsave import OWSave
+
+
+class AddedFormat(FileFormat):
+    EXTENSIONS = ('.234',)
+    DESCRIPTION = "Test if a dialog format works after reading OWSave"
+
+    def write_file(self):
+        pass
+
+
+class TestOWSave(WidgetTest):
+
+    def setUp(self):
+        self.widget = self.create_widget(OWSave)  # type: OWSave
+
+    def test_ordinary_save(self):
+        self.send_signal(self.widget.Inputs.data, Table("iris"))
+
+        for ext, writer in [('.tab', TabReader), ('.pickle', PickleReader)]:
+            with named_file("", suffix=ext) as filename:
+                def choose_file(a, b, c, d, e, fn=filename, w=writer):
+                    return fn, format_filter(w)
+                with patch("AnyQt.QtWidgets.QFileDialog.getSaveFileName", choose_file):
+                    self.widget.save_file_as()
+                self.assertEqual(len(Table(filename)), 150)
+
+    def test_filename_with_fix_extension(self):
+
+        def mock_fix_choice(ret):
+            f = lambda *x: ret
+            f.__dict__.update(fix_extension.__dict__)
+            return f
+
+        change_filter = iter([PickleReader, TabReader])
+
+        for file_choice, fix in [
+                [lambda *x: ("o.pickle", format_filter(TabReader)),
+                 mock_fix_choice(fix_extension.CHANGE_EXT)],
+                [lambda *x: ("o.tab", format_filter(PickleReader)),
+                 mock_fix_choice(fix_extension.CHANGE_FORMAT)],
+                [lambda *x: ("o.tab", format_filter(next(change_filter))),
+                 mock_fix_choice(fix_extension.CANCEL)]
+        ]:
+
+            with patch("AnyQt.QtWidgets.QFileDialog.getSaveFileName", file_choice),\
+                 patch("Orange.widgets.utils.filedialogs.fix_extension", fix):
+                saved_filename, format, filter = \
+                    open_filename_dialog_save(".", None, OWSave.get_writers(False))
+                self.assertEqual(saved_filename, "o.tab")
+                self.assertEqual(format, TabReader)
+                self.assertEqual(filter, format_filter(TabReader))
+
+    def test_added_format(self):
+        """Test that a format added after widget initialization is recognized"""
+        self.send_signal(self.widget.Inputs.data, Table("iris"))
+        called = False
+        with named_file("", suffix=".tab") as filename:
+            def test_format(sd, sf, ff, **kwargs):
+                nonlocal called
+                called = True
+                self.assertIn(AddedFormat, ff)
+                return filename, TabReader, ""
+            with patch("Orange.widgets.utils.filedialogs.open_filename_dialog", test_format):
+                self.widget.save_file_as()
+        self.assertTrue(called)

Orange/widgets/utils/filedialogs.py

@@ -7,6 +7,7 @@
 
 from Orange.data.io import FileFormat
 from Orange.widgets.settings import Setting
+from Orange.util import deprecated
 
 
 def fix_extension(ext, format, suggested_ext, suggested_format):
@@ -41,6 +42,7 @@ def format_filter(writer):
     return '{} (*{})'.format(writer.DESCRIPTION, ' *'.join(writer.EXTENSIONS))
 
 
+@deprecated
 def dialog_formats():
     """
     Return readable file types for QFileDialogs.
@@ -53,59 +55,56 @@ def dialog_formats():
 
 
 def get_file_name(start_dir, start_filter, file_formats):
-    """
-    Get filename for the given possible file formats
+    return open_filename_dialog_save(start_dir, start_filter,
+                                     sorted(set(file_formats.values()), key=lambda x: x.PRIORITY))
+
 
+def open_filename_dialog_save(start_dir, start_filter, file_formats):
+    """
     The function uses the standard save file dialog with filters from the
     given file formats. Extension is added automatically, if missing. If the
     user enters file extension that does not match the file format, (s)he is
     given a dialog to decide whether to fix the extension or the format.
 
-    Function also returns the writer and filter to cover the case where the
-    same extension appears in multiple filters. Although `file_format` is a
-    dictionary that associates its extension with one writer, writers can
-    still have other extensions that are allowed.
-
     Args:
         start_dir (str): initial directory, optionally including the filename
         start_filter (str): initial filter
-        file_formats (dict {extension: Orange.data.io.FileFormat}): file formats
+        file_formats (a list of Orange.data.io.FileFormat): file formats
     Returns:
         (filename, writer, filter), or `(None, None, None)` on cancel
     """
-    writers = sorted(set(file_formats.values()), key=lambda w: w.PRIORITY)
-    filters = [format_filter(w) for w in writers]
-    if start_filter not in filters:
-        start_filter = filters[0]
-
     while True:
-        filename, filter = QFileDialog.getSaveFileName(
-            None, 'Save As...', start_dir, ';;'.join(filters), start_filter)
+        dialog = QFileDialog.getSaveFileName
+        filename, format, filter = \
+            open_filename_dialog(start_dir, start_filter, file_formats,
+                                 add_all=False, title="Save as...", dialog=dialog)
         if not filename:
             return None, None, None
 
-        writer = writers[filters.index(filter)]
         base, ext = os.path.splitext(filename)
         if not ext:
-            filename += writer.EXTENSIONS[0]
-        elif ext not in writer.EXTENSIONS:
-            format = writer.DESCRIPTION
-            suggested_ext = writer.EXTENSIONS[0]
-            suggested_format = \
-                ext in file_formats and file_formats[ext].DESCRIPTION
-            res = fix_extension(ext, format, suggested_ext, suggested_format)
+            filename += format.EXTENSIONS[0]
+        elif ext not in format.EXTENSIONS:
+            suggested_ext = format.EXTENSIONS[0]
+            suggested_format = False
+            for f in file_formats:  # find the first format
+                if ext in f.EXTENSIONS:
+                    suggested_format = f
+                    break
+            res = fix_extension(ext, format.DESCRIPTION, suggested_ext,
+                                suggested_format.DESCRIPTION if suggested_format else False)
             if res == fix_extension.CANCEL:
                 continue
             if res == fix_extension.CHANGE_EXT:
                 filename = base + suggested_ext
             elif res == fix_extension.CHANGE_FORMAT:
-                writer = file_formats[ext]
-                filter = format_filter(writer)
-        return filename, writer, filter
+                format = suggested_format
+                filter = format_filter(format)
+        return filename, format, filter
 
 
-def open_filename_dialog(start_dir, start_filter, file_formats, title="Open...",
-                         dialog=None):
+def open_filename_dialog(start_dir, start_filter, file_formats,
+                         add_all=True, title="Open...", dialog=None):
     """
     Open file dialog with file formats.
 
@@ -116,6 +115,7 @@ def open_filename_dialog(start_dir, start_filter, file_formats, title="Open...",
         start_dir (str): initial directory, optionally including the filename
         start_filter (str): initial filter
         file_formats (a list of Orange.data.io.FileFormat): file formats
+        add_all (bool): add a filter for all supported extensions
         title (str): title of the dialog
         dialog: a function that creates a QT dialog
     Returns:
@@ -125,12 +125,13 @@ def open_filename_dialog(start_dir, start_filter, file_formats, title="Open...",
     filters = [format_filter(f) for f in file_formats]
 
     # add all readable files option
-    all_extensions = set()
-    for f in file_formats:
-        all_extensions.update(f.EXTENSIONS)
-    file_formats.insert(0, None)
-    filters.insert(0, "All readable files (*{})".format(
-        ' *'.join(sorted(all_extensions))))
+    if add_all:
+        all_extensions = set()
+        for f in file_formats:
+            all_extensions.update(f.EXTENSIONS)
+        file_formats.insert(0, None)
+        filters.insert(0, "All readable files (*{})".format(
+            ' *'.join(sorted(all_extensions))))
 
     if start_filter not in filters:
         start_filter = filters[0]