Repository navigation
fix(subtitles): resync tool draws only the cues in view, shift keys no longer throw, left button only - #33
Merged
Merged
Conversation
…o longer throw, left button only
Three upstream bugs in SubtitleSyncer, none of which showed on screen:
- renderTracks() picked the cues in view with `start <= max || end >= min`,
which is true for every cue, so the whole track sat in the DOM and each
cue was repositioned on every frame (300 .timeline_track_cue elements for
a 300-cue track; now ~18). The filter is SubtitleSyncUtils.cuesInRange().
- shiftSubtitles(), what ShiftSubtitlesLater/Earlier call while the tool is
open, ended with this.onVideoTimeUpdate(), which does not exist. The
TypeError was caught and logged by EventEmitter.emit on every key press.
- The track row started a drag on any mouse button; after a right-click the
context menu swallows the mouseup and the cues followed the pointer.
The tool now also says where the track stands ("Shifted subtitles +1.40s",
from SubtitleTrack.shiftTotal), and cues dragged into view show at once
instead of up to 500 ms later.
Tests: tests/unit/SubtitleSyncUtils.test.mjs; tests/e2e/specs/subtitle-sync.e2e.mjs
fails on each of the three bugs against the old code and passes now. lint 0,
typecheck 0, 316 unit tests, 14/14 web e2e spec files.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Nawid3333
added a commit
that referenced
this pull request
Oct 1, 2026
…al.cmd) (#121) * build: one double-click brings this PC's tools up to date (update-local.cmd) GitHub can't update the owner's PC, so update-local.cmd runs tools/update-local.ps1, which brings it to what CI uses, by CI's rule (a release counts once it is 5 days old; tools/newest-release.mjs, on tools/check-toolchain.mjs's helpers): - Node.js of the major .nvmrc names: nodejs.org's MSI, checked against its SHASUMS256.txt, installed with an admin prompt; a newer major is kept; - npm's newest release, and at least the pnpm version package.json pins (both global, %APPDATA%\npm); - on main with nothing uncommitted: git pull --ff-only, pnpm install --frozen-lockfile into the store node_modules was installed from, and fsaunpack's npm ci (scripts off); - the mpv helper again (native-host/install.ps1, with the installed mpv and Node paths) when the repository's host differs from the installed one. Never Firefox, mpv (C:\Program Files\mpv is the owner's own repository with its own updater) or WSL. -DryRun only says what it would do; -Repo points it at another checkout. A failed step is reported and the rest still run. Run for real on this PC against the main checkout: pulled to 6018d62, installed into V:\.pnpm-store, fsaunpack installed, and the mpv helper reinstalled (#109 had changed the host): it answers a ping through its .bat with its mpv path. tests/unit/updateLocal.test.mjs: the version picks, and the script is ASCII and parses in Windows PowerShell. .cmd files check out with CRLF. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(newest-release): encode every / of a package name (code scanning #33) name.replace('/', '%2F') encoded only the first slash. An npm name has at most one (@scope/name), so the lookup worked, but every one is encoded now, as CodeQL's js/incomplete-sanitization asks. Checked: @types/node and npm resolve. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(update-local): 5.1 stderr, version suffixes, MSI staging and signature Windows PowerShell 5.1 turns a native command stderr line into a terminating error when output is redirected (the git pull report, npm warnings): its steps reported failed on success. Judge by the exit code, not by errors. [version] does not read prerelease or build suffixes (a pnpm pin carries sha512) and wants two numbers at least; strip and pad first. The MSI goes into a per-run directory under %TEMP% instead of its predictable name next to everything else, is required to carry the OpenJS Foundation signature (the SHASUMS hash comes from the same host), and msiexec.exe is resolved from System32, not PATH. * feat(update-local): check and report by default; -Apply brings this PC up to date Double-clicking now changes nothing: it reports what is out of date (Node of the .nvmrc major, npm, pinned pnpm, a clean main's distance behind origin, the installed mpv helper) with the exact command for each. Only -Apply installs, pulls or reinstalls, and -DryRun stays as an old name for the check. The Node installer is staged under a per-run %ProgramData%\FastStream directory locked by ACL to this user, Administrators and SYSTEM, so the same-user race between the hash check and the elevated msiexec the review flagged is closed. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Nawid3333 <limex@users.noreply.github.com>
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.
While porting the subtitle resync tool (the hourglass on a subtitle track) to mpv, I found three bugs in it. All three come from upstream's 2023 code, and none of them showed up on screen.
Fixed
renderTracks()picked the cues in view withstart <= max || end >= min, which is true for every cue. So a 300-cue track put 300 elements in the timeline and moved each one on every frame. Only about 18 are needed. The filter now uses&&(SubtitleSyncUtils.cuesInRange).shiftSubtitles()is what ShiftSubtitlesLater/Earlier call while the tool is open. It ended with a call toonVideoTimeUpdate(), whichSubtitleSyncerdoesn't have. The shift itself still happened, so nobody noticed:EventEmitter.emitcatches the TypeError and only logs it. It fired on every key press.Improved
SubtitleTrack.shiftTotal, added up inshift(). Shifting one cue (edit mode) doesn't count toward it. It reuses the existingplayer_subtitlesmenu_shifttool_message, so there are no new strings to translate.Tests
tests/unit/SubtitleSyncUtils.test.mjs: the range filter and the shift formatting.tests/e2e/specs/subtitle-sync.e2e.mjs, run in the real player (web build, headless Firefox) with a 300-cue track onlong-av.mp4:Against the old code it fails on all three bugs: 300 elements, "this.onVideoTimeUpdate is not a function", and a 6.7 s shift from a right-drag. It passes now.
Local results:
test:e2e: 14/14 spec filesNot run locally:
verify:linux, the extension suites and the mpv specs (those kill every mpv.exe on the machine). CI runs them.CLAUDE.md gets an entry under "Known upstream bugs fixed here".
Overlap with other open PRs
This PR changes only its own files: the new
utils/SubtitleSyncUtils.mjsand its test,SubtitleSyncer.mjs,SubtitleTrack.mjs, and the end of CLAUDE.md. None of the other open PRs (#29–#32) touch these, so there should be no conflicts.🤖 Generated with Claude Code