From c0be28ebee3e1837aaf3f30ec534ccd6d038f129 Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 23 Sep 2023 14:05:36 +1200 Subject: Add extra suffixes to filepicker Works around a Qt bug where QFileDialog might not see all the filename extensions you have registered for a mimetype. There is an issue where if you have suffixes (filename extensions) registered in your home dir and in a system dir, it'll only see the ones in your home dir. So get suffixes from python and proactively add them into the list the list provided by the website. Duplicating work that Qt would already do, if it weren't for the bug case we are working around. Note that this does NOT help with mimetypes like `image/*` which, we have seen in at least one example (on #7866). If we wanted to handle that case it it looks like we could get the list from: [suffix for suffix, mime in mimetypes.types_map.items() if mime.startswith('image/')] I added two tests. The first is a simple one but it felt a bit imcomplete without actually testing that we were passing the right thing to `super().chooseFiles()`. Not sure how "bad" it is to be mocking `super()` and what should be instance methods unbound from the class. Regarding the `qtutils.version_check()`, here we are only interested in the Qt version check, but that class also checks the PyQt version. The practical effect of this is that we would probably end up doing the workaround for Qt 6.7.0 if we run against that with PyQt 6.6.x. But since we are just adding stuff, it isn't slow and isn't called in a hot path it doesn't really matter. --- qutebrowser/browser/webengine/webview.py | 32 ++++++++++++- tests/unit/browser/webengine/test_webview.py | 68 ++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/qutebrowser/browser/webengine/webview.py b/qutebrowser/browser/webengine/webview.py index f3f652ad0..4fb48c060 100644 --- a/qutebrowser/browser/webengine/webview.py +++ b/qutebrowser/browser/webengine/webview.py @@ -4,6 +4,7 @@ """The main browser widget for QtWebEngine.""" +import mimetypes from typing import List, Iterable from qutebrowser.qt import machinery @@ -15,7 +16,7 @@ from qutebrowser.qt.webenginecore import QWebEnginePage, QWebEngineCertificateEr from qutebrowser.browser import shared from qutebrowser.browser.webengine import webenginesettings, certificateerror from qutebrowser.config import config -from qutebrowser.utils import log, debug, usertypes +from qutebrowser.utils import log, debug, usertypes, qtutils _QB_FILESELECTION_MODES = { @@ -258,6 +259,26 @@ class WebEnginePage(QWebEnginePage): self.navigation_request.emit(navigation) return navigation.accepted + @staticmethod + def extra_suffixes_workaround(upstream_mimetypes): + """Return any extra suffixes for mimetypes in upstream_mimetypes. + + Return any file extensions (aka suffixes) for mimetypes listed in + upstream_mimetypes that are not already contained in there. + + WORKAROUND: for https://bugreports.qt.io/browse/QTBUG-116905 + Affected Qt versions > 6.2.2 (probably) < 6.7.0 + """ + if not (qtutils.version_check("6.2.3") and not qtutils.version_check("6.7.0")): + return set() + + suffixes = {entry for entry in upstream_mimetypes if entry.startswith(".")} + mimes = {entry for entry in upstream_mimetypes if "/" in entry} + python_suffixes = set() + for mime in mimes: + python_suffixes.update(mimetypes.guess_all_extensions(mime)) + return python_suffixes - suffixes + def chooseFiles( self, mode: QWebEnginePage.FileSelectionMode, @@ -265,6 +286,15 @@ class WebEnginePage(QWebEnginePage): accepted_mimetypes: Iterable[str], ) -> List[str]: """Override chooseFiles to (optionally) invoke custom file uploader.""" + extra_suffixes = self.extra_suffixes_workaround(accepted_mimetypes) + if extra_suffixes: + log.webview.debug( + "adding extra suffixes to filepicker: before=%s added=%s", + accepted_mimetypes, + extra_suffixes, + ) + accepted_mimetypes = accepted_mimetypes + list(extra_suffixes) + handler = config.val.fileselect.handler if handler == "default": return super().chooseFiles(mode, old_files, accepted_mimetypes) diff --git a/tests/unit/browser/webengine/test_webview.py b/tests/unit/browser/webengine/test_webview.py index 98bf34f3b..b213713e2 100644 --- a/tests/unit/browser/webengine/test_webview.py +++ b/tests/unit/browser/webengine/test_webview.py @@ -4,11 +4,14 @@ import re import dataclasses +import mimetypes import pytest +from unittest import mock webview = pytest.importorskip('qutebrowser.browser.webengine.webview') from qutebrowser.qt.webenginecore import QWebEnginePage +from qutebrowser.utils import qtutils from helpers import testutils @@ -58,3 +61,68 @@ def test_enum_mappings(enum_type, naming, mapping): for name, val in members: mapped = mapping[val] assert camel_to_snake(naming, name) == mapped.name + + +@pytest.fixture +def suffix_mocks(monkeypatch): + def guess(mime): + mimetypes_map = { + "image/jpeg": [".jpg", ".jpe"], + "video/mp4": [".m4v", ".mpg4"], + } + return mimetypes_map.get(mime, []) + + monkeypatch.setattr(mimetypes, "guess_all_extensions", guess) + + def version(string): + if string == "6.2.3": + return True + if string == "6.7.0": + return False + raise AssertionError(f"unexpected version {string}") + + monkeypatch.setattr(qtutils, "version_check", version) + + +EXTRA_SUFFIXES_PARAMS = [ + (["image/jpeg"], {".jpg", ".jpe"}), + (["image/jpeg", ".jpeg"], {".jpg", ".jpe"}), + (["image/jpeg", ".jpg", ".jpe"], set()), + ( + [ + ".jpg", + ], + set(), + ), # not sure why black reformats this one and not the others + (["image/jpeg", "video/mp4"], {".jpg", ".jpe", ".m4v", ".mpg4"}), +] + + +@pytest.mark.parametrize("before, extra", EXTRA_SUFFIXES_PARAMS) +def test_suffixes_workaround_extras_returned(suffix_mocks, before, extra): + assert extra == webview.WebEnginePage.extra_suffixes_workaround(before) + + +@pytest.mark.parametrize("before, extra", EXTRA_SUFFIXES_PARAMS) +@mock.patch("qutebrowser.browser.webengine.webview.super") # mock super() calls! +def test_suffixes_workaround_choosefiles_args( + mocked_super, + suffix_mocks, + config_stub, + before, + extra, +): + # We can pass the class as "self" because we are only calling a static + # method of it. That saves us having to initilize the class and mock all + # the stuff required for __init__() + webview.WebEnginePage.chooseFiles( + webview.WebEnginePage, + QWebEnginePage.FileSelectionMode.FileSelectOpen, + [], + before, + ) + expected = set(before).union(extra) + + assert len(mocked_super().chooseFiles.call_args_list) == 1 + called_with = mocked_super().chooseFiles.call_args_list[0][0][2] + assert sorted(called_with) == sorted(expected) -- cgit v1.2.3-54-g00ecf From 5345d534189175a986ae287b3d4a49ecebf7103a Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 23 Sep 2023 14:40:22 +1200 Subject: Support wildcard mimes in filepicker workaround too We've seen it in the wild, so it could happen, and it wasn't hard to add in the end. ref: #7866 --- qutebrowser/browser/webengine/webview.py | 11 ++++++++++- tests/unit/browser/webengine/test_webview.py | 18 ++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/qutebrowser/browser/webengine/webview.py b/qutebrowser/browser/webengine/webview.py index 4fb48c060..61f64724d 100644 --- a/qutebrowser/browser/webengine/webview.py +++ b/qutebrowser/browser/webengine/webview.py @@ -276,7 +276,16 @@ class WebEnginePage(QWebEnginePage): mimes = {entry for entry in upstream_mimetypes if "/" in entry} python_suffixes = set() for mime in mimes: - python_suffixes.update(mimetypes.guess_all_extensions(mime)) + if mime.endswith("/*"): + python_suffixes.update( + [ + suffix + for suffix, mimetype in mimetypes.types_map.items() + if mimetype.startswith(mime[:-1]) + ] + ) + else: + python_suffixes.update(mimetypes.guess_all_extensions(mime)) return python_suffixes - suffixes def chooseFiles( diff --git a/tests/unit/browser/webengine/test_webview.py b/tests/unit/browser/webengine/test_webview.py index b213713e2..2789dcc9f 100644 --- a/tests/unit/browser/webengine/test_webview.py +++ b/tests/unit/browser/webengine/test_webview.py @@ -65,14 +65,22 @@ def test_enum_mappings(enum_type, naming, mapping): @pytest.fixture def suffix_mocks(monkeypatch): + types_map = { + ".jpg": "image/jpeg", + ".jpe": "image/jpeg", + ".png": "image/png", + ".m4v": "video/mp4", + ".mpg4": "video/mp4", + } + mimetypes_map = {} # mimetype -> [suffixes] map + for suffix, mime in types_map.items(): + mimetypes_map[mime] = mimetypes_map.get(mime, []) + [suffix] + def guess(mime): - mimetypes_map = { - "image/jpeg": [".jpg", ".jpe"], - "video/mp4": [".m4v", ".mpg4"], - } return mimetypes_map.get(mime, []) monkeypatch.setattr(mimetypes, "guess_all_extensions", guess) + monkeypatch.setattr(mimetypes, "types_map", types_map) def version(string): if string == "6.2.3": @@ -95,6 +103,8 @@ EXTRA_SUFFIXES_PARAMS = [ set(), ), # not sure why black reformats this one and not the others (["image/jpeg", "video/mp4"], {".jpg", ".jpe", ".m4v", ".mpg4"}), + (["image/*"], {".jpg", ".jpe", ".png"}), + (["image/*", ".jpg"], {".jpe", ".png"}), ] -- cgit v1.2.3-54-g00ecf From fc470a69ed57a2fb0b59c8da5734f0c6400f2094 Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 23 Sep 2023 15:15:56 +1200 Subject: update changelog --- doc/changelog.asciidoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/doc/changelog.asciidoc b/doc/changelog.asciidoc index 557e5cfbb..6fe4d4977 100644 --- a/doc/changelog.asciidoc +++ b/doc/changelog.asciidoc @@ -54,6 +54,8 @@ Fixed - Graphical glitches in Google sheets and PDF.js via a new setting `qt.workarounds.disable_accelerated_2d_canvas` to disable the accelerated 2D canvas feature which defaults to enabled on affected Qt versions. (#7489) +- Workaround a Qt issue causing jpeg files to not show up in the upload file + picker when it was filtering for image filetypes (#7866) [[v3.0.0]] v3.0.0 (2023-08-18) -- cgit v1.2.3-54-g00ecf From a67832ba311fdb0e9d57190d1671241a369b5b0a Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 23 Sep 2023 15:25:25 +1200 Subject: Turn into list before adding for mypy The right hand side is turned into a list because `list + set` isn't supported by python for some reason (why? Just make it a list). The left hand side is turned into a list because Iterable doesn't support + (why?). Possible I could change it to Sequence, but meh. --- qutebrowser/browser/webengine/webview.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/browser/webengine/webview.py b/qutebrowser/browser/webengine/webview.py index 61f64724d..a55fb5c9c 100644 --- a/qutebrowser/browser/webengine/webview.py +++ b/qutebrowser/browser/webengine/webview.py @@ -302,7 +302,7 @@ class WebEnginePage(QWebEnginePage): accepted_mimetypes, extra_suffixes, ) - accepted_mimetypes = accepted_mimetypes + list(extra_suffixes) + accepted_mimetypes = list(accepted_mimetypes) + list(extra_suffixes) handler = config.val.fileselect.handler if handler == "default": -- cgit v1.2.3-54-g00ecf From 7b603dd6bf195e3e723ce08ff64a82b406e3f6b6 Mon Sep 17 00:00:00 2001 From: toofar Date: Mon, 25 Sep 2023 19:23:32 +1300 Subject: Move method to module level. This avoids having to mess about with static methods. And makes the test a bit clearer as we aren't passing a class in place of an instance anymore. Hopefully I put it in the right place. It's above the class where it is used. Should it be at the top of the file? Bottom of the file? IDK --- qutebrowser/browser/webengine/webview.py | 60 ++++++++++++++-------------- tests/unit/browser/webengine/test_webview.py | 4 +- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/qutebrowser/browser/webengine/webview.py b/qutebrowser/browser/webengine/webview.py index a55fb5c9c..1ef8ca9bd 100644 --- a/qutebrowser/browser/webengine/webview.py +++ b/qutebrowser/browser/webengine/webview.py @@ -130,6 +130,35 @@ class WebEngineView(QWebEngineView): super().contextMenuEvent(ev) +def extra_suffixes_workaround(upstream_mimetypes): + """Return any extra suffixes for mimetypes in upstream_mimetypes. + + Return any file extensions (aka suffixes) for mimetypes listed in + upstream_mimetypes that are not already contained in there. + + WORKAROUND: for https://bugreports.qt.io/browse/QTBUG-116905 + Affected Qt versions > 6.2.2 (probably) < 6.7.0 + """ + if not (qtutils.version_check("6.2.3") and not qtutils.version_check("6.7.0")): + return set() + + suffixes = {entry for entry in upstream_mimetypes if entry.startswith(".")} + mimes = {entry for entry in upstream_mimetypes if "/" in entry} + python_suffixes = set() + for mime in mimes: + if mime.endswith("/*"): + python_suffixes.update( + [ + suffix + for suffix, mimetype in mimetypes.types_map.items() + if mimetype.startswith(mime[:-1]) + ] + ) + else: + python_suffixes.update(mimetypes.guess_all_extensions(mime)) + return python_suffixes - suffixes + + class WebEnginePage(QWebEnginePage): """Custom QWebEnginePage subclass with qutebrowser-specific features. @@ -259,35 +288,6 @@ class WebEnginePage(QWebEnginePage): self.navigation_request.emit(navigation) return navigation.accepted - @staticmethod - def extra_suffixes_workaround(upstream_mimetypes): - """Return any extra suffixes for mimetypes in upstream_mimetypes. - - Return any file extensions (aka suffixes) for mimetypes listed in - upstream_mimetypes that are not already contained in there. - - WORKAROUND: for https://bugreports.qt.io/browse/QTBUG-116905 - Affected Qt versions > 6.2.2 (probably) < 6.7.0 - """ - if not (qtutils.version_check("6.2.3") and not qtutils.version_check("6.7.0")): - return set() - - suffixes = {entry for entry in upstream_mimetypes if entry.startswith(".")} - mimes = {entry for entry in upstream_mimetypes if "/" in entry} - python_suffixes = set() - for mime in mimes: - if mime.endswith("/*"): - python_suffixes.update( - [ - suffix - for suffix, mimetype in mimetypes.types_map.items() - if mimetype.startswith(mime[:-1]) - ] - ) - else: - python_suffixes.update(mimetypes.guess_all_extensions(mime)) - return python_suffixes - suffixes - def chooseFiles( self, mode: QWebEnginePage.FileSelectionMode, @@ -295,7 +295,7 @@ class WebEnginePage(QWebEnginePage): accepted_mimetypes: Iterable[str], ) -> List[str]: """Override chooseFiles to (optionally) invoke custom file uploader.""" - extra_suffixes = self.extra_suffixes_workaround(accepted_mimetypes) + extra_suffixes = extra_suffixes_workaround(accepted_mimetypes) if extra_suffixes: log.webview.debug( "adding extra suffixes to filepicker: before=%s added=%s", diff --git a/tests/unit/browser/webengine/test_webview.py b/tests/unit/browser/webengine/test_webview.py index 2789dcc9f..0bc5a8706 100644 --- a/tests/unit/browser/webengine/test_webview.py +++ b/tests/unit/browser/webengine/test_webview.py @@ -110,7 +110,7 @@ EXTRA_SUFFIXES_PARAMS = [ @pytest.mark.parametrize("before, extra", EXTRA_SUFFIXES_PARAMS) def test_suffixes_workaround_extras_returned(suffix_mocks, before, extra): - assert extra == webview.WebEnginePage.extra_suffixes_workaround(before) + assert extra == webview.extra_suffixes_workaround(before) @pytest.mark.parametrize("before, extra", EXTRA_SUFFIXES_PARAMS) @@ -126,7 +126,7 @@ def test_suffixes_workaround_choosefiles_args( # method of it. That saves us having to initilize the class and mock all # the stuff required for __init__() webview.WebEnginePage.chooseFiles( - webview.WebEnginePage, + None, QWebEnginePage.FileSelectionMode.FileSelectOpen, [], before, -- cgit v1.2.3-54-g00ecf From 65bfefe9260ca5a31ef8cae970aaddbf11d6509c Mon Sep 17 00:00:00 2001 From: toofar Date: Mon, 25 Sep 2023 19:25:39 +1300 Subject: Use mocker fixture instead of mock.patch I forgot about that fixture. --- tests/unit/browser/webengine/test_webview.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/unit/browser/webengine/test_webview.py b/tests/unit/browser/webengine/test_webview.py index 0bc5a8706..7eeec3158 100644 --- a/tests/unit/browser/webengine/test_webview.py +++ b/tests/unit/browser/webengine/test_webview.py @@ -7,7 +7,6 @@ import dataclasses import mimetypes import pytest -from unittest import mock webview = pytest.importorskip('qutebrowser.browser.webengine.webview') from qutebrowser.qt.webenginecore import QWebEnginePage @@ -114,17 +113,20 @@ def test_suffixes_workaround_extras_returned(suffix_mocks, before, extra): @pytest.mark.parametrize("before, extra", EXTRA_SUFFIXES_PARAMS) -@mock.patch("qutebrowser.browser.webengine.webview.super") # mock super() calls! def test_suffixes_workaround_choosefiles_args( - mocked_super, + mocker, suffix_mocks, config_stub, before, extra, ): - # We can pass the class as "self" because we are only calling a static - # method of it. That saves us having to initilize the class and mock all - # the stuff required for __init__() + # mock super() to avoid calling into the base class' chooseFiles() + # implementation. + mocked_super = mocker.patch("qutebrowser.browser.webengine.webview.super") + + # We can pass None as "self" because we aren't actually using anything from + # "self" for this test. That saves us having to initialize the class and + # mock all the stuff required for __init__() webview.WebEnginePage.chooseFiles( None, QWebEnginePage.FileSelectionMode.FileSelectOpen, -- cgit v1.2.3-54-g00ecf From 54c0c493b3e560f478c3898627c582cab11fbc2b Mon Sep 17 00:00:00 2001 From: toofar Date: Wed, 27 Sep 2023 08:12:11 +1300 Subject: Change log message to use f-strings TODO: configure the linter to tell people to do this. As an aside, I'm not sure I agree that this is a good idea. It seems the f-string interpolation does have a cost when you are not logging at that level, and we do support disabling the ram logger. But we can change it with automation anyway. --- qutebrowser/browser/webengine/webview.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/webengine/webview.py b/qutebrowser/browser/webengine/webview.py index 1ef8ca9bd..a04d2b813 100644 --- a/qutebrowser/browser/webengine/webview.py +++ b/qutebrowser/browser/webengine/webview.py @@ -298,9 +298,9 @@ class WebEnginePage(QWebEnginePage): extra_suffixes = extra_suffixes_workaround(accepted_mimetypes) if extra_suffixes: log.webview.debug( - "adding extra suffixes to filepicker: before=%s added=%s", - accepted_mimetypes, - extra_suffixes, + "adding extra suffixes to filepicker: " + f"before={accepted_mimetypes} " + f"added={extra_suffixes}", ) accepted_mimetypes = list(accepted_mimetypes) + list(extra_suffixes) -- cgit v1.2.3-54-g00ecf From fea33d607fde83cf505b228238cf365936437a63 Mon Sep 17 00:00:00 2001 From: toofar Date: Fri, 29 Sep 2023 10:06:19 +1300 Subject: Check runtime Qt version only. Since the bug was fixed in qtbase it shouldn't matter what version of PyQt (or webengine for that matter) you are running against. So pass `compiled=False`. Also expand the docstring of `check_version()` to explain a little more what the `compiled` kwarg does. Based on a discussion here: https://github.com/qutebrowser/qutebrowser/pull/7933#issuecomment-1732400960 What's still not clear to me is when the runtime and compiled Qt versions can differ. I always have to rebuild PyQt when I switch Qt versions, but maybe I'm doing something wrong there. --- qutebrowser/browser/webengine/webview.py | 5 ++++- qutebrowser/utils/qtutils.py | 17 ++++++++++++++++- tests/unit/browser/webengine/test_webview.py | 3 ++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/webengine/webview.py b/qutebrowser/browser/webengine/webview.py index a04d2b813..3c63c59e4 100644 --- a/qutebrowser/browser/webengine/webview.py +++ b/qutebrowser/browser/webengine/webview.py @@ -139,7 +139,10 @@ def extra_suffixes_workaround(upstream_mimetypes): WORKAROUND: for https://bugreports.qt.io/browse/QTBUG-116905 Affected Qt versions > 6.2.2 (probably) < 6.7.0 """ - if not (qtutils.version_check("6.2.3") and not qtutils.version_check("6.7.0")): + if not ( + qtutils.version_check("6.2.3", compiled=False) + and not qtutils.version_check("6.7.0", compiled=False) + ): return set() suffixes = {entry for entry in upstream_mimetypes if entry.startswith(".")} diff --git a/qutebrowser/utils/qtutils.py b/qutebrowser/utils/qtutils.py index 5e36a90d2..1f5da2dcd 100644 --- a/qutebrowser/utils/qtutils.py +++ b/qutebrowser/utils/qtutils.py @@ -80,10 +80,25 @@ def version_check(version: str, compiled: bool = True) -> bool: """Check if the Qt runtime version is the version supplied or newer. + By default this function will check `version` against: + + 1. the runtime Qt version (from qVersion()) + 2. the Qt version that PyQt was compiled against (from QT_VERSION_STR) + 3. the PyQt version (from PYQT_VERSION_STR) + + With `compiled=False` only the runtime Qt version (1) is checked. + + You can often run older PyQt versions against newer Qt versions, but you + won't be able to access any APIs that where only added in the newer Qt + version. So if you want to check if a new feature if supported, use the + default behavior. If you just want to check the underlying Qt version, + pass `compiled=False`. + Args: version: The version to check against. exact: if given, check with == instead of >= - compiled: Set to False to not check the compiled version. + compiled: Set to False to not check the compiled Qt version or the + PyQt version. """ if compiled and exact: raise ValueError("Can't use compiled=True with exact=True!") diff --git a/tests/unit/browser/webengine/test_webview.py b/tests/unit/browser/webengine/test_webview.py index 7eeec3158..f14a896b6 100644 --- a/tests/unit/browser/webengine/test_webview.py +++ b/tests/unit/browser/webengine/test_webview.py @@ -81,7 +81,8 @@ def suffix_mocks(monkeypatch): monkeypatch.setattr(mimetypes, "guess_all_extensions", guess) monkeypatch.setattr(mimetypes, "types_map", types_map) - def version(string): + def version(string, compiled=True): + assert compiled is False if string == "6.2.3": return True if string == "6.7.0": -- cgit v1.2.3-54-g00ecf