fix: correctly import and notate multi-staff (piano/keys) tracks from GP8 (#692)

* fix: correctly import and notate multi-staff (piano/keys) tracks from GP8

Fixes bass stave being dropped on import (bar-column enumeration bug)
and wrong hand-split heuristic in notation_lift for chords straddling
middle C.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: byrongamatos <xasiklas@gmail.com>

* fix(gp-import): fold all grand-staff staves, per-stave tuning, playable hand-splits

Addresses review on #692 (topkoa):

- split_hands: only use the middle-C boundary when both resulting hands are
  within HAND_SPLIT_SPAN_SEMITONES, else fall back to the largest-gap
  heuristic — a hard middle-C split otherwise put a 19-semitone (unplayable)
  span in one hand for bass-under-treble voicings (e.g. E2+B3 under an Em7
  shape).
- Treat any multi-stave (grand-staff) track as keys end-to-end, so the
  stave-0 and folded stave-1+ notes share one encoding and note_count (which
  sums every stave column) matches what actually imports — closing the
  phantom-count case for grand-staff instruments the name/program heuristics
  miss (harp, celesta, marimba).
- Fold *every* extra stave (stave_columns[1:]), not just stave 1.
- Per-staff tuning fall-back to the track-level Tuning property so an untuned
  staff never yields an empty pitch list (silent note loss); via a shared
  _parse_tuning helper.
- Extract _collect_column_notes / _merge_lh_notes so the GPX LH/RH pair merge
  and the GP8 grand-staff fold share one implementation and can't drift in
  tie/timing/dedup handling.
- Rebuild filtered_to_raw from the already-computed stave_columns (one source
  of truth for the counting rule) and drop the dead num_raw_tracks/raw_tracks.

Tests: grand-staff fold + bar-column offset (test_gp2notation.py); both
middle-C split cases (test_notation_lift.py). CHANGELOG updated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: byrongamatos <xasiklas@gmail.com>

---------

Signed-off-by: byrongamatos <xasiklas@gmail.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
OmikronApex
2026-07-02 08:40:20 +02:00
committed by GitHub
co-authored by Claude Opus 4.8 byrongamatos
parent 991eadeff6
commit 749af31cc3
5 changed files with 343 additions and 145 deletions
+1
View File
@@ -58,6 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **v3 library: exact artist/album filters + scroll/page-depth restore** (feedBack#857). The v3 Songs toolbar gains Artist and Album dropdowns (Album populates from the selected artist and stays disabled until one is chosen), backed by new exact, case-insensitive (`COLLATE NOCASE`) `artist` / `album` query params threaded through `MetadataDB._build_where``query_page` / `query_artists` / `query_stats` and the `/api/library`, `/api/library/artists`, `/api/library/stats` endpoints (the free-text `q` search stays fuzzy and composes with the exact filters). The artist/album catalog is fetched independently of the active artist/album selection so the dropdowns always list the full set for the current provider/search. The toolbar is now sticky so filter controls stay reachable when browsing deep libraries, and returning from the player restores the previous scroll position **and** the loaded infinite-scroll page depth via a `sessionStorage` snapshot keyed by a filter/sort/view state hash (invalidated whenever those change, so a filter change still resets to the top). Tests: `tests/test_library_filters.py` (backend artist/album filters), `tests/js/v3_songs_scroll.test.js` (state-hash + snapshot helpers).
### Fixed
- **GP8 multi-staff (piano/keys) tracks now import both hands — the bass stave was being silently dropped, and hand-splits landed on the wrong hand.** A GP8 grand-staff keyboard part is one `<Track>` with two `<Staff>` entries, and `MasterBar/Bars` lists one bar id per **stave**, not per track (`lib/gp2rs_gpx.py`). Two bugs fell out of assuming one stave per track: (1) the bar-column lookup used a raw `enumerate(Tracks)` index, so every track *after* a multi-stave track read the wrong column; (2) the string-tuning parse scanned all `.//Property` descendants and let the last stave's `<Tuning>` overwrite the first, so a treble note indexed against the 5-entry bass tuning fell out of range in `_note_midi` and was **dropped without a trace**. The importer now advances a bar-column counter by each track's stave count, reads tuning **per stave** (with a per-staff fall-back to the track-level property so an untuned staff never yields empty pitches), and folds **every** extra stave's notes into the arrangement (not just stave 1), keeping the `note_count` import-preview honest. A grand-staff track is now classified as keys end-to-end so the stave-0 and folded stave-1+ notes share one encoding. Separately, `notation_lift.split_hands` no longer forces a hard middle-C split when doing so produces a physically unplayable hand (e.g. a bass note under an Em7-shape voicing dipping below C4 would put a 19-semitone span in one hand) — it uses the middle-C boundary only when both resulting hands are within `HAND_SPLIT_SPAN_SEMITONES`, else falls back to the largest-gap heuristic. The GPX LH/RH pair merge and the GP8 stave fold now share one `_collect_column_notes` / `_merge_lh_notes` pair so the two formats can't drift in tie/timing/dedup handling. Companion editor change: got-feedback/feedBack-plugin-editor#38. Tests: `tests/test_gp2notation.py` (grand-staff fold + bar-column offset), `tests/test_notation_lift.py` (both middle-C split cases). Follow-up: `lib/gp_autosync.py` still carries the pre-fix bar-column + tuning logic (CLI/tests only, no production caller).
- **Tuner auto-open is now opt-in and persists instead of flashing open-then-shut.** When you entered a song (or switched arrangement) whose tuning differed from the last, the tuner auto-opened and — for some testers — vanished ~1s later (reported macOS+Windows, 0.3.0). Root cause: the tuner closes itself on `song:play` (`plugins/tuner/utils/ui.js` — you don't tune while playing), so a **song switch** fired autoplay → `song:play` → the just-auto-opened tuner closed; an **arrangement switch** (which never arms autoplay) had no `song:play`, so it stayed open — exactly why two testers saw opposite behaviour (it wasn't the mic). Now: (1) the feature is a **new opt-in setting** ("Auto-open on tuning change", in the tuner's Settings panel, persisted as `autoOpenOnTuningChange`, **default OFF**); (2) an **auto**-opened tuner *persists* — it ignores the autoplay `song:play`, stray outside-clicks, and same-screen re-emits, closing only via the new in-panel **`×`** / **"Skip"** buttons or when you leave the song. A *manually* opened tuner keeps its classic click-away / play-to-close behaviour. The panel previously had no in-box close at all; this adds one (`×` + contextual Skip). All in the tuner plugin (`routes.py` config, `screen.js` gate + persist, `utils/ui.js` buttons + `song:play` guard, `settings.html` toggle) — **no core `app.js` changes**. Tests: `tests/js/tuner_auto_open.test.js` (opt-in gate, `{ auto: true }` persist mode, play/click-proofing). **Default (opt-in vs opt-out) is teed up for Byron to decide — flip one boolean.**
- **Tuner auto-open is now tuning-coverage-aware — extended-range players aren't nagged for songs their instrument already covers.** With the opt-in auto-open on, it now prompts only when your **current physical tuning** (from your instrument selection in Settings) doesn't already cover the song. FeedBack is tune-to-song — the highway draws tab in the song's tuning — so the check aligns the song's open-string tuning string-for-string against your instrument: an **8-string F♯-standard** player gets **no** prompt for a 6- or 7-string standard song (its top strings already match those tunings), while a song needing an open string you don't have (e.g. a **Drop-A 7-string**, whose low A isn't an open string on an F♯ 8-string) **still** prompts. A whole-instrument reference difference also prompts — A440 vs A432, or an octave-down `centOffset` (which the auto-open now accounts for; it was previously ignored). The player's instrument is read from core **`/api/settings`** (the v3 instrument selector — a stable physical reference, not the tuner's song-tracking selection); when nothing's declared or the lookup is unavailable it falls back to a conservative prompt, so a real retune is never silently skipped. **v3-only** (the instrument selector is v3). All in the tuner plugin (`plugins/tuner/screen.js`) — **no core changes**. Tests: `tests/js/tuner_auto_open.test.js` (covered vs uncovered, the Drop-A case, reference-pitch mismatch, contiguous alignment). _Follow-up (E1.6): a passive "different tuning" badge cue that names the string(s) to retune, plus the splitscreen / no-usable-input guards._
- **The tuner badge now passively flags when a song needs a different tuning — and names the retune.** Building on the coverage check: when you enter a song your current instrument doesn't cover, the topbar tuner badge gets an amber ring and a tooltip that **names the change** — e.g. *"retune B→A"* for a Drop-A song on an F♯ 8-string, or *"the reference pitch"* for an A440-vs-A432 mismatch. It's purely **advisory** (it never auto-opens the panel — tap the badge to tune), recomputed on `song:ready` and cleared when a new song loads or you leave the player. The retune diff comes from the tuner plugin's coverage report (`window._tunerAutoOpen.coverageReport``{ covered, retune: [{ from, to }], reference, cantCover }`); the cue is CSS-free (an inline ring + native tooltip — no Tailwind rebuild) and no-ops when the tuner plugin isn't installed. **v3-only.** Touches `static/v3/badges.js` (the cue) + `plugins/tuner/screen.js` (the report). Tests: `tests/js/tuner_auto_open.test.js` (the report names the strings; reference mismatch; the badge wiring). _(The splitscreen-suppress and no-usable-input guards move to the playback-gate stage, where they matter for its no-trap rule.)_