From 8c5ad7d46d18489f2caaf180f49afde74631a7f3 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 4 Jan 2017 15:18:56 +0100 Subject: [PATCH 01/12] use download prompt for mhtml downloads Fixes #2204 We didn't previously use PromptMode.download for MHTML download prompts to avoid dealing with thinks like "Open download", but the new download prompt is just way better than the old, which justifies the extra work. This means that MHTML downloads can now also be opened directly. --- qutebrowser/browser/commands.py | 44 ++++++++--------- qutebrowser/browser/downloads.py | 15 +++++- qutebrowser/browser/webkit/mhtml.py | 77 +++++++++++++++++++++++------ 3 files changed, 96 insertions(+), 40 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index e8f15720d..dbbe0a8c0 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1302,26 +1302,21 @@ class CommandDispatcher: # FIXME:qtwebengine do this with the QtWebEngine download manager? download_manager = objreg.get('qtnetwork-download-manager', scope='window', window=self._win_id) + target = None + if dest is not None: + target = downloads.FileDownloadTarget(dest) + if url: if mhtml_: raise cmdexc.CommandError("Can only download the current page" " as mhtml.") url = urlutils.qurl_from_user_input(url) urlutils.raise_cmdexc_if_invalid(url) - if dest is None: - target = None - else: - target = downloads.FileDownloadTarget(dest) download_manager.get(url, target=target) elif mhtml_: self._download_mhtml(dest) else: qnam = self._current_widget().networkaccessmanager() - - if dest is None: - target = None - else: - target = downloads.FileDownloadTarget(dest) download_manager.get(self._current_url(), qnam=qnam, target=target) def _download_mhtml(self, dest=None): @@ -1334,22 +1329,23 @@ class CommandDispatcher: if tab.backend == usertypes.Backend.QtWebEngine: raise cmdexc.CommandError("Download --mhtml is not implemented " "with QtWebEngine yet") - - if dest is None: - suggested_fn = self._current_title() + ".mht" - suggested_fn = utils.sanitize_filename(suggested_fn) - - filename = downloads.immediate_download_path() - if filename is not None: - mhtml.start_download_checked(filename, tab=tab) - else: - question = downloads.get_filename_question( - suggested_filename=suggested_fn, url=tab.url(), parent=tab) - question.answered.connect(functools.partial( - mhtml.start_download_checked, tab=tab)) - message.global_bridge.ask(question, blocking=False) - else: + if dest is not None: mhtml.start_download_checked(dest, tab=tab) + return + + suggested_fn = self._current_title() + ".mht" + suggested_fn = utils.sanitize_filename(suggested_fn) + + filename = downloads.immediate_download_path() + if filename is not None: + target = downloads.FileDownloadTarget(filename) + mhtml.start_download_checked(target, tab=tab) + else: + question = downloads.get_filename_question( + suggested_filename=suggested_fn, url=tab.url(), parent=tab) + question.answered.connect(functools.partial( + mhtml.start_download_checked, tab=tab)) + message.global_bridge.ask(question, blocking=False) @cmdutils.register(instance='command-dispatcher', scope='window') def view_source(self): diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 548313b93..a899fc789 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -158,7 +158,7 @@ def get_filename_question(*, suggested_filename, url, parent=None): q.title = "Save file to:" q.text = "Please enter a location for {}".format( html.escape(url.toDisplayString())) - q.mode = usertypes.PromptMode.text + q.mode = usertypes.PromptMode.download q.completed.connect(q.deleteLater) q.default = _path_suggestion(suggested_filename) return q @@ -197,6 +197,9 @@ class FileDownloadTarget(_DownloadTarget): def suggested_filename(self): return os.path.basename(self.filename) + def __str__(self): + return self.filename + class FileObjDownloadTarget(_DownloadTarget): @@ -216,6 +219,12 @@ class FileObjDownloadTarget(_DownloadTarget): except AttributeError: raise NoFilenameError + def __str__(self): + try: + return 'file object at {}'.format(self.fileobj.name) + except AttributeError: + return 'anonymous file object' + class OpenFileDownloadTarget(_DownloadTarget): @@ -234,6 +243,9 @@ class OpenFileDownloadTarget(_DownloadTarget): def suggested_filename(self): raise NoFilenameError + def __str__(self): + return 'temporary file' + class DownloadItemStats(QObject): @@ -780,7 +792,6 @@ class AbstractDownloadManager(QObject): def _init_filename_question(self, question, download): """Set up an existing filename question with a download.""" - question.mode = usertypes.PromptMode.download question.answered.connect(download.set_target) question.cancelled.connect(download.cancel) download.cancelled.connect(question.abort) diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index fbfeafef4..438db7b28 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -35,10 +35,12 @@ import email.message import quopri from PyQt5.QtCore import QUrl +from PyQt5.QtGui import QDesktopServices from qutebrowser.browser import downloads from qutebrowser.browser.webkit import webkitelem from qutebrowser.utils import log, objreg, message, usertypes, utils, urlutils +from qutebrowser.config import config _File = collections.namedtuple('_File', ['content', 'content_type', 'content_location', @@ -242,7 +244,7 @@ class _Downloader: Attributes: tab: The AbstractTab which contains the website that will be saved. - dest: Destination filename. + target: DownloadTarget where the file should be downloaded to. writer: The MHTMLWriter object which is used to save the page. loaded_urls: A set of QUrls of finished asset downloads. pending_downloads: A set of unfinished (url, DownloadItem) tuples. @@ -252,9 +254,9 @@ class _Downloader: _win_id: The window this downloader belongs to. """ - def __init__(self, tab, dest): + def __init__(self, tab, target): self.tab = tab - self.dest = dest + self.target = target self.writer = None self.loaded_urls = {tab.url()} self.pending_downloads = set() @@ -462,14 +464,57 @@ class _Downloader: return self._finished_file = True log.downloads.debug("All assets downloaded, ready to finish off!") + + if isinstance(self.target, downloads.FileDownloadTarget): + fobj = open(self.target.filename, 'wb') + elif isinstance(self.target, downloads.FileObjDownloadTarget): + fobj = self.target.fileobj + elif isinstance(self.target, downloads.OpenFileDownloadTarget): + try: + fobj = downloads.temp_download_manager.get_tmpfile( + self.tab.title() + '.mht') + except OSError as exc: + msg = "Download error: {}".format(exc) + message.error(msg) + return + else: + raise ValueError("Invalid DownloadTarget given") + try: - with open(self.dest, 'wb') as file_output: - self.writer.write_to(file_output) + with fobj: + self.writer.write_to(fobj) except OSError as error: message.error("Could not save file: {}".format(error)) return log.downloads.debug("File successfully written.") - message.info("Page saved as {}".format(self.dest)) + message.info("Page saved as {}".format(self.target)) + + if isinstance(self.target, downloads.OpenFileDownloadTarget): + filename = fobj.name + # the default program to open downloads with - will be empty string + # if we want to use the default + override = config.get('general', 'default-open-dispatcher') + cmdline = self.target.cmdline + + # precedence order: cmdline > default-open-dispatcher > openUrl + if cmdline is None and not override: + log.downloads.debug("Opening {} with the system application" + .format(filename)) + url = QUrl.fromLocalFile(filename) + QDesktopServices.openUrl(url) + return + + if cmdline is None and override: + cmdline = override + + cmd, *args = shlex.split(cmdline) + args = [arg.replace('{}', filename) for arg in args] + if '{}' not in cmdline: + args.append(filename) + log.downloads.debug("Opening {} with {}" + .format(filename, [cmd] + args)) + proc = guiprocess.GUIProcess(what='download') + proc.start_detached(cmd, args) def _collect_zombies(self): """Collect done downloads and add their data to the MHTML file. @@ -501,26 +546,29 @@ class _NoCloseBytesIO(io.BytesIO): super().close() -def _start_download(dest, tab): +def _start_download(target, tab): """Start downloading the current page and all assets to an MHTML file. This will overwrite dest if it already exists. Args: - dest: The filename where the resulting file should be saved. + target: The DownloadTarget where the resulting file should be saved. tab: Specify the tab whose page should be loaded. """ - loader = _Downloader(tab, dest) + loader = _Downloader(tab, target) loader.run() -def start_download_checked(dest, tab): +def start_download_checked(target, tab): """First check if dest is already a file, then start the download. Args: - dest: The filename where the resulting file should be saved. + target: The DownloadTarget where the resulting file should be saved. tab: Specify the tab whose page should be loaded. """ + if not isinstance(target, downloads.FileDownloadTarget): + _start_download(target, tab) + return # The default name is 'page title.mht' title = tab.title() default_name = utils.sanitize_filename(title + '.mht') @@ -528,7 +576,7 @@ def start_download_checked(dest, tab): # Remove characters which cannot be expressed in the file system encoding encoding = sys.getfilesystemencoding() default_name = utils.force_encoding(default_name, encoding) - dest = utils.force_encoding(dest, encoding) + dest = utils.force_encoding(target.filename, encoding) dest = os.path.expanduser(dest) @@ -549,8 +597,9 @@ def start_download_checked(dest, tab): message.error("Directory {} does not exist.".format(folder)) return + target = downloads.FileDownloadTarget(path) if not os.path.isfile(path): - _start_download(path, tab=tab) + _start_download(target, tab=tab) return q = usertypes.Question() @@ -560,5 +609,5 @@ def start_download_checked(dest, tab): html.escape(path)) q.completed.connect(q.deleteLater) q.answered_yes.connect(functools.partial( - _start_download, path, tab=tab)) + _start_download, target, tab=tab)) message.global_bridge.ask(q, blocking=False) From 4fdd3cd7616b2c07ea0dc7014a7ea537f4cf583e Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 4 Jan 2017 15:31:47 +0100 Subject: [PATCH 02/12] deduplicate download opening code --- qutebrowser/browser/downloads.py | 29 +----------------- qutebrowser/browser/webkit/mhtml.py | 28 +---------------- qutebrowser/utils/utils.py | 47 +++++++++++++++++++++++++++-- 3 files changed, 47 insertions(+), 57 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index a899fc789..3791dfc60 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -20,7 +20,6 @@ """Shared QtWebKit/QtWebEngine code for downloads.""" import sys -import shlex import html import os.path import collections @@ -30,13 +29,11 @@ import tempfile import sip from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QUrl, QModelIndex, QTimer, QAbstractListModel) -from PyQt5.QtGui import QDesktopServices from qutebrowser.commands import cmdexc, cmdutils from qutebrowser.config import config from qutebrowser.utils import (usertypes, standarddir, utils, message, log, qtutils) -from qutebrowser.misc import guiprocess ModelRole = usertypes.enum('ModelRole', ['item'], start=Qt.UserRole, @@ -532,31 +529,7 @@ class AbstractDownloadItem(QObject): if filename is None: # pragma: no cover log.downloads.error("No filename to open the download!") return - - # the default program to open downloads with - will be empty string - # if we want to use the default - override = config.get('general', 'default-open-dispatcher') - - # precedence order: cmdline > default-open-dispatcher > openUrl - - if cmdline is None and not override: - log.downloads.debug("Opening {} with the system application" - .format(filename)) - url = QUrl.fromLocalFile(filename) - QDesktopServices.openUrl(url) - return - - if cmdline is None and override: - cmdline = override - - cmd, *args = shlex.split(cmdline) - args = [arg.replace('{}', filename) for arg in args] - if '{}' not in cmdline: - args.append(filename) - log.downloads.debug("Opening {} with {}" - .format(filename, [cmd] + args)) - proc = guiprocess.GUIProcess(what='download') - proc.start_detached(cmd, args) + utils.open_file(filename, cmdline) def _ensure_can_set_filename(self, filename): """Make sure we can still set a filename.""" diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index 438db7b28..23b962b4f 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -35,12 +35,10 @@ import email.message import quopri from PyQt5.QtCore import QUrl -from PyQt5.QtGui import QDesktopServices from qutebrowser.browser import downloads from qutebrowser.browser.webkit import webkitelem from qutebrowser.utils import log, objreg, message, usertypes, utils, urlutils -from qutebrowser.config import config _File = collections.namedtuple('_File', ['content', 'content_type', 'content_location', @@ -490,31 +488,7 @@ class _Downloader: message.info("Page saved as {}".format(self.target)) if isinstance(self.target, downloads.OpenFileDownloadTarget): - filename = fobj.name - # the default program to open downloads with - will be empty string - # if we want to use the default - override = config.get('general', 'default-open-dispatcher') - cmdline = self.target.cmdline - - # precedence order: cmdline > default-open-dispatcher > openUrl - if cmdline is None and not override: - log.downloads.debug("Opening {} with the system application" - .format(filename)) - url = QUrl.fromLocalFile(filename) - QDesktopServices.openUrl(url) - return - - if cmdline is None and override: - cmdline = override - - cmd, *args = shlex.split(cmdline) - args = [arg.replace('{}', filename) for arg in args] - if '{}' not in cmdline: - args.append(filename) - log.downloads.debug("Opening {} with {}" - .format(filename, [cmd] + args)) - proc = guiprocess.GUIProcess(what='download') - proc.start_detached(cmd, args) + utils.open_file(fobj.name, self.target.cmdline) def _collect_zombies(self): """Collect done downloads and add their data to the MHTML file. diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index 2e5142511..afed444d4 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -29,14 +29,17 @@ import functools import contextlib import itertools import socket +import shlex -from PyQt5.QtCore import Qt -from PyQt5.QtGui import QKeySequence, QColor, QClipboard +from PyQt5.QtCore import Qt, QUrl +from PyQt5.QtGui import QKeySequence, QColor, QClipboard, QDesktopServices from PyQt5.QtWidgets import QApplication import pkg_resources import qutebrowser from qutebrowser.utils import qtutils, log +from qutebrowser.misc import guiprocess +from qutebrowser.config import config fake_clipboard = None @@ -825,3 +828,43 @@ def random_port(): port = sock.getsockname()[1] sock.close() return port + + +def open_file(filename, cmdline=None): + """Open the given file. + + If cmdline is not given, general->default-open-dispatcher is used. + If default-open-dispatcher is unset, the system's default application is + used. + + Args: + filename: The filename to open. + cmdline: The command to use as string. A `{}` is expanded to the + filename. None means to use the system's default application + or `default-open-dispatcher` if set. If no `{}` is found, the + filename is appended to the cmdline. + """ + # the default program to open downloads with - will be empty string + # if we want to use the default + override = config.get('general', 'default-open-dispatcher') + + # precedence order: cmdline > default-open-dispatcher > openUrl + + if cmdline is None and not override: + log.misc.debug("Opening {} with the system application" + .format(filename)) + url = QUrl.fromLocalFile(filename) + QDesktopServices.openUrl(url) + return + + if cmdline is None and override: + cmdline = override + + cmd, *args = shlex.split(cmdline) + args = [arg.replace('{}', filename) for arg in args] + if '{}' not in cmdline: + args.append(filename) + log.misc.debug("Opening {} with {}" + .format(filename, [cmd] + args)) + proc = guiprocess.GUIProcess(what='open-file') + proc.start_detached(cmd, args) From 6497bb5ace557a95a0f2a0063683c6811b31a77e Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 4 Jan 2017 16:04:06 +0100 Subject: [PATCH 03/12] break cicular imports in utils --- qutebrowser/utils/utils.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index afed444d4..681ae1dc8 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -38,8 +38,6 @@ import pkg_resources import qutebrowser from qutebrowser.utils import qtutils, log -from qutebrowser.misc import guiprocess -from qutebrowser.config import config fake_clipboard = None @@ -844,6 +842,11 @@ def open_file(filename, cmdline=None): or `default-open-dispatcher` if set. If no `{}` is found, the filename is appended to the cmdline. """ + # Import late to avoid circular imports: + # utils -> config -> configdata -> configtypes -> cmdutils -> command -> + # utils + from qutebrowser.misc import guiprocess + from qutebrowser.config import config # the default program to open downloads with - will be empty string # if we want to use the default override = config.get('general', 'default-open-dispatcher') From 69001111dadc142cc7d1357bb1af7b2df6dd6606 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 4 Jan 2017 16:32:25 +0100 Subject: [PATCH 04/12] actually use DownloadTarget for :download -m /path --- qutebrowser/browser/commands.py | 10 +++++----- qutebrowser/browser/webkit/mhtml.py | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index dbbe0a8c0..3eb73185a 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -1314,23 +1314,23 @@ class CommandDispatcher: urlutils.raise_cmdexc_if_invalid(url) download_manager.get(url, target=target) elif mhtml_: - self._download_mhtml(dest) + self._download_mhtml(target) else: qnam = self._current_widget().networkaccessmanager() download_manager.get(self._current_url(), qnam=qnam, target=target) - def _download_mhtml(self, dest=None): + def _download_mhtml(self, target=None): """Download the current page as an MHTML file, including all assets. Args: - dest: The file path to write the download to. + target: The download target for the file. """ tab = self._current_widget() if tab.backend == usertypes.Backend.QtWebEngine: raise cmdexc.CommandError("Download --mhtml is not implemented " "with QtWebEngine yet") - if dest is not None: - mhtml.start_download_checked(dest, tab=tab) + if target is not None: + mhtml.start_download_checked(target, tab=tab) return suggested_fn = self._current_title() + ".mht" diff --git a/qutebrowser/browser/webkit/mhtml.py b/qutebrowser/browser/webkit/mhtml.py index 23b962b4f..97da148a0 100644 --- a/qutebrowser/browser/webkit/mhtml.py +++ b/qutebrowser/browser/webkit/mhtml.py @@ -476,7 +476,8 @@ class _Downloader: message.error(msg) return else: - raise ValueError("Invalid DownloadTarget given") + raise ValueError("Invalid DownloadTarget given: {!r}" + .format(self.target)) try: with fobj: From bd5274af5a35c6f51b40d19fc045503e66b312f5 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Wed, 4 Jan 2017 17:56:56 +0100 Subject: [PATCH 05/12] fix tests --- qutebrowser/browser/downloads.py | 2 +- qutebrowser/utils/utils.py | 2 +- tests/end2end/features/downloads.feature | 4 ++-- tests/end2end/test_invocations.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/downloads.py b/qutebrowser/browser/downloads.py index 3791dfc60..378fe9a19 100644 --- a/qutebrowser/browser/downloads.py +++ b/qutebrowser/browser/downloads.py @@ -27,7 +27,7 @@ import functools import tempfile import sip -from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QUrl, QModelIndex, +from PyQt5.QtCore import (pyqtSlot, pyqtSignal, Qt, QObject, QModelIndex, QTimer, QAbstractListModel) from qutebrowser.commands import cmdexc, cmdutils diff --git a/qutebrowser/utils/utils.py b/qutebrowser/utils/utils.py index 681ae1dc8..fbed4ede3 100644 --- a/qutebrowser/utils/utils.py +++ b/qutebrowser/utils/utils.py @@ -855,7 +855,7 @@ def open_file(filename, cmdline=None): if cmdline is None and not override: log.misc.debug("Opening {} with the system application" - .format(filename)) + .format(filename)) url = QUrl.fromLocalFile(filename) QDesktopServices.openUrl(url) return diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 1b0ff1170..6aebab952 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -208,11 +208,11 @@ Feature: Downloading things from a website. When I set storage -> prompt-download-directory to true And I open html And I run :download --mhtml - And I wait for "Asking question text='Please enter a location for http://localhost:*/html' title='Save file to:'>, *" in the log + And I wait for "Asking question text='Please enter a location for http://localhost:*/html' title='Save file to:'>, *" in the log And I run :prompt-accept And I wait for "File successfully written." in the log And I run :download --mhtml - And I wait for "Asking question text='Please enter a location for http://localhost:*/html' title='Save file to:'>, *" in the log + And I wait for "Asking question text='Please enter a location for http://localhost:*/html' title='Save file to:'>, *" in the log And I run :prompt-accept And I wait for "Asking question text='* already exists. Overwrite?' title='Overwrite existing file?'>, *" in the log And I run :prompt-accept yes diff --git a/tests/end2end/test_invocations.py b/tests/end2end/test_invocations.py index d85e2a467..6d1bd6a27 100644 --- a/tests/end2end/test_invocations.py +++ b/tests/end2end/test_invocations.py @@ -96,7 +96,7 @@ def test_ascii_locale(request, httpbin, tmpdir, quteproc_new): .format(sys.executable)) quteproc_new.wait_for(category='downloads', message='Download รค-issue908.bin finished') - quteproc_new.wait_for(category='downloads', + quteproc_new.wait_for(category='misc', message='Opening * with [*python*]') assert len(tmpdir.listdir()) == 1 From 536c76848e9b5d9c58b957884a53e678a9d39970 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 5 Jan 2017 19:02:28 +0100 Subject: [PATCH 06/12] add a test for opening mhtml downloads --- tests/end2end/features/downloads.feature | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature index 6aebab952..d933d32fd 100644 --- a/tests/end2end/features/downloads.feature +++ b/tests/end2end/features/downloads.feature @@ -219,6 +219,15 @@ Feature: Downloading things from a website. And I wait for "File successfully written." in the log Then no crash should happen + @qtwebengine_todo: :download --mhtml is not implemented yet + Scenario: Opening a mhtml download directly + When I set storage -> prompt-download-directory to true + And I open html + And I run :download --mhtml + And I wait for the download prompt for "*" + And I directly open the download + Then "Opening *.mht* with [*python*]" should be logged + ## :download-cancel Scenario: Cancelling a download From 2986f7b615ed8d036b04d02a366b7a2fd36690cc Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 6 Jan 2017 13:32:46 +0100 Subject: [PATCH 07/12] add tests for utils.open_file --- tests/unit/utils/test_utils.py | 43 ++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index 73252ddc4..c64d00e4d 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -27,6 +27,7 @@ import logging import functools import collections import socket +import re from PyQt5.QtCore import Qt from PyQt5.QtGui import QColor, QClipboard @@ -984,3 +985,45 @@ def test_random_port(): sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock.bind(('localhost', port)) sock.close() + + +class TestOpenFile: + + def test_cmdline_without_argument(self, caplog, config_stub): + config_stub.data = {'general': {'default-open-dispatcher': ''}} + cmdline = '{} -c pass'.format(sys.executable) + utils.open_file('/foo/bar', cmdline) + result = caplog.records[0].message + assert re.match( + r'Opening /foo/bar with \[.*python.*/foo/bar.*\]', result) + + def test_cmdline_with_argument(self, caplog, config_stub): + config_stub.data = {'general': {'default-open-dispatcher': ''}} + cmdline = '{} -c pass {{}} raboof'.format(sys.executable) + utils.open_file('/foo/bar', cmdline) + result = caplog.records[0].message + assert re.match( + r"Opening /foo/bar with \[.*python.*/foo/bar.*'raboof'\]", result) + + def test_setting_override(self, caplog, config_stub): + cmdline = '{} -c pass'.format(sys.executable) + config_stub.data = {'general': {'default-open-dispatcher': cmdline}} + utils.open_file('/foo/bar') + result = caplog.records[0].message + assert re.match( + r"Opening /foo/bar with \[.*python.*/foo/bar.*\]", result) + + def test_system_default_application(self, caplog, config_stub, + monkeypatch): + self.open_url_called = False + config_stub.data = {'general': {'default-open-dispatcher': ''}} + monkeypatch.setattr('PyQt5.QtGui.QDesktopServices.openUrl', + self.mock_open_url) + utils.open_file('/foo/bar') + result = caplog.records[0].message + assert re.match( + r"Opening /foo/bar with the system application", result) + assert self.open_url_called + + def mock_open_url(self, url): + self.open_url_called = True From bb135a00e6a714b91c47a76656e0dec982be3f0a Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Fri, 6 Jan 2017 13:53:05 +0100 Subject: [PATCH 08/12] fix lint --- tests/unit/utils/test_utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index c64d00e4d..a765cd977 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -989,6 +989,9 @@ def test_random_port(): class TestOpenFile: + def __init__(self): + self.open_url_called = False + def test_cmdline_without_argument(self, caplog, config_stub): config_stub.data = {'general': {'default-open-dispatcher': ''}} cmdline = '{} -c pass'.format(sys.executable) @@ -1015,7 +1018,6 @@ class TestOpenFile: def test_system_default_application(self, caplog, config_stub, monkeypatch): - self.open_url_called = False config_stub.data = {'general': {'default-open-dispatcher': ''}} monkeypatch.setattr('PyQt5.QtGui.QDesktopServices.openUrl', self.mock_open_url) From ea56ded7fceedf708318d6983bfabe1e66d979cd Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Tue, 10 Jan 2017 14:14:03 +0100 Subject: [PATCH 09/12] fix TestOpenFile pytest doesn't like test classes which define __init__, and pylint doesn't like attributes defined outside __init__. We can disable pylint's check, but we can't force pytest to accept our test class... --- tests/unit/utils/test_utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index a765cd977..ef677f120 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -989,9 +989,6 @@ def test_random_port(): class TestOpenFile: - def __init__(self): - self.open_url_called = False - def test_cmdline_without_argument(self, caplog, config_stub): config_stub.data = {'general': {'default-open-dispatcher': ''}} cmdline = '{} -c pass'.format(sys.executable) @@ -1018,6 +1015,8 @@ class TestOpenFile: def test_system_default_application(self, caplog, config_stub, monkeypatch): + # pylint: disable=attribute-defined-outside-init + self.open_url_called = False config_stub.data = {'general': {'default-open-dispatcher': ''}} monkeypatch.setattr('PyQt5.QtGui.QDesktopServices.openUrl', self.mock_open_url) @@ -1028,4 +1027,5 @@ class TestOpenFile: assert self.open_url_called def mock_open_url(self, url): + # pylint: disable=attribute-defined-outside-init self.open_url_called = True From 07460832b6b5d78a5f8d50da555ca57da947ff14 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 12 Jan 2017 15:38:38 +0100 Subject: [PATCH 10/12] fix open-file tests on windows Windows filenames have backslashes, so we need to escape them, otherwise shlex.split will delete them. Also, we can't prodive our own executable on frozen tests. --- tests/unit/utils/test_utils.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index ef677f120..ef6d1e72c 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -28,6 +28,7 @@ import functools import collections import socket import re +import shlex from PyQt5.QtCore import Qt from PyQt5.QtGui import QColor, QClipboard @@ -989,24 +990,30 @@ def test_random_port(): class TestOpenFile: + @pytest.mark.not_frozen def test_cmdline_without_argument(self, caplog, config_stub): config_stub.data = {'general': {'default-open-dispatcher': ''}} - cmdline = '{} -c pass'.format(sys.executable) + executable = shlex.quote(sys.executable) + cmdline = '{} -c pass'.format(executable) utils.open_file('/foo/bar', cmdline) result = caplog.records[0].message assert re.match( r'Opening /foo/bar with \[.*python.*/foo/bar.*\]', result) + @pytest.mark.not_frozen def test_cmdline_with_argument(self, caplog, config_stub): config_stub.data = {'general': {'default-open-dispatcher': ''}} - cmdline = '{} -c pass {{}} raboof'.format(sys.executable) + executable = shlex.quote(sys.executable) + cmdline = '{} -c pass {{}} raboof'.format(executable) utils.open_file('/foo/bar', cmdline) result = caplog.records[0].message assert re.match( r"Opening /foo/bar with \[.*python.*/foo/bar.*'raboof'\]", result) + @pytest.mark.not_frozen def test_setting_override(self, caplog, config_stub): - cmdline = '{} -c pass'.format(sys.executable) + executable = shlex.quote(sys.executable) + cmdline = '{} -c pass'.format(executable) config_stub.data = {'general': {'default-open-dispatcher': cmdline}} utils.open_file('/foo/bar') result = caplog.records[0].message From 1f170b8746a36ba232e107513e18438f6702f618 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Sat, 4 Feb 2017 18:35:14 +0100 Subject: [PATCH 11/12] Update changelog --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 4a0812ba1..0362f7691 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -39,6 +39,7 @@ Fixed - `:enter-mode` now refuses to enter modes which can't be entered manually (which caused crashes). - `:record-macro` (`q`) now doesn't try to record macros for special keys without a text. - Fixed PAC (proxy autoconfig) not working with QtWebKit +- `:download --mhtml` now uses the new file dialog v0.9.1 ------ From 7b0f4e08129f130d164d0bcb4e8ca5d981d6ef37 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Sat, 4 Feb 2017 18:41:22 +0100 Subject: [PATCH 12/12] Use mock for open_file tests --- tests/unit/utils/test_utils.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index ef6d1e72c..c790e7a31 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -30,7 +30,7 @@ import socket import re import shlex -from PyQt5.QtCore import Qt +from PyQt5.QtCore import Qt, QUrl from PyQt5.QtGui import QColor, QClipboard import pytest @@ -1020,19 +1020,12 @@ class TestOpenFile: assert re.match( r"Opening /foo/bar with \[.*python.*/foo/bar.*\]", result) - def test_system_default_application(self, caplog, config_stub, - monkeypatch): - # pylint: disable=attribute-defined-outside-init - self.open_url_called = False + def test_system_default_application(self, caplog, config_stub, mocker): config_stub.data = {'general': {'default-open-dispatcher': ''}} - monkeypatch.setattr('PyQt5.QtGui.QDesktopServices.openUrl', - self.mock_open_url) + m = mocker.patch('PyQt5.QtGui.QDesktopServices.openUrl', spec={}, + new_callable=mocker.Mock) utils.open_file('/foo/bar') result = caplog.records[0].message assert re.match( r"Opening /foo/bar with the system application", result) - assert self.open_url_called - - def mock_open_url(self, url): - # pylint: disable=attribute-defined-outside-init - self.open_url_called = True + m.assert_called_with(QUrl('file:///foo/bar'))