Merge remote-tracking branch 'origin/pr/3950'

qutebrowser/browser/downloads.py

@@ -135,11 +135,12 @@ def create_full_filename(basename, filename):
     Return:
         The full absolute path, or None if filename creation was not possible.
     """
+    basename = utils.sanitize_filename(basename)
+    # Filename can be a full path so don't use sanitize_filename on it.
     # Remove chars which can't be encoded in the filename encoding.
     # See https://github.com/qutebrowser/qutebrowser/issues/427
     encoding = sys.getfilesystemencoding()
     filename = utils.force_encoding(filename, encoding)
-    basename = utils.force_encoding(basename, encoding)
     if os.path.isabs(filename) and (os.path.isdir(filename) or
                                     filename.endswith(os.sep)):
         # We got an absolute directory from the user, so we save it under
@@ -160,8 +161,7 @@ def get_filename_question(*, suggested_filename, url, parent=None):
         url: The URL the download originated from.
         parent: The parent of the question (a QObject).
     """
-    encoding = sys.getfilesystemencoding()
-    suggested_filename = utils.force_encoding(suggested_filename, encoding)
+    suggested_filename = utils.sanitize_filename(suggested_filename)
 
     q = usertypes.Question(parent)
     q.title = "Save file to:"
@@ -1260,8 +1260,7 @@ class TempDownloadManager:
             A tempfile.NamedTemporaryFile that should be used to save the file.
         """
         tmpdir = self.get_tmpdir()
-        encoding = sys.getfilesystemencoding()
-        suggested_name = utils.force_encoding(suggested_name, encoding)
+        suggested_name = utils.sanitize_filename(suggested_name)
         # Make sure that the filename is not too long
         suggested_name = utils.elide_filename(suggested_name, 50)
         fobj = tempfile.NamedTemporaryFile(dir=tmpdir.name, delete=False,

qutebrowser/utils/utils.py

@@ -500,10 +500,22 @@ def sanitize_filename(name, replacement='_'):
     """
     if replacement is None:
         replacement = ''
-    # Bad characters taken from Windows, there are even fewer on Linux
+
+    # Remove chars which can't be encoded in the filename encoding.
+    # See https://github.com/qutebrowser/qutebrowser/issues/427
+    encoding = sys.getfilesystemencoding()
+    name = force_encoding(name, encoding)
+
     # See also
     # https://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words
-    bad_chars = '\\/:*?"<>|'
+    if is_windows:
+        bad_chars = '\\/:*?"<>|'
+    elif is_mac:
+        # Colons can be confusing in finder https://superuser.com/a/326627
+        bad_chars = '/:'
+    else:
+        bad_chars = '/'
+
     for bad_char in bad_chars:
         name = name.replace(bad_char, replacement)
     return name

tests/helpers/fixtures.py

@@ -515,16 +515,35 @@ def mode_manager(win_registry, config_stub, key_config_stub, qapp):
     objreg.delete('mode-manager', scope='window', window=0)
 
 
+def standarddir_tmpdir(folder, monkeypatch, tmpdir):
+    """Set tmpdir/config as the configdir.
+
+    Use this to avoid creating a 'real' config dir (~/.config/qute_test).
+    """
+    confdir = tmpdir / folder
+    confdir.ensure(dir=True)
+    if hasattr(standarddir, folder):
+        monkeypatch.setattr(standarddir, folder,
+                            lambda **_kwargs: str(confdir))
+    return confdir
+
+
+@pytest.fixture
+def download_tmpdir(monkeypatch, tmpdir):
+    """Set tmpdir/download as the downloaddir.
+
+    Use this to avoid creating a 'real' download dir (~/.config/qute_test).
+    """
+    return standarddir_tmpdir('download', monkeypatch, tmpdir)
+
+
 @pytest.fixture
 def config_tmpdir(monkeypatch, tmpdir):
     """Set tmpdir/config as the configdir.
 
     Use this to avoid creating a 'real' config dir (~/.config/qute_test).
     """
-    confdir = tmpdir / 'config'
-    confdir.ensure(dir=True)
-    monkeypatch.setattr(standarddir, 'config', lambda auto=False: str(confdir))
-    return confdir
+    return standarddir_tmpdir('config', monkeypatch, tmpdir)
 
 
 @pytest.fixture
@@ -533,10 +552,7 @@ def data_tmpdir(monkeypatch, tmpdir):
 
     Use this to avoid creating a 'real' data dir (~/.local/share/qute_test).
     """
-    datadir = tmpdir / 'data'
-    datadir.ensure(dir=True)
-    monkeypatch.setattr(standarddir, 'data', lambda system=False: str(datadir))
-    return datadir
+    return standarddir_tmpdir('data', monkeypatch, tmpdir)
 
 
 @pytest.fixture
@@ -545,10 +561,7 @@ def runtime_tmpdir(monkeypatch, tmpdir):
 
     Use this to avoid creating a 'real' runtime dir.
     """
-    runtimedir = tmpdir / 'runtime'
-    runtimedir.ensure(dir=True)
-    monkeypatch.setattr(standarddir, 'runtime', lambda: str(runtimedir))
-    return runtimedir
+    return standarddir_tmpdir('runtime', monkeypatch, tmpdir)
 
 
 @pytest.fixture
@@ -557,10 +570,7 @@ def cache_tmpdir(monkeypatch, tmpdir):
 
     Use this to avoid creating a 'real' cache dir (~/.cache/qute_test).
     """
-    cachedir = tmpdir / 'cache'
-    cachedir.ensure(dir=True)
-    monkeypatch.setattr(standarddir, 'cache', lambda: str(cachedir))
-    return cachedir
+    return standarddir_tmpdir('cache', monkeypatch, tmpdir)
 
 
 @pytest.fixture

tests/unit/browser/webkit/test_downloads.py

@@ -22,6 +22,14 @@ import pytest
 from qutebrowser.browser import downloads, qtnetworkdownloads
 
 
+@pytest.fixture(autouse=True)
+def is_platform(monkeypatch, platform="windows"):
+    for p in ["mac", "linux", "posix", "windows"]:
+        monkeypatch.setattr(
+            'qutebrowser.utils.utils.is_{}'.format(p),
+            p == platform)
+
+
 def test_download_model(qapp, qtmodeltester, config_stub, cookiejar_and_cache,
                         fake_args):
     """Simple check for download model internals."""
@@ -92,3 +100,27 @@ class TestDownloadTarget:
     ])
     def test_class_hierarchy(self, obj):
         assert isinstance(obj, downloads._DownloadTarget)
+
+
+@pytest.mark.parametrize('raw, platform, expected', [
+    ('http://foo/bar', 'windows', 'bar'),
+    ('A *|<>\\: bear!', 'windows', 'A ______ bear!'),
+    ('A *|<>\\: bear!', 'posix', 'A *|<>\\: bear!')
+])
+def test_sanitized_filenames(raw, platform, expected, config_stub,
+                             download_tmpdir, monkeypatch):
+    is_platform(monkeypatch, platform)
+    manager = downloads.AbstractDownloadManager()
+    target = downloads.FileDownloadTarget(str(download_tmpdir))
+    item = downloads.AbstractDownloadItem()
+
+    # Don't try to start a timer outside of a QThread
+    manager._update_timer.isActive = lambda: True
+
+    # Abstract methods
+    item._ensure_can_set_filename = lambda *args: True
+    item._after_set_filename = lambda *args: True
+
+    manager._init_item(item, True, raw)
+    item.set_target(target)
+    assert item._filename.endswith(expected)

tests/unit/utils/test_utils.py

@@ -113,6 +113,14 @@ class TestElidingFilenames:
         assert utils.elide_filename(filename, length) == expected
 
 
+@pytest.fixture(autouse=True)
+def is_platform(monkeypatch, platform="windows"):
+    for p in ["mac", "linux", "posix", "windows"]:
+        monkeypatch.setattr(
+            'qutebrowser.utils.utils.is_{}'.format(p),
+            p == platform)
+
+
 @pytest.fixture(params=[True, False])
 def freezer(request, monkeypatch):
     if request.param and not getattr(sys, 'frozen', False):
@@ -633,12 +641,15 @@ def test_force_encoding(inp, enc, expected):
     assert utils.force_encoding(inp, enc) == expected
 
 
-@pytest.mark.parametrize('inp, expected', [
-    ('normal.txt', 'normal.txt'),
-    ('user/repo issues.mht', 'user_repo issues.mht'),
-    ('<Test\\File> - "*?:|', '_Test_File_ - _____'),
+@pytest.mark.parametrize('inp, platform, expected', [
+    ('normal.txt', 'windows', 'normal.txt'),
+    ('user/repo issues.mht', 'windows', 'user_repo issues.mht'),
+    ('<Test\\File> - "*?:|', 'windows', '_Test_File_ - _____'),
+    ('<Test\\File> - "*?:|', 'mac', '<Test\\File> - "*?_|'),
+    ('<Test\\File> - "*?:|', 'posix', '<Test\\File> - "*?:|'),
 ])
-def test_sanitize_filename(inp, expected):
+def test_sanitize_filename(inp, platform, expected, monkeypatch):
+    is_platform(monkeypatch, platform)
     assert utils.sanitize_filename(inp) == expected