LiveText: announce large bursts via a pumped generator#20177
Conversation
|
This looks like a super useful fix for anyone regularly running CLI-based AI agents. Might be worth taking this PR for a spin with a similar test case right away. |
|
@cary-rowen I don't use agents myself, but I'd love more testing of it since I don't have a lot of cases to test this on my system atm. One candidate I'm 99 percent sure this solves is things like |
|
@cary-rowen Just confirmed: this fixes the console flood problem with jorunalctl at least. |
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
Co-authored-by: Sean Budd <seanbudd123@gmail.com>
|
can this please get a change log entry and more clearer testing steps in the PR description? |
There was a problem hiding this comment.
Pull request overview
This PR targets a long-standing responsiveness issue where large bursts of live-region text (notably terminal output) can block NVDA’s main thread while announcing many lines. It changes LiveText line reporting to truncate very large bursts and to announce lines via a queued, yielding generator so the main loop can continue pumping input/events.
Changes:
- Add a per-burst cap on announced live-text lines (default 100) and announce how many lines were skipped when truncation occurs.
- Replace synchronous per-line reporting with a
queueHandler-registered generator that yields every N lines to keep NVDA responsive. - Cancel any in-flight live-text reporting generator when speech is canceled or when a new burst supersedes it.
|
@seanbudd I can do the change log at least, but I'm not really sure what a "better testing strategy" entails. Like, you can validate it by SSH'ing into a server that's been up for a month or something and doing |
|
I was asking for a clearer testing strategy, not a better one. The section currently states
Can you provide brief step by step instructions on what you did to test? |
|
All done, ready for merge I think |
|
doesn't look like any commits were pushed |
|
Oh oops, I thought I'd pushed it but. Well. There we go. |
|
might have been a caching issue on my end |
|
@seanbudd No, I hadn't pushed it xD |
|
The one thing we might want to consider: the "xx lines skipped" is reported in the same stream as terminal output and can itself sound like a terminal line. Maybe adding a tone, pitch change, or other signal would more clearly flag this as an out-of-band notification? |
|
@codeofdusk I honestly might remove the string on the basis that it just isn't audible to end-users when NVDA truncates the line list like that. And add-ons like speech history don't let you go back that far unless I'm misremembering, so the only way to "hear" it would be to log speech. |
…t audible by the end-user when it happens
|
@codeofdusk Yeah, I just got rid of the announcement since as I noted in the commit it isn't audible when it happens. I'm not entirely certain why the system checks are failing though. |
|
@ethindp I've actually heard the announcement in a few situations on my machine, particularly when reading long conversations in Gptcmd. I think a (perhaps configurable) The system test failures are probably transient, do |
|
@codeofdusk Yeah I just pushed the change without the announcement for now, unless you want me to revert that? I'm a bit hesitant to add a fully configurable beep and all that given that I don't know the NVDA codebase yet as much as I would like. But I'd be happy to see about working on it in another PR. |
|
I'm happy to do that in a follow-up PR. |
|
Ah alright, sounds good. I think this one might be finished then, I'm... Not really sure what else that needs be done. |
|
You also need to resolve merge conflicts with the master branch, merge the upstream master branch into your branch, and resolve the merge conflicts. |
… into live-text-queue-fixes
|
Your merging method seems to be problematic. The differences in this PR are very large; please do not use the squash method to merge. |
|
@wmhn1872265132 I mean, okay, if that's what you want... |
|
Is there anything else that is required for this PR? Just checking since I'm pretty sure people are clamoring for this to get merged. :D I hope all the testing has worked out well (it seems to work fantastically for me). |
|
Thanks @ethindp |
Co-authored-by: wencong <manchen_0528@outlook.com>
Co-authored-by: wencong <manchen_0528@outlook.com>
|
Will do |
|
@cary-rowen All done. |
Link to issue number:
Fixes #6291. Partially addresses #15786, #15850, #11002, and #14189. All of these describe the same general class of "NVDA stops responding during console flood" problem but also include symptoms (UIA event flooding, NVDA crashes destroying the console window, etc.) that are out of scope for this PR and were partly addressed in #14888 and #14067 anyway. Related to #2977 and #875.
Most of these are already closed, but the underlying root cause (which is the main thread blocking during line announcement) was never actually addressed.
Summary of the issue:
When a live-text region (such as a terminal) produces a large burst of new text in a short window, NVDA's main thread blocks announcing every line sequentially, becoming unresponsive to keyboard input until the burst finishes. In some cases this lasts tens of seconds. In other (more drastic) cases, it can last so long that the user has to sign out, restart, or force-kill NVDA.
Description of user facing changes:
NVDA no longer freezes when large bursts of text are reported in a live region.
Description of developer facing changes:
LiveText._reportNewLinesis no longer guaranteed to report lines synchronously. Internally, it now registers a generator withqueueHandlerwhich yields between batches of lines so the main thread can service input during long announcements.A new class attribute
LiveText.MAX_LINEScaps the number of lines announced per burst. We drop everything before that on the floor.When a new burst arrives while a previous one is still being announced, the previous generator is cancelled and the new burst supersedes it.
Description of development approach:
Live-text regions announce new text via
LiveText._reportNewLines, which was invoked synchronously from the main thread's event queue. For workloads producing substantial amounts of new lines in a short window, every line traverses the full speech pipeline sequentially without yielding, which can leave the main thread unresponsive to keyboard input for tens of seconds or longer.This PR addresses the responsiveness problem at the announcement layer rather than at the diff or event-source layer, which were previously addressed by #14888 and #14067. Specifically:
(1) Bursts larger than
MAX_LINESare truncated to the most recentNlines. Skipping the oldest lines was chosen over skipping the newest because most flooding scenarios carry their actionable content at the end.(2) Reporting now happens via a generator registered with
queueHandler.registerGeneratorObject, yielding every 5 lines. This is the same primitive previously used byspeech.speakSpellingand the originalsayAll. Between yields, the main thread services the event queue, which keeps NVDA responsive.(3) A handler is registered with
pre_speechCanceledso that pressing control (or any other speech-cancel trigger) also cancels the in-flight generator. Perceptibly this may not cancel immediately and up to 5 lines may still be announced past the cancel, but internally the generator won't proceed past that iteration.Testing strategy:
Reproducing this bug is rather tricky and I'm not sure what the most reliable reproducer is. However, here are some things that trigger it for me:
uv sync --upgrade. Another alternative is to justuv adda bunch of packages. UV constantly sends ASCII control sequences and redraw commands to the terminal, and NVDA will freeze trying to process every single one.Get-EventLog -LogName System. Unless PowerShell has changed this command, this should freeze NVDA for at least a second.journalctl -xfe, ordnf install texlive-*.I've no doubt there are many other ways this bug can be triggered. Essentially, any action that sends a huge number of speak requests to NVDA extremely quickly should do it.
Known issues with pull request:
This PR does not address the case where a single line in an editable text control is extremely long (e.g., the Notepad/Notepad++ scenario in #13432). The bottleneck there is
getTextWithFieldsinNVDAObjects/window/edit.py, which does COM round-trips to extract the text and per-character formatting from the host process. In theory it could be addressed by chunking the speech and adding a fast path for uniform-format ranges, but that's a different PR with its own design questions.The supersession behavior silently drops content from the previous burst when a new one arrives. In practice this is what users want for the common flooding scenarios (most recent state is most relevant), but I'm open to feedback if reviewers think a different policy is appropriate.
Code Review Checklist: