diff --git a/CLAUDE.md b/CLAUDE.md index ac199eca..259f4f8a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -558,7 +558,12 @@ There is no visual/screenshot regression testing — see - **Harmonics Mode**: Real-time harmonic calculation and display - **Sidebands Mode**: A pin set with a user-placed origin — the fundamental — with members spread each side of it and labelled by signed offset -- **Doppler Mode**: Speed calculation from f+/f-/f₀ markers +- **Doppler Mode**: Speed calculation from f+/f-/f₀ markers. One curve per gram: + a drag that starts on a marker moves it, and a drag that starts anywhere else + draws a new curve in place of the old one. `isCompleteCurve` in + `utils/doppler.js` is the one rule for "there is a curve" — the renderer, + the mode's capability predicate and storage all read it, so a lone marker + can neither draw, persist nor block a placement ## Active Technologies - Markdown documentation (no code changes) + N/A (documentation-only feature) (154-enrich-docs) diff --git a/docs/Gram-Modes.md b/docs/Gram-Modes.md index 25f1a26b..886919e6 100644 --- a/docs/Gram-Modes.md +++ b/docs/Gram-Modes.md @@ -178,6 +178,10 @@ on a spectrogram. - Click and drag to place the f+ and f− markers in one gesture; the curve previews during the drag. On release, f+ is the later of the two in time. +- There is one curve per gram. A drag that starts clear of the markers draws a + new curve in place of the old one; a drag that starts on a marker moves it. + A press released without moving places nothing and leaves the curve as it + was. - f₀ is placed automatically at the midpoint and can then be dragged independently — dragging f+ or f− leaves it where it is. - An S-curve is drawn between f+ and f−, with vertical extensions clipped to the diff --git a/hygiene-baseline.json b/hygiene-baseline.json index eac8edc5..e4e9c770 100644 --- a/hygiene-baseline.json +++ b/hygiene-baseline.json @@ -38,7 +38,7 @@ "specTypeErrors": 503, "waitForTimeoutOccurrences": 1, "_comment_instanceSurface": "specs/167-structural-refactor Story 5. instanceStateReachIns counts lines containing `instance.state` under src/; instanceFields counts class-field declarations between `export class GramFrame` and its constructor in src/main.js.", - "instanceStateReachIns": 141, + "instanceStateReachIns": 139, "_comment_instanceFields_168": "Raised 11 -> 12 by spec 168: `player` is a fifth grouped sub-object (the audio element, its controller and follow loop share one lifetime), not a flat field returning to the surface.", "instanceFields": 12 } diff --git a/src/core/events.js b/src/core/events.js index c91a40ce..4ab3834b 100644 --- a/src/core/events.js +++ b/src/core/events.js @@ -60,8 +60,7 @@ function handleWheel(instance, event) { */ export function setupEventListeners(instance) { // Every listener is kept as a bound reference so cleanupEventListeners can - // actually remove it (GF-14). Anonymous inline handlers were unremovable, so - // "cleanup" relied entirely on the SVG being dropped from the DOM. + // actually remove it (GF-14); anonymous inline handlers were unremovable. /** @type {Array<{target: EventTarget, type: string, handler: EventListener, options?: AddEventListenerOptions}>} */ const registered = [] @@ -104,9 +103,8 @@ export function setupEventListeners(instance) { handleContextMenu(instance, /** @type {MouseEvent} */ (event)) }) - // Mouse wheel for global zoom (Ctrl+scroll) and horizontal pan (scroll). - // passive:false so the handler can preventDefault() to stop the host page - // from scrolling during a zoom/pan gesture. + // Mouse wheel for global zoom (Ctrl+scroll) and horizontal pan (scroll); + // passive:false so preventDefault() can stop the host page scrolling. listen(instance.ui.svg, 'wheel', (event) => { handleWheel(instance, /** @type {WheelEvent} */ (event)) }, { passive: false }) @@ -129,11 +127,13 @@ export function setupEventListeners(instance) { } }) - // Frequency-rate input UI events removed - the backend frequencyRate is preserved - // Window resize event listen(window, 'resize', instance.viewport._boundHandleResize) + // A release over another window fires neither mouseup nor mouseleave; a drag + // left registered would block every later press in every mode. + listen(window, 'blur', () => cancelActiveDrag(instance)) + instance.interaction._registeredListeners = registered } diff --git a/src/core/storage.js b/src/core/storage.js index 0bc56b53..5a3c494f 100644 --- a/src/core/storage.js +++ b/src/core/storage.js @@ -32,6 +32,7 @@ /// import { normalizeMarkerLabel } from '../utils/markerLabel.js' +import { isCompleteCurve } from '../utils/doppler.js' /** @type {number} */ const SCHEMA_VERSION = 1 @@ -237,9 +238,8 @@ export function buildStorageKey(instanceIndex) { * failed write is worth telling the analyst about: with nothing annotated, * there is nothing to lose yet. * - * Checks all three doppler markers, matching the mode's own - * `hasPersistentFeatures` predicate: an fZero-only state renders, so it must - * also persist rather than deleting the storage key on save (BH-32). + * A doppler curve counts only when complete (`isCompleteCurve`), matching the + * mode's own `hasPersistentFeatures` and what renders (BH-32, corrected). * @param {GramFrameState} state - Current component state * @returns {boolean} True when at least one annotation exists */ @@ -247,7 +247,7 @@ export function hasPersistableAnnotations(state) { const hasMarkers = !!(state.analysis && state.analysis.markers && state.analysis.markers.length > 0) const hasHarmonics = !!(state.harmonics && state.harmonics.harmonicSets && state.harmonics.harmonicSets.length > 0) const hasSidebands = !!(state.sidebands && state.sidebands.sidebandSets && state.sidebands.sidebandSets.length > 0) - const hasDoppler = !!(state.doppler && (state.doppler.fPlus !== null || state.doppler.fMinus !== null || state.doppler.fZero !== null)) + const hasDoppler = isCompleteCurve(state.doppler) return hasMarkers || hasHarmonics || hasSidebands || hasDoppler } diff --git a/src/main.js b/src/main.js index 7c762aaf..af7178c7 100644 --- a/src/main.js +++ b/src/main.js @@ -52,6 +52,7 @@ import { createGramFrameAPI } from './api/GramFrameAPI.js' import { cleanupEventListeners } from './core/events.js' +import { cancelActiveDrag } from './modes/shared/BaseDragHandler.js' import { saveAnnotations, @@ -364,6 +365,8 @@ export class GramFrame { * of the drag record. Writing `state.drag` directly instead left the engine * saying *dragging* while the projection said *idle*, and the next publish * resurrected the stale drag (M4). One place now, not two (issue #268). + * Ends with the engine's own cancellation point, which reaches the region + * and wheel-pan handlers a mode switch could not. * @returns {void} */ _cancelAllDrags() { @@ -372,18 +375,16 @@ export class GramFrame { modeInstance.dragHandler.cancelDrag() } }) + cancelActiveDrag(this) } /** * Clear all annotations from state and storage */ _clearGram() { - // Clearing the gram also cancels a wheel pan, which a mode switch does not: - // the shared helper covers the feature drags both paths cancel. + // Every drag, the wheel pan included: the shared helper ends with the + // engine's own cancellation point, which reaches whichever handler owns it. this._cancelAllDrags() - if (this.interaction._wheelPanHandler) { - this.interaction._wheelPanHandler.cancelDrag() - } // Clear the selection through the selection seam rather than replacing the // selection object: this also re-syncs the style controls, so they stop diff --git a/src/modes/doppler/DopplerMode.js b/src/modes/doppler/DopplerMode.js index 6c93eeea..84dc3b6a 100644 --- a/src/modes/doppler/DopplerMode.js +++ b/src/modes/doppler/DopplerMode.js @@ -3,17 +3,18 @@ import { setLEDValue } from '../../components/LEDDisplay.js' import { updateLEDDisplays } from '../../components/UIComponents.js' import { dispatch, markAnnotationsChanged, recordDopplerDeletion } from '../../core/state.js' // Rendering imports removed - no display element -import { calculateDopplerSpeed, calculateMidpoint, MS_TO_KNOTS } from '../../utils/doppler.js' +import { calculateDopplerSpeed, calculateMidpoint, isCompleteCurve, snapshotCurve, replacedCurveOf, MS_TO_KNOTS } from '../../utils/doppler.js' import { dataToSVG } from '../../utils/coordinates.js' import { BaseDragHandler } from '../shared/BaseDragHandler.js' import { getUniformTolerance, isWithinDataTolerance, findClosestTarget } from '../../utils/tolerance.js' import { IDLE_CURSOR } from '../../utils/cursors.js' +/** @typedef {import('../../utils/doppler.js').DopplerCurveSnapshot} DopplerCurveSnapshot */ // Doppler marker types /** @type {Record} */ const DopplerDraggedMarker = { fPlus: 'fPlus', - fMinus: 'fMinus', + fMinus: 'fMinus', fZero: 'fZero' } @@ -124,7 +125,7 @@ export class DopplerMode extends BaseMode { */ onMarkerDragEnd(target, _position) { if (target && target.kind === 'place') { - this.completeMarkerPlacement() + this.completeMarkerPlacement(replacedCurveOf(target)) } // Nothing else to unwind: the engine clears the drag record itself. } @@ -135,39 +136,26 @@ export class DopplerMode extends BaseMode { * Cancel and end used to share one callback, so a cancelled placement — * mode switch or Escape mid-gesture — *committed* the half-placed f⁺/f⁻ * curve the user thought was discarded (BH-9). A cancelled placement now - * discards the markers it seeded; a cancelled move leaves the marker at its - * last position, like the other modes. + * puts back the curve it was replacing (or clears what it seeded); a + * cancelled move leaves the marker at its last position, like the other modes. * @param {DragTarget} target - Drag target from the engine */ onMarkerDragCancel(target) { if (target && target.kind === 'place') { - const doppler = this.instance.state.doppler - doppler.fPlus = null - doppler.fMinus = null - doppler.fZero = null - doppler.speed = null - doppler.tempFirst = null - doppler.previewEnd = null - this.updateSpeedLED() - this.renderDopplerFeatures() - dispatch(this.instance, { frame: true }) + this.restoreCurve(replacedCurveOf(target)) } } /** - * Resolve what a mousedown in doppler mode starts: moving one of the placed - * markers, or — with nothing placed yet — laying down f+ and dragging out f-. + * Resolve what a mousedown in doppler mode starts: moving the placed marker + * under the pointer, or — anywhere else — laying down f+ and dragging out f-, + * replacing whatever curve there was. Placement used to be offered only while + * *no* marker existed, so once one did every drag clear of it was ignored. * @param {DataCoordinates} position - Position of the mousedown - * @returns {DragTarget|null} A move- or place-kind target + * @returns {DragTarget} A move- or place-kind target */ resolveDopplerDrag(position) { - const doppler = this.instance.state.doppler - - if (doppler.fPlus || doppler.fMinus || doppler.fZero) { - return this.findDopplerMarkerAtPosition(position) - } - - return this.startMarkerPlacement(position) + return this.findDopplerMarkerAtPosition(position) || this.startMarkerPlacement(position) } /** @@ -175,21 +163,27 @@ export class DopplerMode extends BaseMode { * rest of the placement is an ordinary drag with f- following the pointer. * * `tempFirst` and `previewEnd` stay on state.doppler: they are placement - * geometry the renderer needs, not drag bookkeeping (data-model.md §2). + * geometry the renderer needs, not drag bookkeeping (data-model.md §2). The + * curve being replaced rides on the target, so a cancelled or moveless + * placement can put it back. * @param {DataCoordinates} dataCoords - Data coordinates {freq, time} * @returns {DragTarget} A place-kind target */ startMarkerPlacement(dataCoords) { const doppler = this.instance.state.doppler + const replaced = snapshotCurve(doppler) - // Immediately set f+ at the current position + // Immediately set f+ at the current position. The old curve's colour goes + // with it: this is a new curve, and takes the colour currently selected. doppler.fPlus = { time: dataCoords.time, freq: dataCoords.freq } + Object.assign(doppler, { fMinus: null, fZero: null, speed: null, color: null }) // f- will follow the mouse from here doppler.tempFirst = doppler.fPlus doppler.previewEnd = { time: dataCoords.time, freq: dataCoords.freq } // Render initial curve preview + this.updateSpeedLED() this.renderDopplerFeatures() return { @@ -197,19 +191,34 @@ export class DopplerMode extends BaseMode { id: DopplerDraggedMarker.fMinus, type: 'dopplerMarker', position: dataCoords, - data: { markerType: DopplerDraggedMarker.fMinus } + data: { markerType: DopplerDraggedMarker.fMinus, replaced } } } + /** + * Put back the curve a placement was replacing, or with none to put back + * clear what it seeded. The state is what it was before the press either + * way, so nothing is marked changed. + * @param {DopplerCurveSnapshot|null} replaced - The curve to restore, or null + */ + restoreCurve(replaced) { + const empty = { fPlus: null, fMinus: null, fZero: null, speed: null, color: null } + Object.assign(this.instance.state.doppler, replaced || empty, { tempFirst: null, previewEnd: null }) + this.updateSpeedLED() + this.renderDopplerFeatures() + dispatch(this.instance, { frame: true }) + } + /** * Finalise a placement drag: order the markers, derive f₀, and clear the - * placement geometry. + * placement geometry. A release before any movement is a click, not a curve, + * and restores what was there: it used to leave an invisible f+ behind. + * @param {DopplerCurveSnapshot|null} replaced - The curve the placement replaced */ - completeMarkerPlacement() { + completeMarkerPlacement(replaced) { const doppler = this.instance.state.doppler if (!doppler.tempFirst || !doppler.fPlus || !doppler.fMinus) { - doppler.tempFirst = null - doppler.previewEnd = null + this.restoreCurve(replaced) return } @@ -246,7 +255,7 @@ export class DopplerMode extends BaseMode { getGuidanceText() { return { items: [ - { trigger: 'Click & drag', outcome: 'to place f+ and f\u2212 in one gesture; the curve previews during the drag' }, + { trigger: 'Click & drag', outcome: 'to place f+ and f\u2212 in one gesture; the curve previews during the drag. A drag that starts clear of the markers draws a new curve in place of the old one' }, { trigger: 'Drag f+ or f\u2212', outcome: 'to adjust; f\u2080 can be dragged independently' }, { trigger: 'f\u2080 marker', outcome: 'is placed automatically at the midpoint' }, { trigger: 'Right-click', outcome: 'to reset all doppler markers' } @@ -395,14 +404,7 @@ export class DopplerMode extends BaseMode { * Reset doppler-specific state */ resetState() { - const doppler = this.instance.state.doppler - doppler.fPlus = null - doppler.fMinus = null - doppler.fZero = null - doppler.speed = null - doppler.color = null - doppler.tempFirst = null - doppler.previewEnd = null + this.restoreCurve(null) this.dragHandler.reset() // Deleting the curve is an annotation mutation: masked today by the // signature's doppler identity fields, but the mark is the contract (BH-24). @@ -710,11 +712,10 @@ export class DopplerMode extends BaseMode { * Half of the `PersistentFeatureProvider` capability. Lived on * `FeatureRenderer` as `hasDopplerFeatures()` until spec 167 moved it onto * the mode that owns the state it reads. - * @returns {boolean} True if any doppler marker has been placed + * @returns {boolean} True if a complete doppler curve has been placed */ hasPersistentFeatures() { - const doppler = this.instance.state.doppler - return !!(doppler && (doppler.fPlus || doppler.fMinus || doppler.fZero)) + return isCompleteCurve(this.instance.state.doppler) } /** diff --git a/src/utils/doppler.js b/src/utils/doppler.js index e28e40f2..614cf30a 100644 --- a/src/utils/doppler.js +++ b/src/utils/doppler.js @@ -60,3 +60,52 @@ export function calculateDopplerSpeed(fPlus, fMinus, fZero = null, speedOfSound return Math.abs(speed) // Return absolute value for speed } +/** + * Whether all three markers are placed: the one rule for "there is a curve", + * shared by the renderer, the mode's capability predicate and storage. Anything + * less draws nothing, so it must count as nothing too. A lone f+ used to count + * as an annotation and block every later placement. + * @param {DopplerState|null|undefined} doppler - Doppler state + * @returns {boolean} True when f+, f- and f₀ are all set + */ +export function isCompleteCurve(doppler) { + return !!(doppler && doppler.fPlus && doppler.fMinus && doppler.fZero) +} + +/** + * A complete curve, as it stood when a placement began to replace it. + * @typedef {Object} DopplerCurveSnapshot + * @property {DataCoordinates} fPlus - The f+ marker + * @property {DataCoordinates} fMinus - The f- marker + * @property {DataCoordinates} fZero - The f₀ marker + * @property {number|null} speed - The speed it gave, in m/s + * @property {string|null} color - The colour it was drawn in + */ + +/** + * The curve a placement is about to replace, or null when there is not a + * complete one to come back to. + * @param {DopplerState} doppler - Doppler state + * @returns {DopplerCurveSnapshot|null} A copy of the curve, or null + */ +export function snapshotCurve(doppler) { + if (!isCompleteCurve(doppler)) { + return null + } + return { + fPlus: { .../** @type {DataCoordinates} */ (doppler.fPlus) }, + fMinus: { .../** @type {DataCoordinates} */ (doppler.fMinus) }, + fZero: { .../** @type {DataCoordinates} */ (doppler.fZero) }, + speed: doppler.speed, + color: doppler.color + } +} + +/** + * The snapshot a `place` target carries, if any. + * @param {DragTarget} target - Drag target from the engine + * @returns {DopplerCurveSnapshot|null} The replaced curve, or null + */ +export function replacedCurveOf(target) { + return (target.data && target.data.replaced) || null +} diff --git a/tests/doppler-hotspot.spec.js b/tests/doppler-hotspot.spec.js index fa93df18..136291df 100644 --- a/tests/doppler-hotspot.spec.js +++ b/tests/doppler-hotspot.spec.js @@ -90,15 +90,17 @@ test.describe('Doppler marker hotspot', () => { expect(await pressAt(gramFramePage, cx + 7, cy)).toEqual({ active: true, kind: 'move' }) }) - test('a press well clear of every marker grabs nothing', async ({ gramFramePage }) => { + test('a press well clear of every marker grabs none of them', async ({ gramFramePage }) => { const box = await drawCurveAndLocateMarker(gramFramePage) const cx = box.x + box.width / 2 const cy = box.y + box.height / 2 // Far enough out that no marker's region reaches, so the widened tolerance - // has not simply made the whole gram grabbable. + // has not simply made the whole gram grabbable: the press starts a new + // placement (one curve per gram, replaced by a drag clear of the markers), + // not a move of the nearest marker. const drag = await pressAt(gramFramePage, cx + 60, cy + 40) - expect(drag.active).toBe(false) + expect(drag).toEqual({ active: true, kind: 'place' }) }) test('dragging a marker moves it, and moves only it', async ({ gramFramePage }) => { diff --git a/tests/doppler-mode.spec.js b/tests/doppler-mode.spec.js index 10c3cac4..8598cb8e 100644 --- a/tests/doppler-mode.spec.js +++ b/tests/doppler-mode.spec.js @@ -414,4 +414,127 @@ test.describe('Doppler Mode', () => { expect(state.doppler.previewEnd).toBeNull() }) }) + + /** + * One curve per gram. Placement used to be offered only while *no* marker + * existed, so once any one did — visible or not — a drag away from the + * markers was silently ignored and only a right-click got the mode back. + * An analyst who was click-dragging saw "nothing happens". + */ + test.describe('One curve per gram: a drag away from the markers replaces it', () => { + /** + * The curve, without the transient placement geometry. + * @param {GramFrameState} state - Component state + * @returns {Object} The persisted part of the doppler slice + */ + const curveOf = (state) => { + const { fPlus, fMinus, fZero, speed, color } = state.doppler + return { fPlus, fMinus, fZero, speed, color } + } + + test('a drag that starts clear of an existing curve draws a new one in its place', async ({ gramFramePage }) => { + await placeCurve(gramFramePage, [0.2, 0.2], [0.4, 0.4]) + + const rect = await gramRect(gramFramePage) + const start = pointAt(rect, 0.6, 0.6) + const end = pointAt(rect, 0.8, 0.3) + await gramFramePage.page.mouse.move(start.x, start.y) + await gramFramePage.page.mouse.down() + await gramFramePage.page.mouse.move(end.x, end.y, { steps: 5 }) + await gramFramePage.page.mouse.up() + + const expectedMinus = dataAt(0.6, 0.6) + const expectedPlus = dataAt(0.8, 0.3) + await gramFramePage.waitForState((state) => + !!state.doppler.fMinus && Math.abs(state.doppler.fMinus.freq - expectedMinus.freq) < 2) + + const state = await gramFramePage.getState() + const fPlus = /** @type {DataCoordinates} */ (state.doppler.fPlus) + const fMinus = /** @type {DataCoordinates} */ (state.doppler.fMinus) + expect(fPlus.freq).toBeCloseTo(expectedPlus.freq, 0) + expect(fPlus.time).toBeCloseTo(expectedPlus.time, 0) + expect(fMinus.freq).toBeCloseTo(expectedMinus.freq, 0) + expect(fMinus.time).toBeCloseTo(expectedMinus.time, 0) + expect(state.drag.active).toBe(false) + // One curve drawn, not two + await expect(gramFramePage.page.locator('.gram-frame-doppler-curve')).toHaveCount(1) + }) + + test('a press released without moving places nothing', async ({ gramFramePage }) => { + const rect = await gramRect(gramFramePage) + const p = pointAt(rect, 0.5, 0.5) + await gramFramePage.page.mouse.move(p.x, p.y) + await gramFramePage.page.mouse.down() + await gramFramePage.page.mouse.up() + + const state = await gramFramePage.getState() + expect(state.doppler.fPlus).toBeNull() + expect(state.doppler.fMinus).toBeNull() + expect(state.doppler.fZero).toBeNull() + expect(state.drag.active).toBe(false) + await expect(gramFramePage.page.locator('.gram-frame-svg [class*="doppler"]')).toHaveCount(0) + + // ...and the next drag places a curve as if nothing had happened + const placed = await placeCurve(gramFramePage, [0.3, 0.2], [0.6, 0.7]) + expect(placed.doppler.fZero).not.toBeNull() + }) + + test('a press released without moving leaves an existing curve as it was', async ({ gramFramePage }) => { + const before = await placeCurve(gramFramePage, [0.3, 0.2], [0.6, 0.7]) + + const rect = await gramRect(gramFramePage) + const p = pointAt(rect, 0.8, 0.8) + await gramFramePage.page.mouse.move(p.x, p.y) + await gramFramePage.page.mouse.down() + await gramFramePage.page.mouse.up() + + const after = await gramFramePage.getState() + expect(curveOf(after)).toEqual(curveOf(before)) + await expect(gramFramePage.page.locator('.gram-frame-doppler-curve')).toHaveCount(1) + }) + + test('Escape during a replacing drag puts the old curve back', async ({ gramFramePage }) => { + const before = await placeCurve(gramFramePage, [0.3, 0.2], [0.6, 0.7]) + + const rect = await gramRect(gramFramePage) + const start = pointAt(rect, 0.8, 0.8) + const mid = pointAt(rect, 0.9, 0.6) + await gramFramePage.page.mouse.move(start.x, start.y) + await gramFramePage.page.mouse.down() + await gramFramePage.page.mouse.move(mid.x, mid.y, { steps: 5 }) + await gramFramePage.page.keyboard.press('Escape') + await gramFramePage.page.mouse.up() + + const after = await gramFramePage.getState() + expect(after.drag.active).toBe(false) + expect(curveOf(after)).toEqual(curveOf(before)) + }) + + test('an incomplete curve restored from storage does not block placement', async ({ gramFramePage }) => { + await placeCurve(gramFramePage, [0.3, 0.2], [0.6, 0.7]) + + // Damage the saved record to a lone f+ — what a release before any + // movement used to leave behind, and what a record with one bad point + // still restores as. + await gramFramePage.page.evaluate(() => { + const key = 'gramframe::' + window.location.pathname + const record = JSON.parse(sessionStorage.getItem(key) || '{}') + record.doppler.fMinus = null + record.doppler.fZero = null + sessionStorage.setItem(key, JSON.stringify(record)) + }) + await gramFramePage.goto() + await gramFramePage.clickMode('Doppler') + await gramFramePage.waitForImageDimensions() + + const restored = await gramFramePage.getState() + expect(restored.doppler.fPlus).not.toBeNull() + expect(restored.doppler.fMinus).toBeNull() + + const placed = await placeCurve(gramFramePage, [0.6, 0.6], [0.8, 0.3]) + expect(placed.doppler.fMinus).not.toBeNull() + expect(placed.doppler.fZero).not.toBeNull() + await expect(gramFramePage.page.locator('.gram-frame-doppler-curve')).toHaveCount(1) + }) + }) }) diff --git a/tests/region-zoom.spec.js b/tests/region-zoom.spec.js index 7232d3f6..f3cf3f21 100644 --- a/tests/region-zoom.spec.js +++ b/tests/region-zoom.spec.js @@ -357,6 +357,56 @@ test.describe('Feature 170 — Region zoom', () => { }) }) + test.describe('A selection whose release is lost does not wedge the component', () => { + /** + * Start a selection and leave the button down. The pointer stays over the + * gram, so neither mouseup nor mouseleave will arrive on its own. + * @returns {Promise} + */ + async function holdSelection() { + const a = await gfp.imageSVGPoint(0.3, 0.3) + const b = await gfp.imageSVGPoint(0.6, 0.6) + await gfp.shiftDragSVG(a.x, a.y, b.x, b.y, { release: false }) + const during = await gfp.getState() + expect(during.drag).toMatchObject({ active: true, kind: 'region' }) + } + + /** + * Release the held button and Shift, then check the component still takes + * an annotation: a press in Cross Cursor mode places a marker. + * @returns {Promise} + */ + async function expectRecovered() { + await gfp.page.mouse.up() + await gfp.page.keyboard.up('Shift') + await expect(gfp.page.locator(BOX)).toHaveCount(0) + expect((await gfp.getState()).drag.active).toBe(false) + + await gfp.clickMode('Cross Cursor') + const p = await gfp.imageSVGPoint(0.5, 0.5) + const svgBox = await gfp.svg.boundingBox() + await gfp.page.mouse.click((svgBox?.x ?? 0) + p.x, (svgBox?.y ?? 0) + p.y) + await gfp.waitForState((state) => state.analysis.markers.length === 1) + } + + test('losing the window cancels the selection', async () => { + await holdSelection() + await gfp.page.evaluate(() => window.dispatchEvent(new Event('blur'))) + await expectRecovered() + }) + + test('a mode switch cancels the selection', async () => { + await holdSelection() + // Driven through the API rather than the button, because moving the + // pointer to the button would leave the SVG and cancel the drag that way. + await gfp.page.evaluate(() => { + // @ts-ignore - published API + window.GramFrame._getInstances()[0]._switchMode('doppler') + }) + await expectRecovered() + }) + }) + test.describe('US1 supporting — guidance (FR-016)', () => { test('the cross-mode navigation section describes the gesture', async () => { await gfp.showGuidance() diff --git a/tests/storage.spec.js b/tests/storage.spec.js index 4708e0af..cce8cbc2 100644 --- a/tests/storage.spec.js +++ b/tests/storage.spec.js @@ -189,13 +189,19 @@ test.describe('US1: Trainer annotations persist across reloads', () => { await gfp.page.locator('.gram-frame-mode-btn[title="Doppler" i]').click() await waitForPageState(page, (s) => s.mode === 'doppler', 'doppler mode') - // Place two points for doppler curve - await gfp.svg.click({ position: { x: 200, y: 100 } }) - await gfp.svg.click({ position: { x: 200, y: 200 } }) + // Drag out a curve. This used to be two clicks, which only ever "placed" + // the invisible lone f+ a moveless press left behind — the very state a + // click now discards. + const svgBox = await gfp.svg.boundingBox() + if (!svgBox) throw new Error('SVG not laid out') + await page.mouse.move(svgBox.x + 200, svgBox.y + 100) + await page.mouse.down() + await page.mouse.move(svgBox.x + 200, svgBox.y + 200, { steps: 5 }) + await page.mouse.up() await waitForPageState( page, - (s) => s.doppler.fPlus !== null || s.doppler.fMinus !== null, - 'a doppler marker to be placed' + (s) => s.doppler.fPlus !== null && s.doppler.fMinus !== null && s.doppler.fZero !== null, + 'a doppler curve to be placed' ) const stateBefore = await getStateFromPage(page) diff --git a/tests/unit/doppler.test.js b/tests/unit/doppler.test.js index 97461ce6..15db1eb6 100644 --- a/tests/unit/doppler.test.js +++ b/tests/unit/doppler.test.js @@ -1,7 +1,7 @@ import { describe, test, expect } from 'vitest' import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' -import { calculateMidpoint, calculateDopplerSpeed } from '../../src/utils/doppler.js' +import { calculateMidpoint, calculateDopplerSpeed, isCompleteCurve, snapshotCurve, replacedCurveOf } from '../../src/utils/doppler.js' /** * @fileoverview Unit tests for the Doppler speed math (docs/Doppler-Calc.md). @@ -110,3 +110,46 @@ describe('the speed of sound agrees across code, JSDoc and specification (R9-04) .toBeCloseTo(EXPECTED_C / 100, 10) }) }) + +describe('the one rule for "there is a curve"', () => { + const curve = { + fPlus: { time: 20, freq: 101 }, + fMinus: { time: 10, freq: 99 }, + fZero: { time: 15, freq: 100 }, + speed: 15, + color: '#ff0000', + tempFirst: null, + previewEnd: null + } + + test('a curve is complete only with all three markers', () => { + expect(isCompleteCurve(curve)).toBe(true) + for (const key of ['fPlus', 'fMinus', 'fZero']) { + expect(isCompleteCurve({ ...curve, [key]: null }), key).toBe(false) + } + expect(isCompleteCurve(null)).toBe(false) + expect(isCompleteCurve(undefined)).toBe(false) + }) + + test('a snapshot copies a complete curve, markers included', () => { + const snap = snapshotCurve(curve) + expect(snap).toEqual({ + fPlus: curve.fPlus, fMinus: curve.fMinus, fZero: curve.fZero, speed: 15, color: '#ff0000' + }) + // Copies, so a placement that overwrites the live markers cannot reach in + expect(snap && snap.fPlus).not.toBe(curve.fPlus) + }) + + test('an incomplete curve has no snapshot: there is nothing to come back to', () => { + expect(snapshotCurve({ ...curve, fMinus: null })).toBeNull() + }) + + test('the replaced curve rides on the place target, and only there', () => { + const snap = snapshotCurve(curve) + const place = { kind: 'place', id: 'fMinus', type: 'dopplerMarker', position: null, data: { markerType: 'fMinus', replaced: snap } } + expect(replacedCurveOf(/** @type {any} */ (place))).toBe(snap) + const fresh = { ...place, data: { markerType: 'fMinus', replaced: null } } + expect(replacedCurveOf(/** @type {any} */ (fresh))).toBeNull() + expect(replacedCurveOf(/** @type {any} */ ({ ...place, data: null }))).toBeNull() + }) +}) diff --git a/tests/unit/storage-validation.test.js b/tests/unit/storage-validation.test.js index c5e3fed5..c5219794 100644 --- a/tests/unit/storage-validation.test.js +++ b/tests/unit/storage-validation.test.js @@ -252,17 +252,29 @@ describe('isAnnotationExpired clock-skew tolerance (BH-33)', () => { }) }) -describe('hasPersistableAnnotations covers fZero (BH-32)', () => { - it('an fZero-only state is persistable, matching what renders', () => { +describe('hasPersistableAnnotations follows what renders (BH-32)', () => { + it('a complete curve is persistable', () => { const state = /** @type {any} */ ({ analysis: { markers: [] }, harmonics: { harmonicSets: [] }, sidebands: { sidebandSets: [] }, - doppler: { fPlus: null, fMinus: null, fZero: { time: 1, freq: 2 } } + doppler: { fPlus: { time: 3, freq: 4 }, fMinus: { time: 1, freq: 2 }, fZero: { time: 2, freq: 3 } } }) expect(hasPersistableAnnotations(state)).toBe(true) }) + it('a lone marker is not: only a complete curve draws, so only one persists', () => { + for (const key of ['fPlus', 'fMinus', 'fZero']) { + const state = /** @type {any} */ ({ + analysis: { markers: [] }, + harmonics: { harmonicSets: [] }, + sidebands: { sidebandSets: [] }, + doppler: { fPlus: null, fMinus: null, fZero: null, [key]: { time: 1, freq: 2 } } + }) + expect(hasPersistableAnnotations(state), key).toBe(false) + } + }) + it('a sideband set alone is persistable', () => { const state = /** @type {any} */ ({ analysis: { markers: [] },