From 54db4255b144ac13e4fe8782b81eb2a2a28af95c Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 8 Nov 2016 07:21:21 +0100 Subject: [PATCH] Fix handling of temporary files When we use self._set_filename in self._set_fileobj, the file already exists, so we need to force "overwriting" it. Also, move temporary file handling to a dedicated _set_tempfile method, so we can officially claim not supporting _set_fileobj with QtWebEngine instead of supporting it with a hack. --- qutebrowser/browser/downloads.py | 16 +++++++++++----- qutebrowser/browser/qtnetworkdownloads.py | 3 +++ .../browser/webengine/webenginedownloads.py | 5 ++++- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index afb9bb62b..c83af523d 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -482,17 +482,21 @@ class AbstractDownloadItem(QObject): def _set_fileobj(self, fileobj): """Set a file object to save the download to. - Note that some backends (QtWebEngine) will simply access the .name - attribute and not actually use the file object directly. + Not supported by QtWebEngine. """ raise NotImplementedError - def _set_filename(self, filename): + def _set_tempfile(self, fileobj): + """Set a temporary file when opening the download.""" + raise NotImplementedError + + def _set_filename(self, filename, *, force_overwrite=False): """Set the filename to save the download to. Args: filename: The full filename to save the download to. None: special value to stop the download. + force_overwrite: Force overwriting existing files. """ global last_used_directory filename = os.path.expanduser(filename) @@ -525,7 +529,9 @@ class AbstractDownloadItem(QObject): last_used_directory = os.path.dirname(self._filename) log.downloads.debug("Setting filename to {}".format(filename)) - if os.path.isfile(self._filename): + if 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 = "{} already exists. Overwrite?".format( @@ -573,7 +579,7 @@ class AbstractDownloadItem(QObject): return self.finished.connect( functools.partial(self._open_if_successful, target.cmdline)) - self._set_fileobj(fobj) + self._set_tempfile(fobj) else: # pragma: no cover raise ValueError("Unsupported download target: {}".format(target)) diff --git a/qutebrowser/browser/qtnetworkdownloads.py b/qutebrowser/browser/qtnetworkdownloads.py index a6e57e428..5bcc67c0f 100644 --- a/qutebrowser/browser/qtnetworkdownloads.py +++ b/qutebrowser/browser/qtnetworkdownloads.py @@ -214,6 +214,9 @@ class DownloadItem(downloads.AbstractDownloadItem): except OSError as e: self._die(e.strerror) + def _set_tempfile(self, fileobj): + self._set_fileobj(fileobj) + def _finish_download(self): """Write buffered data to disk and finish the QNetworkReply.""" log.downloads.debug("Finishing download...") diff --git a/qutebrowser/browser/webengine/webenginedownloads.py b/qutebrowser/browser/webengine/webenginedownloads.py index 7e878e096..d43916f9d 100644 --- a/qutebrowser/browser/webengine/webenginedownloads.py +++ b/qutebrowser/browser/webengine/webenginedownloads.py @@ -88,7 +88,10 @@ class DownloadItem(downloads.AbstractDownloadItem): return self._filename def _set_fileobj(self, fileobj): - self._set_filename(fileobj.name) + raise downloads.UnsupportedOperationError + + def _set_tempfile(self, fileobj): + self._set_filename(fileobj.name, force_overwrite=True) def _ensure_can_set_filename(self, filename): state = self._qt_item.state()