diff --git a/CHANGELOG.md b/CHANGELOG.md index 891ad0c0..4d84071b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **`EditHistory` now lives in `src/history.js` (R2, step 14).** 121 lines out of + `src/main.js`, which is down to 18,471. The 47 command classes stay put — they + are interleaved with the feature code that constructs them and each reaches + deep into it; only the stack lifts cleanly. Commands were always duck-typed + (`exec`, `rollback`, and the three opt-out flags), so nothing about them had to + change. + `history.js` imports `S`/`bumpEditGen` from `state.js` and the view predicates + from `keys.js`. Its remaining three main.js symbols — `_historyEnsureArr`, + `draw`, `updateStatus` — cannot be imported back without closing a cycle, so + they arrive through `setHistoryHooks()`, the same shape as `canvas.js`'s + `setCanvas()` and `geometry.js`'s `setLaneMetrics()`. The three duplicated + read-only-roll checks in `exec`/`doUndo`/`doRedo` collapse into one `_locked()`. + Thirteen suites used to slice the class out of `main.js` and hand it a + fabricated `S`, with `_rollReadOnly` stubbed to a boolean. They now import the + real class, seed the real `S`, and drive the real lock through real view state + (`tests/_history_env.mjs`). That turned up a stubbed lie: a keys-DATA part in + the roll had been forced read-only in `roll_position_cycle`, when a keys part + is never read-only. Six of them were CJS and are now `.mjs`. + ### Fixed - **Opening a song from the library card or the 3D highway left an invisible diff --git a/src/history.js b/src/history.js new file mode 100644 index 00000000..8ab5927f --- /dev/null +++ b/src/history.js @@ -0,0 +1,132 @@ +// ════════════════════════════════════════════════════════════════════ +// Undo / redo stack. +// +// The 47 command classes still live in src/main.js — they are interleaved with +// the feature code that constructs them, and each reaches deep into it. Only +// the stack itself lifts cleanly. Every command is duck-typed: `exec()`, +// `rollback()`, and the three opt-out flags this file reads (`songScope`, +// `pitchPreserving`, `suggestResolved`). +// +// Browser surface: `document.getElementById` in _ui(), for the toolbar's +// undo/redo buttons. +// +// main.js coupling is three symbols — `_historyEnsureArr`, `draw`, +// `updateStatus` — and importing them would close a cycle (main.js imports this +// module). They arrive through setHistoryHooks() instead, the same shape as +// canvas.js's setCanvas() and geometry.js's setLaneMetrics(). +// ════════════════════════════════════════════════════════════════════ +import { S, bumpEditGen } from './state.js'; +import { isKeysMode, updatePianoRange, _rollReadOnly, _rollLockNotice } from './keys.js'; + +// Cap the undo stack so a marathon session can't grow memory without bound — +// the stack held every command since the last save/load. Oldest entries drop +// first; 500 comfortably exceeds any realistic between-saves editing run. +export const MAX_UNDO = 500; + +// Defaults keep the class usable before main.js wires it up (and in tests that +// only exercise the stack): ensureArr never refuses, the UI callbacks no-op. +const _hooks = { + ensureArr: () => true, + draw: () => {}, + updateStatus: () => {}, +}; + +export function setHistoryHooks(hooks) { Object.assign(_hooks, hooks); } + +// A NOTE-scope command is refused while a fretted part is shown in the +// read-only piano roll (V4). Three carve-outs pass: +// songScope — edits song-level data (drum tab, tempo grid), not the +// fretted chart, so an unrelated part being in the roll +// must not freeze tempo/drum editing. +// pitchPreserving — the VA.5 position cycle and sustain resize can never +// change what a note SOUNDS like, only which string/fret +// plays it (or for how long), so the "no silent pitch +// writes" contract the lock protects is unbreakable here +// by construction. +// suggestResolved — the VA.3 suggest-position writer (resolved adds + +// Accept) IS the sanctioned string/fret write path the +// lock was holding the door for. It marks, never guesses. +// Nothing else opts out. Returns true when the command must not run. +function _locked(cmd) { + if (cmd.songScope === true || cmd.pitchPreserving === true || cmd.suggestResolved === true) return false; + if (!_rollReadOnly()) return false; + _rollLockNotice(); + return true; +} + +export class EditHistory { + constructor() { this.undo = []; this.redo = []; } + + exec(cmd) { + if (_locked(cmd)) return; + // Tag each command with the arrangement it was executed against: most + // commands resolve their target through the notes()/chords() accessors + // at rollback time, so an undo issued after switching arrangements + // would silently mutate the WRONG arrangement's notes. + cmd._arrIdx = (cmd.songScope === true) ? -1 : (S.currentArr ?? -1); + cmd.exec(); + this.undo.push(cmd); + if (this.undo.length > MAX_UNDO) this.undo.shift(); + this.redo = []; + this._afterEdit(); + this._ui(); + } + + doUndo() { + if (!this.undo.length) return; + const c = this.undo[this.undo.length - 1]; + // Peek-then-pop: if the command belongs to another arrangement, + // ensureArr switches to it (or refuses when it's gone) BEFORE the + // command leaves the stack, so a refused undo loses nothing. + if (!_hooks.ensureArr(c)) return; + // Rolling a NOTE-scope command back would write the fretted chart shown + // read-only in the roll, bypassing the exec/drag lock. Refuse — peek + // only, so the command stays on the stack. ensureArr above has already + // switched to the command's arrangement, so this evaluates against the + // part the rollback would actually touch. + if (_locked(c)) return; + this.undo.pop(); c.rollback(); this.redo.push(c); + this._afterEdit(); this._ui(); _hooks.draw(); _hooks.updateStatus(); + } + + doRedo() { + if (!this.redo.length) return; + const c = this.redo[this.redo.length - 1]; + if (!_hooks.ensureArr(c)) return; + // Re-exec of a NOTE-scope command writes the read-only chart: same lock. + if (_locked(c)) return; + this.redo.pop(); c.exec(); this.undo.push(c); + // Re-apply the MAX_UNDO cap: a redo pushes back onto the undo stack, so + // without this a redo-heavy session could grow it past the bound that + // exec()/doUndo already enforce. Oldest drops first, mirroring exec(). + if (this.undo.length > MAX_UNDO) this.undo.shift(); + this._afterEdit(); this._ui(); _hooks.draw(); _hooks.updateStatus(); + } + + // #18: drop the whole stack when the model is rebuilt under us (the save / + // build flatten+reconstructChords round-trip renumbers arr.notes, so every + // index-based command would now roll back into the wrong note). Reuse the + // live instance + its _ui() wiring rather than reassigning S.history. + // Not _afterEdit() — that nudges the piano viewport, which a clear shouldn't. + reset() { this.undo = []; this.redo = []; this._ui(); } + + _afterEdit() { + // Bump the shared edit generation: the section-coverage, chord-display + // and drum-lint memos all key on it. An in-place note-time move keeps + // the notes array's identity and length, so their cheap cache keys + // can't see it — this bump is what forces a recompute. + bumpEditGen(); + // Keep the keys viewport in sync with the current note range so + // multi-octave authoring works without manual range control. + // expandOnly=true so adding a note outside the current viewport + // extends it instead of collapsing to the latest note's octave. + if (isKeysMode()) updatePianoRange(true); + } + + _ui() { + const u = document.getElementById('editor-undo'); + const r = document.getElementById('editor-redo'); + if (u) u.disabled = !this.undo.length; + if (r) r.disabled = !this.redo.length; + } +} diff --git a/src/main.js b/src/main.js index 382e2783..81d506ae 100644 --- a/src/main.js +++ b/src/main.js @@ -21,6 +21,7 @@ import { } from './position.js'; import { setStatus } from './ui.js'; import { hitNote, hitNoteEdge } from './hit-test.js'; +import { EditHistory, setHistoryHooks } from './history.js'; import { _editorCommandById, _editorEffectiveRightClickBehaviorPure, @@ -49,7 +50,7 @@ import { drawSelectionRect, editorKeyHighlightEnabled, } from './draw.js'; -import { S, bumpEditGen, editGen } from './state.js'; +import { S, editGen } from './state.js'; import { LC, _openMidiForArr, @@ -1149,125 +1150,6 @@ function _refreshKeyControls() { // Undo / Redo // ════════════════════════════════════════════════════════════════════ -/* @pure:edit-history:start */ -// Cap the undo stack so a marathon session can't grow memory without bound — -// the stack held every command since the last save/load. Oldest entries drop -// first; 500 comfortably exceeds any realistic between-saves editing run. -const MAX_UNDO = 500; -class EditHistory { - constructor() { this.undo = []; this.redo = []; } - // Tag each command with the arrangement it was executed against: most - // commands resolve their target through the notes()/chords() accessors at - // rollback time, so an undo issued after switching arrangements would - // silently mutate the WRONG arrangement's notes. Commands that edit - // song-level state (drum tab, tempo grid) opt out via `songScope = true`. - // The `typeof` guards keep this block browser-free for the test harness. - exec(cmd) { - // Read-only roll (V4): a FRETTED part shown in the piano roll is - // edit-locked until the suggest-position writer exists, so every - // NOTE-scope command entry point (keys, menus, inspector, dialogs) - // is inert here in one place. Song-scope commands (drum tab, tempo - // grid) edit song-level data, not the fretted chart, so they still - // pass through even while a fretted part is shown in the roll — - // otherwise switching an unrelated part to the roll would freeze - // tempo/drum editing. typeof-guarded so extracted-test envs without - // the view layer are unaffected. `pitchPreserving` commands (the - // VA.5 position cycle) are the one deliberate carve-out: they can - // never change what a note SOUNDS like — only which string/fret - // plays it — so the "no silent pitch writes" contract the lock - // protects is unbreakable by construction. `suggestResolved` commands - // (the VA.3 suggest-position writer: resolved adds + Accept) are the - // OTHER carve-out — they ARE the sanctioned string/fret write path the - // lock was holding the door for, so they pass here (and mark, never - // guess). Nothing else opts out. - if (cmd.songScope !== true && cmd.pitchPreserving !== true && cmd.suggestResolved !== true - && typeof _rollReadOnly === 'function' && _rollReadOnly()) { - if (typeof _rollLockNotice === 'function') _rollLockNotice(); - return; - } - cmd._arrIdx = (cmd.songScope === true) ? -1 - : (typeof S !== 'undefined' ? (S.currentArr ?? -1) : -1); - cmd.exec(); - this.undo.push(cmd); - if (this.undo.length > MAX_UNDO) this.undo.shift(); - this.redo = []; - this._afterEdit(); - this._ui(); - } - doUndo() { - if (!this.undo.length) return; - const c = this.undo[this.undo.length - 1]; - // Peek-then-pop: if the command belongs to another arrangement, - // _historyEnsureArr switches to it (or refuses when it's gone) BEFORE - // the command leaves the stack, so a refused undo loses nothing. - if (typeof _historyEnsureArr === 'function' && !_historyEnsureArr(c)) return; - // Read-only roll (V4): rolling a NOTE-scope command back would write - // the fretted chart currently shown read-only in the piano roll, so - // undo would silently bypass the exec/drag lock. Refuse (peek only — - // the command stays on the stack, nothing is lost). _historyEnsureArr - // above has already switched to the command's arrangement, so this - // evaluates read-only against the part the rollback would touch. - // Song-scope commands (drum/tempo) don't touch the fretted chart and - // undo normally; pitchPreserving commands (VA.5 position cycle) - // can't change pitch and round-trip freely (see exec); suggestResolved - // commands (VA.3 suggest-position writer) are the sanctioned string/fret - // write path and round-trip too. - if (c.songScope !== true && c.pitchPreserving !== true && c.suggestResolved !== true - && typeof _rollReadOnly === 'function' && _rollReadOnly()) { - if (typeof _rollLockNotice === 'function') _rollLockNotice(); - return; - } - this.undo.pop(); c.rollback(); this.redo.push(c); - this._afterEdit(); this._ui(); draw(); updateStatus(); - } - doRedo() { - if (!this.redo.length) return; - const c = this.redo[this.redo.length - 1]; - if (typeof _historyEnsureArr === 'function' && !_historyEnsureArr(c)) return; - // Read-only roll (V4): re-exec of a NOTE-scope command writes the - // fretted chart that is read-only in the roll — same lock (and same - // pitchPreserving / suggestResolved carve-outs) as doUndo. - if (c.songScope !== true && c.pitchPreserving !== true && c.suggestResolved !== true - && typeof _rollReadOnly === 'function' && _rollReadOnly()) { - if (typeof _rollLockNotice === 'function') _rollLockNotice(); - return; - } - this.redo.pop(); c.exec(); this.undo.push(c); - // Re-apply the MAX_UNDO cap: a redo pushes back onto the undo stack, so - // without this a redo-heavy session could grow it past the bound that - // exec()/doUndo already enforce. Oldest drops first, mirroring exec(). - if (this.undo.length > MAX_UNDO) this.undo.shift(); - this._afterEdit(); this._ui(); draw(); updateStatus(); - } - // #18: drop the whole stack when the model is rebuilt under us (the save / - // build flatten+reconstructChords round-trip renumbers arr.notes, so every - // index-based command would now roll back into the wrong note). Reuse the - // live instance + its _ui() wiring rather than reassigning S.history. - // Not _afterEdit() — that nudges the piano viewport, which a clear shouldn't. - reset() { this.undo = []; this.redo = []; this._ui(); } - _afterEdit() { - // Invalidate the section-coverage memo: an in-place note-time move - // keeps the notes array's identity + length, so the cheap cache key - // can't see it — this generation bump is what forces a recompute. - // typeof-guarded (like isKeysMode below): declared outside this @pure block. - // Bump the shared edit generation: the section-coverage, chord-display and - // drum-lint memos all key on it. typeof-guarded (like isKeysMode below): - // declared outside this @pure block, so a sliced sandbox may not have it. - if (typeof bumpEditGen === 'function') bumpEditGen(); - // Keep the keys viewport in sync with the current note range so - // multi-octave authoring works without manual range control. - // expandOnly=true so adding a note outside the current viewport - // extends it instead of collapsing to the latest note's octave. - if (typeof isKeysMode === 'function' && isKeysMode()) updatePianoRange(true); - } - _ui() { - const u = document.getElementById('editor-undo'); - const r = document.getElementById('editor-redo'); - if (u) u.disabled = !this.undo.length; - if (r) r.disabled = !this.redo.length; - } -} -/* @pure:edit-history:end */ // Guard: true only while _historyEnsureArr drives an arrangement switch to // replay an undo/redo. editorSelectArrangement reads it to distinguish a @@ -1304,6 +1186,11 @@ function _historyEnsureArr(cmd) { return true; } +// src/history.js cannot import these three back out of main.js without closing +// a cycle. All are hoisted function declarations, so this top-level call is +// safe wherever it sits. +setHistoryHooks({ ensureArr: _historyEnsureArr, draw, updateStatus }); + class MoveNoteCmd { constructor(indices, dtimes, dstrings, dfrets) { this.indices = indices; diff --git a/tests/_history_env.mjs b/tests/_history_env.mjs new file mode 100644 index 00000000..400245ed --- /dev/null +++ b/tests/_history_env.mjs @@ -0,0 +1,103 @@ +/* + * Shared environment for the suites that exercise src/history.js. + * + * Not a test file — `node --test` globs `*.test.{js,mjs}`, so this is skipped. + * + * EditHistory used to be sliced out of src/main.js and eval'd with a fabricated + * `S` and hand-stubbed `_rollReadOnly` / `_rollLockNotice`. Now it is a real + * import that closes over the REAL `S` from src/state.js and the REAL view + * predicates from src/keys.js, so a suite that fabricates its own `S` would go + * green while testing an object the history never touches. + * + * Two consequences, both handled here: + * + * 1. Seed the real `S` (Object.assign, never reassign — importers hold the + * same object). + * 2. Drive the roll lock through real state instead of stubbing the predicate. + * `_rollReadOnly()` is `isKeysMode() && !isKeysArr()`: a FRETTED part + * (name not matching KEYS_PATTERN) shown in the roll. `_viewPrefs()` + * returns its live cache object, so writing the part key into it is enough + * — no localStorage. The cache is memoised on `S.filename`, which is why + * seedState() always sets the same one. + * + * `EditHistory._ui()` and `setStatus()` reach for `document`; the stub below + * stands in for the toolbar buttons and the status line. + */ +import { S } from '../src/state.js'; +import { _partViewKeyPure, _viewPrefs } from '../src/keys.js'; +import { setHistoryHooks } from '../src/history.js'; + +const SONG = 'history-test.sloppak'; + +const _els = {}; + +// setStatus() writes textContent; log every write so a suite can COUNT lock +// notices (the old sandboxes injected a counting _rollLockNotice stub). +const _statusLog = []; +export const statusEl = { + _v: '', + get textContent() { return this._v; }, + set textContent(v) { this._v = v; _statusLog.push(v); }, +}; + +if (typeof globalThis.document === 'undefined') { + globalThis.document = { + getElementById(id) { + if (id === 'editor-status') return statusEl; + return (_els[id] ||= { id, disabled: false, value: '' }); + }, + }; +} + +export const undoBtn = () => document.getElementById('editor-undo'); +export const redoBtn = () => document.getElementById('editor-redo'); + +/** Messages passed to setStatus() — how a suite observes _rollLockNotice(). + * `statusMessages` is the LIVE array, so a suite can hold it and read it later. */ +export const statusMessages = _statusLog; +export const lastStatus = () => statusEl._v; +export const statusLog = () => _statusLog.slice(); +export const lockNotices = () => _statusLog.filter(m => /read-only/.test(m)).length; +export const clearStatus = () => { _statusLog.length = 0; statusEl._v = ''; }; + +/** + * Seed the real `S`. `rollView: true` puts every arrangement in the piano roll; + * combined with a fretted part name that is exactly the read-only lock. + */ +export function seedState({ arrangements = [], currentArr = 0, rollView = false, ...rest } = {}) { + Object.assign(S, { + filename: SONG, + arrangements, + currentArr, + sel: new Set(), + ...rest, + }); + setRollView(rollView); + clearStatus(); + return S; +} + +/** Move every arrangement in/out of the piano roll, live — the view prefs cache + * is the same object viewFor() reads, so mutating it flips isKeysMode(). */ +export function setRollView(on) { + const prefs = _viewPrefs(); + for (const k of Object.keys(prefs)) delete prefs[k]; + if (on) for (const a of S.arrangements) if (a) prefs[_partViewKeyPure(a)] = 'piano'; +} + +/** + * Install counting stand-ins for the three main.js symbols history.js cannot + * import back without closing a cycle. Pass `ensureArr` to model a refusal. + */ +export function trackHooks({ ensureArr } = {}) { + const calls = { draw: 0, updateStatus: 0, ensureArr: [] }; + setHistoryHooks({ + draw: () => { calls.draw++; }, + updateStatus: () => { calls.updateStatus++; }, + ensureArr: (cmd) => { + calls.ensureArr.push(cmd); + return ensureArr ? ensureArr(cmd) : true; + }, + }); + return calls; +} diff --git a/tests/cross_arr_undo.test.js b/tests/cross_arr_undo.test.mjs similarity index 89% rename from tests/cross_arr_undo.test.js rename to tests/cross_arr_undo.test.mjs index 1fa66de6..741b3f32 100644 --- a/tests/cross_arr_undo.test.js +++ b/tests/cross_arr_undo.test.mjs @@ -28,29 +28,24 @@ * what makes this test FAIL on pre-fix code (no history reset -> the corrupting * cross-arrangement rollback runs) and PASS on the fixed code. * - * Run: node tests/cross_arr_undo.test.js + * Run: node tests/cross_arr_undo.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory, setHistoryHooks } from '../src/history.js'; +import { seedState } from './_history_env.mjs'; -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); +const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); -function extractBlock(name) { - const re = new RegExp( - '/\\* @pure:' + name + ':start \\*/[\\s\\S]*?/\\* @pure:' + name + ':end \\*/'); - const m = src.match(re); - if (!m) { console.error(`FAIL: @pure:${name} block not found`); process.exit(1); } - return m[0]; -} function extractRe(re, label) { const m = src.match(re); if (!m) { console.error(`FAIL: could not extract ${label} from src/main.js`); process.exit(1); } return m[0]; } -const historyBlock = extractBlock('edit-history'); -// Real _undoDrivenArrSwitch flag + _historyEnsureArr (outside any pure block). +// Real _undoDrivenArrSwitch flag + _historyEnsureArr. EditHistory is a real +// import now, so _historyEnsureArr reaches it as a hook — which is exactly how +// main.js wires the two together (they cannot import each other: cycle). const ensureArr = extractRe( /let _undoDrivenArrSwitch = false;[\s\S]*?\nfunction _historyEnsureArr\(cmd\) \{[\s\S]*?\n\}/, '_historyEnsureArr'); @@ -63,15 +58,14 @@ const selectArr = extractRe( // final `arr.notes.sort(...)` — the exact re-sort that renumbers index-based // note commands. notes() returns the active arrangement's notes array. function makeEnv() { - const S = { + const S = seedState({ arrangements: [], currentArr: 0, - sel: new Set(), toneSel: null, anchorSel: null, handshapeSel: null, history: null, - }; + }); const win = {}; const flattenChords = () => { const arr = S.arrangements[S.currentArr]; @@ -82,8 +76,8 @@ function makeEnv() { 'window', 'document', 'S', 'flattenChords', 'isKeysMode', 'updatePianoRange', 'draw', 'updateStatus', 'setStatus', 'notes', '"use strict";' - + historyBlock + '\n' + ensureArr + '\n' + moveNoteCmd + '\n' + selectArr + '\n' - + 'return { EditHistory, _historyEnsureArr, MoveNoteCmd };' + + ensureArr + '\n' + moveNoteCmd + '\n' + selectArr + '\n' + + 'return { _historyEnsureArr, MoveNoteCmd };' )( win, { getElementById: () => null }, @@ -96,7 +90,8 @@ function makeEnv() { () => {}, notes, ); - S.history = new env.EditHistory(); + setHistoryHooks({ ensureArr: env._historyEnsureArr, draw: () => {}, updateStatus: () => {} }); + S.history = new EditHistory(); return { ...env, S, win, history: S.history, flattenChords }; } diff --git a/tests/drum_undo.test.js b/tests/drum_undo.test.mjs similarity index 93% rename from tests/drum_undo.test.js rename to tests/drum_undo.test.mjs index 794f6372..2acc06bd 100644 --- a/tests/drum_undo.test.js +++ b/tests/drum_undo.test.mjs @@ -21,13 +21,14 @@ * `@pure:drum-cmds` + `@pure:edit-history` blocks (browser-free) and eval's * them in isolation — real source, no drift. * - * Run: node tests/drum_undo.test.js + * Run: node tests/drum_undo.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); +const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); function extract(name) { const re = new RegExp( @@ -41,7 +42,6 @@ function extract(name) { } const drumBlock = extract('drum-cmds'); -const historyBlock = extract('edit-history'); // Build an isolated environment. The drum commands read S.drumTab/S.drumSel // and call updateArrangementSelector() (a DOM refresher — stubbed to a call @@ -50,27 +50,27 @@ const historyBlock = extract('edit-history'); // the tagging observable. _historyEnsureArr lives OUTSIDE the pure block on // purpose (it touches window/document); the typeof guard skips it here. function makeEnv() { - const S = { + // EditHistory is a real import and closes over the REAL `S`, so the sliced + // drum commands must share that object rather than a fabricated one. + const S = seedState({ drumTab: { hits: [] }, drumSel: new Set(), drumTabDirty: false, currentArr: 0, - }; + }); const calls = { selector: 0 }; const env = new Function( - 'document', 'S', 'updateArrangementSelector', 'draw', 'updateStatus', + 'S', 'updateArrangementSelector', '"use strict";' - + historyBlock + '\n' + drumBlock + '\n' - + 'return { EditHistory, AddDrumHitCmd, DeleteDrumHitsCmd, ' + + drumBlock + '\n' + + 'return { AddDrumHitCmd, DeleteDrumHitsCmd, ' + 'MoveDrumHitsCmd, ToggleDrumArticulationCmd, _drumSortAndRemapSel };' )( - { getElementById: () => null }, S, () => { calls.selector++; }, - () => {}, - () => {}, ); - return { ...env, S, calls, history: new env.EditHistory() }; + trackHooks(); + return { ...env, S, calls, history: new EditHistory() }; } let pass = 0, fail = 0; diff --git a/tests/drum_velocity.test.js b/tests/drum_velocity.test.mjs similarity index 93% rename from tests/drum_velocity.test.js rename to tests/drum_velocity.test.mjs index ce593619..d4c1ac6c 100644 --- a/tests/drum_velocity.test.js +++ b/tests/drum_velocity.test.mjs @@ -9,13 +9,14 @@ * that wasn't there. These fail on main, where drum velocity is * unauthorable and ghosting leaves a contradictory v:100. * - * Run: node tests/drum_velocity.test.js + * Run: node tests/drum_velocity.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); +const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); function extract(name) { const re = new RegExp( @@ -29,30 +30,29 @@ function extract(name) { } const drumBlock = extract('drum-cmds'); -const historyBlock = extract('edit-history'); function makeEnv() { - const S = { + // EditHistory is a real import and closes over the REAL `S`, so the sliced + // drum commands must share that object rather than a fabricated one. + const S = seedState({ drumTab: { hits: [] }, drumSel: new Set(), drumTabDirty: false, currentArr: 0, - }; + }); const env = new Function( - 'document', 'S', 'updateArrangementSelector', 'draw', 'updateStatus', + 'S', 'updateArrangementSelector', '"use strict";' - + historyBlock + '\n' + drumBlock + '\n' - + 'return { EditHistory, SetDrumVelocityCmd, ToggleDrumArticulationCmd, ' + + drumBlock + '\n' + + 'return { SetDrumVelocityCmd, ToggleDrumArticulationCmd, ' + 'DRUM_GHOST_VELOCITY, _drumClampVelocityPure, _drumVelocityDragValuePure, ' + '_drumImportHitPure };' )( - { getElementById: () => ({ disabled: false }) }, S, () => {}, - () => {}, - () => {}, ); - return { ...env, S, history: new env.EditHistory() }; + trackHooks(); + return { ...env, S, history: new EditHistory() }; } let pass = 0, fail = 0; diff --git a/tests/duplicate_selection.test.js b/tests/duplicate_selection.test.mjs similarity index 90% rename from tests/duplicate_selection.test.js rename to tests/duplicate_selection.test.mjs index e6aced45..9ac64dff 100644 --- a/tests/duplicate_selection.test.js +++ b/tests/duplicate_selection.test.mjs @@ -14,13 +14,14 @@ * NaN, invalid snap step) and proven to use BOTH its arguments. These * assertions fail on main (neither symbol exists there). * - * Run: node tests/duplicate_selection.test.js + * Run: node tests/duplicate_selection.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); +const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); function extract(name) { const re = new RegExp( '/\\* @pure:' + name + ':start \\*/[\\s\\S]*?/\\* @pure:' + name + ':end \\*/'); @@ -29,16 +30,16 @@ function extract(name) { return m[0]; } -// EditHistory.doUndo/doRedo call draw() + updateStatus() at the end and -// _ui() reads two buttons — inject no-ops / a stub document so the REAL -// history drives without a browser. +// EditHistory is a real import now; it closes over the real `S` and calls back +// into main.js through hooks. Seed one and install counting no-ops. +seedState(); +trackHooks(); const api = new Function( - 'document', 'draw', 'updateStatus', '"use strict";' - + extract('edit-history') + '\n' + extract('duplicate') + '\n' - + 'return { EditHistory, AddNotesCmd, _duplicateShiftPure };' -)({ getElementById: () => ({ disabled: false }) }, () => {}, () => {}); -const { EditHistory, AddNotesCmd, _duplicateShiftPure } = api; + + extract('duplicate') + '\n' + + 'return { AddNotesCmd, _duplicateShiftPure };' +)(); +const { AddNotesCmd, _duplicateShiftPure } = api; const clone = (x) => JSON.parse(JSON.stringify(x)); diff --git a/tests/edit_history_reset.test.js b/tests/edit_history_reset.test.mjs similarity index 55% rename from tests/edit_history_reset.test.js rename to tests/edit_history_reset.test.mjs index 5bfafe5e..8108b9c5 100644 --- a/tests/edit_history_reset.test.js +++ b/tests/edit_history_reset.test.mjs @@ -1,4 +1,3 @@ -'use strict'; /* * Tests for EditHistory.reset() (feedback-plugin-editor#18). * @@ -8,7 +7,7 @@ * renumbering the array. The undo history was reset only on song load, so undoing * a pre-save command indexed into the wrong note (or undefined). Fix: reset the * undo/redo stacks in the save/build `finally` blocks (runs even if the POST - * fails, since the model is rebuilt regardless). This unit-tests the new + * fails, since the model is rebuilt regardless). This unit-tests the * EditHistory.reset() that those call sites use. * * Manual repro verified (full headless save round-trip not driven in CI): @@ -17,37 +16,11 @@ * 3. Undo -> previously mutated the wrong note / threw; now a no-op because the * stack was reset by the save's `finally`. * - * src/main.js is a single browser IIFE, so this extracts the `@pure:edit-history` - * marked block (browser-free) and eval's it in isolation — real source, no drift. - * - * Run: node tests/edit_history_reset.test.js + * Run: node tests/edit_history_reset.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); - -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); -const m = src.match(/\/\* @pure:edit-history:start \*\/[\s\S]*?\/\* @pure:edit-history:end \*\//); -if (!m) { - console.error('FAIL: @pure:edit-history block not found in src/main.js'); - process.exit(1); -} - -// _ui() reads document.getElementById('editor-undo'/'editor-redo'); stub it so -// the class is browser-free. Each call returns the same fake button objects so -// the test can assert their `.disabled` state after reset(). -function makeHistory() { - const buttons = { - 'editor-undo': { disabled: false }, - 'editor-redo': { disabled: false }, - }; - const documentStub = { getElementById: (id) => buttons[id] || null }; - const { EditHistory } = new Function( - 'document', - '"use strict";' + m[0] + '\nreturn { EditHistory };' - )(documentStub); - return { history: new EditHistory(), buttons }; -} +import assert from 'node:assert'; +import { EditHistory } from '../src/history.js'; +import { redoBtn, undoBtn } from './_history_env.mjs'; let pass = 0, fail = 0; function t(name, fn) { @@ -55,9 +28,16 @@ function t(name, fn) { catch (e) { fail++; console.error(' FAIL ' + name + ': ' + e.message); } } +function makeHistory() { + const history = new EditHistory(); + undoBtn().disabled = false; + redoBtn().disabled = false; + return history; +} + // ── reset() empties both stacks ────────────────────────────────────────────── t('reset() empties undo and redo stacks', () => { - const { history } = makeHistory(); + const history = makeHistory(); // Simulate post-edit state: index-based commands captured before a save. history.undo.push({ index: 0 }, { index: 1 }); history.redo.push({ index: 2 }); @@ -68,38 +48,40 @@ t('reset() empties undo and redo stacks', () => { // ── reset() refreshes the undo/redo button enabled-state via _ui() ─────────── t('reset() disables both undo and redo buttons (proves _ui ran)', () => { - const { history, buttons } = makeHistory(); + const history = makeHistory(); history.undo.push({ index: 0 }); history.redo.push({ index: 1 }); history._ui(); - assert.strictEqual(buttons['editor-undo'].disabled, false, 'precondition: undo enabled'); - assert.strictEqual(buttons['editor-redo'].disabled, false, 'precondition: redo enabled'); + assert.strictEqual(undoBtn().disabled, false, 'precondition: undo enabled'); + assert.strictEqual(redoBtn().disabled, false, 'precondition: redo enabled'); history.reset(); - assert.strictEqual(buttons['editor-undo'].disabled, true); - assert.strictEqual(buttons['editor-redo'].disabled, true); + assert.strictEqual(undoBtn().disabled, true); + assert.strictEqual(redoBtn().disabled, true); }); // ── reset() is safe / idempotent on an already-empty history ───────────────── t('reset() is a safe no-op on an empty history', () => { - const { history, buttons } = makeHistory(); + const history = makeHistory(); history.reset(); history.reset(); assert.strictEqual(history.undo.length, 0); assert.strictEqual(history.redo.length, 0); - assert.strictEqual(buttons['editor-undo'].disabled, true); - assert.strictEqual(buttons['editor-redo'].disabled, true); + assert.strictEqual(undoBtn().disabled, true); + assert.strictEqual(redoBtn().disabled, true); }); // ── reset() tolerates missing DOM buttons (jsdom-free / pre-mount) ─────────── t('reset() tolerates absent undo/redo buttons', () => { - const { EditHistory } = new Function( - 'document', - '"use strict";' + m[0] + '\nreturn { EditHistory };' - )({ getElementById: () => null }); - const h = new EditHistory(); - h.undo.push({ index: 0 }); - assert.doesNotThrow(() => h.reset()); - assert.strictEqual(h.undo.length, 0); + const real = globalThis.document; + globalThis.document = { getElementById: () => null }; + try { + const h = new EditHistory(); + h.undo.push({ index: 0 }); + assert.doesNotThrow(() => h.reset()); + assert.strictEqual(h.undo.length, 0); + } finally { + globalThis.document = real; + } }); console.log(`\n${pass} passed, ${fail} failed`); diff --git a/tests/inspector_time.test.js b/tests/inspector_time.test.mjs similarity index 89% rename from tests/inspector_time.test.js rename to tests/inspector_time.test.mjs index c23358de..0449f79a 100644 --- a/tests/inspector_time.test.js +++ b/tests/inspector_time.test.mjs @@ -13,13 +13,14 @@ * with adversarial inputs. The time-bounds assertions fail on main (no `time` * key exists there). * - * Run: node tests/inspector_time.test.js + * Run: node tests/inspector_time.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); +const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); // Brace-match extraction of a named class/function/const (the waveform_render // harness pattern) — drives the real source, no re-implementation. @@ -34,28 +35,21 @@ function extractNamed(decl) { } throw new Error(`unbalanced braces for ${decl}`); } -function extractPure(name) { - const re = new RegExp( - '/\\* @pure:' + name + ':start \\*/[\\s\\S]*?/\\* @pure:' + name + ':end \\*/'); - const m = src.match(re); - if (!m) { console.error(`FAIL: @pure:${name} missing`); process.exit(1); } - return m[0]; -} - // Injected `notes()` reads a mutable outer array so each test controls it. let CURRENT = []; +seedState(); +trackHooks(); const api = new Function( - 'notes', 'document', 'draw', 'updateStatus', + 'notes', '"use strict";' - + extractPure('edit-history') + '\n' + extractNamed('class MoveNoteCmd') + '\n' + extractNamed('class ResizeSustainGroupCmd') + '\n' + extractNamed('const _INSPECTOR_BOUNDS =') + '\n' + extractNamed('function _coerceInspectorNumber') + '\n' - + 'return { EditHistory, MoveNoteCmd, ResizeSustainGroupCmd,' + + 'return { MoveNoteCmd, ResizeSustainGroupCmd,' + ' _INSPECTOR_BOUNDS, _coerceInspectorNumber };' -)(() => CURRENT, { getElementById: () => ({ disabled: false }) }, () => {}, () => {}); -const { EditHistory, MoveNoteCmd, ResizeSustainGroupCmd, +)(() => CURRENT); +const { MoveNoteCmd, ResizeSustainGroupCmd, _INSPECTOR_BOUNDS, _coerceInspectorNumber } = api; const clone = (x) => JSON.parse(JSON.stringify(x)); @@ -143,33 +137,33 @@ let DISPATCH_NOTES = []; const dispatchS = { sel: new Set(), drumEditMode: false, tempoMapMode: false, history: null }; let renderCount = 0; // # of _renderInspector() calls (reject branch) const win = {}; // captures `window.editorInspectorSetField = …` -const dispatch = new Function( - 'notes', 'S', 'document', 'draw', 'updateStatus', '_renderInspector', 'window', +// `dispatchS` stays sandbox-local: editorInspectorSetField reads its `sel` and +// mode flags. EditHistory itself closes over the real `S` (seeded above), which +// only supplies the arrangement tag — irrelevant to these cases. +new Function( + 'notes', 'S', 'draw', 'updateStatus', '_renderInspector', 'window', '"use strict";' - + extractPure('edit-history') + '\n' + extractNamed('class MoveNoteCmd') + '\n' + extractNamed('class ResizeSustainGroupCmd') + '\n' + extractNamed('const _INSPECTOR_BOUNDS =') + '\n' + extractNamed('function _coerceInspectorNumber') + '\n' + extractNamed('function _editorCurrentNoteIndices') + '\n' + extractNamed('window.editorInspectorSetField =') + '\n' - + 'return { EditHistory };' + + 'return {};' )( () => DISPATCH_NOTES, dispatchS, - { getElementById: () => ({ disabled: false }) }, - () => {}, () => {}, + () => {}, () => {}, // editorInspectorSetField's own draw/updateStatus () => { renderCount++; }, win, ); const setField = win.editorInspectorSetField; -// Fresh notes + selection + history per case. History built with the dispatcher -// scope's own EditHistory so exec/undo share the injected notes()/draw stubs. +// Fresh notes + selection + history per case. function resetDispatch(arr, sel) { DISPATCH_NOTES = arr; dispatchS.sel = new Set(sel); - dispatchS.history = new dispatch.EditHistory(); + dispatchS.history = new EditHistory(); renderCount = 0; } diff --git a/tests/rename_part.test.mjs b/tests/rename_part.test.mjs index aba0c4b1..c53b7811 100644 --- a/tests/rename_part.test.mjs +++ b/tests/rename_part.test.mjs @@ -11,6 +11,8 @@ * Run: node tests/rename_part.test.mjs */ import assert from 'node:assert'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; import fs from 'node:fs'; import { KEYS_PATTERN } from '../src/keys.js'; @@ -125,24 +127,23 @@ t('empty, too-long, duplicate, and no-op inputs are handled', () => { // ── The real command, round-tripped through EditHistory ────────────── +// EditHistory is a real import and closes over the REAL `S`, so the sliced +// command must share that same object rather than a fabricated one. function makeEnv() { - const S = { - currentArr: 0, - arrangements: [{ id: 'a1', name: 'Lead' }, { id: 'a2', name: 'Rhythm' }], - }; + const S = seedState({ + arrangements: [{ id: 'a1', name: 'Lead', notes: [] }, { id: 'a2', name: 'Rhythm', notes: [] }], + }); const calls = { selector: 0 }; const env = new Function( - 'document', 'S', 'updateArrangementSelector', 'draw', 'updateStatus', - '"use strict";' + extractBlock('edit-history') + '\n' + extractClass('RenameArrangementCmd') - + '\nreturn { EditHistory, RenameArrangementCmd };' + 'S', 'updateArrangementSelector', + '"use strict";' + extractClass('RenameArrangementCmd') + + '\nreturn { RenameArrangementCmd };' )( - { getElementById: () => ({ disabled: false }) }, S, () => { calls.selector++; }, - () => {}, - () => {}, ); - return { ...env, S, calls, history: new env.EditHistory() }; + trackHooks(); + return { ...env, S, calls, history: new EditHistory() }; } t('rename round-trips: exec applies, undo restores, redo replays; selector follows', () => { diff --git a/tests/roll_edge_resize.test.mjs b/tests/roll_edge_resize.test.mjs index 702d0bd4..536b8afb 100644 --- a/tests/roll_edge_resize.test.mjs +++ b/tests/roll_edge_resize.test.mjs @@ -16,20 +16,17 @@ */ import assert from 'node:assert'; import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { _rollReadOnly } from '../src/keys.js'; import { _maxSustainBeforeCollisionPure, _resizeSustainsForDeltaPure, } from '../src/notes.js'; +import { lastStatus, seedState, trackHooks } from './_history_env.mjs'; // The undo commands still live in src/main.js, so they are still sliced; the -// pure resize arithmetic they were paired with now comes from src/notes.js. +// pure resize arithmetic they were paired with now comes from src/notes.js, and +// the stack itself from src/history.js. const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); - -function extractBlock(name) { - const re = new RegExp('/\\* @pure:' + name + ':start[\\s\\S]*?@pure:' + name + ':end \\*/'); - const m = src.match(re); - if (!m) { console.error(`FAIL: @pure:${name} block not found in src/main.js`); process.exit(1); } - return m[0]; -} function extractClass(name) { const start = src.indexOf('class ' + name); assert.ok(start >= 0, `class ${name} must exist in src/main.js`); @@ -48,31 +45,28 @@ function t(name, fn) { catch (e) { fail++; console.error(' FAIL ' + name + ': ' + (e && e.message)); } } -// The locked-roll harness: _rollReadOnly ⇒ true, like a fretted part in the roll. +// The locked-roll harness. EditHistory now imports _rollReadOnly for real, so the +// lock is driven through actual state rather than a stubbed predicate: a part +// named 'Lead' is fretted, and rollView puts it in the piano roll. That IS +// _rollReadOnly() — asserted below, so the harness can't silently unlock. function makeEnv(seed) { - const S = { - currentArr: 0, + const S = seedState({ arrangements: [{ id: 'a1', name: 'Lead', notes: seed.map(n => ({ ...n })) }], - }; - const notices = []; + rollView: true, + }); + assert.ok(_rollReadOnly(), 'harness precondition: the roll must be read-only'); + trackHooks(); const fullSrc = '"use strict";' - + extractBlock('edit-history') - + '\n' + extractClass('ResizeSustainCmd') + + extractClass('ResizeSustainCmd') + '\n' + extractClass('ResizeSustainGroupCmd') - + '\nconst history = new EditHistory(); S.history = history;' - + '\nreturn { history, ResizeSustainCmd, ResizeSustainGroupCmd };'; - const env = new Function( - 'S', 'document', 'notes', 'draw', 'updateStatus', '_rollReadOnly', '_rollLockNotice', - fullSrc - )( + + '\nreturn { ResizeSustainCmd, ResizeSustainGroupCmd };'; + const env = new Function('S', 'notes', fullSrc)( S, - { getElementById: () => null }, () => S.arrangements[S.currentArr].notes, - () => {}, () => {}, - () => true, // read-only fretted roll - () => notices.push('LOCKED'), ); - return { S, env, notices, notes: () => S.arrangements[0].notes }; + env.history = new EditHistory(); + S.history = env.history; + return { S, env, notes: () => S.arrangements[0].notes }; } // ── the flag itself ─────────────────────────────────────────────────────────── @@ -108,13 +102,13 @@ t('a group resize applies in the read-only fretted roll and round-trips', () => // ── the lock is still REAL — only pitchPreserving passes ────────────────────── t('the read-only-roll lock still blocks an ordinary (non-pitchPreserving) command', () => { - const { env, notes, notices } = makeEnv([{ time: 0, string: 3, fret: 1, sustain: 0.5 }]); + const { env, notes } = makeEnv([{ time: 0, string: 3, fret: 1, sustain: 0.5 }]); let ran = false; env.history.exec({ exec() { ran = true; notes()[0].sustain = 9; }, rollback() {} }); // no pitchPreserving assert.strictEqual(ran, false, 'an unflagged command stays inert in the locked roll'); assert.strictEqual(notes()[0].sustain, 0.5, 'nothing was written'); assert.strictEqual(env.history.undo.length, 0); - assert.ok(notices.includes('LOCKED'), 'the user is told why'); + assert.match(lastStatus(), /read-only/, 'the user is told why (real _rollLockNotice)'); }); // ── group resize preserves per-member relative durations (does NOT flatten) ─── diff --git a/tests/roll_position_cycle.test.mjs b/tests/roll_position_cycle.test.mjs index 2d6a2a82..88984605 100644 --- a/tests/roll_position_cycle.test.mjs +++ b/tests/roll_position_cycle.test.mjs @@ -12,6 +12,10 @@ */ import assert from 'node:assert'; import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { _rollLockNotice, _rollReadOnly } from '../src/keys.js'; +import { setStatus } from '../src/ui.js'; +import { seedState, statusMessages, trackHooks } from './_history_env.mjs'; import { _soundingPitchPure } from '../src/lanes.js'; import { _cyclePositionCandidatesPure, _cycleStepPure, @@ -19,16 +23,6 @@ import { const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); -function extractBlock(name) { - const re = new RegExp( - '/\\* @pure:' + name + ':start \\*/[\\s\\S]*?/\\* @pure:' + name + ':end \\*/'); - const m = src.match(re); - if (!m) { - console.error(`FAIL: @pure:${name} block not found in src/main.js`); - process.exit(1); - } - return m[0]; -} function extractFn(name) { const start = src.indexOf('function ' + name); assert.ok(start >= 0, `function ${name} must exist`); @@ -129,16 +123,21 @@ t('single-position and corrupt-current are null (no-op, never a guess)', () => { // ── Driver + lock carve-out round-trip ─────────────────────────────── +// EditHistory imports _rollReadOnly for real, so `locked` is expressed as real +// state: `rollView` puts the part in the piano roll, and a fretted part name +// there IS the read-only condition. A keys-named part in the roll is editable. function makeCycleEnv({ arrName = 'Lead', noteSeed, sel, locked = true } = {}) { - const S = { - currentArr: 0, + const S = seedState({ arrangements: [{ id: 'a1', name: arrName, tuning: [0, 0, 0, 0, 0, 0], notes: noteSeed.map(n => ({ ...n })) }], + rollView: true, sel: new Set(sel), drumEditMode: false, tempoMapMode: false, - }; - const statuses = []; + }); + assert.strictEqual(_rollReadOnly(), locked, 'harness precondition: lock state'); + trackHooks(); + const statuses = statusMessages; // live array of real setStatus() writes // MoveToStringCmd is a class — extractFn targets functions, so pull the // class by brace-matching from its declaration instead. const clsStart = src.indexOf('class MoveToStringCmd'); @@ -152,31 +151,30 @@ function makeCycleEnv({ arrName = 'Lead', noteSeed, sel, locked = true } = {}) { // position-cycle moved to src/position.js — injected below; edit-history and // _execCyclePosition are still in src/main.js and still sliced. const fullSrc = '"use strict";' - + extractBlock('edit-history') - + '\n' + clsSrc + + clsSrc + '\n' + extractFn('_execCyclePosition') - + '\nconst history = new EditHistory(); S.history = history;' - + '\nreturn { _execCyclePosition, history };'; + + '\nreturn { _execCyclePosition };'; const env = new Function( - 'S', 'document', 'notes', 'setStatus', 'draw', 'updateStatus', + 'S', 'history', 'notes', 'setStatus', 'draw', 'updateStatus', '_renderInspector', '_editBlipAt', '_rollReadOnly', '_rollLockNotice', '_editorCurrentNoteIndices', 'isKeysArr', '_stringCountFor', '_openMidiForArr', '_soundingPitchPure', '_cyclePositionCandidatesPure', '_cycleStepPure', fullSrc )( S, - { getElementById: () => ({ disabled: false }) }, + (S.history = new EditHistory()), () => S.arrangements[S.currentArr].notes, - m => statuses.push(m), + setStatus, () => {}, () => {}, () => {}, () => {}, - () => locked, - () => statuses.push('LOCKED'), + _rollReadOnly, + _rollLockNotice, () => (S.sel && S.sel.size ? [...S.sel] : []), () => /^(piano|keys|synth)/i.test(S.arrangements[S.currentArr].name), () => 6, () => STD.slice(), _soundingPitchPure, _cyclePositionCandidatesPure, _cycleStepPure, ); + env.history = S.history; return { S, env, statuses }; } @@ -201,7 +199,7 @@ t('edit-lock still blocks every command WITHOUT the pitchPreserving flag', () => env.history.exec({ exec() { ran++; }, rollback() { ran--; } }); assert.strictEqual(ran, 0, 'unflagged command must stay inert'); assert.strictEqual(env.history.undo.length, 0); - assert.ok(statuses.includes('LOCKED'), 'the user is told why'); + assert.ok(statuses.some(m => /read-only/.test(m)), 'the user is told why (real _rollLockNotice)'); }); t('multi-select cycles each note independently, skipping single-position notes', () => { @@ -240,8 +238,11 @@ t('all-single-position selection is a status no-op, never a history entry', () = }); t('keys DATA and empty selection are guarded no-ops', () => { + // A keys-DATA part in the roll is fully editable — never read-only. The old + // harness stubbed _rollReadOnly() ⇒ true even here; the real predicate says + // false, so state it. const keys = makeCycleEnv({ - arrName: 'Piano', noteSeed: [{ string: 1, fret: 0, time: 0 }], sel: [0] }); + arrName: 'Piano', noteSeed: [{ string: 1, fret: 0, time: 0 }], sel: [0], locked: false }); keys.env._execCyclePosition(+1); assert.strictEqual(keys.env.history.undo.length, 0, 'keys packing has no positions'); const none = makeCycleEnv({ noteSeed: [{ string: 1, fret: 0, time: 0 }], sel: [] }); diff --git a/tests/section_coverage.test.mjs b/tests/section_coverage.test.mjs index e2ed79f8..1bede5ab 100644 --- a/tests/section_coverage.test.mjs +++ b/tests/section_coverage.test.mjs @@ -14,13 +14,12 @@ import assert from 'node:assert'; import fs from 'node:fs'; import { _sectionCoveragePure } from '../src/draw.js'; +import { EditHistory } from '../src/history.js'; +import { editGen } from '../src/state.js'; -// The pure helper is a real import now. Two cases still assert on code SHAPE — -// that drawSections goes through the memo, and that EditHistory._afterEdit() -// invalidates it — so they read the two files those live in. +// The pure helper is a real import now. One case still asserts on code SHAPE — +// that drawSections goes through the memo rather than recomputing per frame. const drawSrc = fs.readFileSync(new URL('../src/draw.js', import.meta.url), 'utf8'); -const mainSrc = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); -const stateSrc = fs.readFileSync(new URL('../src/state.js', import.meta.url), 'utf8'); const sec = (t, name) => ({ name: name || 's', start_time: t }); @@ -133,28 +132,15 @@ t('drawSections uses the memo, not a per-frame recompute', () => { }); t('the coverage memo is invalidated on edit via _afterEdit()', () => { - // Brace-match the METHOD body rather than slicing a fixed character - // window: a fixed window silently shrinks by one char per line on a - // CRLF (Windows) checkout, and comment growth inside the method had - // already pushed the bump statement past the old 400-char cutoff — - // green on CI's LF checkout, red on every Windows clone. - const start = mainSrc.indexOf('_afterEdit() {'); - assert.ok(start >= 0, '_afterEdit() must exist'); - const open = mainSrc.indexOf('{', start); - let depth = 0, end = -1; - for (let i = open; i < mainSrc.length; i++) { - if (mainSrc[i] === '{') depth++; - else if (mainSrc[i] === '}' && --depth === 0) { end = i + 1; break; } - } - assert.ok(end > 0, '_afterEdit() must have a balanced body'); - const body = mainSrc.slice(start, end); - // The shared edit generation lives in src/state.js now. A counter cannot be - // written across a module boundary (import bindings are read-only), so - // _afterEdit calls the exported bumper instead of incrementing it directly. - assert.ok(/bumpEditGen\(\)/.test(body), + // EditHistory is a real import now, so drive the method instead of asserting + // on the shape of its source. `editGen` is a live binding: reading it here + // always sees src/state.js's current value, and a counter cannot be written + // across a module boundary (import bindings are read-only), which is why + // _afterEdit() calls the exported bumper rather than incrementing directly. + const before = editGen; + new EditHistory()._afterEdit(); + assert.strictEqual(editGen, before + 1, '_afterEdit() must bump the shared edit generation so in-place moves recompute'); - assert.ok(/editGen\+\+/.test(stateSrc), - 'bumpEditGen() must increment the shared edit generation'); assert.ok(/editGen[\s\S]*?_covCache/.test(drawSrc), 'the coverage memo must key on the edit generation counter'); }); diff --git a/tests/section_undo.test.js b/tests/section_undo.test.mjs similarity index 91% rename from tests/section_undo.test.js rename to tests/section_undo.test.mjs index e2329530..ae605364 100644 --- a/tests/section_undo.test.js +++ b/tests/section_undo.test.mjs @@ -21,13 +21,14 @@ * documented behavior is exactly what the test pins). These assertions fail on * main (the command classes don't exist there). * - * Run: node tests/section_undo.test.js + * Run: node tests/section_undo.test.mjs */ -const fs = require('fs'); -const path = require('path'); -const assert = require('assert'); +import assert from 'node:assert'; +import fs from 'node:fs'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; -const src = fs.readFileSync(path.join(__dirname, '..', 'src', 'main.js'), 'utf8'); +const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); function extract(name) { const re = new RegExp( '/\\* @pure:' + name + ':start \\*/[\\s\\S]*?/\\* @pure:' + name + ':end \\*/'); @@ -36,21 +37,20 @@ function extract(name) { return m[0]; } -// Build an isolated world: the REAL EditHistory + the REAL section commands, -// with S / draw / updateStatus injected. EditHistory._ui reads two buttons — -// stub getElementById to return fakes. draw/updateStatus are no-ops. +// The REAL EditHistory (a real import now) + the REAL section commands. History +// closes over the real `S`, so the sliced commands must share that same object. function makeEnv(initialSections) { - const S = { sections: initialSections || [], history: null }; - const documentStub = { getElementById: () => ({ disabled: false }) }; + const S = seedState({ sections: initialSections || [], history: null }); const api = new Function( - 'S', 'document', 'draw', 'updateStatus', + 'S', '"use strict";' - + extract('edit-history') + '\n' + extract('section-cmds') + '\n' - + 'return { EditHistory, AddSectionCmd, RemoveSectionCmd, RenameSectionCmd,' + + extract('section-cmds') + '\n' + + 'return { AddSectionCmd, RemoveSectionCmd, RenameSectionCmd,' + ' _sectionNearestIndexPure };' - )(S, documentStub, () => {}, () => {}); - S.history = new api.EditHistory(); - return { ...api, S }; + )(S); + trackHooks(); + S.history = new EditHistory(); + return { ...api, S, history: S.history }; } // Deep clone for before/after model comparison. diff --git a/tests/strings_modal.test.mjs b/tests/strings_modal.test.mjs index 8c2fb82e..10b8bbba 100644 --- a/tests/strings_modal.test.mjs +++ b/tests/strings_modal.test.mjs @@ -15,6 +15,8 @@ * Run: node tests/strings_modal.test.mjs */ import assert from 'node:assert'; +import { EditHistory } from '../src/history.js'; +import { seedState, trackHooks } from './_history_env.mjs'; import fs from 'node:fs'; import { _stringCountFor } from '../src/lanes.js'; @@ -72,7 +74,6 @@ function extractWindowFn(name) { } const tuningBlock = extractBlock('string-tuning'); -const historyBlock = extractBlock('edit-history'); const addStringSrc = extractClass('AddStringCmd'); const removeStringSrc = extractClass('RemoveStringCmd'); const normalizeSrc = extractFn('_normalizeTuningToLanes'); @@ -85,16 +86,17 @@ const removeStringHandlerSrc = extractWindowFn('editorRemoveString'); // path that carried the corruption bugs), not the tuning-length stub the // pure-command harness uses. This is what catches an add/remove at an end // the pitch/label model can't represent. -function makeHandlerEnv(S) { +function makeHandlerEnv(seed) { + const S = seedState(seed); const env = new Function( 'window', 'document', 'S', 'draw', 'updateStatus', '_renderStringsModal', '_resizeForLaneChange', '_stringCountFor', '"use strict";' - + historyBlock + '\n' + tuningBlock + '\n' + + tuningBlock + '\n' + normalizeSrc + '\n' + notesOnStringSrc + '\n' + addStringSrc + '\n' + removeStringSrc + '\n' + addStringHandlerSrc + '\n' + removeStringHandlerSrc + '\n' - + 'return { window, EditHistory, _stringCountFor, _addPositionPure,' + + 'return { window, _stringCountFor, _addPositionPure,' + ' _removePositionPure };' )( {}, // window @@ -106,17 +108,18 @@ function makeHandlerEnv(S) { () => {}, // _resizeForLaneChange _stringCountFor, // the REAL one, imported from src/lanes.js ); - S.history = new env.EditHistory(); + S.history = new EditHistory(); return env; } -function makeEnv(S) { +function makeEnv(seed) { + const S = seedState(seed); const env = new Function( 'document', 'S', 'draw', 'updateStatus', '_normalizeTuningToLanes', '_stringCountFor', '_resizeForLaneChange', '"use strict";' - + historyBlock + '\n' + tuningBlock + '\n' + addStringSrc + '\n' - + 'return { EditHistory, SetStringTuningCmd, AddStringCmd,' + + tuningBlock + '\n' + addStringSrc + '\n' + + 'return { SetStringTuningCmd, AddStringCmd,' + ' _stringsRangePure, _stringTuningClampPure,' + ' _addPositionPure, _removePositionPure };' )( @@ -128,9 +131,11 @@ function makeEnv(S) { (arr) => (arr.tuning || []).length, // count = real tuning length () => {}, // resize: no-op off-DOM ); - return { ...env, S, history: new env.EditHistory() }; + return { ...env, S, history: new EditHistory() }; } +trackHooks(); + let pass = 0, fail = 0; function t(name, fn) { try { fn(); pass++; console.log(' ok ' + name); } diff --git a/tests/suggest_position_wiring.test.mjs b/tests/suggest_position_wiring.test.mjs index 8356526e..d4a9c1b1 100644 --- a/tests/suggest_position_wiring.test.mjs +++ b/tests/suggest_position_wiring.test.mjs @@ -33,20 +33,15 @@ import { } from '../src/position.js'; import { LC, lanes } from '../src/lanes.js'; import { S as realS } from '../src/state.js'; +import { EditHistory } from '../src/history.js'; +import { _rollLockNotice, _rollReadOnly } from '../src/keys.js'; +import { lastStatus, seedState, trackHooks } from './_history_env.mjs'; // The suggest blocks and the roll-add helpers still live in src/main.js and are // still sliced; `reconstructChords` moved to src/chords.js and is imported, so // section 6 drives it against the REAL `S` rather than a fabricated one. const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); -function extractBlock(name) { - // Lenient start: some blocks carry trailing prose after `:start` (e.g. - // chord-relink) rather than an immediate `*/`, so match up to the :end. - const re = new RegExp('/\\* @pure:' + name + ':start[\\s\\S]*?@pure:' + name + ':end \\*/'); - const m = src.match(re); - if (!m) { console.error(`FAIL: @pure:${name} block not found in src/main.js`); process.exit(1); } - return m[0]; -} function extractByKeyword(keyword, label) { const start = src.indexOf(keyword); assert.ok(start >= 0, `${label || keyword} must exist in src/main.js`); @@ -87,18 +82,20 @@ function assertNoForbidden(obj, label) { // sandbox has to share that object rather than fabricate its own — otherwise the // count would read an arrangement the commands never touched. function makeEnv({ notes: seed = [], arrName = 'Lead' } = {}) { - Object.assign(realS, { - currentArr: 0, + // A fretted part shown in the roll IS the read-only lock, so express it as + // real state rather than stubbing _rollReadOnly — EditHistory imports the + // real predicate now. + const S = seedState({ arrangements: [{ id: 'a1', name: arrName, tuning: TUN.slice(), notes: seed.map(n => ({ ...n })), anchors_user: [], anchors: [] }], - sel: new Set(), + rollView: true, }); - const S = realS; + assert.ok(_rollReadOnly(), 'harness precondition: the roll must be read-only'); + trackHooks(); const statuses = []; const refusals = []; // records _rollConfirmPosition handoffs const fullSrc = '"use strict";' - + extractBlock('edit-history') - + '\n' + extractFn('_withStableSelection') + + extractFn('_withStableSelection') + '\n' + extractClass('AddNoteCmd') + '\n' + extractClass('MoveToStringCmd') + '\n' + extractClass('AcceptPositionsCmd') @@ -109,12 +106,11 @@ function makeEnv({ notes: seed = [], arrName = 'Lead' } = {}) { + '\n' + extractFn('_commitAddResolved') + '\n' + extractFn('_rollAddByPitch') + '\n' + extractFn('_execAcceptPositions') - + '\nconst history = new EditHistory(); S.history = history;' - + '\nreturn { history, _rollAddByPitch, _commitAddResolved, _execAcceptPositions,' + + '\nreturn { _rollAddByPitch, _commitAddResolved, _execAcceptPositions,' + ' _isSuggested, _markSuggested, _clearSuggested, _suggestedCount,' + ' AddNoteCmd, MoveToStringCmd };'; const env = new Function( - 'S', 'document', 'notes', 'setStatus', 'draw', 'updateStatus', '_renderInspector', + 'S', 'history', 'notes', 'setStatus', 'draw', 'updateStatus', '_renderInspector', '_editBlipAt', '_rollReadOnly', '_rollLockNotice', '_editorCurrentNoteIndices', '_rollPitchCtx', '_rollConfirmPosition', '_markSuggested', '_clearSuggested', '_isSuggested', '_suggestedNotes', @@ -123,12 +119,12 @@ function makeEnv({ notes: seed = [], arrName = 'Lead' } = {}) { fullSrc )( S, - { getElementById: () => ({ disabled: false }) }, + (S.history = new EditHistory()), () => S.arrangements[S.currentArr].notes, m => statuses.push(m), () => {}, () => {}, () => {}, () => {}, - () => true, // _rollReadOnly: the locked fretted roll - () => statuses.push('LOCKED'), + _rollReadOnly, // the real locked-fretted-roll predicate + () => { statuses.push('LOCKED'); _rollLockNotice(); }, () => (S.sel && S.sel.size ? [...S.sel] : []), () => ({ openMidi: OPEN, tuning: TUN.slice(), capo: 0 }), (res, pitch, time) => refusals.push({ reason: res.reason, pitch, time, candidates: res.candidates }), @@ -136,6 +132,7 @@ function makeEnv({ notes: seed = [], arrName = 'Lead' } = {}) { _suggestPositionPure, _enumerateFrettedPositionsPure, _activeAnchorAtPure, _suggestedCount, ); + env.history = S.history; return { S, env, statuses, refusals, notes: () => S.arrangements[0].notes }; } @@ -230,11 +227,11 @@ t('Accept confirms the selected suggested notes in one undo step; undo re-marks // ── 5. the lock still blocks an ordinary command ────────────────────────────── t('the read-only-roll lock still blocks an ordinary (unflagged) command', () => { - const { env, notes, statuses } = makeEnv(); + const { env, notes } = makeEnv(); env.history.exec(new env.AddNoteCmd({ time: 0, string: 0, fret: 0, sustain: 0, techniques: {} })); assert.strictEqual(notes().length, 0, 'an add WITHOUT suggestResolved stays inert in the locked roll'); assert.strictEqual(env.history.undo.length, 0); - assert.ok(statuses.includes('LOCKED'), 'the user is told why'); + assert.match(lastStatus(), /read-only/, 'the user is told why (real _rollLockNotice)'); }); // ── 6. WIRE PURITY through the real reconstructChords (solo AND chord) ───────── diff --git a/tests/view_switcher.test.mjs b/tests/view_switcher.test.mjs index 9b978789..0a579216 100644 --- a/tests/view_switcher.test.mjs +++ b/tests/view_switcher.test.mjs @@ -12,6 +12,8 @@ * Run: node tests/view_switcher.test.mjs */ import assert from 'node:assert'; +import { EditHistory } from '../src/history.js'; +import { lockNotices, seedState, setRollView, trackHooks } from './_history_env.mjs'; import fs from 'node:fs'; import * as keys from '../src/keys.js'; import { @@ -22,17 +24,6 @@ import { S } from '../src/state.js'; // Only @pure:edit-history is still sliced — it lives in src/main.js. const src = fs.readFileSync(new URL('../src/main.js', import.meta.url), 'utf8'); -function extractBlock(name) { - const re = new RegExp( - '/\\* @pure:' + name + ':start \\*/[\\s\\S]*?/\\* @pure:' + name + ':end \\*/'); - const m = src.match(re); - if (!m) { - console.error(`FAIL: @pure:${name} block not found in src/main.js`); - process.exit(1); - } - return m[0]; -} - let pass = 0, fail = 0; function t(name, fn) { try { fn(); pass++; console.log(' ok ' + name); } @@ -168,19 +159,16 @@ t('a KEYS part uses the wire packing, not sounding pitch', () => { // ── The read-only-roll gate in EditHistory ─────────────────────────── +// EditHistory imports _rollReadOnly for real, so the gate is driven through +// actual state: a part named 'Lead' is fretted, and rollView puts it in the +// piano roll — which IS the read-only condition. The precondition assert keeps a +// future keys-pattern change from silently unlocking this suite. function makeHistory(locked) { - const S = { currentArr: 0 }; - const notices = { count: 0 }; - const env = new Function( - 'document', 'S', 'draw', 'updateStatus', '_rollReadOnly', '_rollLockNotice', - '"use strict";' + extractBlock('edit-history') + '\nreturn { EditHistory };' - )( - { getElementById: () => ({ disabled: false }) }, - S, () => {}, () => {}, - () => locked.value, - () => { notices.count++; }, - ); - return { history: new env.EditHistory(), notices }; + seedState({ arrangements: [{ id: 'a1', name: 'Lead', notes: [] }], rollView: locked.value }); + assert.strictEqual(keys._rollReadOnly(), locked.value, 'harness precondition'); + trackHooks(); + const before = lockNotices(); + return { history: new EditHistory(), notices: { get count() { return lockNotices() - before; } } }; } t('read-only roll: exec is inert — no mutation, no undo entry, one notice', () => { @@ -211,7 +199,7 @@ t('lock lifts live: the same history accepts commands once unlocked', () => { const cmd = { exec() { applied++; }, rollback() { applied--; } }; history.exec(cmd); assert.strictEqual(applied, 0); - locked.value = false; // user switches back to String view + setRollView(false); // user switches back to String view history.exec(cmd); assert.strictEqual(applied, 1); }); @@ -244,7 +232,7 @@ t('read-only roll: undo/redo of a NOTE-scope command is inert (no chart write)', const cmd = { exec() { applied++; }, rollback() { applied--; } }; history.exec(cmd); assert.strictEqual(applied, 1); - locked.value = true; // user switches this part into the roll + setRollView(true); // user switches this part into the roll const noticesBefore = notices.count; history.doUndo(); assert.strictEqual(applied, 1, 'undo must not roll back a read-only fretted chart');