diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c304a21..ec13d163 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **Saving no longer strips `type` / `centOffset` / unknown keys from manifest + arrangement entries.** The full-snapshot save path rebuilt every manifest + arrangement entry from scratch (`{id, name, file, tuning, capo}`), silently + dropping spec fields the editor doesn't author — `type` (§5.2), `centOffset`, + and any future additive key — on every save, violating the format's + unknown-key preservation rule (§1.2). Rebuilt entries now merge onto the + existing entry for the same id, with editor-owned keys taking the fresh + values. Tests: `tests/test_manifest_type_preserve.py`. + +### Added +- **Infer-once arrangement `type` stamping.** On save, an arrangement entry with + no `type` gets one inferred from its display name (keys-family → `piano`, bass + names → `bass`, classic guitar roles → `guitar`) — conservative on purpose: + vocals/drums/ambiguous names stay untyped, and an authored `type` is never + clobbered. `type` is the queryable instrument facet the spec defines; display + names stay free labels — groundwork for safe track renaming in the Parts view. + ### Changed - **Canvas repaints are coalesced to one per animation frame.** `draw()` was called imperatively from ~150 sites and each call repainted the whole diff --git a/routes.py b/routes.py index 3015a8f6..b6ad2eee 100644 --- a/routes.py +++ b/routes.py @@ -175,6 +175,58 @@ def _load_arrangement_json(path) -> dict: return json.loads(raw) +# Keys-family arrangement names (piano/keyboard/synth). The SAME matcher — a +# word-boundary search over the SAME keyword set — decides both the manifest +# ``type`` facet (below) and the keys notation sidecar (see the sidecar detector +# further down, which reuses this regex). Keeping them identical guarantees a +# name that earns a keys sidecar also infers a keys/piano type, so the two never +# disagree (e.g. "Electric Piano", "Grand Piano", "Lead Synth"). +_KEYS_NAME_RE = re.compile(r"\b(keys|piano|keyboard|synth)\b", re.IGNORECASE) +_TYPE_BASS_RE = re.compile(r"bass", re.I) +_TYPE_SKIP_RE = re.compile(r"vocal|voice|sing|drum", re.I) +_TYPE_GUITAR_RE = re.compile(r"guitar|lead|rhythm|combo|acoustic|electric", re.I) + + +def _infer_arrangement_type(name) -> str: + """Manifest ``type`` (feedpak-spec §5.2) inferred from a display name. + + The infer-once migration: callers write this only when an entry carries + no ``type`` yet, never clobbering an authored value. Conservative on + purpose — a name that doesn't clearly identify an instrument returns "" + and the entry stays untyped, because writing a WRONG type is worse than + writing none. Vocals/drums names also return "": per the spec those are + side-file mechanisms, not arrangement types. + """ + n = (name or "").strip() + if not n: + return "" + # keys checked before bass so "Synth Bass" resolves to piano (intentional: + # a keys arrangement that happens to sit in the bass register still wants + # keys notation). Word-boundary search shared with the sidecar detector. + if _KEYS_NAME_RE.search(n): + return "piano" + if _TYPE_BASS_RE.search(n): + return "bass" + if _TYPE_SKIP_RE.search(n): + return "" + if _TYPE_GUITAR_RE.search(n): + return "guitar" + return "" + + +def _merge_manifest_entry(old_entry, rebuilt: dict) -> dict: + """Merge a rebuilt manifest arrangement entry ONTO the existing entry + with the same id, so spec fields the editor doesn't author (``type``, + ``centOffset``, future additive keys) survive a full-snapshot save — + the format's unknown-key preservation rule (feedpak-spec §1.2). The + editor-owned keys (id/name/file/tuning/capo) always take the rebuilt + value. Previously the full-snapshot save path rebuilt every entry from + scratch and silently dropped everything else.""" + out = dict(old_entry) if isinstance(old_entry, dict) else {} + out.update(rebuilt) + return out + + def _timeline_round_time(value) -> float: try: return round(float(value), 3) @@ -1724,7 +1776,9 @@ def _note_in_chord(n): # shared core in slopsmith `lib/notation_lift.py` (factored out of the one-time # scripts/lift_keys_notation.py lifter), reached here the same way the rest of # this module reaches core helpers — `lib/` is on the host app's import path. -_KEYS_NAME_RE = re.compile(r"\b(keys|piano|keyboard|synth)\b", re.IGNORECASE) +# The keys-family name matcher (``_KEYS_NAME_RE``) is defined up by the manifest +# type-inference helpers and shared here, so the sidecar and the ``type`` facet +# always agree on what counts as a keys arrangement. _NOTATION_SAFE_ID_RE = re.compile(r"[A-Za-z0-9_-]+") @@ -3471,6 +3525,13 @@ def _build_wire(arr_dict, is_first): # removed or for safety on every save. used_ids: set = set() merged_arrangements = [] + # Existing manifest entries by id: rebuilt entries merge onto + # these (see _merge_manifest_entry) so `type`/`centOffset`/ + # unknown additive keys survive the full-snapshot save. + _old_by_id = { + e.get("id"): e for e in old_entries + if isinstance(e, dict) and e.get("id") + } for i, ad in enumerate(all_arrangements): raw_id = ad.get("id") or "" if raw_id and raw_id not in used_ids: @@ -3479,13 +3540,13 @@ def _build_wire(arr_dict, is_first): aid = _arrangement_id(ad.get("name", "arr"), used_ids) used_ids.add(aid) wire = _build_wire(ad, i == 0) - _entry = { + _entry = _merge_manifest_entry(_old_by_id.get(aid), { "id": aid, "name": ad.get("name", "arr"), "file": f"arrangements/{aid}.json", "tuning": list(ad.get("tuning", [0]*6)), "capo": int(ad.get("capo", 0)), - } + }) # Carry any GP-import notation alongside the entry (NOT on # it — keeping it off the manifest entry means it can never # leak into manifest.yaml); the sidecar writer consumes it. @@ -3502,6 +3563,13 @@ def _build_wire(arr_dict, is_first): for item in merged_arrangements: entry = item["entry"] wire = item["wire"] + # Infer-once `type` stamping (both save paths flow through + # here): only when the entry has no type yet — an authored + # value is never clobbered, and ambiguous names stay untyped. + if isinstance(entry, dict) and not entry.get("type"): + _t = _infer_arrangement_type(entry.get("name", "")) + if _t: + entry["type"] = _t if wire is not None: rel = entry.get("file") or f"arrangements/{entry.get('id', 'arr')}.json" arr_path = (source_dir / rel).resolve() diff --git a/tests/test_manifest_type_preserve.py b/tests/test_manifest_type_preserve.py new file mode 100644 index 00000000..5770636c --- /dev/null +++ b/tests/test_manifest_type_preserve.py @@ -0,0 +1,121 @@ +"""Tests for manifest `type` preservation + infer-once stamping. + +The full-snapshot save path rebuilt every manifest arrangement entry from +scratch (`{id, name, file, tuning, capo}`), silently dropping spec fields the +editor doesn't author — `type` (§5.2), `centOffset`, and any future additive +key — on EVERY save, violating the format's unknown-key preservation rule +(feedpak-spec §1.2). Rebuilt entries now merge onto the existing entry for +the same id (`_merge_manifest_entry`), and entries with no `type` get an +inferred one from the display name exactly once (`_infer_arrangement_type`) +— conservative: ambiguous / vocals / drums names stay untyped, because a +wrong type is worse than none. + +Run: python -m pytest tests/test_manifest_type_preserve.py -q +""" + +from routes import _infer_arrangement_type, _merge_manifest_entry # noqa: E402 + + +# ── inference ──────────────────────────────────────────────────────────────── + +def test_infer_keys_family_maps_to_piano(): + # Spec §5.2 spells the keyboard type `piano` (`keys` is a read alias). + for name in ("Keys", "Piano", "keyboard", "Synth Lead", "PIANO RH"): + assert _infer_arrangement_type(name) == "piano", name + + +def test_infer_keys_family_matches_mid_name_like_the_sidecar_detector(): + # Regression: type inference must use the SAME word-boundary matcher as the + # keys notation-sidecar detector (`_KEYS_NAME_RE`), not a prefix-anchored + # one. Names where the keys keyword isn't the first word still earn a keys + # sidecar, so they must infer a keys/piano `type` too — otherwise the + # manifest facet and the notation renderer disagree for the same entry. + for name in ("Electric Piano", "Grand Piano", "Lead Synth", "Rhodes Keys"): + assert _infer_arrangement_type(name) == "piano", name + + +def test_infer_bass_anywhere_in_the_name(): + assert _infer_arrangement_type("Bass") == "bass" + assert _infer_arrangement_type("5-string Bass") == "bass" + assert _infer_arrangement_type("bass (DI)") == "bass" + # Keys is checked before bass, so synth-family names take the piano-roll + # pathway even when they also read as bass (intentional, unchanged by the + # prefix→word-boundary switch: "synth" still matches at a word boundary). + assert _infer_arrangement_type("Synth Bass") == "piano" + + +def test_infer_guitar_roles(): + for name in ("Lead", "Rhythm 2", "Combo", "Guitar 3", "Acoustic", "Electric"): + assert _infer_arrangement_type(name) == "guitar", name + + +def test_infer_stays_silent_on_ambiguous_vocals_and_drums_names(): + # Vocals/drums are side-file mechanisms per the spec, never arrangement + # types; unknown names stay untyped rather than guessing wrong. + for name in ("Vocals", "Voice", "Singing", "Drums", "Solo Thing", "Track 7", "", None): + assert _infer_arrangement_type(name) == "", repr(name) + + +# ── merge ──────────────────────────────────────────────────────────────────── + +def test_merge_preserves_spec_and_unknown_keys(): + old = { + "id": "lead", + "name": "Old Name", + "file": "arrangements/lead.json", + "tuning": [0, 0, 0, 0, 0, 0], + "capo": 0, + "type": "guitar", + "centOffset": -6, + "notation": "notation_lead.json", + "x_future_key": {"nested": True}, + } + rebuilt = { + "id": "lead", + "name": "Lead (renamed)", + "file": "arrangements/lead.json", + "tuning": [-2, -2, -2, -2, -2, -2], + "capo": 2, + } + out = _merge_manifest_entry(old, rebuilt) + # Editor-owned keys take the rebuilt values… + assert out["name"] == "Lead (renamed)" + assert out["tuning"] == [-2, -2, -2, -2, -2, -2] + assert out["capo"] == 2 + # …and everything the editor doesn't author survives. + assert out["type"] == "guitar" + assert out["centOffset"] == -6 + assert out["notation"] == "notation_lead.json" + assert out["x_future_key"] == {"nested": True} + + +def test_merge_preserves_zero_cent_offset(): + # Guard against a truthiness regression: a deliberate `centOffset: 0` + # (retuned back to concert pitch) must survive the merge exactly like a + # non-zero offset. The merge keeps it because the rebuilt entry never + # carries centOffset, so `out.update(rebuilt)` can't clobber it — but a + # future "only copy truthy fields" shortcut would silently drop the 0. + old = {"id": "lead", "name": "Old", "centOffset": 0, "type": "guitar"} + rebuilt = {"id": "lead", "name": "Lead", "file": "arrangements/lead.json", + "tuning": [0] * 6, "capo": 0} + out = _merge_manifest_entry(old, rebuilt) + assert "centOffset" in out + assert out["centOffset"] == 0 + assert out["type"] == "guitar" + + +def test_merge_with_no_old_entry_is_just_the_rebuilt_entry(): + rebuilt = {"id": "new", "name": "New", "file": "arrangements/new.json", + "tuning": [0] * 6, "capo": 0} + assert _merge_manifest_entry(None, rebuilt) == rebuilt + assert _merge_manifest_entry("garbage", rebuilt) == rebuilt + + +def test_merge_does_not_mutate_inputs(): + old = {"id": "a", "type": "bass"} + rebuilt = {"id": "a", "name": "A"} + out = _merge_manifest_entry(old, rebuilt) + out["type"] = "changed" + out["name"] = "changed" + assert old["type"] == "bass" + assert rebuilt["name"] == "A"