Relying on `hasattr()` made me feel a bit guilty. So I've moved these
conditionals to be backed by a class. The only class based alternative I
can think of is putting it on the base type and leaving all the other
config variables to raise errors. But that doesn't tell the type system
anything.
All the promptable feature permissions so far have been of type BoolAsk.
The prompt uses a "yesno" mode prompt and only results in a bool. The
persistence logic only supports bools.
Previously I made the shared prompt support the String type clipboard
permission setting by treating non "ask" values as False (you only get
prompted if the global setting is "none" anyway), but saving the prompt
results with `:prompt-accept --save` didn't work because the persistence
code only supported bools.
What we want to do when saving is convert `False` to "none" and `True`
to "access-paste". This mirrors the new webengine logic. It does mean we
can't let users choose to persist either none/access/access-paste, but
webengine doesn't prompt us if the page is already allowed "access" but
is trying to paste anyway. If it did we would have to use a non-yesno
prompt for this (perhaps it could be driven directly by the ConfigType).
For now I've added a new concept to the ConfigTypes to allow them to be
casted to and from bools, so that we can plumb this String type from the
boolean yesno prompt.
TODO:
* try to make it an interface so we don't have to use `hasattr` (even if
it means multiple inheritance)
* add test coverage to test_configtypes.py
The test page is using a JS API that is too new for qtwebkit:
23:07:54.898 DEBUG js shared:javascript_log_message:190 [http://localhost:37635/data/prompt/clipboard.html:97] TypeError: undefined is not an object (evaluating 'navigator.clipboard.readText')
This is failing with:
ERROR tests/end2end/test_invocations.py::test_permission_prompt_across_restart - PermissionError: [WinError 5] Access is denied: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpytep6gj0\\cache\\webengine\\Cache\\Cache_Data\\data_0'
In pytest teardown, while trying to clean up a temp dir, probably the
basedir. The test above waits for shutdown at the end of the test, maybe
that's what's needed here.
Otherwise maybe just `@pytest.mark.skipif(utils.is_windows)` 🤷
For tests that do the same prompt in the same session, they are failing
on Qt6.8 and PyQWebEnginet6.7 because we can't disable the new WebEngine
behaviour.
We can work around this by getting a fresh instance for each test, but I
don't want to make more tests slower if I don't have to. So move that
logic into a custom "fresh instance" prompt that will only create one
when needed.
WebEngine now supports prompting for permission when a web page tries to
access the clipboard. Previously we only supported fixed permissions that
applied globally.
This commit
1. adds support for permission requests with the new ClipboardReadWrite
permission
2. tweaks the generic JS prompt handler in browser/shared.py to handle
seeing a setting which isn't of type BoolAsk
3. adds end2end tests around clipboard permissions, both the existing
global ones and the new per-URL ones
1. the ClipboardReadWrite permission
I added this as an int because the relevant PyQt isn't out yet and users
with 6.8 running with PyQt6.7 are already seeing this.
I added an "ask" value to the existing String type
`content.javascript.clipboard` setting and set the
default/global/fallback permissions to False if ask is set globally.
Hmm, maybe we should change the default actually... I'll have to check
what the other prompt supporting settings default to.
2. tweaked prompt handler
This was treating the string values that weren't "ask" (like "none" and
"access") as truthy and allowing the action. I've changed the bool
checks to be exact checks to add a warning if this happens again.
Then I added an exception to the warning logging for known cases like
this. I did try looking at adding a new setting type. Something that was
descended from String but had an `__eq__` method that understood bools
and would treat `access-paste` as True and everything else as False. But
that didn't work out because it looks like config values are stored as
python values and the config classes are just static and don't actually
hold values. Oh well, maybe a better pattern will emerge with time.
3. tests
Apparently there were no tests around the clipboard settings. Perhaps
not a coincidence given how confusing they are (what does access-paste
mean?).
I copied a test file from some random test site, tweaked it a little bit
and got to work. For the paste test it's a bit awkward because I don't
know if we have a way to fill the clipboard in a nice way for the tests.
So I'm just matching on "Text pasted: *", which is usually an empty
string.
The prompt tests require running against Qt6.8 to get the new prompt
behaviour (don't need pyqt68 though).
There are some changes in this area in Qt6.8, so it would be good to
have some test coverage.
The "access permission - copy" one is broken in 6.8. Still need to raise
that upstream.
QtWebEngine has a new feature where it will remember what permissions
you have granted or denied. It has three options regarding permissions
storage:
AskEveryTime -- don't store
StoreInMemory -- store in memory only
StoreOnDisk -- store in memory and on disk
By default it does the StoreOnDisk behavior. Having webengine remember
whether you granted or denied a permission would make the qutebrowser UX
around that area inconsistent. For example the default y/n actions for a
permission prompt in qutebrowser will only accept that single
permission request, and you'll be re-prompted if you reload the page.
Users may be used to this and if webengine started remembering
permission grants the users may be surprised to find that the page was
accessing features without prompting them for permission anymore.
Additionally we already have our own permission storage machinery in
autoconfig.yml.
This commit will set the webengine feature to AskEveryTime, which disables
any storing of permission grants.
Also adjusts the skip marker of the affected tests so they'll be enabled
again on Qt versions with the appropriate permissions API.
This re-enables the pylint too-many-positional-arguments for the main
application code. It's still disabled for tests because that's how you pull in
pytlint fixtures, and I don't think we want to push people into being creative
with fixtures just to get around that.
When functions are called with many positional arguments the reader has to do
a bit of heavy lifting to figure out in what position a value is being passed,
and it's easier to make mistakes. So I would like to encourage using keyword
arguments for long argument lists.
I've set the `max-positional-arguments` to a completely arbitrary 7, from a
completely arbitrary 5, because there were many more violations under 7. If
like 99% of our functions fit under 7 it's probably fine.
Regarding the exceptions:
* objreg.register: I grepped it and it looks like everything is only passing
the first two args as positional already, lucky!
* `_get_color_percentage`: only one usage of it, but I was in "add directive
comment" mode
* update_3rdparty.py: only one usage, already using kwargs
* pyqtProperty: idk
* commands.py: "its complicated". Many methods in this file map to commands
used in qutebrowser's command mode. In that case it's usual for them to be
called as flags, rather than positional. But it could be complicated to wade
into that, and having one file excluded isn't so bad.
With the upgrade to MarkupSafe 3.0, something funny happened when trying to pass
the GUIProcess object to jinja after launching a userscript:
[...]
File "[...]/qutebrowser/browser/qutescheme.py", line 291, in qute_process
src = jinja.render('process.html', title=f'Process {pid}', proc=proc)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "[...]/qutebrowser/utils/jinja.py", line 123, in render
return environment.get_template(template).render(**kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[...]
File "html/process.html", line 11, in block 'content'
File "[...]/lib/python3.11/site-packages/markupsafe/__init__.py", line 42, in escape
if hasattr(s, "__html__"):
^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: wrapped C/C++ object of type GUIProcess has been deleted
This can be reproduced with:
qutebrowser --temp-basedir ':cmd-later 0 spawn -u -o /bin/echo test'
We pass the `GUIProcess` to the Jinja template as `proc`, which then formats it as
`{{ proc }}`` (to stringify it). For some reason, with the newest MarkupSafe/Jinja
versions, this now triggers the `if hasattr(s, "__html__")` check in MarkupSafe
(which has been around for a while). That then presumably causes PyQt to try and
access the underlying C++ object for `GUIProcess``, but that has already been
deleted.
But why is it deleted in the first place, if we keep track of even completed
processes data ever since we added `:process` in a3adba81c? It looks like the Qt
parent-child relationship is the culprit here: When we pass a parent to the
`GUIProcess`` from the userscript runner, it will get deleted as soon as said
runner is cleaned up (which happens after the userscript has finished).
We probably never noticed this before because we only accessed data from the
Python wrapper and not from the C++ side, but it still seems like a good idea
to avoid passing a parent for a long-lived object (with time-based cleanup) in
the first place.
Added in 3.3.0:
https://pylint.pycqa.org/en/latest/whatsnew/3/3.3/index.html
Some of those arguments could probably indeed be keyword-only,
but for some of the functions shown by pylint, those are qutebrowser command
handlers where a positional argument has different semantics.
Did run with ruff pretending to use Python 3.10,
because otherwise it won't reformat those:
ruff check --select 'UP035' --fix --config 'target-version = "py310"' --unsafe-fixes
This is because collections.abc.Callable inside Optional[...] and Union[...] is
broken with Python 3.9.0 and 3.9.1:
https://github.com/asottile/pyupgrade/issues/677https://github.com/astral-sh/ruff/issues/2690https://github.com/python/cpython/issues/87131
However, pylint can detect problematic usages (of which we only have one),
so we might as well use the new thing everywhere possible for consistency.
Also see #7098
toofar
qutebrowser bot
Florian Bruhin
Marcel Schilling
Willow Barraco