Add local file paths to URL completion

[amacfie]

In regards to #32 and #5792

This adds a new completion category for :open for local files. URLs starting with ~ work.

[The-Compiler]

Oh, that turned out to be simpler than I would've imagined, at least code-wise - great work so far, I don't think I'd have managed such a simple implementation!

Not a full review yet, but a couple of things coming to mind apart from the ones already added by the linters:

• Right now, without any input this displays paths relative to the current directory. However, those don't actually work, because :open doesn't handle them. It seems to me like it should only be triggered if the input actually is an absolute path?
• We're trying to move away from os.path / glob.glob, see Use pathlib #176 - could you please use pathlib instead?
• Any particular reason why you didn't add it to the default completion categories? I think once it doesn't display relative paths, it shouldn't be in the way, so why not make it the default?
• It'd be good if this could handle file:/// URLs as well.
• This will need some tests in tests/unit/completion/test_models.py (you can use pytest's tmp_path to build a file structure)

[amacfie]

I'm not sure if the current code works in Windows.

[The-Compiler]

Thanks for the quick changes! I did a full review and added some more comments.

Other than those, I think the only thing missing is the migration to pathlib (unless you see a good reason to not do that) and tests.

[The-Compiler]

I like the idea of offering the home directory here! A couple of thoughts:

• Should we really expand it, or just display ~ initially?
• Are there more useful directories we could add? Perhaps the downloads directory?
• Or should we even offer a config option so the user could add their preferred directories here? That seems like something that could be pretty useful, e.g. for people doing web development.

[rcorre]

Should we really expand it, or just display ~ initially?

I'd say just ~, and do the expansion behind-the-scenes, to keep paths shorter.

offer a config option so the user could add their preferred directories here

I like this idea. It might be worth considering completion.file_path.exclude too, in case the user has something like an sshfs that is slow to access (would that hold up the entire completion?)

[rcorre]

Actually I'd say we should do here whatever we do later. If we can always keep ~ un-expanded (which would be ideal IMHO), we should show ~ here. If that's infeasible, then we should expand here too.

[The-Compiler]

Maybe add it as a default here (I'd say above searchengines even) for discoverability?

[rcorre]

I'm not sure. Opening files in a browser is pretty rare for me, so it feels off to put it at the top. Still, you probably won't notice this feature unless you see the category or read the release notes, so 🤷

[The-Compiler]

Perhaps as a compromise, if we have a completion.filesystem.favorites (or whatever) setting, we could have that empty by default (i.e., not even including ~)? Then the (empty) category is hidden when just doing :open, but still appears when starting to enter an absolute path.

[amacfie]

The semantics should now be implemented. I will write tests. I've written some tests.

[The-Compiler]

Thanks! Merged with a couple of changes:

• 88cd06c Minor reformat
• fc07f2c Refactor FilePathCategory
• 12583d3 Update docs
• 6fd2077 Improve tests for filesystem completion
• f5d89fc Handle null bytes in filepath completion

[The-Compiler]

There are also some commands taking a filename:

• :config-source
• :config-write-py
• :download (but with a --dest argument, might be tricky)
• :print (but with a --pdf argument, might be tricky)
• :session-load (can take a .yml file instead of a session)
• :session-save (same)

Would you like to add completion for those as well? I think this would be an excellent base for doing so.

[amacfie]

There are also some commands taking a filename:

• :config-source
• :config-write-py
• :download (but with a --dest argument, might be tricky)
• :print (but with a --pdf argument, might be tricky)
• :session-load (can take a .yml file instead of a session)
• :session-save (same)

Would you like to add completion for those as well? I think this would be an excellent base for doing so.

I'll take a look.

[The-Compiler]

Looks like my fuzzing test (using hypothesis) caught a bug: https://github.com/qutebrowser/qutebrowser/runs/1735417829?check_suite_focus=true#step:8:135

I can't quite explain why it happens, but I now pushed another commit to avoid pathlib entirely and only use os.path, as I suspect that os.path.expanduser and pathlib.Path.expanduser disagree sometimes:

• 673c0be completion: Avoid pathlib in filepathcategory

From what I can see, this seems to have fixed the issue. Weird...

[The-Compiler]

I'll take a look.

Awesome, thank you!