From 0a753915ffbee88c7f993fe2aefe4e30b75d727f Mon Sep 17 00:00:00 2001 From: Panagiotis K Date: Wed, 11 Oct 2017 13:40:53 +0300 Subject: [PATCH 1/6] Prompt for non-existing download directories. Closes #2120 --- qutebrowser/browser/downloads.py | 37 ++++++++++++++++++- qutebrowser/browser/qtnetworkdownloads.py | 11 ++++++ .../browser/webengine/webenginedownloads.py | 16 ++++++++ tests/end2end/features/downloads.feature | 7 +++- 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 07e5c7c30..19880535d 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -27,6 +27,7 @@ import collections import functools import pathlib import tempfile +import errno import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QModelIndex, @@ -137,7 +138,8 @@ def create_full_filename(basename, filename): encoding = sys.getfilesystemencoding() filename = utils.force_encoding(filename, encoding) basename = utils.force_encoding(basename, encoding) - if os.path.isabs(filename) and os.path.isdir(filename): + if os.path.isabs(filename) and (os.path.isdir(filename) or + os.path.join(filename, "") == filename): # We got an absolute directory from the user, so we save it under # the default filename in that directory. return os.path.join(filename, basename) @@ -657,11 +659,42 @@ class AbstractDownloadItem(QObject): self._filename = create_full_filename(self.basename, os.path.expanduser('~')) + dirname = os.path.dirname(self._filename) + if not os.path.exists(dirname): + txt = ("{} does not exist. Create it?". + format(html.escape( + os.path.join(dirname, "")))) + self._ask_create_parent_question("Create directory?", txt, + force_overwrite, + remember_directory) + else: + self._after_create_parent_question(force_overwrite, + remember_directory) + + def _after_create_parent_question(self, + force_overwrite, remember_directory): + """After asking about parent directory. + + Args: + force_overwrite: Force overwriting existing files. + remember_directory: If True, remember the directory for future + downloads. + """ + global last_used_directory + + try: + os.makedirs(os.path.dirname(self._filename)) + except OSError as e: + # Unlikely, but could be created before + # we get a chance to create it. + if e.errno != errno.EEXIST: + raise + self.basename = os.path.basename(self._filename) if remember_directory: last_used_directory = os.path.dirname(self._filename) - log.downloads.debug("Setting filename to {}".format(filename)) + log.downloads.debug("Setting filename to {}".format(self._filename)) if force_overwrite: self._after_set_filename() elif os.path.isfile(self._filename): diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index 921fee54d..709c8207b 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -203,6 +203,17 @@ class DownloadItem(downloads.AbstractDownloadItem): no_action=no_action, cancel_action=no_action, abort_on=[self.cancelled, self.error]) + def _ask_create_parent_question(self, title, msg, + force_overwrite, remember_directory): + no_action = functools.partial(self.cancel, remove_data=False) + message.confirm_async(title=title, text=msg, + yes_action=(lambda: + self._after_create_parent_question( + force_overwrite, + remember_directory)), + no_action=no_action, cancel_action=no_action, + abort_on=[self.cancelled, self.error]) + def _set_fileobj(self, fileobj, *, autoclose=True): """"Set the file object to write the download to. diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index ebb03828b..03baac359 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -120,6 +120,22 @@ class DownloadItem(downloads.AbstractDownloadItem): "state {} (not in requested state)!".format( filename, self, state_name)) + def _ask_create_parent_question(self, title, msg, + force_overwrite, remember_directory): + no_action = functools.partial(self.cancel, remove_data=False) + question = usertypes.Question() + question.title = title + question.text = msg + question.mode = usertypes.PromptMode.yesno + question.answered_yes.connect(lambda: + self._after_create_parent_question( + force_overwrite, remember_directory)) + question.answered_no.connect(no_action) + question.cancelled.connect(no_action) + self.cancelled.connect(question.abort) + self.error.connect(question.abort) + message.global_bridge.ask(question, blocking=True) + def _ask_confirm_question(self, title, msg): no_action = functools.partial(self.cancel, remove_data=False) question = usertypes.Question() diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 103ad0123..3df2e18a4 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -256,8 +256,11 @@ Feature: Downloading things from a website. Then the error "Can only download the current page as mhtml." should be shown Scenario: :download with a directory which doesn't exist - When I run :download --dest (tmpdir)/downloads/somedir/filename http://localhost:(port)/ - Then the error "Download error: No such file or directory" should be shown + When I run :download --dest (tmpdir)/downloads/somedir/filename http://localhost:(port)/data/downloads/download.bin + And I wait for "Asking question text='* does not exist. Create it?' title='Create directory?'>, *" in the log + And I run :prompt-accept yes + And I wait until the download filename is finished + Then the downloaded file filename should not exist ## mhtml downloads From 10388b0515581e8c2a1294ecb6fca5396383a23c Mon Sep 17 00:00:00 2001 From: Panagiotis K Date: Thu, 12 Oct 2017 15:16:35 +0300 Subject: [PATCH 2/6] Remove an unused variable. --- qutebrowser/browser/downloads.py | 1 - 1 file changed, 1 deletion(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 19880535d..2b14438a4 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -632,7 +632,6 @@ class AbstractDownloadItem(QObject): remember_directory: If True, remember the directory for future downloads. """ - global last_used_directory filename = os.path.expanduser(filename) self._ensure_can_set_filename(filename) From 630384e07f48200a132f601ac12d72dc21cf9f5c Mon Sep 17 00:00:00 2001 From: Panagiotis K Date: Fri, 13 Oct 2017 10:39:34 +0300 Subject: [PATCH 3/6] Fix tests. --- tests/end2end/features/downloads.feature | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 3df2e18a4..7648ad888 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -255,12 +255,19 @@ Feature: Downloading things from a website. When I run :download --mhtml http://foobar/ Then the error "Can only download the current page as mhtml." should be shown - Scenario: :download with a directory which doesn't exist - When I run :download --dest (tmpdir)/downloads/somedir/filename http://localhost:(port)/data/downloads/download.bin + Scenario: :download with a filename and directory which doesn't exist + When I run :download --dest (tmpdir)/downloads/somedir/file http://localhost:(port)/data/downloads/download.bin And I wait for "Asking question text='* does not exist. Create it?' title='Create directory?'>, *" in the log And I run :prompt-accept yes - And I wait until the download filename is finished - Then the downloaded file filename should not exist + And I wait until the download is finished + Then the downloaded file somedir/file should exist + + Scenario: :download with a directory which doesn't exist + When I run :download --dest (tmpdir)/downloads/somedir/ http://localhost:(port)/data/downloads/download.bin + And I wait for "Asking question text='* does not exist. Create it?' title='Create directory?'>, *" in the log + And I run :prompt-accept yes + And I wait until the download is finished + Then the downloaded file somedir/download.bin should exist ## mhtml downloads From 5215321e64fb47908cd6827086dcc4d02279d596 Mon Sep 17 00:00:00 2001 From: Panagiotis K Date: Sat, 4 Nov 2017 20:11:58 +0200 Subject: [PATCH 4/6] Error handling, better code quality. Handling of os errors raised during parent directory creation. --- qutebrowser/browser/downloads.py | 14 ++++++---- .../browser/webengine/webenginedownloads.py | 26 +++++++++---------- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 2b14438a4..a6134b382 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -139,7 +139,7 @@ def create_full_filename(basename, filename): filename = utils.force_encoding(filename, encoding) basename = utils.force_encoding(basename, encoding) if os.path.isabs(filename) and (os.path.isdir(filename) or - os.path.join(filename, "") == filename): + filename.endswith(os.sep)): # We got an absolute directory from the user, so we save it under # the default filename in that directory. return os.path.join(filename, basename) @@ -606,6 +606,11 @@ class AbstractDownloadItem(QObject): """Ask a confirmation question for the download.""" raise NotImplementedError + def _ask_create_parent_question(self, title, msg, + force_overwrite, remember_directory): + """Ask a confirmation question for the parent directory.""" + raise NotImplementedError + def _set_fileobj(self, fileobj, *, autoclose=True): """Set a file object to save the download to. @@ -683,11 +688,10 @@ class AbstractDownloadItem(QObject): try: os.makedirs(os.path.dirname(self._filename)) + except FileExistsError: + pass except OSError as e: - # Unlikely, but could be created before - # we get a chance to create it. - if e.errno != errno.EEXIST: - raise + self._die(e.strerror) self.basename = os.path.basename(self._filename) if remember_directory: diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 03baac359..d467724d5 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -120,6 +120,19 @@ class DownloadItem(downloads.AbstractDownloadItem): "state {} (not in requested state)!".format( filename, self, state_name)) + def _ask_confirm_question(self, title, msg): + no_action = functools.partial(self.cancel, remove_data=False) + question = usertypes.Question() + question.title = title + question.text = msg + question.mode = usertypes.PromptMode.yesno + question.answered_yes.connect(self._after_set_filename) + question.answered_no.connect(no_action) + question.cancelled.connect(no_action) + self.cancelled.connect(question.abort) + self.error.connect(question.abort) + message.global_bridge.ask(question, blocking=True) + def _ask_create_parent_question(self, title, msg, force_overwrite, remember_directory): no_action = functools.partial(self.cancel, remove_data=False) @@ -136,19 +149,6 @@ class DownloadItem(downloads.AbstractDownloadItem): self.error.connect(question.abort) message.global_bridge.ask(question, blocking=True) - def _ask_confirm_question(self, title, msg): - no_action = functools.partial(self.cancel, remove_data=False) - question = usertypes.Question() - question.title = title - question.text = msg - question.mode = usertypes.PromptMode.yesno - question.answered_yes.connect(self._after_set_filename) - question.answered_no.connect(no_action) - question.cancelled.connect(no_action) - self.cancelled.connect(question.abort) - self.error.connect(question.abort) - message.global_bridge.ask(question, blocking=True) - def _after_set_filename(self): self._qt_item.setPath(self._filename) self._qt_item.accept() From 59cebae28ef2dbf43d43438c2d122c191ac97d5d Mon Sep 17 00:00:00 2001 From: Panagiotis K Date: Wed, 8 Nov 2017 18:23:51 +0200 Subject: [PATCH 5/6] Remove redundant import. --- qutebrowser/browser/downloads.py | 1 - 1 file changed, 1 deletion(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index a6134b382..c6b5f257a 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -27,7 +27,6 @@ import collections import functools import pathlib import tempfile -import errno import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QModelIndex, From e4be834b395efdbbee0af8b0c4c240ea047beb66 Mon Sep 17 00:00:00 2001 From: Panagiotis K Date: Wed, 15 Nov 2017 09:51:05 +0200 Subject: [PATCH 6/6] Platform-agnostic test. --- tests/end2end/features/downloads.feature | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 7648ad888..69f47603b 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -256,14 +256,14 @@ Feature: Downloading things from a website. Then the error "Can only download the current page as mhtml." should be shown Scenario: :download with a filename and directory which doesn't exist - When I run :download --dest (tmpdir)/downloads/somedir/file http://localhost:(port)/data/downloads/download.bin + When I run :download --dest (tmpdir)(dirsep)downloads(dirsep)somedir(dirsep)file http://localhost:(port)/data/downloads/download.bin And I wait for "Asking question text='* does not exist. Create it?' title='Create directory?'>, *" in the log And I run :prompt-accept yes And I wait until the download is finished Then the downloaded file somedir/file should exist Scenario: :download with a directory which doesn't exist - When I run :download --dest (tmpdir)/downloads/somedir/ http://localhost:(port)/data/downloads/download.bin + When I run :download --dest (tmpdir)(dirsep)downloads(dirsep)somedir(dirsep) http://localhost:(port)/data/downloads/download.bin And I wait for "Asking question text='* does not exist. Create it?' title='Create directory?'>, *" in the log And I run :prompt-accept yes And I wait until the download is finished