OWSave: Use the same dialog for Windows and macOS

Author: janezd

Orange/widgets/data/owsave.py

@@ -215,31 +215,33 @@ def _default_valid_filter(self):
 
     # 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 == "darwin":
-        # On macOS, is double suffixes are passed to the dialog, they are
-        # appended multiple times even if already present (QTBUG-44227).
-        # The only known workaround with native dialog is to use suffix *.*.
-        # We add the correct suffix after closing the dialog and only then check
+    if sys.platform in ("darwin", "win32"):
+        # macOS and Windows native dialogs do not correctly handle double
+        # extensions. We thus don't pass any suffixes to the dialog and add
+        # the correct suffix after closing the dialog and only then check
         # if the file exists and ask whether to override.
         # It is a bit confusing that the user does not see the final name in the
         # dialog, but I see no better solution.
         def get_save_filename(self):  # pragma: no cover
-            def no_suffix(filt):
-                return filt.split("(")[0] + "(*.*)"
+            if sys.platform == "darwin":
+                def remove_star(filt):
+                    return filt.replace(" (*.", " (.")
+            else:
+                def remove_star(filt):
+                    return filt
 
-            mac_filters = {no_suffix(f): f for f in self._valid_filters()}
+            no_ext_filters = {remove_star(f): f for f in self._valid_filters()}
             filename = self._initial_start_dir()
             while True:
                 dlg = QFileDialog(
-                    None, "Save File", filename, ";;".join(mac_filters))
+                    None, "Save File", filename, ";;".join(no_ext_filters))
                 dlg.setAcceptMode(dlg.AcceptSave)
-                dlg.selectNameFilter(no_suffix(self._default_valid_filter()))
-                dlg.setOption(QFileDialog.HideNameFilterDetails)
+                dlg.selectNameFilter(remove_star(self._default_valid_filter()))
                 dlg.setOption(QFileDialog.DontConfirmOverwrite)
                 if dlg.exec() == QFileDialog.Rejected:
                     return "", ""
                 filename = dlg.selectedFiles()[0]
-                selected_filter = mac_filters[dlg.selectedNameFilter()]
+                selected_filter = no_ext_filters[dlg.selectedNameFilter()]
                 filename = self._replace_extension(
                     filename, self._extension_from_filter(selected_filter))
                 if not os.path.exists(filename) or QMessageBox.question(
@@ -248,15 +250,6 @@ def no_suffix(filt):
                         "Overwrite?") == QMessageBox.Yes:
                     return filename, selected_filter
 
-    elif sys.platform == "win32":
-        # TODO: Behaviour of getSaveFileName on Windows is not tested!!!
-        # Windows native dialog may work correctly; if not, we may do the same
-        # as for macOS?
-        def get_save_filename(self):  # pragma: no cover
-            return QFileDialog.getSaveFileName(
-                self, "Save File", self._initial_start_dir(),
-                ";;".join(self._valid_filters()), self._default_valid_filter())
-
     else:  # Linux and any unknown platforms
         # Qt does not use a native dialog on Linux, so we can connect to
         # filterSelected and to overload selectFile to change the extension
@@ -279,7 +272,7 @@ def updateDefaultExtension(self, selected_filter):
                 self.suffix = OWSave._extension_from_filter(selected_filter)
                 files = self.selectedFiles()
                 if files and not os.path.isdir(files[0]):
-                    self.selectFile(files[0].split(".")[0])
+                    self.selectFile(files[0])
 
             def selectFile(self, filename):
                 filename = OWSave._replace_extension(filename, self.suffix)

Orange/widgets/data/tests/test_owsave.py

@@ -3,7 +3,6 @@
 from unittest.mock import patch, Mock
 import os
 import sys
-import re
 
 import scipy.sparse as sp
 from AnyQt.QtWidgets import QFileDialog
