Avoid starting downloads before we know the filename.

Closes #278.

qutebrowser/browser/downloads.py

@@ -490,6 +490,16 @@ class DownloadManager(QAbstractListModel):
     def __repr__(self):
         return utils.get_repr(self, downloads=len(self.downloads))
 
+    def _prepare_question(self):
+        """Prepare a Question object to be asked."""
+        q = usertypes.Question(self)
+        q.text = "Save file to:"
+        q.mode = usertypes.PromptMode.text
+        q.completed.connect(q.deleteLater)
+        q.destroyed.connect(functools.partial(self.questions.remove, q))
+        self.questions.append(q)
+        return q
+
     @cmdutils.register(instance='download-manager', scope='window')
     def download(self, url, dest=None):
         """Download a given URL, given as string.
@@ -513,7 +523,10 @@ class DownloadManager(QAbstractListModel):
             filename: A path to write the data to.
 
         Return:
-            The created DownloadItem.
+            If the download could start immediately, (fileobj/filename given),
+            the created DownloadItem.
+
+            If not, None.
         """
         if fileobj is not None and filename is not None:
             raise TypeError("Only one of fileobj/filename may be given!")
@@ -521,32 +534,59 @@ class DownloadManager(QAbstractListModel):
             urlutils.invalid_url_error(self._win_id, url, "start download")
             return
         req = QNetworkRequest(url)
+        return self.get_request(req, page, fileobj, filename)
+
+    def get_request(self, request, page=None, fileobj=None, filename=None):
+        """Start a download with a QNetworkRequest.
+
+        Args:
+            request: The QNetworkRequest to download.
+            page: The QWebPage to use.
+            fileobj: The file object to write the answer to.
+            filename: A path to write the data to.
+
+        Return:
+            If the download could start immediately, (fileobj/filename given),
+            the created DownloadItem.
+
+            If not, None.
+        """
+        if fileobj is not None and filename is not None:
+            raise TypeError("Only one of fileobj/filename may be given!")
         # WORKAROUND for Qt corrupting data loaded from cache:
         # https://bugreports.qt-project.org/browse/QTBUG-42757
-        req.setAttribute(QNetworkRequest.CacheLoadControlAttribute,
+        request.setAttribute(QNetworkRequest.CacheLoadControlAttribute,
-                         QNetworkRequest.AlwaysNetwork)
+                             QNetworkRequest.AlwaysNetwork)
+        if fileobj is not None or filename is not None:
+            return self.fetch_request(request, filename, fileobj, page)
+        q = self._prepare_question()
+        q.default = urlutils.filename_from_url(request.url())
+        message_bridge = objreg.get('message-bridge', scope='window',
+                                    window=self._win_id)
+        q.answered.connect(
+            lambda fn: self.fetch_request(request, filename=fn, page=page))
+        message_bridge.ask(q, blocking=False)
+        return None
+
+    def fetch_request(self, request, page=None, fileobj=None, filename=None):
+        """Download a QNetworkRequest to disk.
+
+        Args:
+            request: The QNetworkRequest to download.
+            page: The QWebPage to use.
+            fileobj: The file object to write the answer to.
+            filename: A path to write the data to.
+
+        Return:
+            The created DownloadItem.
+        """
         if page is None:
             nam = self._networkmanager
         else:
             nam = page.networkAccessManager()
-        reply = nam.get(req)
+        reply = nam.get(request)
         return self.fetch(reply, fileobj, filename)
 
