Merge branch 'Kingdread-open-download-fixes'

CHANGELOG.asciidoc

@@ -40,6 +40,8 @@ Changed
   now prefer the count instead of showing an error message.
 - `:open` now has an `--implicit` argument to treat the opened tab as implicit
   (i.e. to open it at the position it would be opened if it was a clicked link)
+- `:download-open` and `:prompt-open-download` now have an optional `cmdline`
+  argument to pass a commandline to open the download with.
 
 v0.8.2
 ------

doc/help/commands.asciidoc

@@ -215,11 +215,24 @@ The index of the download to delete.
 
 [[download-open]]
 === download-open
+Syntax: +:download-open ['cmdline']+
+
 Open the last/[count]th download.
 
+If no specific command is given, this will use the system's default application to open the file.
+
+==== positional arguments
+* +'cmdline'+: The command which should be used to open the file. A `{}` is expanded to the temporary file name. If no `{}` is
+ present, the filename is automatically appended to the
+ cmdline.
+
+
 ==== count
 The index of the download to open.
 
+==== note
+* This command does not split arguments after the last argument and handles quotes literally.
+
 [[download-remove]]
 === download-remove
 Syntax: +:download-remove [*--all*]+
@@ -1168,8 +1181,21 @@ Answer no to a yes/no prompt.
 
 [[prompt-open-download]]
 === prompt-open-download
+Syntax: +:prompt-open-download ['cmdline']+
+
 Immediately open a download.
 
+If no specific command is given, this will use the system's default application to open the file.
+
+==== positional arguments
+* +'cmdline'+: The command which should be used to open the file. A `{}` is expanded to the temporary file name. If no `{}` is
+ present, the filename is automatically appended to the
+ cmdline.
+
+
+==== note
+* This command does not split arguments after the last argument and handles quotes literally.
+
 [[prompt-yes]]
 === prompt-yes
 Answer yes to a yes/no prompt.

qutebrowser/browser/webkit/downloads.py

@@ -22,6 +22,7 @@
 import io
 import os
 import sys
+import shlex
 import os.path
 import shutil
 import functools
@@ -40,6 +41,7 @@ from qutebrowser.config import config
 from qutebrowser.commands import cmdexc, cmdutils
 from qutebrowser.utils import (message, usertypes, log, utils, urlutils,
                                objreg, standarddir, qtutils)
+from qutebrowser.misc import guiprocess
 from qutebrowser.browser.webkit import http
 from qutebrowser.browser.webkit.network import networkmanager
 
@@ -528,8 +530,15 @@ class DownloadItem(QObject):
         self.cancel()
 
     @pyqtSlot()
-    def open_file(self):
+    def open_file(self, cmdline=None):
-        """Open the downloaded file."""
+        """Open the downloaded file.
+
+        Args:
+            cmdline: The command to use as string. A `{}` is expanded to the
+                     filename. None means to use the system's default
+                     application. If no `{}` is found, the filename is appended
+                     to the cmdline.
+        """
         assert self.successful
         filename = self._filename
         if filename is None:
@@ -537,8 +546,22 @@ class DownloadItem(QObject):
         if filename is None:
             log.downloads.error("No filename to open the download!")
             return
+
+        if cmdline is None:
+            log.downloads.debug("Opening {} with the system application"
+                                .format(filename))
             url = QUrl.fromLocalFile(filename)
             QDesktopServices.openUrl(url)
+            return
+
+        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(self._win_id, what='download')
+        proc.start_detached(cmd, args)
 
     def set_filename(self, filename):
         """Set the filename to save the download to.
@@ -955,19 +978,25 @@ class DownloadManager(QAbstractListModel):
                 download.cancel()
                 return
             download.finished.connect(
-                functools.partial(self._open_download, download))
+                functools.partial(self._open_download, download,
+                                  target.cmdline))
             download.autoclose = True
             download.set_fileobj(fobj)
         else:
             log.downloads.error("Unknown download target: {}".format(target))
 
-    def _open_download(self, download):
+    def _open_download(self, download, cmdline):
-        """Open the given download but only if it was successful."""
+        """Open the given download but only if it was successful.
-        if download.successful:
+
-            download.open_file()
+        Args:
-        else:
+            download: The DownloadItem to use.
+            cmdline: Passed to DownloadItem.open_file().
+        """
+        if not download.successful:
             log.downloads.debug("{} finished but not successful, not opening!"
                                 .format(download))
+            return
+        download.open_file(cmdline)
 
     def raise_no_download(self, count):
         """Raise an exception that the download doesn't exist.