@@ -410,27 +409,26 @@ def test_get_save_filename_linux(self):
 
     def test_save_file_dialog_enforces_extension_linux(self):
         dialog = OWSave.SaveFileDialog(
-            None, "Save File", "high.txt",
-            "Bar files (*.bar);;Low files (*.low)")
+            None, "Save File", "foo.bar",
+            "Bar files (*.tab);;Low files (*.csv)")
 
-        dialog.selectNameFilter("Low files (*.low)")
-        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.low"))
+        dialog.selectNameFilter("Low files (*.csv)")
+        self.assertTrue(dialog.selectedFiles()[0].endswith("/foo.csv"))
 
-        dialog.selectFile("high.txt")
-        print(dialog.selectedFiles()[0])
-        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.txt.low"))
+        dialog.selectFile("high.bar")
+        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.bar.csv"))
 
-        dialog.selectNameFilter("Bar files (*.bar)")
-        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.bar"))
+        dialog.selectNameFilter("Bar files (*.tab)")
+        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.bar.tab"))
 
         dialog.selectFile("middle.pkl")
-        self.assertTrue(dialog.selectedFiles()[0].endswith("/middle.bar"))
+        self.assertTrue(dialog.selectedFiles()[0].endswith("/middle.tab"))
 
-        dialog.filterSelected.emit("Low files (*.low)")
-        self.assertTrue(dialog.selectedFiles()[0].endswith("/middle.low"))
+        dialog.filterSelected.emit("Low files (*.csv)")
+        self.assertTrue(dialog.selectedFiles()[0].endswith("/middle.csv"))
 
         dialog.selectFile("high.tab.gz")
-        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.low"))
+        self.assertTrue(dialog.selectedFiles()[0].endswith("/high.csv"))
 
     def test_save_file_dialog_uses_valid_filters_linux(self):
         widget = self.widget
@@ -444,26 +442,35 @@ def test_save_file_dialog_uses_valid_filters_linux(self):
         instance.selectNameFilter.assert_called_with("a (*.a)")
 
 
-@unittest.skipUnless(sys.platform == "darwin", "Test for dialog for macOS")
+@unittest.skipUnless(sys.platform in ("darwin", "win32"),
+                     "Test for native dialog on Windows and macOS")
 class TestOWSaveDarwinDialog(TestOWSaveBase):  # pragma: no cover
-    @staticmethod
-    def _mac_filter(filt):
-        return re.sub(r"\(\*\..*\)", "(*.*)", filt)
+    if sys.platform == "darwin":
+        @staticmethod
+        def remove_star(filt):
+            return filt.replace(" (*.", " (.")
+    else:
+        @staticmethod
+        def remove_star(filt):
+            return filt
 
     @patch("Orange.widgets.data.owsave.QFileDialog")
     def test_get_save_filename_darwin(self, dlg):
         widget = self.widget
         widget._initial_start_dir = lambda: "baz"
-        widget.filters = dict.fromkeys(("a (*.a)", "b (*.b)", "c (*.c)"))
-        widget.filter = "b (*.b)"
+        widget.filters = dict.fromkeys(("aa (*.a)", "bb (*.b)", "cc (*.c)"))
+        widget.filter = "bb (*.b)"
         instance = dlg.return_value
         instance.exec.return_value = dlg.Accepted = QFileDialog.Accepted
         instance.selectedFiles.return_value = ["foo"]
-        instance.selectedNameFilter.return_value = "a (*.*)"
-        self.assertEqual(widget.get_save_filename(), ("foo.a", "a (*.a)"))
+        instance.selectedNameFilter.return_value = self.remove_star("aa (*.a)")
+        self.assertEqual(widget.get_save_filename(), ("foo.a", "aa (*.a)"))
         self.assertEqual(dlg.call_args[0][2], "baz")
-        self.assertEqual(dlg.call_args[0][3], "a (*.*);;b (*.*);;c (*.*)")
-        instance.selectNameFilter.assert_called_with("b (*.*)")
+        self.assertEqual(
+            dlg.call_args[0][3],
+            self.remove_star("aa (*.a);;bb (*.b);;cc (*.c)"))
+        instance.selectNameFilter.assert_called_with(
+            self.remove_star("bb (*.b)"))
 
         instance.exec.return_value = dlg.Rejected = QFileDialog.Rejected
         self.assertEqual(widget.get_save_filename(), ("", ""))
