diff --git a/app/api/jobs.py b/app/api/jobs.py index 7091a5e5..c27775fd 100644 --- a/app/api/jobs.py +++ b/app/api/jobs.py @@ -499,6 +499,12 @@ class SectionItem(BaseModel): Literal["intro", "outro", "break", "bridge", "inst", "solo", "verse", "chorus", "part"] | None ) = None + # Pins a section against drag and resize in the timeline editor (#573). + # It must be declared here to exist at all: this model takes Pydantic's + # default extra="ignore", so an undeclared field sent by the editor is + # dropped on save and answered 200, and the flag comes back false on the + # next load with nothing anywhere saying why. + locked: bool = False @field_validator("id") @classmethod diff --git a/static/css/daw.css b/static/css/daw.css index affda047..6435728a 100644 --- a/static/css/daw.css +++ b/static/css/daw.css @@ -1667,6 +1667,36 @@ input, textarea { font-family: inherit; } .section-del:hover { opacity: 1; background: rgba(0,0,0,0.3); } .section-block:hover .section-del { display: flex; } +.section-lock { + display: none; + flex-shrink: 0; + width: 16px; height: 16px; + margin-right: 2px; + border: none; background: none; + color: var(--sc, #4a7fff); + cursor: pointer; border-radius: 3px; + align-items: center; justify-content: center; + opacity: 0.7; + padding: 0; +} +.section-lock:hover { opacity: 1; background: rgba(0,0,0,0.3); } +.section-block:hover .section-lock { display: flex; } +/* A locked section shows its padlock without being hovered, and in red. Hiding + it would make locked and unlocked look identical at rest, and the only way to + find a locked one would be to try to drag it. Red because the padlock is the + one control that is saying no to the others. */ +.section-block.sec-locked .section-lock { + display: flex; + opacity: 1; + color: var(--danger); +} +.section-block.sec-locked .section-lock:hover { opacity: 1; } + +/* The grab and col-resize cursors promise a drag that cannot happen. */ +.section-block.sec-locked, +.section-block.sec-locked .section-handle { cursor: default; } +.section-block.sec-locked .section-handle:hover { background: none; } + .section-handle { position: absolute; top: 0; bottom: 0; @@ -1707,6 +1737,9 @@ input, textarea { font-family: inherit; } } .sections-save-indicator.hidden { display: none; } .sections-save-indicator.saved { color: #4caf7d; } +/* A refusal, not a failure of something already in flight. It borrows this + element because it belongs beside the button that was pressed. */ +.sections-save-indicator.notice { color: var(--warn, #e6a23c); white-space: normal; } .sections-save-indicator::before { content: ""; display: block; @@ -1716,6 +1749,14 @@ input, textarea { font-family: inherit; } border-top-color: transparent; animation: sections-spin 600ms linear infinite; } +.sections-save-indicator.notice::before { + animation: none; + border: none; + width: auto; height: auto; + content: "!"; + font-weight: 700; + font-size: 11px; +} .sections-save-indicator.saved::before { animation: none; border: none; diff --git a/static/js/i18n.js b/static/js/i18n.js index c9b26296..dc3ba924 100644 --- a/static/js/i18n.js +++ b/static/js/i18n.js @@ -788,6 +788,9 @@ const en = { "sections.saving": "Saving", "sections.saved": "Saved", "sections.deleteAria": "Delete section", + "sections.lockAria": "Lock position", + "sections.unlockAria": "Unlock position", + "sections.loopOverlaps": "Loop overlaps a section", "sections.defaultName": "Section", // ── Playlist import confirm (static/js/playlist.js) ── @@ -1394,6 +1397,9 @@ const pl = { "sections.saving": "Zapisywanie", "sections.saved": "Zapisano", "sections.deleteAria": "Usuń sekcję", + "sections.lockAria": "Zablokuj pozycję", + "sections.unlockAria": "Odblokuj pozycję", + "sections.loopOverlaps": "Pętla nachodzi na sekcję", "sections.defaultName": "Sekcja", "playlist.skip.unavailable.one": "{count} niedostępny", @@ -1983,6 +1989,9 @@ const ja = { "sections.saving": "保存中", "sections.saved": "保存済み", "sections.deleteAria": "セクションを削除", + "sections.lockAria": "位置をロック", + "sections.unlockAria": "位置のロックを解除", + "sections.loopOverlaps": "ループが既存のセクションと重なっています", "sections.defaultName": "セクション", "playlist.skip.unavailable.other": "利用不可 {count}件", @@ -2562,6 +2571,9 @@ const zhHans = { "sections.saving": "正在保存", "sections.saved": "已保存", "sections.deleteAria": "删除段落", + "sections.lockAria": "锁定位置", + "sections.unlockAria": "解锁位置", + "sections.loopOverlaps": "循环区域与已有段落重叠", "sections.defaultName": "段落", "playlist.skip.unavailable.other": "{count} 首不可用", @@ -3149,6 +3161,9 @@ const de = { "sections.saving": "Wird gespeichert", "sections.saved": "Gespeichert", "sections.deleteAria": "Abschnitt löschen", + "sections.lockAria": "Position sperren", + "sections.unlockAria": "Position entsperren", + "sections.loopOverlaps": "Loop überschneidet einen Abschnitt", "sections.defaultName": "Abschnitt", "playlist.skip.unavailable.other": "{count} nicht verfügbar", @@ -3739,6 +3754,9 @@ const pt = { "sections.saving": "Salvando", "sections.saved": "Salvo", "sections.deleteAria": "Excluir seção", + "sections.lockAria": "Bloquear posição", + "sections.unlockAria": "Desbloquear posição", + "sections.loopOverlaps": "O loop sobrepõe uma seção", "sections.defaultName": "Seção", "playlist.skip.unavailable.one": "{count} indisponível", @@ -4323,6 +4341,9 @@ const id = { "sections.saving": "Menyimpan", "sections.saved": "Tersimpan", "sections.deleteAria": "Hapus bagian", + "sections.lockAria": "Kunci posisi", + "sections.unlockAria": "Buka kunci posisi", + "sections.loopOverlaps": "Loop bertumpang tindih dengan bagian", "sections.defaultName": "Bagian", "playlist.skip.unavailable.other": "{count} tidak tersedia", @@ -4910,6 +4931,9 @@ const fr = { "sections.saving": "Enregistrement", "sections.saved": "Enregistré", "sections.deleteAria": "Supprimer la section", + "sections.lockAria": "Verrouiller la position", + "sections.unlockAria": "Déverrouiller la position", + "sections.loopOverlaps": "La boucle chevauche une section", "sections.defaultName": "Section", "playlist.skip.unavailable.one": "{count} indisponible", @@ -5627,6 +5651,9 @@ const es = { "sections.saving": "Guardando", "sections.saved": "Guardado", "sections.deleteAria": "Eliminar sección", + "sections.lockAria": "Bloquear posición", + "sections.unlockAria": "Desbloquear posición", + "sections.loopOverlaps": "El bucle se superpone a una sección", "sections.defaultName": "Sección", // ── Playlist import confirm (static/js/playlist.js) ── @@ -6223,6 +6250,9 @@ const ko = { "sections.saving": "저장 중", "sections.saved": "저장됨", "sections.deleteAria": "구간 삭제", + "sections.lockAria": "위치 잠금", + "sections.unlockAria": "위치 잠금 해제", + "sections.loopOverlaps": "루프가 기존 구간과 겹칩니다", "sections.defaultName": "구간", "playlist.skip.unavailable.other": "사용할 수 없는 {count}개", diff --git a/static/js/main.js b/static/js/main.js index f4987726..c96fd60c 100644 --- a/static/js/main.js +++ b/static/js/main.js @@ -10,7 +10,8 @@ import { wireJobForm, showError } from "./job.js"; import { initSearch } from "./search.js"; import { wireTransportButtons } from "./transport.js"; import { wireBeatGridUi } from "./beatgridUi.js"; -import { togglePlayPause, updateLoopRegionVisual, toggleMetronome, transport, setPlayheadTime } from "./transport.js"; +import { togglePlayPause, updateLoopRegionVisual, toggleMetronome, transport, setPlayheadTime, setLoopRange } from "./transport.js"; +import { setSectionsLoopBridge } from "./sections.js"; import { wireStemListControls, wireMixerToolbar, laneDragIcon } from "./mixer.js"; import { initCatalog, collectDiagnostics } from "./catalog.js"; import { initNotifications, notifyFailure, dismissFailuresByJobId } from "./notifications.js"; @@ -222,6 +223,12 @@ initSearch((item) => { urlInput.setSelectionRange(urlInput.value.length, urlInput.value.length); }); wireTransportButtons(); +// Sections deliberately does not import the transport, so that it stays +// loadable without a DOM. This is the one place that owns both. +setSectionsLoopBridge({ + getLoop: () => ({ enabled: loopEnabled, start: loopStart, end: loopEnd }), + setLoopRange, +}); wireBeatGridUi(); wireFooterControls(); requestAnimationFrame(drawFooterPlaceholder); diff --git a/static/js/sections.js b/static/js/sections.js index c832346c..901ede56 100644 --- a/static/js/sections.js +++ b/static/js/sections.js @@ -26,6 +26,26 @@ let _container = null; let _saveTimer = null; let _saveChain = Promise.resolve(); +// The loop lives in transport.js, which reaches the DOM at import time through +// state.js. Importing it here would drag a document into every Node test that +// loads this module, so the two functions sections needs are handed in instead. +// Same reason loopRegion.js is its own module: the testable part stays +// reachable without a browser. +let _loopBridge = null; + +/// Wire sections to the transport's loop. Called once, from the module that +/// already owns both. Unset, the loop features are inert rather than broken. +export function setSectionsLoopBridge(bridge) { + _loopBridge = bridge; +} + +function _armedLoop() { + const loop = _loopBridge?.getLoop?.(); + if (!loop || !loop.enabled) return null; + if (!(loop.end > loop.start)) return null; + return loop; +} + onLanguageChange(() => _render()); // ─── Public API ─────────────────────────────────────────── @@ -94,25 +114,41 @@ function _makeSectionEl(section) { const pctWidth = ((section.end - section.start) / _duration) * 100; const el = document.createElement("div"); - el.className = "section-block"; + el.className = section.locked ? "section-block sec-locked" : "section-block"; el.dataset.id = section.id; el.style.cssText = `left:${pctStart.toFixed(4)}%;width:${pctWidth.toFixed(4)}%;--sc:${section.color}`; + const lockAria = section.locked ? t("sections.unlockAria") : t("sections.lockAria"); el.innerHTML = `
${_esc(sectionDisplayName(section, _sections))} +
`; + el.querySelector(".section-lock").addEventListener("click", (e) => { + e.stopPropagation(); + _toggleLock(section.id); + }); + el.querySelector(".section-del").addEventListener("click", (e) => { e.stopPropagation(); _deleteSection(section.id); }); - el.querySelector(".section-label").addEventListener("dblclick", (e) => { + // Bound to the block, not to the label inside it. The drag calls + // setPointerCapture, which retargets the rest of the gesture to the capturing + // element, so the dblclick that ends a real double-click is delivered to the + // block and a listener on the child label never runs. Synthetic events do not + // capture, which is why this looked fine in a console and failed for every + // actual user. + el.addEventListener("dblclick", (e) => { + if (e.target.closest(".section-handle,.section-del,.section-lock")) return; e.stopPropagation(); - _openRename(section.id, el.querySelector(".section-label")); + if (section.locked) return; + const labelEl = el.querySelector(".section-label"); + if (labelEl) _openRename(section.id, labelEl); }); _wireDrag(el, section); @@ -149,6 +185,20 @@ function _esc(str) { .replace(/"/g, """); } +// Closed or open padlock, in the same stroked 24x24 shape as the rest of the +// app's icons so it inherits --sc through currentColor. Drawn rather than +// written as an emoji: this ships on three platforms and the emoji padlock is +// a different colour and weight on each of them. +function _lockIcon(locked) { + const shackle = locked ? "M7 11V7a5 5 0 0 1 10 0v4" : "M7 11V7a5 5 0 0 1 9.9-1"; + return ( + '` + ); +} + // ─── Drag to move ───────────────────────────────────────── function _wireDrag(el, section) { @@ -158,13 +208,24 @@ function _wireDrag(el, section) { let origEnd = 0; let changed = false; + // A press that never moves is a click, and a click loops the section. That is + // tracked separately from `active` so a locked section can still be looped: + // lock is about position, and refusing to loop one would be a second rule + // nobody asked for. + let pressed = false; + el.addEventListener("pointerdown", (e) => { - if (e.target.closest(".section-handle,.section-del")) return; + if (e.target.closest(".section-handle,.section-del,.section-lock")) return; + pressed = true; + changed = false; + // Read the flag here, not at wire time: the block is rebuilt on every + // render, but a stale closure would still be the kind of bug that only + // shows after a toggle and before the next redraw. + if (section.locked) return; active = true; startX = e.clientX; origStart = section.start; origEnd = section.end; - changed = false; el.setPointerCapture(e.pointerId); el.classList.add("sec-dragging"); e.preventDefault(); @@ -185,10 +246,20 @@ function _wireDrag(el, section) { }); el.addEventListener("pointerup", () => { - if (!active) return; - active = false; - el.classList.remove("sec-dragging"); - if (changed) _scheduleSave(); + const wasPressed = pressed; + pressed = false; + if (active) { + active = false; + el.classList.remove("sec-dragging"); + if (changed) { + _scheduleSave(); + return; + } + } + // Nothing moved, so this was a click: loop over what it covers. MIN_SEC is + // 0.5 s and MIN_LOOP_SEC is 0.2 s, so any section that exists can be a + // loop and setLoopRange cannot refuse one. + if (wasPressed && !changed) _loopBridge?.setLoopRange?.(section.start, section.end); }); el.addEventListener("pointercancel", () => { @@ -198,6 +269,7 @@ function _wireDrag(el, section) { _render(); } active = false; + pressed = false; el.classList.remove("sec-dragging"); }); } @@ -214,6 +286,7 @@ function _wireResize(handle, el, section) { let changed = false; handle.addEventListener("pointerdown", (e) => { + if (section.locked) return; active = true; startX = e.clientX; origTime = edge === "left" ? section.start : section.end; @@ -310,7 +383,28 @@ function _rightNeighborStart(id) { // ─── CRUD ───────────────────────────────────────────────── -function _addSection() { +// Where a new section goes. +// +// With a loop armed, Add means "make this selection a section", which is the +// direct route from hearing a part to naming it (#573, and #474 asked for the +// same thing). Without one it falls back to the first gap that fits. +// +// Returns null when a loop is armed but cannot become a section, so the caller +// can say why rather than silently building one somewhere else. That silent +// fallback is the whole complaint: you select a region, press Add, and a +// section appears at the start of the track instead. +function _newSectionBounds() { + const loop = _armedLoop(); + if (loop && loop.end - loop.start >= MIN_SEC) { + const start = Math.max(0, loop.start); + const end = Math.min(_duration, loop.end); + if (end - start < MIN_SEC) return null; + // Sections cannot overlap, so a loop drawn across one cannot become a + // section without moving something the user did not ask to move. + if (_sections.some((s) => start < s.end && end > s.start)) return null; + return { start, end }; + } + const defW = _duration * DEFAULT_WIDTH_FRAC; const sorted = [..._sections].sort((a, b) => a.start - b.start); @@ -323,12 +417,24 @@ function _addSection() { // Clamp and verify room start = Math.min(start, _duration - MIN_SEC); - if (start < 0) return; + if (start < 0) return null; const end = Math.min(start + defW, _duration); - if (end - start < MIN_SEC) return; + if (end - start < MIN_SEC) return null; // Verify no overlap - if (_sections.some((s) => start < s.end && end > s.start)) return; + if (_sections.some((s) => start < s.end && end > s.start)) return null; + return { start, end }; +} + +function _addSection() { + const bounds = _newSectionBounds(); + if (!bounds) { + // Only worth explaining when a loop was the thing that failed. A full + // track with no gap left is visible on its own. + if (_armedLoop()) _showNotice(t("sections.loopOverlaps")); + return; + } + const { start, end } = bounds; const color = _nextColor(); const section = { id: _nextId(), name: t("sections.defaultName"), start, end, color }; @@ -423,10 +529,31 @@ function _deleteSection(id) { _scheduleSave(); } +// Lock covers editing: drag, resize and rename all refuse while it is set, and +// the padlock turns red so a locked section is obvious at rest (#573). +// +// Looping over a locked section still works. That reads the section without +// changing it, and it is the gesture most likely to be wanted on one pinned +// precisely because it matters. +// +// Delete is still allowed: the cross is an explicit press on one named target, +// not something a pointer does on the way to somewhere else. +function _toggleLock(id) { + const section = _sections.find((s) => s.id === id); + if (!section) return; + section.locked = !section.locked; + _render(); + _scheduleSave(); +} + function _openRename(id, labelEl) { if (!labelEl) return; const section = _sections.find((s) => s.id === id); if (!section) return; + // Also checked here, not only at the gesture. _addSection opens a rename on + // the block it just made, and a future caller has no reason to know that a + // locked section must not get an editable field. + if (section.locked) return; const input = document.createElement("input"); input.className = "section-rename-input"; @@ -482,6 +609,21 @@ function _showSaved() { }, 1800); } +// A refusal, shown where the save state already appears. That element is +// aria-live="polite", so this reaches a screen reader without stealing focus, +// and it is beside the button that was just pressed rather than in the import +// panel at the other end of the page. +function _showNotice(message) { + const el = document.getElementById("sectionsSaveIndicator"); + if (!el) return; + clearTimeout(_savedTimer); + el.textContent = message; + el.className = "sections-save-indicator notice"; + _savedTimer = setTimeout(() => { + el.className = "sections-save-indicator hidden"; + }, 3200); +} + function _hideSaveIndicator() { const el = document.getElementById("sectionsSaveIndicator"); if (el) el.className = "sections-save-indicator hidden"; diff --git a/static/js/transport.js b/static/js/transport.js index dd1fbd46..e715be20 100644 --- a/static/js/transport.js +++ b/static/js/transport.js @@ -338,11 +338,23 @@ function commitLoopInput(which) { revert(); return; } + setLoopRange(start, end); +} + +/// Arm the loop over an explicit span, and show it. +/// +/// Callers that already know both bounds all need the same five steps, and the +/// button class and the overlay redraw are the two that are silent when +/// forgotten: the loop plays correctly and nothing on screen says it is on. +/// Returns false when the span is too short to loop, so a caller can say why. +export function setLoopRange(start, end) { + if (!(end - start >= MIN_LOOP_SEC)) return false; setLoopStart(start); setLoopEnd(end); setLoopEnabled(true); loopBtn.classList.add("active"); updateLoopRegionVisual(); + return true; } // Wheel over a loop field nudges it, in seconds on the left of the decimal @@ -390,12 +402,7 @@ function nudgeLoopInput(which, direction, unit) { if (next < 0 || next > totalDuration) return; const start = which === "start" ? next : loopStart; const end = which === "end" ? next : loopEnd; - if (end - start < MIN_LOOP_SEC) return; - setLoopStart(start); - setLoopEnd(end); - setLoopEnabled(true); - loopBtn.classList.add("active"); - updateLoopRegionVisual(); + setLoopRange(start, end); } function wireLoopInputs() { diff --git a/tests/e2e/sections-lock.spec.mjs b/tests/e2e/sections-lock.spec.mjs new file mode 100644 index 00000000..1014cf9a --- /dev/null +++ b/tests/e2e/sections-lock.spec.mjs @@ -0,0 +1,132 @@ +// A locked section is pinned against editing: it cannot be dragged, resized or +// renamed, and its padlock turns red so a locked one is obvious without +// hovering every block to find it (#573). +// +// Looping over one still works. That reads the section without changing it. +import { test, expect } from "@playwright/test"; +import { openStudio, JOB_ID } from "./helpers.mjs"; + +test.afterEach(async ({ page }) => { + await page.request.patch(`/api/jobs/${JOB_ID}/sections`, { data: { sections: [] } }); +}); + +async function seed(page, sections) { + const res = await page.request.patch(`/api/jobs/${JOB_ID}/sections`, { data: { sections } }); + expect(res.ok(), "seeding sections").toBe(true); +} + +const savedSections = async (page) => { + const res = await page.request.get(`/api/jobs/${JOB_ID}`); + return (await res.json()).sections ?? []; +}; + +const LOCKED = { id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff", locked: true }; +const UNLOCKED = { id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff" }; + +/** Drag from the middle of the block by dx pixels. */ +async function dragBy(page, dx) { + const box = await page.locator('.section-block[data-id="s1"]').boundingBox(); + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.mouse.move(box.x + box.width / 2 + dx, box.y + box.height / 2, { steps: 8 }); + await page.mouse.up(); +} + +test.describe("locking a section", () => { + test("the padlock is red, and visible without hovering", async ({ page }) => { + await seed(page, [LOCKED]); + await openStudio(page); + + const lock = page.locator('.section-block[data-id="s1"] .section-lock'); + // Visible at rest: an unlocked padlock only appears on hover, so a locked + // one that did the same would be indistinguishable until you went looking. + await expect(lock).toBeVisible(); + + const { locked, danger } = await page.evaluate(() => ({ + locked: getComputedStyle(document.querySelector(".section-block.sec-locked .section-lock")).color, + danger: getComputedStyle(document.documentElement).getPropertyValue("--danger").trim(), + })); + expect(danger, "--danger must be defined for this to mean anything").not.toBe(""); + // Same colour the rest of the app uses to say no, not a second red. + const asRgb = await page.evaluate((c) => { + const probe = document.createElement("span"); + probe.style.color = c; + document.body.appendChild(probe); + const rgb = getComputedStyle(probe).color; + probe.remove(); + return rgb; + }, danger); + expect(locked).toBe(asRgb); + }); + + test("an unlocked section is not red", async ({ page }) => { + await seed(page, [UNLOCKED]); + await openStudio(page); + + await expect(page.locator('.section-block[data-id="s1"]')).not.toHaveClass(/sec-locked/); + }); + + test("a locked section refuses to be dragged", async ({ page }) => { + await seed(page, [LOCKED]); + await openStudio(page); + + await dragBy(page, 80); + + const [section] = await savedSections(page); + expect(section.start).toBeCloseTo(1, 2); + expect(section.end).toBeCloseTo(3, 2); + }); + + test("an unlocked section still drags, or the test above proves nothing", async ({ page }) => { + await seed(page, [UNLOCKED]); + await openStudio(page); + + await dragBy(page, 80); + + await expect.poll(async () => (await savedSections(page))[0]?.start > 1.05).toBe(true); + }); + + test("a locked section refuses to be resized", async ({ page }) => { + await seed(page, [LOCKED]); + await openStudio(page); + + const handle = page.locator('.section-block[data-id="s1"] .section-handle-r'); + const box = await handle.boundingBox(); + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.mouse.move(box.x + box.width / 2 + 60, box.y + box.height / 2, { steps: 8 }); + await page.mouse.up(); + + const [section] = await savedSections(page); + expect(section.end).toBeCloseTo(3, 2); + }); + + test("the lock survives a reload", async ({ page }) => { + await seed(page, [UNLOCKED]); + await openStudio(page); + + // On an unlocked section the padlock only appears on hover, exactly like + // the delete cross beside it. Once locked it stays visible, which is what + // the first test in this file asserts. + await page.locator('.section-block[data-id="s1"]').hover(); + await page.locator('.section-block[data-id="s1"] .section-lock').click(); + await expect(page.locator('.section-block[data-id="s1"]')).toHaveClass(/sec-locked/); + await expect.poll(async () => (await savedSections(page))[0]?.locked).toBe(true); + + // The half that was silently broken before `locked` was declared on + // SectionItem: it saved, answered 200, and came back false. + await page.reload(); + await openStudio(page); + await expect(page.locator('.section-block[data-id="s1"]')).toHaveClass(/sec-locked/); + }); + + test("clicking the padlock again unlocks it", async ({ page }) => { + await seed(page, [LOCKED]); + await openStudio(page); + + await page.locator('.section-block[data-id="s1"] .section-lock').click(); + + await expect(page.locator('.section-block[data-id="s1"]')).not.toHaveClass(/sec-locked/); + await expect.poll(async () => (await savedSections(page))[0]?.locked).toBe(false); + }); +}); diff --git a/tests/e2e/sections-loop.spec.mjs b/tests/e2e/sections-loop.spec.mjs new file mode 100644 index 00000000..527d6fc4 --- /dev/null +++ b/tests/e2e/sections-loop.spec.mjs @@ -0,0 +1,148 @@ +// The loop and the sections ribbon describe the same spans, so each should be +// able to become the other (#573, and #474 asked for the same thing). +// +// Before this, arming a loop and pressing Add built a section at the start of +// the track instead. Add only ever looked for the first gap that fit, so the +// selection you had just made was ignored with nothing saying so, which reads +// as the button being broken rather than as a feature that does not exist. +// +// Everything here goes through the real controls: the loop is typed into the +// transport's own fields and read back from them, so nothing depends on a test +// hook that could keep passing after the UI stopped working. +import { test, expect } from "@playwright/test"; +import { openStudio, JOB_ID } from "./helpers.mjs"; + +// seed.py writes a 6 second fixture, so every span here stays inside it. +const FIXTURE_SEC = 6; + +test.afterEach(async ({ page }) => { + await page.request.patch(`/api/jobs/${JOB_ID}/sections`, { data: { sections: [] } }); +}); + +/** "00:02.000", the format fmtTimeMs writes into the loop fields. */ +const tc = (seconds) => { + const ms = Math.round(seconds * 1000); + const m = String(Math.floor(ms / 60000)).padStart(2, "0"); + const s = String(Math.floor((ms % 60000) / 1000)).padStart(2, "0"); + return `${m}:${s}.${String(ms % 1000).padStart(3, "0")}`; +}; + +/** Arm the loop the way a user does: type both bounds, blur to commit. */ +async function armLoop(page, start, end) { + await page.locator("#t-loop-end").fill(tc(end)); + await page.locator("#t-loop-end").blur(); + await page.locator("#t-loop-start").fill(tc(start)); + await page.locator("#t-loop-start").blur(); + await expect(page.locator("#t-loop")).toHaveClass(/active/); +} + +async function loopState(page) { + return { + enabled: await page.locator("#t-loop").evaluate((el) => el.classList.contains("active")), + start: await page.locator("#t-loop-start").inputValue(), + end: await page.locator("#t-loop-end").inputValue(), + }; +} + +async function seed(page, sections) { + const res = await page.request.patch(`/api/jobs/${JOB_ID}/sections`, { data: { sections } }); + expect(res.ok(), "seeding sections").toBe(true); +} + +const savedSections = async (page) => { + const res = await page.request.get(`/api/jobs/${JOB_ID}`); + return (await res.json()).sections ?? []; +}; + +const blockIds = (page) => + page.evaluate(() => + [...document.querySelectorAll(".section-block")].map((el) => el.dataset.id), + ); + +test.describe("sections and the loop", () => { + test("Add builds the section on the armed loop, not at the start of the track", async ({ + page, + }) => { + await openStudio(page); + await armLoop(page, 2, 4); + + await page.locator("#sectionsAddBtn").click(); + + await expect.poll(async () => (await savedSections(page)).length).toBe(1); + const [section] = await savedSections(page); + // The whole complaint: this used to be 0, wherever the loop was. + expect(section.start).toBeCloseTo(2, 2); + expect(section.end).toBeCloseTo(4, 2); + }); + + test("Add still finds a gap when no loop is armed", async ({ page }) => { + await openStudio(page); + + await page.locator("#sectionsAddBtn").click(); + + await expect.poll(async () => (await savedSections(page)).length).toBe(1); + const [section] = await savedSections(page); + expect(section.start).toBeCloseTo(0, 2); + expect(section.end).toBeLessThan(FIXTURE_SEC); + }); + + test("a loop across an existing section is refused, and says why", async ({ page }) => { + await seed(page, [{ id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff" }]); + await openStudio(page); + await expect(page.locator(".section-block")).toHaveCount(1); + + await armLoop(page, 2, 4); // straddles s1 + await page.locator("#sectionsAddBtn").click(); + + // Refused rather than built somewhere else, and rather than displacing a + // section the user may well have locked. + expect(await blockIds(page)).toEqual(["s1"]); + const notice = page.locator("#sectionsSaveIndicator.notice"); + await expect(notice).toBeVisible(); + await expect(notice).not.toBeEmpty(); + }); + + test("clicking a section loops over it", async ({ page }) => { + await seed(page, [{ id: "s1", name: "Chorus", start: 1, end: 3, color: "#4a7fff" }]); + await openStudio(page); + + await page.locator('.section-block[data-id="s1"]').click(); + + expect(await loopState(page)).toEqual({ + enabled: true, + start: tc(1), + end: tc(3), + }); + }); + + test("a locked section can still be clicked to loop", async ({ page }) => { + await seed(page, [ + { id: "s1", name: "Chorus", start: 1, end: 3, color: "#4a7fff", locked: true }, + ]); + await openStudio(page); + + const block = page.locator('.section-block[data-id="s1"]'); + await expect(block).toHaveClass(/sec-locked/); + await block.click(); + + // Lock is about position. Refusing to loop a locked section would be a + // second rule nobody asked for. + expect((await loopState(page)).enabled).toBe(true); + expect((await loopState(page)).start).toBe(tc(1)); + }); + + test("dragging a section does not also arm a loop over it", async ({ page }) => { + await seed(page, [{ id: "s1", name: "Verse", start: 1, end: 2, color: "#4a7fff" }]); + await openStudio(page); + + const box = await page.locator('.section-block[data-id="s1"]').boundingBox(); + await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2); + await page.mouse.down(); + await page.mouse.move(box.x + box.width / 2 + 40, box.y + box.height / 2, { steps: 8 }); + await page.mouse.up(); + + // A drag ends in a pointerup too, so without the "nothing moved" check + // every move would silently retarget the loop as well. + expect((await loopState(page)).enabled).toBe(false); + }); +}); diff --git a/tests/e2e/sections-rename.spec.mjs b/tests/e2e/sections-rename.spec.mjs new file mode 100644 index 00000000..bb32a8fb --- /dev/null +++ b/tests/e2e/sections-rename.spec.mjs @@ -0,0 +1,89 @@ +// Renaming a section is double-click, type, Enter. It had no browser test at +// all, which is how adding a click gesture to the same element could break it +// without anything going red. +import { test, expect } from "@playwright/test"; +import { openStudio, JOB_ID } from "./helpers.mjs"; + +test.afterEach(async ({ page }) => { + await page.request.patch(`/api/jobs/${JOB_ID}/sections`, { data: { sections: [] } }); +}); + +async function seed(page, sections) { + const res = await page.request.patch(`/api/jobs/${JOB_ID}/sections`, { data: { sections } }); + expect(res.ok(), "seeding sections").toBe(true); +} + +const savedSections = async (page) => { + const res = await page.request.get(`/api/jobs/${JOB_ID}`); + return (await res.json()).sections ?? []; +}; + +test.describe("renaming a section", () => { + test("double-clicking the label opens an input that keeps focus", async ({ page }) => { + await seed(page, [{ id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff" }]); + await openStudio(page); + + await page.locator('.section-block[data-id="s1"] .section-label').dblclick(); + + // The input must still be there a moment later. If anything steals focus, + // the blur handler commits and re-renders, and the field vanishes as fast + // as it appeared, which reads as "renaming does not work". + const input = page.locator(".section-rename-input"); + await expect(input).toBeVisible(); + await page.waitForTimeout(250); + await expect(input).toBeVisible(); + await expect(input).toBeFocused(); + }); + + test("typing a new name and pressing Enter saves it", async ({ page }) => { + await seed(page, [{ id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff" }]); + await openStudio(page); + + await page.locator('.section-block[data-id="s1"] .section-label').dblclick(); + await page.locator(".section-rename-input").fill("Chorus"); + await page.locator(".section-rename-input").press("Enter"); + + await expect(page.locator('.section-block[data-id="s1"] .section-label')).toHaveText("Chorus"); + await expect.poll(async () => (await savedSections(page))[0]?.name).toBe("Chorus"); + }); + + test("a locked section refuses to open a rename field", async ({ page }) => { + await seed(page, [ + { id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff", locked: true }, + ]); + await openStudio(page); + + await page.locator('.section-block[data-id="s1"] .section-label').dblclick(); + + await expect(page.locator(".section-rename-input")).toHaveCount(0); + await expect(page.locator('.section-block[data-id="s1"] .section-label')).toHaveText("Verse"); + }); + + test("unlocking restores renaming", async ({ page }) => { + await seed(page, [ + { id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff", locked: true }, + ]); + await openStudio(page); + + // A locked padlock is visible at rest, so no hover is needed here. + await page.locator('.section-block[data-id="s1"] .section-lock').click(); + await expect(page.locator('.section-block[data-id="s1"]')).not.toHaveClass(/sec-locked/); + + await page.locator('.section-block[data-id="s1"] .section-label').dblclick(); + await page.locator(".section-rename-input").fill("Bridge"); + await page.locator(".section-rename-input").press("Enter"); + + await expect.poll(async () => (await savedSections(page))[0]?.name).toBe("Bridge"); + }); + + test("Escape leaves the original name alone", async ({ page }) => { + await seed(page, [{ id: "s1", name: "Verse", start: 1, end: 3, color: "#4a7fff" }]); + await openStudio(page); + + await page.locator('.section-block[data-id="s1"] .section-label').dblclick(); + await page.locator(".section-rename-input").fill("Discarded"); + await page.locator(".section-rename-input").press("Escape"); + + await expect(page.locator('.section-block[data-id="s1"] .section-label')).toHaveText("Verse"); + }); +}); diff --git a/tests/test_jobs_api.py b/tests/test_jobs_api.py index e8447c0a..3a48d042 100644 --- a/tests/test_jobs_api.py +++ b/tests/test_jobs_api.py @@ -340,6 +340,47 @@ def test_sections_accepts_neutral_part_kind(client, done_job): assert response.json()["sections"][0]["kind"] == "part" +def test_sections_round_trip_the_locked_flag(client, done_job, tmp_path): + """A locked section must still be locked after a reload. + + This model takes Pydantic's default extra="ignore", so before `locked` was + declared the editor's flag was dropped here and answered 200. Locking + looked like it worked and came back false on the next load, with nothing in + the response, the logs or the file saying otherwise (#573). + """ + payload = { + "sections": [ + { + "id": "sec1", + "name": "Chorus", + "start": 0.0, + "end": 30.0, + "color": "#ff0000", + "locked": True, + } + ] + } + + response = client.patch(f"/api/jobs/{done_job.id}/sections", json=payload) + + assert response.status_code == 200 + assert response.json()["sections"][0]["locked"] is True + meta = json.loads((tmp_path / done_job.id / "metadata.json").read_text()) + assert meta["sections"][0]["locked"] is True + + +def test_sections_default_to_unlocked(client, done_job): + """Every section saved before this feature existed omits the field.""" + payload = { + "sections": [{"id": "sec1", "name": "Verse", "start": 0.0, "end": 30.0, "color": "#ff0000"}] + } + + response = client.patch(f"/api/jobs/{done_job.id}/sections", json=payload) + + assert response.status_code == 200 + assert response.json()["sections"][0]["locked"] is False + + def _section(index: int) -> dict: return { "id": f"sec{index}",