Save dialogs: Avoid weird behaviour with extensions on macOS

Author: janezd

Orange/widgets/data/owsave.py

@@ -128,14 +128,13 @@ def migrate_to_version_2():
 
     def initial_start_dir(self):
         if self.filename and os.path.exists(os.path.split(self.filename)[0]):
-            return self.filename
+            return os.path.splitext(self.filename)[0]
         else:
             data_name = getattr(self.data, 'name', '')
             if data_name:
                 if self.writer is None:
                     self.filter = self.default_filter()
                 assert self.writer is not None
-                data_name += self.writer.EXTENSIONS[0]
             return os.path.join(self.last_dir or _userhome, data_name)
 
     def valid_filters(self):

Orange/widgets/data/tests/test_owsave.py

@@ -73,20 +73,19 @@ def test_initial_start_dir(self):
 
         with patch("os.path.exists", return_value=True):
             widget.filename = _w("/usr/foo/bar.csv")
-            self.assertEqual(widget.initial_start_dir(), widget.filename)
+            self.assertEqual(widget.initial_start_dir(), widget.filename[:-4])
 
             widget.filename = ""
             widget.last_dir = _w("/usr/bar")
             self.assertEqual(widget.initial_start_dir(), _w("/usr/bar/"))
 
             widget.last_dir = _w("/usr/bar")
             self.send_signal(widget.Inputs.data, self.iris)
-            self.assertEqual(widget.initial_start_dir(),
-                             _w("/usr/bar/iris.csv"))
+            self.assertEqual(widget.initial_start_dir(), _w("/usr/bar/iris"))
 
             widget.last_dir = ""
             self.assertEqual(widget.initial_start_dir(),
-                             os.path.expanduser(_w("~/iris.csv")))
+                             os.path.expanduser(_w("~/iris")))
 
     @patch("Orange.widgets.utils.save.owsavebase.QFileDialog.getSaveFileName")
     def test_save_file_sets_name(self, _filedialog):
@@ -521,15 +520,6 @@ def test_save_file_dialog_uses_valid_filters_linux(self):
 @unittest.skipUnless(sys.platform in ("darwin", "win32"),
                      "Test for native dialog on Windows and macOS")
 class TestOWSaveDarwinDialog(OWSaveTestBase):  # pragma: no cover
-    if sys.platform == "darwin":
-        @staticmethod
-        def remove_star(filt):
-            return filt.replace(" (*.", " (.")
-    else:
-        @staticmethod
-        def remove_star(filt):
-            return filt
-
     @patch("Orange.widgets.utils.save.owsavebase.QFileDialog")
     def test_get_save_filename_darwin(self, dlg):
         widget = self.widget
@@ -540,14 +530,11 @@ def test_get_save_filename_darwin(self, dlg):
         instance = dlg.return_value
         instance.exec.return_value = dlg.Accepted = QFileDialog.Accepted
         instance.selectedFiles.return_value = ["foo"]
-        instance.selectedNameFilter.return_value = self.remove_star("aa (*.a)")
+        instance.selectedNameFilter.return_value = "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],
-            self.remove_star("aa (*.a);;bb (*.b);;cc (*.c)"))
-        instance.selectNameFilter.assert_called_with(
-            self.remove_star("bb (*.b)"))
+        self.assertEqual(dlg.call_args[0][3], "aa (*.a);;bb (*.b);;cc (*.c)")
+        instance.selectNameFilter.assert_called_with("bb (*.b)")
 
         instance.exec.return_value = dlg.Rejected = QFileDialog.Rejected
         self.assertEqual(widget.get_save_filename(), ("", ""))
@@ -567,7 +554,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.remove_star(filter1)
+        instance.selectedNameFilter.return_value = filter1
         instance.selectedFiles.return_value = ["foo"]
         self.assertEqual(widget.get_save_filename()[0], "foo.tab")
         instance.selectedFiles.return_value = ["foo.pkl"]
@@ -579,7 +566,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.remove_star(filter2)
+        instance.selectedNameFilter.return_value = filter2
         instance.selectedFiles.return_value = ["foo"]
         self.assertEqual(widget.get_save_filename()[0], "foo.csv.gz")
         instance.selectedFiles.return_value = ["foo.pkl"]
@@ -591,36 +578,6 @@ 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.csv.gz")
 