-    @cmdutils.register(instance='download-manager', scope='window')
-    def cancel_download(self, count: {'special': 'count'}=1):
-        """Cancel the first/[count]th download.
-
-        Args:
-            count: The index of the download to cancel.
-        """
-        if count == 0:
-            return
-        try:
-            download = self.downloads[count - 1]
-        except IndexError:
-            raise cmdexc.CommandError("There's no download {}!".format(count))
-        download.cancel()
-
     @pyqtSlot('QNetworkReply')
     def fetch(self, reply, fileobj=None, filename=None):
         """Download a QNetworkReply to disk.
@@ -589,15 +629,10 @@ class DownloadManager(QAbstractListModel):
             download.set_fileobj(fileobj)
             download.autoclose = False
         else:
-            q = usertypes.Question(self)
+            q = self._prepare_question()
-            q.text = "Save file to:"
-            q.mode = usertypes.PromptMode.text
             q.default = suggested_filename
             q.answered.connect(download.set_filename)
             q.cancelled.connect(download.cancel)
-            q.completed.connect(q.deleteLater)
-            q.destroyed.connect(functools.partial(self.questions.remove, q))
-            self.questions.append(q)
             download.cancelled.connect(q.abort)
             download.error.connect(q.abort)
             message_bridge = objreg.get('message-bridge', scope='window',
@@ -606,6 +641,21 @@ class DownloadManager(QAbstractListModel):
 
         return download
 
+    @cmdutils.register(instance='download-manager', scope='window')
+    def cancel_download(self, count: {'special': 'count'}=1):
+        """Cancel the first/[count]th download.
+
+        Args:
+            count: The index of the download to cancel.
+        """
+        if count == 0:
+            return
+        try:
+            download = self.downloads[count - 1]
+        except IndexError:
+            raise cmdexc.CommandError("There's no download {}!".format(count))
+        download.cancel()
+
     @pyqtSlot(QNetworkRequest, QNetworkReply)
     def on_redirect(self, download, request, reply):
         """Handle a HTTP redirect of a download.

qutebrowser/browser/webpage.py

@@ -21,9 +21,9 @@
 
 import functools
 
-from PyQt5.QtCore import pyqtSignal, pyqtSlot, PYQT_VERSION, Qt, QUrl
+from PyQt5.QtCore import pyqtSlot, PYQT_VERSION, Qt, QUrl
 from PyQt5.QtGui import QDesktopServices
-from PyQt5.QtNetwork import QNetworkReply
+from PyQt5.QtNetwork import QNetworkReply, QNetworkRequest
 from PyQt5.QtWidgets import QFileDialog
 from PyQt5.QtPrintSupport import QPrintDialog
 from PyQt5.QtWebKitWidgets import QWebPage
@@ -42,13 +42,8 @@ class BrowserPage(QWebPage):
         _extension_handlers: Mapping of QWebPage extensions to their handlers.
         _networkmnager: The NetworkManager used.
         _win_id: The window ID this BrowserPage is associated with.
-
-    Signals:
-        start_download: Emitted when a file should be downloaded.
     """
 
-    start_download = pyqtSignal('QNetworkReply*')
-
     def __init__(self, win_id, parent=None):
         super().__init__(parent)
         self._win_id = win_id
@@ -166,9 +161,17 @@ class BrowserPage(QWebPage):
 
     @pyqtSlot('QNetworkRequest')
     def on_download_requested(self, request):
-        """Called when the user wants to download a link."""
+        """Called when the user wants to download a link.
-        reply = self.networkAccessManager().get(request)
+
-        self.start_download.emit(reply)
+        We need to construct a copy of the QNetworkRequest here as the
+        download_manager needs it async and we'd get a segfault otherwise as
+        soon as the user has entered the filename, as Qt seems to delete it
+        after this slot returns.
+        """
+        req = QNetworkRequest(request)
+        download_manager = objreg.get('download-manager', scope='window',
+                                      window=self._win_id)
+        download_manager.get_request(req, page=self)
 
     @pyqtSlot('QNetworkReply')
     def on_unsupported_content(self, reply):
@@ -181,9 +184,11 @@ class BrowserPage(QWebPage):
         here: http://mimesniff.spec.whatwg.org/
         """
         inline, _suggested_filename = http.parse_content_disposition(reply)
+        download_manager = objreg.get('download-manager', scope='window',
+                                      window=self._win_id)
         if not inline:
             # Content-Disposition: attachment -> force download
-            self.start_download.emit(reply)
+            download_manager.fetch(reply)
             return
         mimetype, _rest = http.parse_content_type(reply)
         if mimetype == 'image/jpg':
@@ -198,7 +203,7 @@ class BrowserPage(QWebPage):
                     self.display_content, reply, 'image/jpeg'))
         else:
             # Unknown mimetype, so download anyways.
