The previous implementation of the lifecycle timer had one timer that we
would change based on what state were were heading into. Because we
connected a new function to it every time we had to disconnect previous
connections before using it again which was a bit awkward, it raised a
TypeError if there were no connections, which isn't very specific.
This commit is an attempt to switch to using two timers, each with their
own callback.
I took a stateless/idempotent approach where the logic doesn't know what
state it might have been in previously so needs to stop any timers that
might be set each time, and needs to check if the timer it wants to set
is already active. Potentially the line count could be reduced by
encoding a bit more state in the logic. But hopefully it's simpler to
maintain like this.
Since I had need to (potentially) stop the timers at several points in
the logic, I pulled that out to a helper function which is where most of
the complexity is now. I tried to make the helper function a little more
dynamic by using a dict lookup to get the new timer and a loop to
reactive the old one. This didn't really add much, I think it's a
slightly higher line count, and probably a little harder to parse. The
previous implementation just looked a bit repetitive and hard to scan
over:
to_start = delay = None
if state == QWebEnginePage.LifecycleState.Frozen:
to_start = self._lifecycle_timer_freeze
self._lifecycle_timer_discard.stop()
delay = config.val.qt.chromium.lifecycle_state_freeze_delay
elif state == QWebEnginePage.LifecycleState.Discarded:
self._lifecycle_timer_freeze.stop()
to_start = self._lifecycle_timer_discard
delay = config.val.qt.chromium.lifecycle_state_discard_delay
elif state == None:
self._lifecycle_timer_freeze.stop()
self._lifecycle_timer_discard.stop()
else:
raise utils.Unreachable(recommended_state)
if to_start and not to_start.isActive() and delay != -1:
log.webview.debug(f"Scheduling recommended lifecycle change {delay=} {state=} tab={self}")
to_start.start(delay)
I re-worked the logic of the `_on_recommended_state_changed()` changed
function a little. Not moving into the `Active` state is done immediately
instead of via a 0ms timer. So it's more like an guard clause and early
exit. I think it flows a little better now with all the decisions layed
out clearly and acted on one at a time instead of having the disabled
check right at the start but still proceeding through the function in
one case.
The meaning of the terms mock/stub/spy can differ accross languages,
projects and frameworks. Python doesn't have a super consistent
terminology but often a "spy" won't prevent a real object being called
by default, but will let you verify that a method was called. In this
case I'm preventing the `setLifecycleState()` method from being called
at all. So calling it a spy is a bit misleading. The reason I'm
preventing it being called is because I feel like calling into the real
browser code isn't essential for these tests and could complicated the
ideally simple unit tests
This commit renames it to be called a mock, which is hopefully more
clear.
These xunit docs has a decent overview of the various different terms
like mock/stub/spy: http://xunitpatterns.com/Test%20Double.html
It doesn't make sense to allow setting these to negative values, so lets
help the user out by refusing to do so if they try.
But leave `-1` as an allowed value for the "discard" option so users can
have "freeze" enabled but turn the discard state off.
Remove the mention of -1 from the freeze setting since we don't allow
that anymore.
There is some weirdness preventing us from connecting to the
`QWebEnginePage.recommendedStateChanged` on WebEngine 6.4 and below. We
get this message from QT:
qt.core.qobject.connect: QObject::connect: No such signal WebEnginePage::recommendedStateChanged(QWebEnginePage::LifecycleState)
and this message from PyQt:
TypeError: connect() failed between recommendedStateChanged(QWebEnginePage::LifecycleState) and _on_recommended_state_changed()
We couldn't get to the root of it, but we shouldn't have a pressing need
to support this on older versions anyway since later Qt versions are
already widespread.
I want to make sure tabs in the background that might show notifications
or update their favicons or whatever can keep running if I want them to,
and not get frozen. Previous iterations of this feature have re-used the
pinned state to demark tabs that shouldn't be frozen. So that's what
I've implemented here. What could go wrong?
If kept the same "do act on the state change if we are transitioning to
an active state" check from the top of the function for consistency, but
neither check is needed. Qt will put tabs into the Active state when
they are shown without anything needed from our code.
I also haven't implemented a lifecycle change when you un-pin a tab.
It'll get picked up when you switch away from the tab.
This is inspired by previous iterations of this feature. Discarding a
page can lose information on the page, for example, if you go to
duckduckgo.com and put some text in the search box, then switch to a
different tab and wait for the ddg tab to be discarded, when you switch
back to the ddg tab the search box will be empty.
Until a point where we have more sophisticated controls of what sites
these transitions apply to, it may be useful for users to disable the
discard state completely. I know that's how I've had the lazier loading
implementation set up for years.
Negative numbers would previously have raised a warning on every state
transition where the time complained that negative numbers weren't
supported.
I was looking at the logs to verify this behavior and I thought this
message could be a little clearer. It didn't mention the tab being acted
on (important when tying together logs across signal firings and with
multiple tabs open) and I switched it to be in logfmt instead of a
whole sentence for ease of parsing.
I also added a `if current == new: return` check, again because of the
logs. Having log messages that won't have any effect probably aren't
needed, and I was confusing them with the important logs.
Unfortunately exciting early means we don't connect new signals, so next
time we come through there is nothing to disconnect. Which makes the
`disconnect()` call grumpy. TypeError is not a super meaningful error to
be throwing here, but I assume that's a result of the python and c++
integration. If the signal was connected to a slot we could
unambiguously disconnect just that one slot, but it's anonymous
currently so we'll see how this goes.
I tried to add most of them as "unit" tests (but with a real tab). For
testing that the lifecycle state actually changes though we need an
integration tests, but the tab won't change state if it's focused, and
the unit tests don't have a tab widget.
libwayland-client.so is development symlink used during linking and there's no need to
have it installed (usually shipped in -devel/-dev packages) on user's machines. Instead
of hardcoding library file name, use same mechanism as in libX11 which let's Python
figure the details and share common logic between X11 and Wayland.
Fixes#8771
owl
toofar
owl
dependabot[bot]
qutebrowser bot
Florian Bruhin
Jan Palus