-    @patch("Orange.widgets.utils.save.owsavebase.QFileDialog")
-    @patch("os.path.exists", new=lambda x: x == "old.tab")
-    @patch("Orange.widgets.utils.save.owsavebase.QMessageBox")
-    def test_save_file_dialog_asks_for_overwrite_darwin(self, msgbox, dlg):
-        def selected_files():
-            nonlocal attempts
-            attempts += 1
-            return [["old.tab", "new.tab"][attempts]]
-
-        widget = self.widget
-        widget.initial_start_dir = lambda: "baz"
-        filter1 = ""  # prevent pylint warning 'undefined-loop-variable'
-        for filter1 in widget.get_filters():
-            if OWSaveBase._extension_from_filter(filter1) == ".tab":
-                break
-
-        widget.filter = filter1
-        instance = dlg.return_value
-        instance.exec.return_value = QFileDialog.Accepted
-        instance.selectedFiles = selected_files
-        instance.selectedNameFilter.return_value = self.remove_star(filter1)
-
-        attempts = -1
-        msgbox.question.return_value = msgbox.Yes = 1
-        self.assertEqual(widget.get_save_filename()[0], "old.tab")
-
-        attempts = -1
-        msgbox.question.return_value = msgbox.No = 0
-        self.assertEqual(widget.get_save_filename()[0], "new.tab")
-
     @patch("Orange.widgets.utils.save.owsavebase.QFileDialog")
     def test_save_file_dialog_uses_valid_filters_darwin(self, dlg):
         widget = self.widget
@@ -629,10 +586,8 @@ def test_save_file_dialog_uses_valid_filters_darwin(self, dlg):
         instance = dlg.return_value
         instance.exec.return_value = dlg.Rejected = QFileDialog.Rejected
         widget.get_save_filename()
-        self.assertEqual(
-            dlg.call_args[0][3], self.remove_star("aa (*.a);;bb (*.b)"))
-        instance.selectNameFilter.assert_called_with(
-            self.remove_star("aa (*.a)"))
+        self.assertEqual(dlg.call_args[0][3], "aa (*.a);;bb (*.b)")
+        instance.selectNameFilter.assert_called_with("aa (*.a)")
 
 
 if __name__ == "__main__":

Orange/widgets/utils/save/owsavebase.py

@@ -10,6 +10,8 @@
 
 _userhome = os.path.expanduser(f"~{os.sep}")
 
+_IS_DARWIN = sys.platform == "darwin"
+_IS_WIN32 = sys.platform == "win32"
 
 class OWSaveBase(widget.OWWidget, openclass=True):
     """
@@ -300,7 +302,7 @@ def initial_start_dir(self):
         Return either the current file's path, the last directory or home.
         """
         if self.filename and os.path.exists(os.path.split(self.filename)[0]):
-            return self.filename
+            return os.path.splitext(self.filename)[0]
         else:
             return self.last_dir or _userhome
 
@@ -353,39 +355,30 @@ def migrate_settings(cls, settings, version):
 
     # 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"):
-        # 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.
+    if _IS_DARWIN or _IS_WIN32:
         def get_save_filename(self):  # pragma: no cover
-            if sys.platform == "darwin":
-                def remove_star(filt):
-                    return filt.replace(" (*.", " (.")
-            else:
-                def remove_star(filt):
-                    return filt
-
-            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(no_ext_filters))
+                    None, "Save File", filename, ";;".join(self.valid_filters()))
                 dlg.setAcceptMode(dlg.AcceptSave)
-                dlg.selectNameFilter(remove_star(self.default_valid_filter()))
-                dlg.setOption(QFileDialog.DontConfirmOverwrite)
+                dlg.selectNameFilter(self.default_valid_filter())
+                # MacOs (currently) ignores DontConfirmOverwrite
+                # Let us not set it, so we know it's not set in the future
+                if _IS_WIN32:
+                    dlg.setOption(QFileDialog.DontConfirmOverwrite)
                 if dlg.exec() == QFileDialog.Rejected:
                     return "", ""
                 filename = dlg.selectedFiles()[0]
-                selected_filter = no_ext_filters[dlg.selectedNameFilter()]
+                selected_filter = dlg.selectedNameFilter()
                 filename = self._replace_extension(
                     filename, self._extension_from_filter(selected_filter))
-                if not os.path.exists(filename) or QMessageBox.question(
+                if (not os.path.exists(filename)
+                    or _IS_DARWIN  # MacOs already asked for confirmation
+                    or QMessageBox.question(
                         self, "Overwrite file?",
                         f"File {os.path.split(filename)[1]} already exists.\n"
-                        "Overwrite?") == QMessageBox.Yes:
+                        "Overwrite?") == QMessageBox.Yes):
                     return filename, selected_filter
 
     else:  # Linux and any unknown platforms

i18n/si/msgs.jaml

@@ -13642,6 +13642,8 @@ widgets/utils/plot/owplotgui_obsolete.py:
             Dark: false
 widgets/utils/save/owsavebase.py:
     ~{os.sep}: false
+    darwin: false
+    win32: false
     class `OWSaveBase`:
         class `Information`:
             Empty input; nothing was saved.: Ni podatkov.
@@ -13678,8 +13680,6 @@ widgets/utils/save/owsavebase.py:
             stored_path: false
             filename: false
             stored_name: false
-        darwin: false
-        win32: false
         def `get_save_filename`:
             Save File: Shrani datoteko
             ;;: false