Fix #13885: coalesce per-line OCR (and auto-translate) UI updates so input stays responsive - #13887
Merged
Merged
Conversation
Fast OCR engines finish a line in milliseconds, and every line posted selection, scroll, text and progress updates at Normal dispatcher priority - which outranks input processing. During a run the queue never drained, so Pause/Cancel clicks and even the window's minimize were starved until OCR completed (issue 1 and the delayed-minimize half of issue 2; the post-restore wedge of issues 2/3 is the #13865 minimize mirror bug already fixed by #13877 after beta 18 was cut). Per-line UI feedback now goes through a coalescing queue flushed by a one-shot Background-priority timer at most every 200 ms: data updates (line text, fix results, unknown-word/fix/guess rows) are applied in order, while selection+scroll and progress are latest-wins - a newly OCR'ed line replaces any still-pending selection. This also moves the UnknownWords/AllFixes/AllGuesses ObservableCollection mutations onto the UI thread; the loop engines previously added to these bound collections from their worker threads. The queue is flushed synchronously before any modal that reads line text (unknown-word prompt, nOCR/binary add-character windows) and in OK, so dialogs and the final subtitle never see a half-flushed queue. OCR UI tests: 665 passed. Full UI suite: 3234 passed, 1 pre-existing failure (BatchConvertStatusColor test stale since #13884, unrelated). Co-Authored-By: Claude Fable 5 <[email protected]>
The pure trailing 200 ms delay made the grid visibly lag behind the run. An update arriving after a quiet period now flushes immediately; only while updates keep streaming are they batched, at most one flush per 100 ms. Flushes stay at Background priority, so input still wins. Co-Authored-By: Claude Fable 5 <[email protected]>
The OCR throttle is now CoalescedUiUpdateQueue in Logic, so auto-translate gets the same treatment: per-batch progress and the follow-along selection are queued and applied leading-edge throttled at Background priority, instead of a synchronous Dispatcher.Invoke per batch at Normal priority. Only the pure UI feedback is queued for translate. The row TranslatedText writes deliberately stay immediate: AdvancedTranslatorBase.CollectHistory reads them back to build the next batch's rolling context, and MergeAndSplitHelper reads existing translations when re-applying formatting, so deferring those writes would degrade the translation itself, not just the display. The finally block flushes before applying the final progress/selection so a stale queued update cannot override it. Adds unit tests for the queue: leading-edge apply, batching while streaming, in-order data updates with latest-wins selection/progress, no replay after an explicit flush, and worker-thread enqueue applying on the UI thread. Co-Authored-By: Claude Fable 5 <[email protected]>
The converter's non-error default changed from AvaloniaProperty.UnsetValue to the theme text brush in #13884 (so in-progress statuses stop rendering black on the dark theme), which left this test asserting a null that can no longer happen - it fails on main. Assert against the default brush instead: after the language swap the old prefix gets the default brush and only the new prefix gets the error brush, which is what the test is actually about. Fixing it here because it is the only red test on this PR's CI run. Co-Authored-By: Claude Fable 5 <[email protected]>
niksedk
changed the title
Both view models reached the queue through a lazy `??=` property, and both are touched from a worker thread (the OCR/translate loop) and the UI thread (OK, the modal flushes). A simultaneous first access could build two queues and drop whatever was pending in the loser. Constructing them up front removes the race; the field initializer cannot do it because the callbacks are instance methods. Co-Authored-By: Claude Fable 5 <[email protected]>
…ssue-13885-3a7a95 # Conflicts: # tests/UI/Logic/ValueConverters/ValueConverterTests.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes issue 1 of #13885 (buttons dead during OCR) and the "minimize only happens after the run completes" half of issue 2.
Why the UI froze
Fast OCR engines (binary image compare, nOCR) finish a line in milliseconds, and every line posted selection, scroll, text and progress updates at Normal dispatcher priority — which outranks input processing in Avalonia. During a run the dispatcher queue never drained, so Pause/OK/Cancel clicks and even the window's minimize were starved until OCR completed.
The post-restore wedge in issues 2/3 (window hangs / lines unclickable after minimize+restore) is the #13865 minimize-mirror bug, already fixed in #13877 — which merged a few hours after beta 18 was cut, so the reporter can't have it yet. That part should be retested on the next beta.
The fix
New
CoalescedUiUpdateQueue(src/ui/Logic): per-line UI feedback is queued and applied in batched dispatcher jobs at Background priority, below input, so a flood of updates can never starve clicks or window messages.The flush is leading-edge throttled: an update arriving after a quiet period is applied immediately (so slower engines still feel live), and only while updates keep streaming are they batched to at most one flush per 100 ms.
OCR
All the per-line engine loops now go through the queue. Side effect fixed for free: the loops previously mutated the bound
UnknownWords/AllFixes/AllGuessesObservableCollections from their worker threads; those adds now ride in the coalesced update on the UI thread.Ordering safety: the queue is flushed synchronously before any modal that reads line text (unknown-word prompt, nOCR/binary add-character windows) and at the top of OK, so dialogs and the final subtitle never see a half-flushed queue. User-driven navigation (inspect, go-to-line, pre-processing) keeps the immediate
SelectAndScrollToRowpath.Auto-translate
Same treatment for the per-batch progress and follow-along selection, replacing a synchronous
Dispatcher.Invokeper batch at Normal priority.Deliberately narrower than OCR: the row
TranslatedTextwrites stay immediate, becauseAdvancedTranslatorBase.CollectHistoryreads them back to build the next batch's rolling context andMergeAndSplitHelperreads existing translations when re-applying formatting — deferring those would degrade the translation itself, not just the display. The finally block flushes before applying the final progress/selection so a stale queued update cannot override it.Tests
New
CoalescedUiUpdateQueueTestscovers the leading-edge apply, batching while streaming, in-order data updates with latest-wins selection/progress, no replay after an explicit flush, and worker-thread enqueue applying on the UI thread.Full UI suite: 3242 passed, 0 failed.
The last commit fixes
BatchConvertStatusColor_StillFollowsTheErrorTextAfterItChanges, which is red on main: #13884 changed the converter's non-error default fromUnsetValueto the theme text brush, and the test still asserted the old null. It was the only red test on this PR's CI, so it is fixed here rather than in a separate PR.🤖 Generated with Claude Code