[players] Fix 33 bugs in the preview, playlist and shared players#2147
Merged
Conversation
toPrecision(4) keeps 4 significant digits, not 4 decimals: past 100s the time channel quantized to 0.1s steps (1s past 1000s), so annotations were saved and looked up on neighbouring frames, the frame counter stuck and jumped, and VideoViewer emitted frame numbers in steps of 10 past frame 9999. roundToFrame already provides stable 4-decimal times. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ctrl+Shift+Left/Right read entityList after moveSelectedEntity had synchronously emptied it: TypeError on every use and the order-change event was never emitted, so the visual reorder was never saved. Build the payload before the move, like the drag path does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The picture branches of PlaylistPlayer.updateMainAnchor and the shared
overlay's videoBounds clamped width and height independently: with a
container narrower than the image but taller than it, the anchor ended
up at a wrong aspect ratio ({cw x mh}) while the image displayed at
{cw x cw*mh/mw}. Annotations were drawn and, in the shared player,
saved against that wrong box, misaligning them everywhere else. Use a
single contain scale capped at 1 (never upscaled), matching the sizing
PictureViewer actually applies.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ests Comments rendered in the shared playlist panel built attachment URLs with the JWT-protected /api/data/attachment-files route, so guests got a 401 on every attachment (upload worked, display did not). Route them through zou's anonymous /api/shared/playlists/<token>/attachment-files endpoint via a urlPrefix prop on Comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
postAnnotationUpdate emitted the literal 'preview-update-annotation' and the receiver listened on the same dead name, but zou only relays 'preview-room:update-annotation': moving or resizing an annotation never reached the other participants (adds and removes did). Add the missing PREVIEW_ROOM_EVENTS constant and use it on both sides, and read the broadcast time from eventData.data.time where the senders actually nest it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The non-reactive previewFileMap was only rebuilt on mount and on task-type changes: switching tasks in TaskInfo (same task type id) or uploading a new revision left it resolving stale preview files, so the comparison bar showed a selected revision while the comparison pane stayed empty. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PreviewViewer.pause() only paused the wavesurfer when the CURRENT kind was sound, but every pause path runs after the preview swap, so leaving a playing sound preview never stopped it (v-show keeps it mounted and the empty-URL watcher ignored the blanked source). Pause the sound viewer unconditionally and stop playback when the URL is blanked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fallback computed 2 * fps * frameDuration, which is just 2 frames (fps * frameDuration is 1): pictures without preview_nb_frames got a near-invisible strip slot and were skipped in about 80ms during continuous playback while the counter announced 2 seconds worth of frames. Use Math.round(2 * fps) like every other fallback site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The opacity applies to the MAIN viewer (painted on top), so compared visibility is 1 - opacity. Only the extremes had been inverted: Overlay 25% showed 75% of the compared clip and vice versa, and the same menu behaved opposite to the PreviewPlayer's for the mid steps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
toggleFullOverlay enabled comparison without selecting a task type or revision: in PlaylistPlayer the first Alt+O faded the main viewer to zero opacity over an empty comparison pane (black player until the next press), and in PreviewPlayer it was a silent no-op. Default the target the way the compare button does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
setPlaylistProgress guarded playEntity with a truthy check on the index, so index 0 never matched: when the concatenated build looped or restarted from the beginning, the task panel, annotations and the highlighted entity stayed on the last played one. The sibling onProgressPlaylistChanged already compares without the truthy check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The only writer of the player:muted preference sat in onToggleSoundClicked, reachable through PlayerPlaybackBar's @change-sound listener on ButtonSound, which never emits that event (it drives isMuted through v-model): muting never survived a reload. Persist from a watcher on isMuted instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every mounted player registers the global keydown listener, and the playlist modal keeps the task page's PreviewPlayer mounted underneath: Ctrl+Z undid (and saved) annotations on the hidden player, d/e toggled both drawing modes and arrows stepped both. Add an isActive gate to usePreviewShortcuts and generalize the modal check PreviewPlayer already applied to play/pause only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
visibleImage selects the original (pictureBig) when fullScreen OR big, but endLoading, getNaturalDimensions and resetPicture only tested fullScreen: in big mode the spinner cleared when the hidden small variant loaded (blank viewer on slow links), measurements returned the downscaled dimensions (low-resolution annotation snapshots, display capped at the preview's width) and size-changed measured a hidden img. Also stop relying on pictureGif.complete, which is true for every non-gif preview (an img without src reports complete). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The object viewer only mounts for ready previews, but the parent's extension-based is3DModel is true as soon as the file is a glb/gltf: pressing play or space during processing (or on a broken model) threw on the null viewer ref and left the playing state inconsistent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previewRoom and annotationBroadcast specs pinned the dead 'preview-update-annotation' literal and the flat time payload; the playlistComparison spec pinned the non-inverted overlay mid steps. Update them to the corrected contracts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MultiPictureViewer mounts one eagerly-loading PictureViewer per picture preview of the whole playlist and forwarded every viewer's loaded event: each background image finishing its download reset the live annotation canvas (flicker, wiped in-progress strokes) for seconds after opening a picture-heavy playlist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shared player advanced to the next entity on every video end, so enabling repeat did nothing: MultiVideoViewer's trim-loop path is inactive without handles and its playNext repeat branch was never called. Loop the current movie like the studio player does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
start_duration is built with the +1 slot convention, so the shared strip handler resolved clicked global frame N to local frame N-1; the studio player compensates with +1. One frame off on every click or drag of the playlist strip in a frame-accurate review tool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The shared overlay's render() started with a full canvas clear and rtk diff reset, and runs on every resize, comments-panel toggle, frame step and playback tick: with an explicit-save UX, anything not saved yet was silently destroyed. Capture the pending additions before the reset, repaint the displayed frame's ones, re-adopt the diff (data only while playing), and clear it explicitly once a save succeeds. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
toggleFullscreen exited fullscreen whenever ANY element was in that state: with the player fullscreen (the comments column lives inside it), the attachment's maximize button kicked the whole app out of fullscreen instead of showing the video. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A task deleted from another session leaves its id in the edit's tasks list (the edits store only prunes displayed edits) while the tasks store already dropped it: the unguarded map lookups threw during render and broke the Edit page header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The drop zones accepted any drag (dragover was cancelled) but never cancelled the drop nor validated the payload: dropping an OS file threw in the reorder handler and let the browser open the file over the app. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The d/e tool toggles fired on any modifier combination and cancelled the default, hijacking Ctrl+D (bookmark) and Ctrl+E whenever a player was mounted. Guard them like the arrow shortcuts already are. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Alt+Tab swallows the keyup, so isAltHeld stayed true after returning: the annotation overlay remained pointer-transparent (no drawing, no selection) until the user tapped Alt again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only currentEntity was optional-chained: an entity payload without preview_files threw on the bracket access when clicking a timecode in a comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ties An entity without a preview has no duration: the unguarded reduce made playlistDuration NaN and broke the playlist strip in full mode. resetPlaylistFrameData already defaults the same field to 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The non-movie branch destroyed the instance without clearing the reference: coming back to a movie entity double-destroyed it inside loadWaveForm's try, which could skip the rebuild and leave the waveform dead until the toggle was flipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The post-extraction restore subtracted 1 from the 0-based currentFrame (copied from PlaylistPlayer where the counterpart value is a 1-based label), stepping the playhead one frame back after attaching snapshots. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The isTyping watcher dereferenced the canvas wrapper and the upper-canvas element without guards on the teardown branch: leaving type mode before fabric was initialised threw on removeEventListener. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The comparison overlay looked its annotation up with strict float equality between differently-derived times (raw frame products, rVFC media times) and stored rounded times: the compared revision's drawing silently failed to display on most frames. Round both sides to the frame like the main-canvas lookups do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paste re-added the copied instances themselves (multi-selection) or an Object.create wrapper inheriting the original's id (single object): moving the "copy" posted updates under the original's id, so the server and remote viewers moved the original and the local duplicate vanished on reload; fabric also stacked duplicate _objects entries of one instance, leaving it locally undeletable. Clone each source with a new uuid and a small offset, and drop the double undo entry per paste. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The undo/redo stacks survive frame navigation and the replay records its delta at the CURRENT time: deleting a stroke on frame 10, seeking to frame 20 and pressing Ctrl+Z restored the stroke onto frame 20 (and persisted it there for everyone) while frame 10's deletion stayed queued. Stamp every history entry with its time and reset the stacks instead of replaying an entry made on another frame; same-frame round-trips keep their history. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Receivers decided whether a remote stroke was ephemeral from their OWN laser toggle: a presenter's laser gestures accumulated as permanent strokes on every other screen, and a laser-holding viewer faded real notes away. Send the flag with the addition and fade on it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both group save paths overwrote the normalized serialization with live-canvas pixels (getNewAnnotations with a translation-only formula, onObjectModified with transformPoint plus naive scale multiplication): dragging, scaling or rotating a marquee selection on a canvas that differs from the authored size (fullscreen, other window, comparison) stored coordinates in the wrong reference frame and the strokes drifted on reload and on every other screen. Detach the child, apply its absolute matrix with util.applyTransformToObject, serialize (which normalizes), then restore the group-relative state. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only pencil strokes and erases cleared the redo stack: after an undo, adding a text note, finishing a shape, deleting an object or pasting kept the undone entry replayable, and Ctrl+Shift+Z resurrected stale history on top of the new state (posting a fresh addition delta). Clear on every new user action, guarded so undo/redo's own re-adds and re-deletes don't wipe the stack they are working through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vases The comparison loader only checked its token before each await and the onion ghost renderer not at all: an object whose async build was in flight when a clear or fresher load hit still landed, leaving orphan strokes on the comparison overlay and partial ghosts on the onion canvas during fast navigation. Apply the main-canvas pattern (check after the await, remove the late object). Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
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.
Problem
Solution
Note: enforcing can_comment on view-only share links needs zou to expose the flag first; left out on purpose.