@@ -1026,12 +1055,19 @@ class DownloadManager(QAbstractListModel):
         self.remove_item(download)
         log.downloads.debug("deleted download {}".format(download))
 
-    @cmdutils.register(instance='download-manager', scope='window')
+    @cmdutils.register(instance='download-manager', scope='window', maxsplit=0)
     @cmdutils.argument('count', count=True)
-    def download_open(self, count=0):
+    def download_open(self, cmdline: str=None, count=0):
         """Open the last/[count]th download.
 
+        If no specific command is given, this will use the system's default
+        application to open the file.
+
         Args:
+            cmdline: The command which should be used to open the file. A `{}`
+                     is expanded to the temporary file name. If no `{}` is
+                     present, the filename is automatically appended to the
+                     cmdline.
             count: The index of the download to open.
         """
         try:
@@ -1042,7 +1078,7 @@ class DownloadManager(QAbstractListModel):
             if not count:
                 count = len(self.downloads)
             raise cmdexc.CommandError("Download {} is not done!".format(count))
-        download.open_file()
+        download.open_file(cmdline)
 
     @cmdutils.register(instance='download-manager', scope='window')
     @cmdutils.argument('count', count=True)
@@ -1334,8 +1370,7 @@ class TempDownloadManager(QObject):
         encoding = sys.getfilesystemencoding()
         suggested_name = utils.force_encoding(suggested_name, encoding)
         # Make sure that the filename is not too long
-        if len(suggested_name) > 50:
+        suggested_name = utils.elide_filename(suggested_name, 50)
-            suggested_name = suggested_name[:25] + '...' + suggested_name[-25:]
         fobj = tempfile.NamedTemporaryFile(dir=tmpdir.name, delete=False,
                                            suffix=suggested_name)
         self.files.append(fobj)

qutebrowser/mainwindow/statusbar/prompter.py

@@ -312,13 +312,23 @@ class Prompter(QObject):
         self._question.done()
 
     @cmdutils.register(instance='prompter', hide=True, scope='window',
-                       modes=[usertypes.KeyMode.prompt])
+                       modes=[usertypes.KeyMode.prompt], maxsplit=0)
-    def prompt_open_download(self):
+    def prompt_open_download(self, cmdline: str=None):
-        """Immediately open a download."""
+        """Immediately open a download.
+
+        If no specific command is given, this will use the system's default
+        application to open the file.
+
+        Args:
+            cmdline: The command which should be used to open the file. A `{}`
+                     is expanded to the temporary file name. If no `{}` is
+                     present, the filename is automatically appended to the
+                     cmdline.
+        """
         if self._question.mode != usertypes.PromptMode.download:
             # We just ignore this if we don't have a download question.
             return
-        self._question.answer = usertypes.OpenFileDownloadTarget()
+        self._question.answer = usertypes.OpenFileDownloadTarget(cmdline)
         modeman.maybe_leave(self._win_id, usertypes.KeyMode.prompt,
                             'download open')
         self._question.done()

qutebrowser/utils/usertypes.py

@@ -297,11 +297,17 @@ class FileObjDownloadTarget(DownloadTarget):
 
 class OpenFileDownloadTarget(DownloadTarget):
 
-    """Save the download in a temp dir and directly open it."""
+    """Save the download in a temp dir and directly open it.
 
-    def __init__(self):
+    Attributes:
+        cmdline: The command to use as string. A `{}` is expanded to the
+                 filename. None means to use the system's default application.
+                 If no `{}` is found, the filename is appended to the cmdline.
+    """
+
+    def __init__(self, cmdline=None):
         # pylint: disable=super-init-not-called
-        pass
+        self.cmdline = cmdline
 
 
 class Question(QObject):

qutebrowser/utils/utils.py

@@ -58,6 +58,38 @@ def elide(text, length):
         return text[:length - 1] + '\u2026'
 
 