-            self.start_download.emit(reply)
+            download_manager.fetch(reply)
 
     def userAgentForUrl(self, url):
         """Override QWebPage::userAgentForUrl to customize the user agent."""

qutebrowser/test/utils/test_urlutils.py

@@ -206,5 +206,25 @@ class QurlFromUserInputTests(unittest.TestCase):
             'http://[::1]')
 
 
+class FilenameFromUrlTests(unittest.TestCase):
+
+    """Tests for filename_from_url."""
+
+    def test_invalid_url(self):
+        """Test with an invalid QUrl."""
+        self.assertEqual(urlutils.filename_from_url(QUrl()), None)
+
+    def test_url_path(self):
+        """Test with an URL with path."""
+        url = QUrl('http://qutebrowser.org/test.html')
+        self.assertEqual(urlutils.filename_from_url(url), 'test.html')
+
+    def test_url_host(self):
+        """Test with an URL with no path."""
+        url = QUrl('http://qutebrowser.org/')
+        self.assertEqual(urlutils.filename_from_url(url),
+                         'qutebrowser.org.html')
+
+
 if __name__ == '__main__':
     unittest.main()

qutebrowser/utils/urlutils.py

@@ -22,6 +22,7 @@
 import re
 import os.path
 import ipaddress
+import posixpath
 import urllib.parse
 
 from PyQt5.QtCore import QUrl
@@ -288,6 +289,26 @@ def raise_cmdexc_if_invalid(url):
         raise cmdexc.CommandError(errstr)
 
 
+def filename_from_url(url):
+    """Get a suitable filename from an URL.
+
+    Args:
+        url: The URL to parse, as a QUrl.
+
+    Return:
+        The suggested filename as a string, or None.
+    """
+    if not url.isValid():
+        return None
+    pathname = posixpath.basename(url.path())
+    if pathname:
+        return pathname
+    elif url.host():
+        return url.host() + '.html'
+    else:
+        return None
+
+
 class FuzzyUrlError(Exception):
 
     """Exception raised by fuzzy_url on problems."""

qutebrowser/widgets/mainwindow.py

@@ -200,7 +200,6 @@ class MainWindow(QWidget):
         message_bridge = self._get_object('message-bridge')
         mode_manager = self._get_object('mode-manager')
         prompter = self._get_object('prompter')
-        download_manager = self._get_object('download-manager')
 
         # misc
         self._tabbed_browser.close_window.connect(self.close)
@@ -262,9 +261,6 @@ class MainWindow(QWidget):
             completion_obj.on_clear_completion_selection)
         cmd.hide_completion.connect(completion_obj.hide)
 
-        # downloads
-        tabs.start_download.connect(download_manager.fetch)
-
         # quickmark completion
         quickmark_manager = objreg.get('quickmark-manager')
         quickmark_manager.changed.connect(completer.init_quickmark_completions)

qutebrowser/widgets/tabbedbrowser.py

@@ -76,8 +76,6 @@ class TabbedBrowser(tabwidget.TabWidget):
         resized: Emitted when the browser window has resized, so the completion
                  widget can adjust its size to it.
                  arg: The new size.
-        start_download: Emitted when any tab wants to start downloading
-                        something.
         current_tab_changed: The current tab changed to the emitted WebView.
     """
 
@@ -89,7 +87,6 @@ class TabbedBrowser(tabwidget.TabWidget):
     cur_link_hovered = pyqtSignal(str, str, str)
     cur_scroll_perc_changed = pyqtSignal(int, int)
     cur_load_status_changed = pyqtSignal(str)
-    start_download = pyqtSignal('QNetworkReply*')
     close_window = pyqtSignal()
     resized = pyqtSignal('QRect')
     got_cmd = pyqtSignal(str)
@@ -163,8 +160,6 @@ class TabbedBrowser(tabwidget.TabWidget):
             self._filter.create(self.cur_load_status_changed, tab))
         tab.url_text_changed.connect(
             functools.partial(self.on_url_text_changed, tab))
-        # downloads
-        page.start_download.connect(self.start_download)
         # misc
         tab.titleChanged.connect(
             functools.partial(self.on_title_changed, tab))