diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index c157104b0..4ef072f35 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -1057,6 +1057,17 @@ export function PassageDetailGuidedPhraseRecord({ handleRegionPlayEndRef.current(region); }, []); + /** + * A click on the waveform is a deliberate selection, so it can never be the + * playback overshoot the swallow exists to absorb. Disarm it here, before the + * segment change it produces reaches the navigation effect below — otherwise + * clicking the segment right after the current one is indistinguishable from + * overshoot and gets eaten, leaving the user's first click with no effect. + */ + const handleSegmentClick = useCallback(() => { + pendingOvershootSwallowRef.current = false; + }, []); + useEffect(() => { if (!bootstrapped || !entryPositioned || entryPauseDoneRef.current) return; if (recordingPassStarted) return; // recording pass auto-plays on entry; don't pause @@ -1684,6 +1695,7 @@ export function PassageDetailGuidedPhraseRecord({ controlsRef={playerControlsRef} applyRegionColor={applyRegionColor} onSegmentPlaybackEnd={onSegmentPlaybackEnd} + onSegmentClick={handleSegmentClick} highlightPlay={highlightPlayButton} onPlayStatusNotify={handlePlayStatusNotify} beforePlay={handleBeforeSourcePlay} diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx index e9564439a..92b9385fc 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx @@ -54,37 +54,41 @@ describe('PBT waveform segment selection', () => { unitLabel('0:06', '0:09').should('be.visible'); }); - it( - 'DEFECT: the first click on the very next segment is ignored', - { tags: '@known-defect' }, - () => { - // After a segment finishes playing, handleRegionPlayEnd arms - // pendingOvershootSwallowRef so the +1 segment change that playback - // overshoot produces can be absorbed. It cannot tell that change apart from - // the user clicking the next segment, so the click is swallowed too: the - // playhead snaps back, the label never changes, and the user has to click - // again. Record also stays enabled for the segment they were leaving. - clickSegmentOnWaveform(1); - unitLabel('0:03', '0:06').should('be.visible'); - } - ); + it('acts on the first click even on the very next segment', () => { + // After a segment finishes playing, handleRegionPlayEnd arms + // pendingOvershootSwallowRef so the +1 segment change that playback + // overshoot produces can be absorbed. It could not tell that change apart + // from the user clicking the next segment, so the click was swallowed too: + // the playhead snapped back, the label never changed, and the user had to + // click again. The player now reports a click distinctly, which disarms the + // swallow. + clickSegmentOnWaveform(1); + unitLabel('0:03', '0:06').should('be.visible'); + }); it('keeps Record off while a clicked segment plays', () => { clickSegmentOnWaveform(2); unitLabel('0:06', '0:09').should('be.visible'); - sampleDom( - (doc) => ({ - playing: readSourcePlaying(doc), - record: readRecordEnabled(doc), - }), - { forMs: 6000 } - ).then((samples) => { - const bothLive = samples.filter((s) => s.playing && s.record); + // One reading, taken from the middle of the segment. `playing` is the + // player's own state and Record's operability is the step's, so at either + // end of playback the two flip on unrelated renders and a sample can + // legitimately catch both live for a frame. Segment 3 runs 0:06-0:09, so + // waiting for playback to start and then settling for most of a second + // lands clear of both edges. Asserting `playing` in the same reading is + // what makes the Record assertion mean anything. + cy.document().should((doc) => { + expect(readSourcePlaying(doc), 'reference audio started').to.equal(true); + }); + cy.wait(800); + cy.document().then((doc) => { + const playing = readSourcePlaying(doc); + const record = readRecordEnabled(doc); + expect(playing, 'reference audio still playing').to.equal(true); expect( - bothLive, - 'Record was never operable while the reference audio played' - ).to.have.length(0); + record, + 'Record operable while the reference audio plays' + ).to.equal(false); }); }); }); @@ -106,7 +110,8 @@ describe('PBT segment selection after a take exists', () => { // during playing which should not be possible". Recording over the // reference audio is what the listen-then-record flow prevents everywhere // else, and Record is correctly off for this same click before any take - // exists (see the previous describe). + // exists (see the previous describe). Fixed separately - this change only + // stops the click itself being swallowed. clickSegmentOnWaveform(2); sampleDom( @@ -130,10 +135,10 @@ describe('PBT segment selection after a take exists', () => { { tags: '@known-defect' }, () => { // Reported as "the yellow highlighting briefly jumps to the next segment". - // The waveform paints from the engine's currentSegmentIndex while the label - // comes from the step's own currentIndex; when a click only reaches the - // engine, the two disagree - briefly in the good case, indefinitely in the - // one below. + // The waveform paints from the engine's currentSegmentIndex while the + // label comes from the step's own currentIndex, so any segment change the + // step does not act on shows up as the two disagreeing. Fixed separately - + // this change only stops the click itself being swallowed. clickSegmentOnWaveform(2); sampleDom( @@ -188,25 +193,37 @@ describe('PBT recording out of order (1, 3, then 2)', () => { }); }); - it( - 'DEFECT: clicking back to segment 2 leaves the step on segment 3', - { tags: '@known-defect' }, - () => { - // Reported: "I record the first segment, then the third, and then try to - // go back and record the second, it records into and replaces the third". - // Clicking segment 2 moves the waveform selection and the playhead there, - // but the step stays on segment 3 - so the next take is filed under - // segment 3, on top of the take already there. Asserting the label is - // enough: while it still reads segment 3, anything recorded goes to the - // wrong segment. - recordAndSettle(1); // segment 1 + it('follows a click back to an earlier segment and files the take there', () => { + // Reported: "I record the first segment, then the third, and then try to go + // back and record the second, it records into and replaces the third". + // The click moved the waveform selection to segment 2 while the step stayed + // on segment 3, so the take was filed on segment 3 over the one already + // there. The engine reports a 1-based segment index and the step sets a + // 0-based one, so this move arrived carrying the index the step had just + // written (engine 1+1 vs step 2) and the navigation effect never re-ran. + recordAndSettle(1); // segment 1 - clickSegmentOnWaveform(2); - unitLabel('0:06', '0:09').should('be.visible'); - recordAndSettle(2); // segment 3 + clickSegmentOnWaveform(2); + unitLabel('0:06', '0:09').should('be.visible'); + recordAndSettle(2); // segment 3 - clickSegmentOnWaveform(1); - unitLabel('0:03', '0:06').should('be.visible'); - } - ); + clickSegmentOnWaveform(1); + unitLabel('0:03', '0:06').should('be.visible'); + recordTake(); + waitForUploads(3); + + cy.then(() => { + const segs = postedTakes().map((t) => t.parsedSegments); + expect(segs[2], 'take recorded on segment 2 lands there').to.deep.include( + { + start: 3, + end: 6, + } + ); + const onSegment3 = segs.filter( + (s) => s?.start === 6 && s?.end === 9 + ).length; + expect(onSegment3, 'segment 3 still has exactly one take').to.equal(1); + }); + }); }); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx index 8a5387b61..5703a0d30 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx @@ -103,6 +103,8 @@ export interface DetailPlayerProps { /** Tool-specific waveform region coloring (Mark Verses, Careful Speech, etc.). */ applyRegionColor?: ApplyRegionColor; onSegmentPlaybackEnd?: (region: IRegion) => void; + /** A segment was clicked on the waveform (not selected by the playhead). */ + onSegmentClick?: (region: IRegion) => void; /** Called when waveform play/pause changes (in addition to internal player logic). */ onPlayStatusNotify?: (playing: boolean) => void; highlightPlay?: boolean; @@ -155,6 +157,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) { controlsRef, applyRegionColor, onSegmentPlaybackEnd, + onSegmentClick, onPlayStatusNotify, highlightPlay, playerState, @@ -441,6 +444,7 @@ export function PassageDetailPlayer(props: DetailPlayerProps) { controlsRef={controlsRef} applyRegionColor={applyRegionColor} onSegmentPlaybackEnd={onSegmentPlaybackEnd} + onSegmentClick={onSegmentClick} blob={audioBlob} initialposition={initialposition} setInitialPosition={setInitialPosition} diff --git a/src/renderer/src/components/WSAudioPlayer.tsx b/src/renderer/src/components/WSAudioPlayer.tsx index 2416fde82..60d14bcab 100644 --- a/src/renderer/src/components/WSAudioPlayer.tsx +++ b/src/renderer/src/components/WSAudioPlayer.tsx @@ -175,6 +175,8 @@ interface IProps { index?: number ) => void; onSegmentPlaybackEnd?: (segment: IRegion) => void; + /** A segment was clicked on the waveform (not selected by the playhead). */ + onSegmentClick?: (segment: IRegion) => void; forceRegionOnly?: boolean; /** When true, user-initiated selection (clicks, prev/next) is ignored. Playhead-driven region-in is not blocked; consumers that must hold the current clause during recording/saving should guard their segment effects separately (e.g. Careful Speech). */ lockSegmentSelection?: boolean; @@ -366,6 +368,7 @@ function WSAudioPlayer(props: IProps) { onRecording, onCurrentSegment, onSegmentPlaybackEnd, + onSegmentClick, forceRegionOnly, lockSegmentSelection, onMarkerClick, @@ -718,7 +721,8 @@ function WSAudioPlayer(props: IProps) { verses, hasSegmentUndo, applyRegionColor, - lockSegmentSelection + lockSegmentSelection, + onSegmentClick ); //because we have to call hooks consistently, call this even if we aren't going to record diff --git a/src/renderer/src/crud/useWaveSurfer.tsx b/src/renderer/src/crud/useWaveSurfer.tsx index d2ed59f92..25680b9f2 100644 --- a/src/renderer/src/crud/useWaveSurfer.tsx +++ b/src/renderer/src/crud/useWaveSurfer.tsx @@ -61,7 +61,9 @@ export function useWaveSurfer( verses?: string, hasSegmentUndo?: boolean, applyRegionColor?: ApplyRegionColor, - lockSegmentSelection?: boolean + lockSegmentSelection?: boolean, + /** A region was clicked, as opposed to selected by the playhead. */ + onSegmentClick?: (region: IRegion) => void ) { const { isMobile } = useMobile(); const [errorReporter] = useGlobal('errorReporter'); @@ -308,7 +310,8 @@ export function useWaveSurfer( hasSegmentUndo, applyRegionColor, lockSegmentSelection, - () => blobAudioRef.current + () => blobAudioRef.current, + onSegmentClick ); const setPlayingx = (value: boolean, regionOnly: boolean) => { diff --git a/src/renderer/src/crud/useWavesurferRegions.tsx b/src/renderer/src/crud/useWavesurferRegions.tsx index 1c48ee072..c86b7ab48 100644 --- a/src/renderer/src/crud/useWavesurferRegions.tsx +++ b/src/renderer/src/crud/useWavesurferRegions.tsx @@ -124,7 +124,12 @@ export function useWaveSurferRegions( hasSegmentUndo?: boolean, applyRegionColor?: ApplyRegionColor, lockSegmentSelection?: boolean, - getDecodedBuffer?: () => AudioBuffer | undefined + getDecodedBuffer?: () => AudioBuffer | undefined, + /** + * A region was clicked. Distinct from onCurrentRegion, which also fires for + * playhead-driven selection: only a deliberate user click reaches this. + */ + onRegionClicked?: (region: IRegion) => void ) { const theme = useTheme(); const wsRef = useRef(ws); @@ -284,6 +289,14 @@ export function useWaveSurferRegions( } else { setCurrentRegion(r); if (!wasCurrentRegion) { + // Only a click that actually changes the selection counts as deliberate + // navigation. Clicking the already-current region is a no-op that must + // not disarm a pending overshoot swallow (see onRegionClicked consumers). + onRegionClicked?.({ + start: r.start, + end: r.end, + label: r.content?.textContent || '', + }); goto(r.start, false, { start: r.start, end: r.end }); e.stopPropagation(); }