+def elide_filename(filename, length):
+    """Elide a filename to the given length.
+
+    The difference to the elide() is that the text is removed from
+    the middle instead of from the end. This preserves file name extensions.
+    Additionally, standard ASCII dots are used ("...") instead of the unicode
+    "…" (U+2026) so it works regardless of the filesystem encoding.
+
+    This function does not handle path separators.
+
+    Args:
+        filename: The filename to elide.
+        length: The maximum length of the filename, must be at least 3.
+
+    Return:
+        The elided filename.
+    """
+    elidestr = '...'
+    if length < len(elidestr):
+        raise ValueError('length must be greater or equal to 3')
+    if len(filename) <= length:
+        return filename
+    # Account for '...'
+    length -= len(elidestr)
+    left = length // 2
+    right = length - left
+    if right == 0:
+        return filename[:left] + elidestr
+    else:
+        return filename[:left] + elidestr + filename[-right:]
+
+
 def compact_text(text, elidelength=None):
     """Remove leading whitespace and newlines from a text and maybe elide it.
 

tests/end2end/data/downloads/issue1725.html

@@ -0,0 +1,13 @@
+<html>
+    <head>
+        <title>test case for issue 1725</title>
+    </head>
+    <body>
+        <p>Using <code>:prompt-open-download</code> with a file that has a loooooong filename</p>
+        <p>
+        <a href="/response-headers?Content-Disposition=download;%20filename%2A%3DUTF-8%27%27I%2527ve%2520actually%2520felt%2520slightly%2520uncomfortable%2520at%2520TED%2520for%2520the%2520last%2520two%2520days%252C%2520because%2520there%2527s%2520a%2520lot%2520of%2520vision%2520going%2520on%252C%2520right%253F%2520And%2520I%2520am%2520not%2520a%2520visionary.%2520I%2520do%2520not%2520have%2520a%2520five-year%2520plan.%2520I%2527m%2520an%2520engineer.%2520And%2520I%2520think%2520it%2527s%2520really%2520--%2520I%2520mean%2520--%2520I%2527m%2520perfectly%2520happy%2520with%2520all%2520the%2520people%2520who%2520are%2520walking%2520around%2520and%2520just%2520staring%2520at%2520the%2520clouds%2520and%2520looking%2520at%2520the%2520stars%2520and%2520saying%252C%2520%2522I%2520want%2520to%2520go%2520there.%2522%2520But%2520I%2527m%2520looking%2520at%2520the%2520ground%252C%2520and%2520I%2520want%2520to%2520fix%2520the%2520pothole%2520that%2527s%2520right%2520in%2520front%2520of%2520me%2520before%2520I%2520fall%2520in.%2520This%2520is%2520the%2520kind%2520of%2520person%2520I%2520am.txt">
+            Download me!
+            </a>
+        </p>
+    </body>
+</html>

tests/end2end/features/downloads.feature

@@ -158,11 +158,11 @@ Feature: Downloading things from a website.
 
     ## :download-open
 
-    # Scenario: Opening a download
+    Scenario: Opening a download
-    #     When I open data/downloads/download.bin
+        When I open data/downloads/download.bin
-    #     And I wait until the download is finished
+        And I wait until the download is finished
-    #     And I run :download-open
+        And I open the download
-    #     Then ...
+        Then "Opening *download.bin* with [*python*]" should be logged
 
     Scenario: Opening a download which does not exist
         When I run :download-open with count 42
@@ -178,6 +178,36 @@ Feature: Downloading things from a website.
         And I run :download-open with count 1
         Then the error "Download 1 is not done!" should be shown
 
+    ## opening a file directly (prompt-open-download)
+
+    Scenario: Opening a download directly
+        When I set storage -> prompt-download-directory to true
+        And I open data/downloads/download.bin
+        And I directly open the download
+        And I wait until the download is finished
+        Then "Opening *download.bin* with [*python*]" should be logged
+
+    ## https://github.com/The-Compiler/qutebrowser/issues/1728
+
+    Scenario: Cancelling a download that should be opened
+        When I set storage -> prompt-download-directory to true
+        And I run :download http://localhost:(port)/drip?numbytes=128&duration=5
+        And I directly open the download
+        And I run :download-cancel
+        Then "* finished but not successful, not opening!" should be logged
+
+    ## https://github.com/The-Compiler/qutebrowser/issues/1725
+
+    Scenario: Directly open a download with a very long filename
+        When I set storage -> prompt-download-directory to true
+        And I open data/downloads/issue1725.html
+        And I run :hint
+        And I run :follow-hint a
+        And I wait for "Asking question <qutebrowser.utils.usertypes.Question default=* mode=<PromptMode.download: 5> text='Save file to:'>, *" in the log
+        And I directly open the download
+        And I wait until the download is finished
+        Then "Opening * with [*python*]" should be logged
+
     ## completion -> download-path-suggestion
 
     Scenario: completion -> download-path-suggestion = path

tests/end2end/features/test_downloads_bdd.py

@@ -18,6 +18,8 @@
 # along with qutebrowser.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import sys
+import shlex
 
 import pytest_bdd as bdd
 bdd.scenarios('downloads.feature')
@@ -68,3 +70,14 @@ def download_prompt(tmpdir, quteproc, path):
            "text='Save file to:'>, *".format(full_path=full_path))
     quteproc.wait_for(message=msg)
     quteproc.send_cmd(':leave-mode')
+
+
+@bdd.when("I open the download")
+def download_open(quteproc):
+    cmd = '{} -c pass'.format(shlex.quote(sys.executable))
+    quteproc.send_cmd(':download-open {}'.format(cmd))
+
+@bdd.when("I directly open the download")
+def download_open_with_prompt(quteproc):
+    cmd = '{} -c pass'.format(shlex.quote(sys.executable))
+    quteproc.send_cmd(':prompt-open-download {}'.format(cmd))

tests/end2end/test_invocations.py

@@ -19,6 +19,8 @@
 
 """Test starting qutebrowser with special arguments/environments."""
 
+import sys
+
 import pytest
 
 BASE_ARGS = ['--debug', '--json-logging', '--no-err-windows', 'about:blank']
@@ -74,14 +76,28 @@ def test_ascii_locale(httpbin, tmpdir, quteproc_new):
     """Test downloads with LC_ALL=C set.
 
     https://github.com/The-Compiler/qutebrowser/issues/908