@@ -482,7 +489,7 @@ def test_save_file_dialog_enforces_extension_darwin(self, dlg):
         instance = dlg.return_value
         instance.exec.return_value = QFileDialog.Accepted
 
-        instance.selectedNameFilter.return_value = self._mac_filter(filter1)
+        instance.selectedNameFilter.return_value = self.remove_star(filter1)
         instance.selectedFiles.return_value = ["foo"]
         self.assertEqual(widget.get_save_filename()[0], "foo.tab")
         instance.selectedFiles.return_value = ["foo.pkl"]
@@ -494,7 +501,7 @@ def test_save_file_dialog_enforces_extension_darwin(self, dlg):
         instance.selectedFiles.return_value = ["foo.bar"]
         self.assertEqual(widget.get_save_filename()[0], "foo.bar.tab")
 
-        instance.selectedNameFilter.return_value = self._mac_filter(filter2)
+        instance.selectedNameFilter.return_value = self.remove_star(filter2)
         instance.selectedFiles.return_value = ["foo"]
         self.assertEqual(widget.get_save_filename()[0], "foo.csv.gz")
         instance.selectedFiles.return_value = ["foo.pkl"]
@@ -525,7 +532,7 @@ def selected_files():
         instance = dlg.return_value
         instance.exec.return_value = QFileDialog.Accepted
         instance.selectedFiles = selected_files
-        instance.selectedNameFilter.return_value = self._mac_filter(filter1)
+        instance.selectedNameFilter.return_value = self.remove_star(filter1)
 
         attempts = -1
         msgbox.question.return_value = msgbox.Yes = 1
@@ -538,26 +545,15 @@ def selected_files():
     @patch("Orange.widgets.data.owsave.QFileDialog")
     def test_save_file_dialog_uses_valid_filters_darwin(self, dlg):
         widget = self.widget
-        widget._valid_filters = lambda: ["a (*.a)", "b (*.b)"]
-        widget._default_valid_filter = lambda: "a (*.a)"
+        widget._valid_filters = lambda: ["aa (*.a)", "bb (*.b)"]
+        widget._default_valid_filter = lambda: "aa (*.a)"
         instance = dlg.return_value
         instance.exec.return_value = dlg.Rejected = QFileDialog.Rejected
         widget.get_save_filename()
-        self.assertEqual(dlg.call_args[0][3], "a (*.*);;b (*.*)")
-        instance.selectNameFilter.assert_called_with("a (*.*)")
-
-
-@unittest.skipUnless(sys.platform == "win32", "Test for dialog for Windows")
-class TestOWSaveWindowsDialog(TestOWSaveBase):  # pragma: no cover
-    @patch("Orange.widgets.data.owsave.QFileDialog.getSaveFileName")
-    def test_save_file_dialog_uses_valid_filters_windows(self, dlg):
-        widget = self.widget
-        widget._valid_filters = lambda: ["a (*.a)", "b (*.b)"]
-        widget._default_valid_filter = lambda: "a (*.a)"
-        widget.get_save_filename()
-        call_args = dlg.call_args
-        self.assertEqual(call_args[0][3], "a (*.a);;b (*.b)")
-        self.assertEqual(call_args[0][4], "a (*.a)")
+        self.assertEqual(
+            dlg.call_args[0][3], self.remove_star("aa (*.a);;bb (*.b)"))
+        instance.selectNameFilter.assert_called_with(
+            self.remove_star("aa (*.a)"))
 
 
 class TestOWSaveUtils(unittest.TestCase):
@@ -592,6 +588,5 @@ def test_extension_from_filter(self):
             OWSave._extension_from_filter("Description (.foo.bar)"), ".foo.bar")
 
 
-
 if __name__ == "__main__":
     unittest.main()