Repository: qutebrowser/qutebrowser
Pull request: feat(download) support external location picker
Closes #1365 

I haven't added tests yes cause it would be great to have some feedback if others think this is a good way to handle things.

What this MR enables is a new command and keybind in the download prompt to trigger an external folder picker. When the picker finishes the prompt gets updated with a new default value.

One could do some things differently here. For example:
* When a folder is chosen, directly accept the prompt. However I thought it would be better to first see what was chosen and then accept.
* Allow a setting such that the external picker is triggered directly. But is seems nice to still have the option to for example choose to open the download etc.

Problem statement:
Add an option to use the native filepicker for downloads
Some users would prefer a native filepicker dialog to the current statusbar input (without completion), so there should be an option to use that.

edit: In addition, this should allow to switch from the default behavior of using the native file dialog for uploads to using the statusbar input as well.

Hints from issue discussion:
+1 this is helpful when you want to preview images before uploading

Hi, I'm trying to get involved in the development of Qutebrowser and I figured this may be a good place for me to start, could you tell me what files I should look at and any other tips you have for me. Thanks!
Hey @BunnyApocalypse!

Right now is a bit of a bad timing as I have a quite big backlog (as I was busy with exams and then holidays until this week), and because a new config system is soon going to be merged, which will make contributions against the `master` branch conflict once that's in.

I'll get back to you in a few days though - if you don't hear back within a week or so, feel free to ping me again!

