Prevent stale frames from being drawn / always ensure up-to-date contents in webview

[Lolle2000la]

Following up on #3656

Short description

At key points where external changes enter the webview, stale images might get rendered. This ensures that a frame showing current state is always shown.

What it solves

Sometimes previous or corrupted frames get rendered, only showing the current state of the page (i.e. the back of the card or the next card) when an action that causes a repaint is done by the user (for example: selecting text).

How it solves it

At key points where the webview is being interacted with externally, a full update (which includes a repaint) is done.

Potential implications for performance

While this ensures that no more stale frames get shown, it also causes full repaints to happen quite often and in addition to the render operations QWebEngineView might do on its own. I think this is negligible in my testing, but needs to be pointed out.

Request for comments

In my testing this solved the issue, and this time it seems to do so reliably, however, please leave feedback or opinions on this. It is also a bit of a wooden-hammer-approach, but it does fix the issue.

[abdnh]

Didn't notice a difference or issues testing this, and it looks harmless to include. Will leave it to @dae though.

[dae]

I presume the answer is yes, but could you confirm the issue you're trying to address also exists when the software driver is enabled?

My understanding of this rendering issue is that for most people, they either won't see the problem, or only see it with particular video drivers. The changes in this PR would thus make things better for a relatively small portion of the userbase, so it would be nice not to impose a performance penalty on unaffected users.

You've placed .update() calls in four separate locations. In your testing, were all locations required, or did you add some "just in case"?

If you experiment with scheduling an .update() for a few seconds after JavaScript is evaluated, instead of calling .update() immediately, does the corrupt display refresh correctly after those seconds elapse?

[Lolle2000la]

Apologies for the late reply, I am currently preparing for my exams so I am currently slower to respond.

I presume the answer is yes, but could you confirm the issue you're trying to address also exists when the software driver is enabled?

Yes, I have tried basically all driver configurations and run them for prolonged amount of times and they occured all across them, including software. Vulkan is the one where it occurs the least.

My understanding of this rendering issue is that for most people, they either won't see the problem, or only see it with particular video drivers. The changes in this PR would thus make things better for a relatively small portion of the userbase, so it would be nice not to impose a performance penalty on unaffected users.

It shouldn't have a large penalty since it essentially just causes a second frame to be rendered when the webview is externally interacted with from python code. In essence this makes sure that a frame with the current state is being put to the screen.
This is, as far as I can see, only the case when the cards back gets shown or another is displayed, as well as when the card is faded out on inactivity.

You've placed .update() calls in four separate locations. In your testing, were all locations required, or did you add some "just in case"?

self._evalWithCallback could alone fix this, because all the other APIs call onto this. My original rational was that every method affecting the webview could cause the same problems, but tracing the calls a bit more I notice that load_url and _setHtml cause _on_load_finished to be called, which in turn calls eval, which calls evalWithCallback, queues the action and then, finally, calls _evalWithCallback. I will test this. Though, due to how update works, calling it multiple times will not schedule another render if one is already queued.

If you experiment with scheduling an .update() for a few seconds after JavaScript is evaluated, instead of calling .update() immediately, does the corrupt display refresh correctly after those seconds elapse?

I will look into that and verify and share my results here. As I said in the beginning I am currently preparing for my exams so it could take a bit, but maybe someone else can also reproduce the bug.

I will also share the deck I am currently using (漢字.apkg.gz). The bug happens regularly on my Surface Pro 9 every about ~10-100 times in the 漢字::意味、音訓::意味-subdeck (less often on the other subdecks). I have sessions where this doesn't happen and sessions where it does constantly for a long time now. On my PC (Ryzen 9 3900x) this happens less but notably enough.

[dae]

Thanks for your patience Luca. I'm inclined to agree with your assessment that this shouldn't cause a large performance impact, and if it's addressing issues that aren't fixed by a video driver switch, it's probably for the best. If you wouldn't mind just briefly trying to remove any superfluous update() calls so we can get this down to the minimum required changes, that'd be appreciated.

[Lolle2000la]

I have removed the calls in any place except _evalWithCallback, which is called transitively by all other places I had it before. I flipped around ~300 cards from that deck which previously would have triggered it for sure, so I think this is the case. Not exactly the same but I did notice at least one card very briefly flashing the wrong contents but then correcting itself, which makes me confident that it resolves the issue (although this also hints at something bigger but I have no idea where that issue comes).

My testing scheme is stochastic to put it in a nice way, but I don't know how else to test such an issue. I can say that this seems to fix the issue for my deck though.

Side question: Should I add a comment to the instruction linking to this PR or is the git blame enough? I could see someone stumbling upon this and wondering why its there.

[dae]

Thanks for making those changes. Happy to approve this as soon as the conflict is resolved. A one-line comment above the .update() call that explains why we're doing it might be worth adding.

[Lolle2000la]

Thank you for the reply. I have added a comment referencing this PR and explaining what it prevents.