From b6f4609dfed2ddd26bf79c39710f45de8443709e Mon Sep 17 00:00:00 2001 From: Andrew MacFie Date: Tue, 2 Feb 2021 18:53:26 -0500 Subject: [PATCH 01/11] Enhance non-overwriting --- qutebrowser/browser/downloads.py | 21 +++++++++++++++++-- qutebrowser/browser/qtnetworkdownloads.py | 6 ++++-- .../browser/webengine/webenginedownloads.py | 6 ++++-- tests/end2end/features/downloads.feature | 4 +++- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 9525c0618..8dfa62d02 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -492,6 +492,21 @@ class AbstractDownloadItem(QObject): remaining=remaining, perc=perc, down=down, total=total, errmsg=errmsg)) + def _find_next_filename(self) -> None: + assert self._filename is not None + path = pathlib.Path(self._filename) + suffix = ''.join(path.suffixes) + base = path.name[:-len(suffix)] + + i = 1 + while True: + i += 1 + path_to_try = path.parent / '{}_{}{}'.format(base, i, suffix) + self._filename = str(path_to_try) + if not (path_to_try.exists() or self._get_conflicting_download()): + break + self._set_filename(str(path_to_try)) + def _do_die(self): """Do cleanup steps after a download has died.""" raise NotImplementedError @@ -642,7 +657,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 @@ -750,7 +766,8 @@ class AbstractDownloadItem(QObject): # overwritten. txt = "{} already exists. Overwrite?".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)): diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 9dd507ab5..a9f87d8b6 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -225,9 +225,11 @@ 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) + no_action = custom_no_action or functools.partial( + self.cancel, remove_data=False) url = 'file://{}'.format(self._filename) message.confirm_async(title=title, text=msg, yes_action=yes_action, no_action=no_action, cancel_action=no_action, diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index e8e6418e0..5b7a38c4d 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/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) + no_action = custom_no_action or functools.partial( + self.cancel, remove_data=False) question = usertypes.Question() question.title = title question.text = msg diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index c2f359f14..889223aa9 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -546,11 +546,13 @@ 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: Overwriting an existing file When I set downloads.location.prompt to false From f5ba48a1ede730f38f06da3d5e4e1da78c3b60ac Mon Sep 17 00:00:00 2001 From: Andrew MacFie Date: Wed, 3 Feb 2021 10:48:55 -0500 Subject: [PATCH 02/11] Add docstring --- qutebrowser/browser/downloads.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 8dfa62d02..fe7562636 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -493,6 +493,10 @@ class AbstractDownloadItem(QObject): total=total, errmsg=errmsg)) def _find_next_filename(self) -> None: + """Append unique numeric suffix to filename. + + After finding a unique filename, restart the process of setting the + filename which will update the filename shown in the downloads list.""" assert self._filename is not None path = pathlib.Path(self._filename) suffix = ''.join(path.suffixes) From 5cc9261df2c70dd6f39c8fee226af0118a46f5e2 Mon Sep 17 00:00:00 2001 From: Andrew Date: Tue, 2 Feb 2021 19:09:47 -0500 Subject: [PATCH 03/11] Save if conflicting download not canceled --- qutebrowser/browser/downloads.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index fe7562636..64a3aa37f 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -762,7 +762,8 @@ class AbstractDownloadItem(QObject): "re-download?".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): From 3aaf7616efc5f9a5c355c51a96b93acffcdfe0cd Mon Sep 17 00:00:00 2001 From: Andrew MacFie Date: Wed, 3 Feb 2021 19:44:28 -0500 Subject: [PATCH 04/11] Resolve couple of things --- qutebrowser/browser/downloads.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 64a3aa37f..705eec18c 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -502,14 +502,14 @@ class AbstractDownloadItem(QObject): suffix = ''.join(path.suffixes) base = path.name[:-len(suffix)] - i = 1 - while True: - i += 1 - path_to_try = path.parent / '{}_{}{}'.format(base, i, suffix) + for i in range(2, 1000): + path_to_try = path.parent / f'{base}_{i}{suffix}' self._filename = str(path_to_try) if not (path_to_try.exists() or self._get_conflicting_download()): + self._set_filename(self._filename) break - self._set_filename(str(path_to_try)) + else: + self._die('Alternative filename not available.') def _do_die(self): """Do cleanup steps after a download has died.""" From c46a4522664575021da3fda7bfc2319809377877 Mon Sep 17 00:00:00 2001 From: Andrew MacFie Date: Thu, 11 Feb 2021 11:48:05 -0500 Subject: [PATCH 05/11] Fix suffixless files and aborting --- qutebrowser/browser/downloads.py | 5 ++++- qutebrowser/browser/qtnetworkdownloads.py | 6 +++--- qutebrowser/browser/webengine/webenginedownloads.py | 6 +++--- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 705eec18c..9ee4f219e 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -500,7 +500,10 @@ class AbstractDownloadItem(QObject): assert self._filename is not None path = pathlib.Path(self._filename) suffix = ''.join(path.suffixes) - base = path.name[:-len(suffix)] + if suffix: + base = path.name[:-len(suffix)] + else: + base = path.name for i in range(2, 1000): path_to_try = path.parent / f'{base}_{i}{suffix}' diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index a9f87d8b6..c91e3f452 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -228,11 +228,11 @@ class DownloadItem(downloads.AbstractDownloadItem): 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 = custom_no_action or 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, diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 5b7a38c4d..e8981bd5f 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -145,8 +145,8 @@ class DownloadItem(downloads.AbstractDownloadItem): 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 = custom_no_action or 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 @@ -154,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) From b956917b29d7082bcc5e5ea948aff472b4181af2 Mon Sep 17 00:00:00 2001 From: arza Date: Fri, 6 Oct 2023 20:40:36 +0300 Subject: [PATCH 06/11] Clarify download overwrite prompt --- qutebrowser/browser/downloads.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 9ee4f219e..e555b78c7 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -772,7 +772,7 @@ class AbstractDownloadItem(QObject): elif os.path.isfile(self._filename): # The file already exists, so ask the user if it should be # overwritten. - txt = "{} already exists. Overwrite?".format( + txt = "{} already exists. Overwrite? (\"No\" renames)".format( html.escape(self._filename)) self._ask_confirm_question("Overwrite existing file?", txt, custom_no_action=self._find_next_filename) From e6fd8a70512d61766abdf6b7df00e707422f1cd7 Mon Sep 17 00:00:00 2001 From: arza Date: Fri, 6 Oct 2023 21:06:21 +0300 Subject: [PATCH 07/11] Tune filename suffix generation --- qutebrowser/browser/downloads.py | 16 ++++++------- .../data/downloads/download_with_dots.tar.bz2 | Bin 0 -> 1 bytes .../downloads/download_with_dots_11.22.33.bin | Bin 0 -> 1 bytes tests/end2end/features/downloads.feature | 22 ++++++++++++++++++ 4 files changed, 30 insertions(+), 8 deletions(-) create mode 100644 tests/end2end/data/downloads/download_with_dots.tar.bz2 create mode 100644 tests/end2end/data/downloads/download_with_dots_11.22.33.bin diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index e555b78c7..ff21bd282 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -498,17 +498,17 @@ class AbstractDownloadItem(QObject): After finding a unique filename, restart the process of setting the filename which will update the filename shown in the downloads list.""" assert self._filename is not None - path = pathlib.Path(self._filename) - suffix = ''.join(path.suffixes) - if suffix: - base = path.name[:-len(suffix)] + path, file = os.path.split(self._filename) + match = re.fullmatch(r'(.+?)((\.[a-z]+)?\.[^.]+)', file) + if match: + base, suffix = match[1], match[2] else: - base = path.name + base = file + suffix = '' for i in range(2, 1000): - path_to_try = path.parent / f'{base}_{i}{suffix}' - self._filename = str(path_to_try) - if not (path_to_try.exists() or self._get_conflicting_download()): + self._filename = os.path.join(path, f'{base}_{i}{suffix}') + if not (os.path.exists(self._filename) or self._get_conflicting_download()): self._set_filename(self._filename) break else: diff --git a/tests/end2end/data/downloads/download_with_dots.tar.bz2 b/tests/end2end/data/downloads/download_with_dots.tar.bz2 new file mode 100644 index 0000000000000000000000000000000000000000..f76dd238ade08917e6712764a16a22005a50573d GIT binary patch literal 1 IcmZPo000310RR91 literal 0 HcmV?d00001 diff --git a/tests/end2end/data/downloads/download_with_dots_11.22.33.bin b/tests/end2end/data/downloads/download_with_dots_11.22.33.bin new file mode 100644 index 0000000000000000000000000000000000000000..f76dd238ade08917e6712764a16a22005a50573d GIT binary patch literal 1 IcmZPo000310RR91 literal 0 HcmV?d00001 diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 889223aa9..cb5ebc6cf 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -554,6 +554,28 @@ Feature: Downloading things from a website. 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 And I run :download http://localhost:(port)/data/downloads/download.bin From f9186b3b55e1c26ec54b808d593288fd40ddc202 Mon Sep 17 00:00:00 2001 From: arza Date: Fri, 6 Oct 2023 21:07:48 +0300 Subject: [PATCH 08/11] Fix style --- qutebrowser/browser/downloads.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index ff21bd282..f303b1e28 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -496,7 +496,8 @@ class AbstractDownloadItem(QObject): """Append unique numeric suffix to filename. After finding a unique filename, restart the process of setting the - filename which will update the filename shown in the downloads list.""" + filename which will update the filename shown in the downloads list. + """ assert self._filename is not None path, file = os.path.split(self._filename) match = re.fullmatch(r'(.+?)((\.[a-z]+)?\.[^.]+)', file) From aa4c8e4126a1e140baa74896965210b91dbdead8 Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 13 Jul 2024 16:45:29 +1200 Subject: [PATCH 09/11] Add unit tests for suffix generation for downloads The pushpin is my standard high unicode value codepoint. Some things handle umlauts etc fine but choke on higher values. The behavior seems fine enough I guess. Some people might prefer " (2)" instead of "_2". Also maybe we should start at 1 instead of 2? --- qutebrowser/browser/downloads.py | 3 +++ tests/unit/browser/test_downloads.py | 29 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index f303b1e28..d0297ff4e 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -500,6 +500,9 @@ class AbstractDownloadItem(QObject): """ 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] diff --git a/tests/unit/browser/test_downloads.py b/tests/unit/browser/test_downloads.py index 8dd4f0c31..87c63f0f0 100644 --- a/tests/unit/browser/test_downloads.py +++ b/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 @@ -113,6 +115,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 From 5d33d20d0c2bb7076f2e9436805272f171c5aab9 Mon Sep 17 00:00:00 2001 From: arza Date: Thu, 3 Jul 2025 22:09:57 +0300 Subject: [PATCH 10/11] Clarify conflicting download prompt --- qutebrowser/browser/downloads.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index d0297ff4e..6aadeea75 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -765,8 +765,8 @@ class AbstractDownloadItem(QObject): log.downloads.debug("Setting filename to {}".format(self._filename)) if self._get_conflicting_download(): - txt = ("{} is already downloading. Cancel and " - "re-download?".format(html.escape(self._filename))) + txt = ("{} is already downloading. Cancel and re-download?" + "(\"No\" renames.)".format(html.escape(self._filename))) self._ask_confirm_question( "Cancel other download?", txt, custom_yes_action=self._cancel_conflicting_download, From 5048eac9e8986e5942455b57ed55d7cde105845b Mon Sep 17 00:00:00 2001 From: arza Date: Thu, 3 Jul 2025 22:10:26 +0300 Subject: [PATCH 11/11] Improve code style, grammar and a docstring --- qutebrowser/browser/downloads.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 6aadeea75..7b9b3150a 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -493,11 +493,7 @@ class AbstractDownloadItem(QObject): total=total, errmsg=errmsg)) def _find_next_filename(self) -> None: - """Append unique numeric suffix to filename. - - After finding a unique filename, restart the process of setting the - filename which will update the filename shown in the downloads list. - """ + """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 @@ -511,9 +507,9 @@ class AbstractDownloadItem(QObject): suffix = '' for i in range(2, 1000): - self._filename = os.path.join(path, f'{base}_{i}{suffix}') - if not (os.path.exists(self._filename) or self._get_conflicting_download()): - self._set_filename(self._filename) + 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.') @@ -776,7 +772,7 @@ class AbstractDownloadItem(QObject): elif os.path.isfile(self._filename): # The file already exists, so ask the user if it should be # overwritten. - txt = "{} already exists. Overwrite? (\"No\" renames)".format( + txt = "{} already exists. Overwrite? (\"No\" renames.)".format( html.escape(self._filename)) self._ask_confirm_question("Overwrite existing file?", txt, custom_no_action=self._find_next_filename)