+    https://github.com/The-Compiler/qutebrowser/issues/1726
     """
     args = ['--temp-basedir'] + BASE_ARGS
     quteproc_new.start(args, env={'LC_ALL': 'C'})
     quteproc_new.set_setting('storage', 'download-directory', str(tmpdir))
+
+    # Test a normal download
     quteproc_new.set_setting('storage', 'prompt-download-directory', 'false')
     url = 'http://localhost:{port}/data/downloads/ä-issue908.bin'.format(
         port=httpbin.port)
     quteproc_new.send_cmd(':download {}'.format(url))
+    quteproc_new.wait_for(category='downloads', message='Download finished')
+
+    # Test :prompt-open-download
+    quteproc_new.set_setting('storage', 'prompt-download-directory', 'true')
+    quteproc_new.send_cmd(':download {}'.format(url))
+    quteproc_new.send_cmd(':prompt-open-download "{}" -c pass'
+                          .format(sys.executable))
+    quteproc_new.wait_for(category='downloads', message='Download finished')
+    quteproc_new.wait_for(category='downloads',
+                          message='Opening * with [*python*]')
+
     quteproc_new.send_cmd(':quit')
     quteproc_new.wait_for_quit()
 

tests/unit/utils/test_utils.py

@@ -94,6 +94,24 @@ class TestEliding:
         assert utils.elide(text, length) == expected
 
 
+class TestElidingFilenames:
+
+    """Test elide_filename."""
+
+    def test_too_small(self):
+        """Test eliding to less than 3 characters which should fail."""
+        with pytest.raises(ValueError):
+            utils.elide_filename('foo', 1)
+
+    @pytest.mark.parametrize('filename, length, expected', [
+        ('foobar', 3, '...'),
+        ('foobar.txt', 50, 'foobar.txt'),
+        ('foobarbazqux.py', 10, 'foo...x.py'),
+    ])
+    def test_elided(self, filename, length, expected):
+        assert utils.elide_filename(filename, length) == expected
+
+
 @pytest.fixture(params=[True, False])
 def freezer(request, monkeypatch):
     if request.param and not getattr(sys, 'frozen', False):

tests/unit/utils/usertypes/test_downloadtarget.py

@@ -41,8 +41,13 @@ def test_fileobj():
 
 
 def test_openfile():
-    # Just make sure no error is raised, that should be enough.
+    target = usertypes.OpenFileDownloadTarget()
-    usertypes.OpenFileDownloadTarget()
+    assert target.cmdline is None
+
+
+def test_openfile_custom_command():
+    target = usertypes.OpenFileDownloadTarget('echo')
+    assert target.cmdline == 'echo'
 
 
 @pytest.mark.parametrize('obj', [