Merge 5048eac9e8 into 7e3df43463

qutebrowser/browser/downloads.py

@@ -500,6 +500,28 @@ class AbstractDownloadItem(QObject):
                         remaining=remaining, perc=perc, down=down,
                         total=total, errmsg=errmsg))
 
+    def _find_next_filename(self) -> None:
+        """Append unique numeric suffix to filename and rerun _set_filename."""
+        assert self._filename is not None
+        path, file = os.path.split(self._filename)
+        # Pull out filename extension which could be a two part one like
+        # `tar.gz`. Use a more restrictive character set for the first part of
+        # a two part extension to avoid matching numbers.
+        match = re.fullmatch(r'(.+?)((\.[a-z]+)?\.[^.]+)', file)
+        if match:
+            base, suffix = match[1], match[2]
+        else:
+            base = file
+            suffix = ''
+
+        for i in range(2, 1000):
+            filename = os.path.join(path, f'{base}_{i}{suffix}')
+            if not (os.path.exists(filename) or self._get_conflicting_download()):
+                self._set_filename(filename)
+                break
+        else:
+            self._die('Alternative filename not available.')
+
     def _do_die(self):
         """Do cleanup steps after a download has died."""
         raise NotImplementedError
@@ -650,7 +672,8 @@ class AbstractDownloadItem(QObject):
         """Finish initialization based on self._filename."""
         raise NotImplementedError
 
-    def _ask_confirm_question(self, title, msg, *, custom_yes_action=None):
+    def _ask_confirm_question(self, title, msg, *, custom_yes_action=None,
+                              custom_no_action=None):
         """Ask a confirmation question for the download."""
         raise NotImplementedError
 
@@ -746,19 +769,21 @@ class AbstractDownloadItem(QObject):
 
         log.downloads.debug("Setting filename to {}".format(self._filename))
         if self._get_conflicting_download():
-            txt = ("<b>{}</b> is already downloading. Cancel and "
+            txt = ("<b>{}</b> is already downloading. Cancel and re-download?"
-                   "re-download?".format(html.escape(self._filename)))
+                   "(\"No\" renames.)".format(html.escape(self._filename)))
             self._ask_confirm_question(
                 "Cancel other download?", txt,
-                custom_yes_action=self._cancel_conflicting_download)
+                custom_yes_action=self._cancel_conflicting_download,
+                custom_no_action=self._find_next_filename)
         elif force_overwrite:
             self._after_set_filename()
         elif os.path.isfile(self._filename):
             # The file already exists, so ask the user if it should be
             # overwritten.
-            txt = "<b>{}</b> already exists. Overwrite?".format(
+            txt = "<b>{}</b> already exists. Overwrite? (\"No\" renames.)".format(
                 html.escape(self._filename))
-            self._ask_confirm_question("Overwrite existing file?", txt)
+            self._ask_confirm_question("Overwrite existing file?", txt,
+                custom_no_action=self._find_next_filename)
         # FIFO, device node, etc. Make sure we want to do this
         elif (os.path.exists(self._filename) and
               not os.path.isdir(self._filename)):

qutebrowser/browser/qtnetworkdownloads.py

@@ -225,12 +225,14 @@ class DownloadItem(downloads.AbstractDownloadItem):
     def _after_set_filename(self):
         self._create_fileobj()
 
-    def _ask_confirm_question(self, title, msg, *, custom_yes_action=None):
+    def _ask_confirm_question(self, title, msg, *, custom_yes_action=None,
+                              custom_no_action=None):
         yes_action = custom_yes_action or self._after_set_filename
-        no_action = functools.partial(self.cancel, remove_data=False)
+        cancel_action = functools.partial(self.cancel, remove_data=False)
+        no_action = custom_no_action or cancel_action
         url = 'file://{}'.format(self._filename)
         message.confirm_async(title=title, text=msg, yes_action=yes_action,
-                              no_action=no_action, cancel_action=no_action,
+                              no_action=no_action, cancel_action=cancel_action,
                               abort_on=[self.cancelled, self.error], url=url)
 
     def _ask_create_parent_question(self, title, msg,

qutebrowser/browser/webengine/webenginedownloads.py

@@ -142,9 +142,11 @@ class DownloadItem(downloads.AbstractDownloadItem):
                              "state {} (not in requested state)!".format(
                                  filename, self, state_name))
 
-    def _ask_confirm_question(self, title, msg, *, custom_yes_action=None):
+    def _ask_confirm_question(self, title, msg, *, custom_yes_action=None,
+                              custom_no_action=None):
         yes_action = custom_yes_action or self._after_set_filename
-        no_action = functools.partial(self.cancel, remove_data=False)
+        cancel_action = functools.partial(self.cancel, remove_data=False)
+        no_action = custom_no_action or cancel_action
         question = usertypes.Question()
         question.title = title
         question.text = msg
@@ -152,7 +154,7 @@ class DownloadItem(downloads.AbstractDownloadItem):
         question.mode = usertypes.PromptMode.yesno
         question.answered_yes.connect(yes_action)
         question.answered_no.connect(no_action)
-        question.cancelled.connect(no_action)
+        question.cancelled.connect(cancel_action)
         self.cancelled.connect(question.abort)
         self.error.connect(question.abort)
         message.global_bridge.ask(question, blocking=True)

tests/end2end/features/downloads.feature

@@ -560,11 +560,35 @@ Feature: Downloading things from a website.
     Scenario: Not overwriting an existing file
         When I set downloads.location.prompt to false
         And I run :download http://localhost:(port)/data/downloads/download.bin
-        And I wait until the download is finished
         And I run :download http://localhost:(port)/data/downloads/download2.bin --dest download.bin
         And I wait for "Entering mode KeyMode.yesno *" in the log
         And I run :prompt-accept no
+        And I wait until the download download.bin is finished
+        And I wait until the download download_2.bin is finished
         Then the downloaded file download.bin should be 1 bytes big
+        And the downloaded file download_2.bin should be 2 bytes big
+
+    Scenario: Not overwriting an existing file with double extension
+        When I set downloads.location.prompt to false
+        And I run :download http://localhost:(port)/data/downloads/download_with_dots.tar.bz2
+        And I run :download http://localhost:(port)/data/downloads/download_with_dots.tar.bz2
+        And I wait for "Entering mode KeyMode.yesno *" in the log
+        And I run :prompt-accept no
+        And I wait until the download download_with_dots.tar.bz2 is finished
+        And I wait until the download download_with_dots_2.tar.bz2 is finished
+        Then the downloaded file download_with_dots.tar.bz2 should be 1 bytes big
+        Then the downloaded file download_with_dots_2.tar.bz2 should be 1 bytes big
+
+    Scenario: Not overwriting an existing file with dots
+        When I set downloads.location.prompt to false
+        And I run :download http://localhost:(port)/data/downloads/download_with_dots_11.22.33.bin
+        And I run :download http://localhost:(port)/data/downloads/download_with_dots_11.22.33.bin
+        And I wait for "Entering mode KeyMode.yesno *" in the log
+        And I run :prompt-accept no
+        And I wait until the download download_with_dots_11.22.33.bin is finished
+        And I wait until the download download_with_dots_11.22.33_2.bin is finished
+        Then the downloaded file download_with_dots_11.22.33.bin should be 1 bytes big
+        Then the downloaded file download_with_dots_11.22.33_2.bin should be 1 bytes big
 
     Scenario: Overwriting an existing file
         When I set downloads.location.prompt to false

tests/unit/browser/test_downloads.py

@@ -2,6 +2,8 @@
 #
 # SPDX-License-Identifier: GPL-3.0-or-later
 
+import os
+
 import pytest
 
 from qutebrowser.browser import downloads, qtnetworkdownloads
@@ -115,6 +117,33 @@ def test_sanitized_filenames(raw, expected,
     assert item._filename.endswith(expected)
 
 
+@pytest.mark.parametrize('filename, expected', [
+    ("noext", "noext_2"),
+    ("simple.gif", "simple_2.gif"),
+    ("twoparts.tar.gz", "twoparts_2.tar.gz"),
+    ("many.dots.in.the.name", "many.dots.in_2.the.name"),
+    ("a. space.gif", "a. space_2.gif"),
+    ("non-ascii-📍.gif", "non-ascii-📍_2.gif"),
+    ("non-ascii.📍", "non-ascii_2.📍"),
+    ("non-ascii.📍.gif", "non-ascii.📍_2.gif"),
+    ("numbers_22.10.05.jpeg", "numbers_22.10.05_2.jpeg"),
+    ("Sentance..gif", "Sentance._2.gif"),
+])
+def test_generated_filename_suffix(
+    filename, expected, config_stub, download_tmpdir, monkeypatch
+):
+    manager = downloads.AbstractDownloadManager()
+    item = downloads.AbstractDownloadItem(manager=manager)
+
+    # Abstract methods
+    item._ensure_can_set_filename = lambda *args: True
+    item._after_set_filename = lambda *args: True
+
+    item._set_filename(filename)
+    item._find_next_filename()
+    assert os.path.basename(item._filename) == expected
+
+
 class TestConflictingDownloads:
 
     @pytest.fixture