In the meantime, you might want to take a look at the [contribution docs](https://github.com/qutebrowser/qutebrowser/blob/master/CONTRIBUTING.asciidoc) and get the testsuite to run on your machine.
@The-Compiler Isn't this done already? I have no idea what the "statusbar input" is.
@user202729 What's the file prompt thingy nowadays used to be an input in the statusbar. However, this isn't done yet -- there still isn't an option to use Qt's native file picker instead.
I have currently the problem that when using qutebrowser under Gnome 3.34, the GTK-file-chooser comes up when trying to upload files, but is not usable because it is listing only directories.

I don't mind which file-chooser comes up, but a non-functional one does not help me. When I read „native“, does this mean it will be an upload dialog like it is already done for saving files?
@poinck The GTK file picker *is* the native file picker (rather than qutebrowser's), so this issue is pretty much the opposite of what you describe. You're probably seeing #5231, which is a Qt bug fixed in Qt 5.14.2.
@The-Compiler Unfortunatly I have only 5.14.1 available on my distro. /:
For downloads, this is also useful for Flatpak, see #4879. Additionally, it'd be nice to have a shortcut in qutebrowser's prompt to open the native one instead.
For what it's worth, you can _kind of_ use an external file manager in place of the download prompt if your external file manager has decent commandline options and you're willing to create an alias and trigger a custom userscript.

Here's a proof-of-concept userscript that downloads the current page and lets you choose a destination with the help of ranger:

```
#!/usr/bin/env bash
set -o errexit
set -o pipefail

if [ -z "$QUTE_FIFO" ] ; then
    help
    exit
fi

tmp_file="/tmp/qute-ranger-chosen"

if test -f "$tmp_file"; then
    rm "$tmp_file"
fi

xterm -e "ranger --choosedir=\"$tmp_file\" --show-only-dirs"

if test -f "$tmp_file"; then
    chosen="$(cat "$tmp_file")"
    rm "$tmp_file"
    xterm -e "read -p \"File to save as? [d]efault/(o)verwrite/(n)ew: \" -N 1 choice; echo \"\$choice\" > \"$tmp_file\""
    choice="$(cat "$tmp_file")"
    rm "$tmp_file"
    case $choice in
        o)
            xterm -e "ranger --choosefile=\"$tmp_file\" --selectfile=\"$chosen\""
            if test -f "$tmp_file"; then
                chosen="$(cat "$tmp_file")"
                rm "$tmp_file"
            fi
            ;;
        n)
            xterm -e "read -p \"Name: \" name; echo \"\$name\" > \"$tmp_file\""
            if test -f "$tmp_file"; then
                chosen="$chosen/$(cat "$tmp_file")"
                rm "$tmp_file"
            fi
            ;;
    esac
    echo "download --dest \"$chosen\"" >> "$QUTE_FIFO"
fi
```

Commit message: test(downloads) wip test for external fileselect

Patch to review:
diff --git a/qutebrowser/browser/shared.py b/qutebrowser/browser/shared.py
index f195bbf28cc..7d11773ae7c 100644
--- a/qutebrowser/browser/shared.py
+++ b/qutebrowser/browser/shared.py
@@ -404,7 +404,7 @@ class FileSelectionMode(enum.Enum):
 
 
 def choose_file(qb_mode: FileSelectionMode) -> List[str]:
-    """Select file(s)/folder for uploading, using external command defined in config.
+    """Select file(s)/folder for uploading/downloading, using external command defined in config.
 
     Args:
         qb_mode: File selection mode
@@ -450,7 +450,7 @@ def _execute_fileselect_command(
     """Execute external command to choose file.
 
     Args:
-        multiple: Should selecting multiple files be allowed.
+        qb_mode: Should selecting multiple files be allowed.
         tmpfilename: Path to the temporary file if used, otherwise None.
 
     Return:
diff --git a/qutebrowser/config/configdata.yml b/qutebrowser/config/configdata.yml
index 95338b36fdc..ff333d21d18 100644
--- a/qutebrowser/config/configdata.yml
+++ b/qutebrowser/config/configdata.yml
@@ -3780,6 +3780,7 @@ bindings.default:
       <Down>: prompt-item-focus next
       <Alt-Y>: prompt-yank
       <Alt-Shift-Y>: prompt-yank --sel
+      <Alt-E>: prompt-fileselect-external
       <Ctrl-B>: rl-backward-char
       <Ctrl-F>: rl-forward-char
       <Alt-B>: rl-backward-word
diff --git a/qutebrowser/mainwindow/prompt.py b/qutebrowser/mainwindow/prompt.py
index 29ea9a45fc3..6d41a26ffc8 100644
--- a/qutebrowser/mainwindow/prompt.py
+++ b/qutebrowser/mainwindow/prompt.py
@@ -467,6 +467,31 @@ def prompt_yank(self, sel=False):
         utils.set_clipboard(question.url, sel)
         message.info("Yanked to {}: {}".format(target, question.url))
 
+    @cmdutils.register(
+        instance='prompt-container', scope='window',
+        modes=[usertypes.KeyMode.prompt])
+    def prompt_fileselect_external(self):
+        """Choose a location using a configured external picker."""
+        if not (self._prompt and isinstance(self._prompt, FilenamePrompt)):
+            raise cmdutils.CommandError(
+                f"Can only launch external fileselect for FilenamePrompt, not {self._prompt}"
+            )
+        picker = config.val.fileselect.folder.command
+        if not picker:
+            raise cmdutils.CommandError(
+                "No external location picker (fileselect.folder.command) configured."
+            )
+        # TODO fix cyclic import
+        from qutebrowser.browser import shared
+        folders = shared.choose_file(shared.FileSelectionMode.folder)
+        if not folders:
+            message.info("No folder chosen.")
+            return
+        # choose_file already checks that this is max one folder
+        assert len(folders) == 1
+        folder = folders[0]
+        self._prompt.accept(folder)
+
 
 class LineEdit(QLineEdit):
 
@@ -834,6 +859,7 @@ def _allowed_commands(self):
             ('prompt-open-download', "Open download"),
             ('prompt-open-download --pdfjs', "Open download via PDF.js"),
             ('prompt-yank', "Yank URL"),
+            ('prompt-fileselect-external', "Launch external fileselect"),
         ]
         return cmds
 
diff --git a/tests/end2end/features/downloads.feature b/tests/end2end/features/downloads.feature
index dfdb247040a..2d5ed8a2c0e 100644
--- a/tests/end2end/features/downloads.feature
+++ b/tests/end2end/features/downloads.feature
@@ -684,3 +684,16 @@ Feature: Downloading things from a website.
         When I set downloads.location.prompt to false
         And I open 500-inline
         Then the error "Download error: *INTERNAL SERVER ERROR" should be shown
+
+    ## External download path fileselector
+
+    Scenario: Select download path
+        When I set downloads.location.prompt to true
+        And I set fileselect.handler to external
+        And I setup a fake folder fileselector selecting "tests/end2end/data/backforward/" and writes to a temporary file
+        And I open data/downloads/downloads.html
+        And I run :click-element id download
+        And I wait for the download prompt for "*/download.bin"
+        And I run :prompt-external-picker
+        # TODO what should the check be?
+        Then the javascript message "Files: 1.txt" should be logged
diff --git a/tests/end2end/features/test_downloads_bdd.py b/tests/end2end/features/test_downloads_bdd.py
index 804ed40fefd..79cc84551a6 100644
--- a/tests/end2end/features/test_downloads_bdd.py
+++ b/tests/end2end/features/test_downloads_bdd.py
@@ -19,6 +19,7 @@
 
 import os
 import sys
+import json
 import shlex
 
 import pytest
@@ -167,3 +168,34 @@ def fifo_should_be_fifo(tmpdir):
     download_dir = tmpdir / 'downloads'
     assert download_dir.exists()
     assert not os.path.isfile(str(download_dir / 'fifo'))
+
+
+# TODO how to share this with test_editor_bdd.py?
+@bdd.when(bdd.parsers.parse('I setup a fake {kind} fileselector '
+                            'selecting "{files}" and writes to {output_type}'))
+def set_up_fileselector(quteproc, py_proc, kind, files, output_type):
+    """Set up fileselect.xxx.command to select the file(s)."""
+    cmd, args = py_proc(r"""
+        import os
+        import sys
+        tmp_file = None
+        for i, arg in enumerate(sys.argv):
+            if arg.startswith('--file='):
+                tmp_file = arg[len('--file='):]
+                sys.argv.pop(i)
+                break
+        selected_files = sys.argv[1:]
+        if tmp_file is None:
+            for selected_file in selected_files:
+                print(os.path.abspath(selected_file))
+        else:
+            with open(tmp_file, 'w') as f:
+                for selected_file in selected_files:
+                    f.write(os.path.abspath(selected_file) + '\n')
+    """)
+    args += files.split(' ')
+    if output_type == "a temporary file":
+        args += ['--file={}']
+    fileselect_cmd = json.dumps([cmd, *args])
+    quteproc.set_setting('fileselect.handler', 'external')
+    quteproc.set_setting(f'fileselect.{kind}.command', fileselect_cmd)


Code review comments:
diff_hunk: @@ -404,7 +404,7 @@ class FileSelectionMode(enum.Enum):
 
 
 def choose_file(qb_mode: FileSelectionMode) -> List[str]:
-    """Select file(s)/folder for uploading, using external command defined in config.
+    """Select file(s)/folder for uploading/downloading, using external command defined in config.
path: qutebrowser/browser/shared.py
text: @user1:
```suggestion
    """Select file(s)/folder for up-/downloading, using an external command.
```

or so.

diff_hunk: @@ -684,3 +684,16 @@ Feature: Downloading things from a website.
         When I set downloads.location.prompt to false
         And I open 500-inline
         Then the error "Download error: *INTERNAL SERVER ERROR" should be shown
+
+    ## External download path fileselector
+
+    Scenario: Select download path
+        When I set downloads.location.prompt to true
+        And I set fileselect.handler to external
+        And I setup a fake folder fileselector selecting "tests/end2end/data/backforward/" and writes to a temporary file
+        And I open data/downloads/downloads.html
+        And I run :click-element id download
+        And I wait for the download prompt for "*/download.bin"
path: tests/end2end/features/downloads.feature
text: @user1:
This doesn't work because the tests end up waiting for:

```
----> Waiting for "Asking question <qutebrowser.utils.usertypes.Question default='*/download.bin' mode=<PromptMode.do… in the log
```

but the log output reads:

```
11:25:14.821 DEBUG    prompt     prompt:ask_question:197 Starting loop.exec() for <qutebrowser.utils.usertypes.Question default='/tmp/pytest-of-florian/pytest-1/test_select_download_path0/downloads/' mode=<PromptMode.download: 5> option=None text='Please enter a location for <b>http://localhost:59185/data/downloads/download.bin</b>' title='Save file to:'>
```

note the `default='*/download.bin'` vs. `default='/tmp/pytest-of-florian/pytest-1/test_select_download_path0/downloads/' - in other words, matching the filename here requires `And I set downloads.location.suggestion to both`.

I'd just do

```suggestion
        And I wait for the download prompt for "*"
```

like in some of the other tests, since the filename doesn't matter for what we actually want to test.

diff_hunk: @@ -684,3 +684,16 @@ Feature: Downloading things from a website.
         When I set downloads.location.prompt to false
         And I open 500-inline
         Then the error "Download error: *INTERNAL SERVER ERROR" should be shown
+
+    ## External download path fileselector
+
+    Scenario: Select download path
+        When I set downloads.location.prompt to true
+        And I set fileselect.handler to external
+        And I setup a fake folder fileselector selecting "tests/end2end/data/backforward/" and writes to a temporary file
+        And I open data/downloads/downloads.html
+        And I run :click-element id download
+        And I wait for the download prompt for "*/download.bin"
+        And I run :prompt-external-picker
path: tests/end2end/features/downloads.feature
text: @user1:
```suggestion
        And I run :prompt-fileselect-external
```

diff_hunk: @@ -684,3 +684,16 @@ Feature: Downloading things from a website.
         When I set downloads.location.prompt to false
         And I open 500-inline
         Then the error "Download error: *INTERNAL SERVER ERROR" should be shown
+
+    ## External download path fileselector
+
+    Scenario: Select download path
+        When I set downloads.location.prompt to true
+        And I set fileselect.handler to external
+        And I setup a fake folder fileselector selecting "tests/end2end/data/backforward/" and writes to a temporary file
path: tests/end2end/features/downloads.feature
text: @user1:
```suggestion
        And I setup a fake folder fileselector selecting "(tmpdir)(dirsep)downloads(dirsep)subdir" and writes to a temporary file
```

to place the file into a well-defined directory which isn't the default. Search for `(tmpdir)` in this file for some inspiration - the subdir gets created by `Given I set up a temporary download dir` via the `download_dir` fixture in `test_downloads_bdd.py`.

Note that you will need to also adjust the Python code to carry out those replacements, as they are not done universally (thinking about it, maybe they should be?). See `run_command` in `tests/end2end/features/conftest.py` for inspiration.

diff_hunk: @@ -834,6 +859,7 @@ def _allowed_commands(self):
             ('prompt-open-download', "Open download"),
             ('prompt-open-download --pdfjs', "Open download via PDF.js"),
             ('prompt-yank', "Yank URL"),
+            ('prompt-fileselect-external', "Launch external fileselect"),
path: qutebrowser/mainwindow/prompt.py
text: @user1:
```suggestion
            ('prompt-fileselect-external', "Launch external file selector"),
```

since the description is a bit more "human readable" than command/setting names.

diff_hunk: @@ -467,6 +467,31 @@ def prompt_yank(self, sel=False):
         utils.set_clipboard(question.url, sel)
         message.info("Yanked to {}: {}".format(target, question.url))
 
+    @cmdutils.register(
+        instance='prompt-container', scope='window',
+        modes=[usertypes.KeyMode.prompt])
+    def prompt_fileselect_external(self):
+        """Choose a location using a configured external picker."""
+        if not (self._prompt and isinstance(self._prompt, FilenamePrompt)):
+            raise cmdutils.CommandError(
+                f"Can only launch external fileselect for FilenamePrompt, not {self._prompt}"
path: qutebrowser/mainwindow/prompt.py
text: @user1:
```suggestion
                f"Can only launch external fileselect for FilenamePrompt, not {self._prompt.__class__.__name__}"
```

Right now e.g. doing `:later 2000 prompt-fileselect-external` and then doing `m` (`:quickmark-save`) displays `Can only launch external fileselect for FilenamePrompt, not qutebrowser.mainwindow.prompt.LineEditPrompt(question=<qutebrowser.utils.usertypes.Question default=None mode=<PromptMode.text: 2> option=None text='Please enter a quickmark name for<br/><b>https://start.duckduckgo.com/</b>' title='Add quickmark:'>)` which is a bit much from an user's perspective :wink:

diff_hunk: @@ -467,6 +467,31 @@ def prompt_yank(self, sel=False):
         utils.set_clipboard(question.url, sel)
         message.info("Yanked to {}: {}".format(target, question.url))
 
+    @cmdutils.register(
+        instance='prompt-container', scope='window',
+        modes=[usertypes.KeyMode.prompt])
+    def prompt_fileselect_external(self):
+        """Choose a location using a configured external picker."""
+        if not (self._prompt and isinstance(self._prompt, FilenamePrompt)):
path: qutebrowser/mainwindow/prompt.py
text: @user1:
`self._prompt` should never be `None` in prompt mode, so I'd suggest

```suggestion
        assert self._prompt is not None
        if not isinstance(self._prompt, FilenamePrompt):
```

diff_hunk: @@ -684,3 +684,16 @@ Feature: Downloading things from a website.
         When I set downloads.location.prompt to false
         And I open 500-inline
         Then the error "Download error: *INTERNAL SERVER ERROR" should be shown
+
+    ## External download path fileselector
+
+    Scenario: Select download path
+        When I set downloads.location.prompt to true
+        And I set fileselect.handler to external
+        And I setup a fake folder fileselector selecting "tests/end2end/data/backforward/" and writes to a temporary file
+        And I open data/downloads/downloads.html
+        And I run :click-element id download
+        And I wait for the download prompt for "*/download.bin"
+        And I run :prompt-external-picker
+        # TODO what should the check be?
+        Then the javascript message "Files: 1.txt" should be logged
path: tests/end2end/features/downloads.feature
text: @user1:
With the change above:

```suggestion
        And I wait until the download is finished
        Then the downloaded file subdir/download.bin should exist
```

diff_hunk: @@ -167,3 +168,34 @@ def fifo_should_be_fifo(tmpdir):
     download_dir = tmpdir / 'downloads'
     assert download_dir.exists()
     assert not os.path.isfile(str(download_dir / 'fifo'))
+
+
+# TODO how to share this with test_editor_bdd.py?
path: tests/end2end/features/test_downloads_bdd.py
text: @user1:
Move it to `tests/end2end/features/conftest.py` instead.

@author:
Done :+1: