Fix 20 bugs found in a bug hunt - #14203
Merged
Merged
Conversation
Backlog from the previous sweeps' agent reports, plus fresh passes over the Tools dialogs and the remaining libse areas. Wrong results - Apply minimum gap could pull a line's end BEFORE its own start (a short line followed closely by the next), writing a negative duration to the grid and the saved file while counting it as a fix. - Adjust durations (Fixed and Percent) capped the end flat at the next line's start, so two rows sharing a start time produced a zero-duration line and out-of-order rows a negative one. Now leaves the minimum gap and floors the duration, as libse and the batch path already do. - Sort by: removing the last criterion left the previous sorted order in the preview, and OK applied it. ApplySorting already handles an empty list; the early return stopped it running. - Convert actors applied whatever the 500 ms preview timer last computed, so OK right after a settings change used the old settings while saving the new ones. Every sibling dialog in that folder already refreshes on OK. - Merge lines with same time codes tested "- " for an existing dash in the default dialog style, so "-Hello" became "- -Hello". Its sibling case in the same switch tests "-". - FirstOnOrAfter/FirstOnOrBefore ran their tolerance probe even on an exact BinarySearch hit and returned the neighbour on the wrong side - a cue snapped onto a shot change found the shot change 20 ms earlier. - StrippableText took the text before "[" as [0, idx - 1), dropping the very character the test looks at: "Yes.[Motor roaring] hello." left "hello" lowercase. - The word-split exception tested a lowercase 'a' that the enclosing "starts with its own uppercase" guard had already excluded, so "Acat" was never split while "Iam" was. - Google Lens sent LocaleContext.Language "en" whatever OCR language was picked: the code reached the Accept-Language header but not the protobuf request, and nothing ever populates the config it read instead. - The grid's ASSA highlighter treated "\clip" as a colour tag (the test excluded a digit but not a letter), so the rest of the line lost its colour. - Bridge gaps wrote fractional milliseconds; rounded at the producer (#14056). - On Windows, a file association reassigned by the user in Explorer still reported SE as the default, because a non-matching UserChoice fell through to the legacy key SE had written itself. Never displayed / never persisted - 15 buttons set an icon and then bound ContentProperty over it, so the icon never rendered and the buttons had no accessible name; one did it in the other order and lost the bound text instead. - The three download dialogs set Error on every failure path but bound only ProgressText, so the user saw "Download failed" and never the reason. - The auto-cast speakers dialog restored a window position that nothing ever saved. - Accepting the IndexTTS 2.5 licence was not persisted immediately, so it could be asked again. - The speech history dialog's play buttons opened disabled until an unrelated watchdog tick enabled them. - Chatterbox's install-folder path was added one grid row below its label. Leaks - Cancelling an engine download stranded the partial .7z (up to ~1.5 GB) in the engine folder; only the success path deleted it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
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
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.
Sweep 18. The verified backlog from earlier sweeps' agent reports, plus fresh passes over the Tools dialogs and the remaining libse areas.
Suites: libse 1635 / libuilogic 708 / seconv 422 / UI 4159.
Wrong results
next.Start - gapcan land before that line's own start when the line is short and the next one starts soon after. It was applied anyway and counted as a fix, so the bad duration reached the grid and the saved file.Secondsbranch in the same file already floors its result, and libse'sSetFixedDuration/AdjustDisplayTimeUsingPercentleave the minimum gap — so the same operation behaved differently in Batch convert.ApplySortingalready returns the rows unsorted for an empty criteria list; an early return inUpdatePreviewstopped it ever running.ClearSortCriteriarepopulates by hand for exactly this reason.SaveSettingspersisted the new ones — and before the first tick it applied nothing at all. Five sibling dialogs in that folder already refresh on OK and carry the comment.StartsWith("- ")for an existing dash while its sibling case tests"-", so"-Hello"came back as"- -Hello". Fixed in both the libse helper and the live copy in the dialog.FirstOnOrAfter/FirstOnOrBeforeran their tolerance probe even on an exactBinarySearchhit and returned the neighbour on the wrong side. A cue already snapped onto a shot change asked for "the next shot change" and got one 20 ms earlier. The generic overloads in the same file only rewrite the index when the search missed.StrippableTexttook the text before[as[0, idx - 1), dropping the very character the test inspects:"Yes.[Motor roaring] hello."saw"Yes"instead of"Yes."and lefthellolowercase. The documented"I [Motor roaring]"case is unaffected.'a'that the enclosing "starts with its own uppercase" guard had already excluded, so"Acat"was never split while the'I'sibling ("Iam") worked.LocaleContext.Language = "en". The chosen language reaches theAccept-Languageheader but not the protobuf request, which read a config that nothing ever populates (new Lens()takes no config andUpdateOptionsis never called).\clipas a colour tag — the test excluded a digit aftercbut not a letter — so a{\clip(...)}block cleared the colour state and the rest of the line lost its colour in the grid while libass and SE's own preview kept it.UserChoicefell through to the legacySoftware\Classeskey that SE itself had written.Never displayed, never persisted
ContentPropertyover it, so the icon never rendered and — since the label starts empty — the buttons had no accessible name either. One did it in the opposite order and lost the bound text instead.UiUtil.WithIconLeftBindTextexists for this and is used correctly by five sibling dialogs.Erroron every failure path but bound onlyProgressText, so the user saw a generic "Download failed" while the real cause went to a property no window rendered. Three other download windows bind it.Leaks
Cancelling an engine download stranded the partial
.7z— up to ~1.5 GB for Faster-Whisper-XXL / WhisperX — in the engine folder, since only the success path deleted it.A fix the tests corrected
My first
ApplyMinGappatch used the samenewDuration > SubtitleMinimumDisplayMillisecondsguard the batch converter uses. That broke two existing tests — correctly: the default minimum is 1000 ms, so the guard skipped ordinary shortening and defeated the feature. Those tests encode the dialog's real contract. The shipped fix guards only against a non-positive duration, which is the actual defect, and a new test pins it (it fails without the fix).Flagged but not fixed
Two CEA-708 / MacCaption findings need someone who can validate against real
.mccfiles from other tools, so I have left them alone rather than change what SE writes on a static-analysis argument:Smpte291Mnever skips the optional CDPtime_code_section(id0x71) whenCaptionDistributionPacketTimeCodeAddedis set — the reader hardcodes the cc-data offset, while the siblingServiceInfoAddedflag is honoured. Files from other tools would decode to nothing.0xbb. The arithmetic suggests it should be0xB4, and0xBBhappens to equal0x61 + 0x01 + 0x59where0x59is a stale data count the writer later overwrites with 82 — so it looks like a genuine leftover. SE re-reads its own files because the reader never verifies the checksum.The mpv event-loop test still fails intermittently here; per the previous PR that is a worktree artifact, not a source issue, and nothing in this PR touches the video players.
🤖 Generated with Claude Code