From 45303af2610dbae9921f3ae8966c5dfbae91d125 Mon Sep 17 00:00:00 2001 From: Thales <> Date: Sun, 13 Sep 2026 09:50:46 +0100 Subject: [PATCH 1/2] Pin a section against accidental moves A padlock beside the delete cross locks a section's position: drag and resize both refuse while it is set. The flag had to be declared on SectionItem to exist at all. That model takes Pydantic's default extra="ignore", so an undeclared `locked` sent by the editor was dropped on save and answered 200: locking would have looked right until the next load, with nothing in the response, the logs or metadata.json saying otherwise. Both new tests fail without the field. A locked block shows its padlock without being hovered. Hiding it would leave locked and unlocked identical at rest, and the only way to find a locked one would be to try to drag it. Addresses part of discussion #573. --- app/api/jobs.py | 6 ++++++ static/css/daw.css | 24 ++++++++++++++++++++++++ static/js/i18n.js | 20 ++++++++++++++++++++ static/js/sections.js | 42 ++++++++++++++++++++++++++++++++++++++++-- tests/test_jobs_api.py | 41 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 2 deletions(-) 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..782bd1d9 100644 --- a/static/css/daw.css +++ b/static/css/daw.css @@ -1667,6 +1667,30 @@ 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. 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. */ +.section-block.sec-locked .section-lock { display: flex; 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; diff --git a/static/js/i18n.js b/static/js/i18n.js index c9b26296..11eccd11 100644 --- a/static/js/i18n.js +++ b/static/js/i18n.js @@ -788,6 +788,8 @@ const en = { "sections.saving": "Saving", "sections.saved": "Saved", "sections.deleteAria": "Delete section", + "sections.lockAria": "Lock position", + "sections.unlockAria": "Unlock position", "sections.defaultName": "Section", // ── Playlist import confirm (static/js/playlist.js) ── @@ -1394,6 +1396,8 @@ const pl = { "sections.saving": "Zapisywanie", "sections.saved": "Zapisano", "sections.deleteAria": "Usuń sekcję", + "sections.lockAria": "Zablokuj pozycję", + "sections.unlockAria": "Odblokuj pozycję", "sections.defaultName": "Sekcja", "playlist.skip.unavailable.one": "{count} niedostępny", @@ -1983,6 +1987,8 @@ const ja = { "sections.saving": "保存中", "sections.saved": "保存済み", "sections.deleteAria": "セクションを削除", + "sections.lockAria": "位置をロック", + "sections.unlockAria": "位置のロックを解除", "sections.defaultName": "セクション", "playlist.skip.unavailable.other": "利用不可 {count}件", @@ -2562,6 +2568,8 @@ const zhHans = { "sections.saving": "正在保存", "sections.saved": "已保存", "sections.deleteAria": "删除段落", + "sections.lockAria": "锁定位置", + "sections.unlockAria": "解锁位置", "sections.defaultName": "段落", "playlist.skip.unavailable.other": "{count} 首不可用", @@ -3149,6 +3157,8 @@ const de = { "sections.saving": "Wird gespeichert", "sections.saved": "Gespeichert", "sections.deleteAria": "Abschnitt löschen", + "sections.lockAria": "Position sperren", + "sections.unlockAria": "Position entsperren", "sections.defaultName": "Abschnitt", "playlist.skip.unavailable.other": "{count} nicht verfügbar", @@ -3739,6 +3749,8 @@ const pt = { "sections.saving": "Salvando", "sections.saved": "Salvo", "sections.deleteAria": "Excluir seção", + "sections.lockAria": "Bloquear posição", + "sections.unlockAria": "Desbloquear posição", "sections.defaultName": "Seção", "playlist.skip.unavailable.one": "{count} indisponível", @@ -4323,6 +4335,8 @@ const id = { "sections.saving": "Menyimpan", "sections.saved": "Tersimpan", "sections.deleteAria": "Hapus bagian", + "sections.lockAria": "Kunci posisi", + "sections.unlockAria": "Buka kunci posisi", "sections.defaultName": "Bagian", "playlist.skip.unavailable.other": "{count} tidak tersedia", @@ -4910,6 +4924,8 @@ 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.defaultName": "Section", "playlist.skip.unavailable.one": "{count} indisponible", @@ -5627,6 +5643,8 @@ const es = { "sections.saving": "Guardando", "sections.saved": "Guardado", "sections.deleteAria": "Eliminar sección", + "sections.lockAria": "Bloquear posición", + "sections.unlockAria": "Desbloquear posición", "sections.defaultName": "Sección", // ── Playlist import confirm (static/js/playlist.js) ── @@ -6223,6 +6241,8 @@ const ko = { "sections.saving": "저장 중", "sections.saved": "저장됨", "sections.deleteAria": "구간 삭제", + "sections.lockAria": "위치 잠금", + "sections.unlockAria": "위치 잠금 해제", "sections.defaultName": "구간", "playlist.skip.unavailable.other": "사용할 수 없는 {count}개", diff --git a/static/js/sections.js b/static/js/sections.js index c832346c..67860107 100644 --- a/static/js/sections.js +++ b/static/js/sections.js @@ -94,17 +94,24 @@ 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); @@ -149,6 +156,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) { @@ -159,7 +180,11 @@ function _wireDrag(el, section) { let changed = 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; + // 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; @@ -214,6 +239,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; @@ -423,6 +449,18 @@ function _deleteSection(id) { _scheduleSave(); } +// Lock covers position only: drag and resize. Delete and rename stay available +// on a locked section, because both are deliberate acts on one named target, +// and the ask was to stop a section sliding when the pointer was aimed at +// something else (#573). +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); 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}", From a4901fd14367c65803a4adce75988b8fa09feca9 Mon Sep 17 00:00:00 2001 From: Thales <> Date: Sun, 13 Sep 2026 10:40:17 +0100 Subject: [PATCH 2/2] Turn a loop into a section, a section into a loop, and fix rename Add now builds the section on the armed loop instead of hunting for the first gap. That was the complaint: you select a region, press Add, and a section appears at the start of the track with nothing saying why. With no loop armed the old gap search is unchanged. Clicking a section arms the loop over it. A press that never moved was already doing nothing, so the gesture was free. It is tracked apart from the drag so a locked section can still be looped: that reads the section without changing it. Sections cannot overlap, so a loop drawn across one is refused and says so beside the button that was pressed, rather than being trimmed to fit or displacing a neighbour. Either would hand back a different region than the one selected, and displacing could move a section just locked. Renaming was already broken on main, and this is where it surfaced. The drag calls setPointerCapture, which retargets the rest of the gesture to the capturing element, so the dblclick ending a real double-click was delivered to the block while the listener sat on the child label and never ran. Synthetic events do not capture, which is why dispatching the sequence by hand worked perfectly and every actual double-click did not. The listener moves to the block. Rename had no browser test at all. Lock now covers editing rather than position alone: drag, resize and rename all refuse, and the padlock turns red, using the same --danger the rest of the app uses to say no. Delete stays available, because the cross is an explicit press on one named target. sections.js deliberately does not import transport.js. transport reaches the DOM at import time through state.js, and pulling it in broke this module's Node test outright. main.js owns both and hands the two functions over instead, the same reason loopRegion.js is separate. Seventeen browser tests, where there were none for lock or rename. Each was confirmed to fail without the change it covers, including the pair that would otherwise let a drag retarget the loop on every move, and the one asserting an unlocked section still drags, without which the locked case proves nothing. Addresses discussion #573, and #474 asked for the loop pairing too. --- static/css/daw.css | 25 ++++- static/js/i18n.js | 10 ++ static/js/main.js | 9 +- static/js/sections.js | 134 +++++++++++++++++++++++--- static/js/transport.js | 19 ++-- tests/e2e/sections-lock.spec.mjs | 132 +++++++++++++++++++++++++ tests/e2e/sections-loop.spec.mjs | 148 +++++++++++++++++++++++++++++ tests/e2e/sections-rename.spec.mjs | 89 +++++++++++++++++ 8 files changed, 540 insertions(+), 26 deletions(-) create mode 100644 tests/e2e/sections-lock.spec.mjs create mode 100644 tests/e2e/sections-loop.spec.mjs create mode 100644 tests/e2e/sections-rename.spec.mjs diff --git a/static/css/daw.css b/static/css/daw.css index 782bd1d9..6435728a 100644 --- a/static/css/daw.css +++ b/static/css/daw.css @@ -1681,10 +1681,16 @@ input, textarea { font-family: inherit; } } .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. 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. */ -.section-block.sec-locked .section-lock { display: flex; opacity: 1; } +/* 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, @@ -1731,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; @@ -1740,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 11eccd11..dc3ba924 100644 --- a/static/js/i18n.js +++ b/static/js/i18n.js @@ -790,6 +790,7 @@ const en = { "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) ── @@ -1398,6 +1399,7 @@ const pl = { "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", @@ -1989,6 +1991,7 @@ const ja = { "sections.deleteAria": "セクションを削除", "sections.lockAria": "位置をロック", "sections.unlockAria": "位置のロックを解除", + "sections.loopOverlaps": "ループが既存のセクションと重なっています", "sections.defaultName": "セクション", "playlist.skip.unavailable.other": "利用不可 {count}件", @@ -2570,6 +2573,7 @@ const zhHans = { "sections.deleteAria": "删除段落", "sections.lockAria": "锁定位置", "sections.unlockAria": "解锁位置", + "sections.loopOverlaps": "循环区域与已有段落重叠", "sections.defaultName": "段落", "playlist.skip.unavailable.other": "{count} 首不可用", @@ -3159,6 +3163,7 @@ const de = { "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", @@ -3751,6 +3756,7 @@ const pt = { "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", @@ -4337,6 +4343,7 @@ const id = { "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", @@ -4926,6 +4933,7 @@ const fr = { "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", @@ -5645,6 +5653,7 @@ const es = { "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) ── @@ -6243,6 +6252,7 @@ const ko = { "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 67860107..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 ─────────────────────────────────────────── @@ -117,9 +137,18 @@ function _makeSectionEl(section) { _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); @@ -179,8 +208,16 @@ 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,.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. @@ -189,7 +226,6 @@ function _wireDrag(el, section) { startX = e.clientX; origStart = section.start; origEnd = section.end; - changed = false; el.setPointerCapture(e.pointerId); el.classList.add("sec-dragging"); e.preventDefault(); @@ -210,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", () => { @@ -223,6 +269,7 @@ function _wireDrag(el, section) { _render(); } active = false; + pressed = false; el.classList.remove("sec-dragging"); }); } @@ -336,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); @@ -349,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 }; @@ -449,10 +529,15 @@ function _deleteSection(id) { _scheduleSave(); } -// Lock covers position only: drag and resize. Delete and rename stay available -// on a locked section, because both are deliberate acts on one named target, -// and the ask was to stop a section sliding when the pointer was aimed at -// something else (#573). +// 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; @@ -465,6 +550,10 @@ 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"; @@ -520,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"); + }); +});