Merge branch 'Kingdread-issue-2204'

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
 ------

qutebrowser/browser/commands.py

@@ -1302,54 +1302,50 @@ 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)
+            self._download_mhtml(target)
         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):
+    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 target is not None:
+            mhtml.start_download_checked(target, tab=tab)
+            return
 
-        if dest is None:
+        suggested_fn = self._current_title() + ".mht"
-            suggested_fn = self._current_title() + ".mht"
+        suggested_fn = utils.sanitize_filename(suggested_fn)
-            suggested_fn = utils.sanitize_filename(suggested_fn)
 
-            filename = downloads.immediate_download_path()
+        filename = downloads.immediate_download_path()
-            if filename is not None:
+        if filename is not None:
-                mhtml.start_download_checked(filename, tab=tab)
+            target = downloads.FileDownloadTarget(filename)
-            else:
+            mhtml.start_download_checked(target, tab=tab)
-                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:
-            mhtml.start_download_checked(dest, tab=tab)
+            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):

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
@@ -28,15 +27,13 @@ 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 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,
@@ -158,7 +155,7 @@ def get_filename_question(*, suggested_filename, url, parent=None):
     q.title = "Save file to:"
     q.text = "Please enter a location for <b>{}</b>".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 +194,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 +216,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 +240,9 @@ class OpenFileDownloadTarget(_DownloadTarget):
     def suggested_filename(self):
         raise NoFilenameError
 
+    def __str__(self):
+        return 'temporary file'
+
 
 class DownloadItemStats(QObject):
 
@@ -520,31 +529,7 @@ class AbstractDownloadItem(QObject):
         if filename is None:  # pragma: no cover
             log.downloads.error("No filename to open the download!")
             return
-
+        utils.open_file(filename, 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.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 _ensure_can_set_filename(self, filename):
         """Make sure we can still set a filename."""
@@ -780,7 +765,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)

qutebrowser/browser/webkit/mhtml.py

@@ -242,7 +242,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 +252,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 +462,34 @@ 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: {!r}"
+                             .format(self.target))
+
         try:
-            with open(self.dest, 'wb') as file_output:
+            with fobj:
-                self.writer.write_to(file_output)
+                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):
+            utils.open_file(fobj.name, self.target.cmdline)
 
     def _collect_zombies(self):
         """Collect done downloads and add their data to the MHTML file.
@@ -501,26 +521,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 +551,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 +572,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 +584,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)

qutebrowser/utils/utils.py

@@ -29,9 +29,10 @@ import functools
 import contextlib
 import itertools
 import socket
+import shlex
 
-from PyQt5.QtCore import Qt
+from PyQt5.QtCore import Qt, QUrl
-from PyQt5.QtGui import QKeySequence, QColor, QClipboard
+from PyQt5.QtGui import QKeySequence, QColor, QClipboard, QDesktopServices
 from PyQt5.QtWidgets import QApplication
 import pkg_resources
 
@@ -825,3 +826,48 @@ 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.
+    """
+    # 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')
+
+    # 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)

tests/end2end/features/downloads.feature

@@ -216,17 +216,26 @@ 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 <qutebrowser.utils.usertypes.Question default='*' mode=<PromptMode.text: 2> text='Please enter a location for <b>http://localhost:*/html</b>' title='Save file to:'>, *" in the log
+        And I wait for "Asking question <qutebrowser.utils.usertypes.Question default='*' mode=<PromptMode.download: 5> text='Please enter a location for <b>http://localhost:*/html</b>' 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 <qutebrowser.utils.usertypes.Question default='*' mode=<PromptMode.text: 2> text='Please enter a location for <b>http://localhost:*/html</b>' title='Save file to:'>, *" in the log
+        And I wait for "Asking question <qutebrowser.utils.usertypes.Question default='*' mode=<PromptMode.download: 5> text='Please enter a location for <b>http://localhost:*/html</b>' title='Save file to:'>, *" in the log
         And I run :prompt-accept
         And I wait for "Asking question <qutebrowser.utils.usertypes.Question default=None mode=<PromptMode.yesno: 1> text='<b>*</b> already exists. Overwrite?' title='Overwrite existing file?'>, *" in the log
         And I run :prompt-accept yes
         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

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

tests/unit/utils/test_utils.py

@@ -27,8 +27,10 @@ import logging
 import functools
 import collections
 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
 
@@ -984,3 +986,46 @@ def test_random_port():
     sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
     sock.bind(('localhost', port))
     sock.close()
+
+
+class TestOpenFile:
+
+    @pytest.mark.not_frozen
+    def test_cmdline_without_argument(self, caplog, config_stub):
+        config_stub.data = {'general': {'default-open-dispatcher': ''}}
+        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': ''}}
+        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):
+        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
+        assert re.match(
+            r"Opening /foo/bar with \[.*python.*/foo/bar.*\]", result)
+
+    def test_system_default_application(self, caplog, config_stub, mocker):
+        config_stub.data = {'general': {'default-open-dispatcher': ''}}
+        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)
+        m.assert_called_with(QUrl('file:///foo/bar'))