mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-23 05:11:34 +00:00
Fix tuner auto-open flash: opt-in + persist (issue E, stage 1/3) (#655)
The tuner self-closes on song:play; autoplay fires it right after a song switch, so an auto-opened tuner flashed shut ~1s later. An arrangement switch (which never arms autoplay) instead persisted — the opposite tester reports, and not the mic. - New opt-in setting autoOpenOnTuningChange (tuner Settings, default OFF) - 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 x / Skip buttons or leaving the song. A manual open keeps the classic click-away / play-to-close behaviour. - Adds the panel's first in-box close (x + contextual Skip). - All in the tuner plugin; no core app.js changes. Default (opt-in vs opt-out) is teed up for Byron to flip one boolean. Staged follow-ups: E1.5 = instrument-coverage smart prompting + badge cue; E2 = holdAutoplay gate. Tests: tests/js/tuner_auto_open.test.js (opt-in gate, persist mode, play/click-proofing). Claude-Session: https://claude.ai/code/session_01QbexxfTt8q2tAn436MqGWF Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
ChrisBeWithYou
GitHub
parent
bc4d2a3592
commit
6bfd92aa06
@ -50,6 +50,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
|
||||
- **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.**
|
||||
- **v3 player: opening another rail popover now closes the Section Practice popover (no more two stacked popovers).** Opening the **Practice** pill's popover and then clicking a different player-rail icon (e.g. **Plugins**) left the Practice popover open underneath the new one — looked broken (reported on macOS, 0.3.0 / 2026-06-28). The rail icons call `e.stopPropagation()` in their click handler (`static/v3/player-chrome.js`), which killed bubbling before it reached the Practice popover's outside-click dismiss bound on `document`. The dismiss (`_installSectionPracticeDismiss` in `static/app.js`) now binds in the **capture phase**, which runs before the target's handler so a descendant's `stopPropagation()` can't swallow it — mirroring how the audio-mixer popover already dismisses. Esc handling stays bubble-phase (the player's Escape-to-exit ordering is unchanged). v2 shares `app.js` and is only hardened (no rail `stopPropagation` there). Tests: `tests/js/section_practice_dismiss.test.js`.
|
||||
- **v3 UI no longer lets you accidentally text-select the chrome.** Dragging or double-clicking across the interface used to marquee-highlight buttons, labels, the sidebar, the transport, and the note-highway HUD — which looks broken (reported on Mac + Windows). The v3 shell now defaults to `user-select: none` on `html` (`static/v3/v3.css`), then opts *content* back in — so chrome is non-selectable but the text you actually copy still works. Decided by a 4-lens panel (UX / accessibility / dev-ops / plugin-ecosystem); the guardrails are deliberate: **form fields are always re-enabled** (never break the caret / IME — no `* { user-select:none }`, which trips a WebKit input bug); **plugin screens (`.screen[id^="plugin-"]`) stay selectable by default** so a plugin's copyable text (lyrics, chord names, results) — including community plugins that don't know about this — isn't silently locked; and **core read-only content opts back in by container** via a new hand-authored **`.fb-selectable`** class — applied to the whole **Settings** panel (paths, device names, version, diagnostics, About — answering "is settings still copyable?": yes), the **now-playing song metadata** (with `pointer-events` re-enabled so the HUD text is actually reachable), and the focused **modals / dialogs / toasts / scan banner** that carry copyable errors, IDs, paths, and file names. It's cosmetic only (it protects nothing) and never used to lock copy-worthy text — errors, IDs, paths, versions, and metadata stay selectable per WCAG 2.2 (copy-paste as a permitted mechanism). Dense card lists (library grid, dashboard, profile) stay non-selectable by design — making them selectable would reintroduce the marquee-mess across cards. **v3-only** (v2 unchanged); plain CSS, no Tailwind rebuild; no desktop changes (standard OS-framed window). Plugin authors: `.fb-selectable` is documented in `CLAUDE.md` for re-enabling copyable content rendered outside a plugin screen. Tests: `tests/js/v3_user_select_policy.test.js`.
|
||||
- **Input-setup wizard no longer collapses an audio device's driver-type variants into one entry.** On Windows the desktop engine enumerates the same interface once per host API (ASIO / Windows Audio / DirectSound), and the wizard's audio picker (`plugins/input_setup/screen.js`) de-duped the source list by display **label** — so the variants (which share a name) collapsed to a single choice, silently keeping whichever sorted first (often *not* the low-latency ASIO one the player wants). The audio-input capability already collapses true duplicates by `logicalSourceKey` (`_visibleInputSources` in `static/capabilities/audio-session.js`), and the variants each have a **distinct** key, so the wizard's extra label-collapse was redundant for real dupes and destructive for these — it also could drop the variant that was actually `selected`. Removed it; the picker now lists every selectable input. Pairs with feedBack-desktop's change to label each source with its driver type (e.g. "Focusrite (ASIO)") so the now-distinct entries are legible.
|
||||
|
||||
@ -37,6 +37,7 @@ def setup(app: FastAPI, context: dict):
|
||||
"showFloatingButton": True,
|
||||
"visualizationMode": "default",
|
||||
"audioInputMode": "auto",
|
||||
"autoOpenOnTuningChange": False,
|
||||
}
|
||||
if not config_file.exists():
|
||||
return defaults
|
||||
@ -55,6 +56,7 @@ def setup(app: FastAPI, context: dict):
|
||||
res["visualizationMode"] = str(data.get("visualizationMode", "default"))
|
||||
raw_mode = str(data.get("audioInputMode", "auto"))
|
||||
res["audioInputMode"] = raw_mode if raw_mode in ("auto", "browser") else "auto"
|
||||
res["autoOpenOnTuningChange"] = bool(data.get("autoOpenOnTuningChange", False))
|
||||
|
||||
if not isinstance(res["customTunings"], dict):
|
||||
res["customTunings"] = {}
|
||||
|
||||
@ -142,6 +142,12 @@
|
||||
async function _maybeAutoOpenOnTuningChange() {
|
||||
if (!document.getElementById('player')?.classList.contains('active')) return;
|
||||
|
||||
// Opt-in (default off): only auto-open when the user enabled it in the
|
||||
// tuner settings. Ensure config is loaded so the first song:ready after
|
||||
// boot still reads the real flag; fail closed if it can't load.
|
||||
if (!_state._serverConfig) { try { await loadConfig(); } catch (_) { /* */ } }
|
||||
if (!_state._serverConfig || !_state._serverConfig.autoOpenOnTuningChange) return;
|
||||
|
||||
const songInfo = window.highway?.getSongInfo?.() || window.feedBack?.currentSong;
|
||||
if (!songInfo) return;
|
||||
|
||||
@ -167,7 +173,7 @@
|
||||
|
||||
_lastAutoOpenSessionKey = sessionKey;
|
||||
try {
|
||||
await window.tuner.enable();
|
||||
await window.tuner.enable({ auto: true });
|
||||
if (myGen !== _autoOpenGeneration) return;
|
||||
} catch (e) {
|
||||
console.warn('Tuner: auto-open failed:', e && e.message ? e.message : e);
|
||||
@ -341,8 +347,15 @@
|
||||
}
|
||||
}
|
||||
|
||||
async function enable() {
|
||||
async function enable(opts) {
|
||||
if (_state.enabled) return;
|
||||
// An AUTO-open (the "this song needs a different tuning" nudge) must
|
||||
// PERSIST: it is NOT dismissed by the autoplay song:play that follows
|
||||
// song entry, a stray click, or a same-screen re-emit — only by the
|
||||
// Skip/× buttons or leaving the song. A manual open keeps the classic
|
||||
// click-away / play-to-close behaviour.
|
||||
const auto = !!(opts && opts.auto);
|
||||
_state.autoOpened = auto;
|
||||
await _loadScript('/api/plugins/tuner/utils/tuning-utils.js');
|
||||
await _loadScript('/api/plugins/tuner/utils/audio.js');
|
||||
await _loadScript('/api/plugins/tuner/utils/ui.js');
|
||||
@ -371,15 +384,27 @@
|
||||
_state.uiContainer.classList.add('flex');
|
||||
_tunerUIApi.positionPanel();
|
||||
_tunerUIApi.updateFreeTuneUI();
|
||||
// "Skip" is the auto-open nudge's explicit dismiss; hidden for a manual
|
||||
// open (the × / click-away already close those).
|
||||
if (_state.skipBtn) _state.skipBtn.classList.toggle('hidden', !auto);
|
||||
|
||||
// Close when clicking outside the panel. Deferred so the badge's
|
||||
// opening click doesn't bubble up to the document and fire immediately.
|
||||
if (_outsideClickClose) document.removeEventListener('click', _outsideClickClose);
|
||||
_outsideClickClose = () => { if (_state.enabled) disable(); };
|
||||
setTimeout(() => { if (_outsideClickClose) document.addEventListener('click', _outsideClickClose, { once: true }); }, 0);
|
||||
// Close when clicking outside the panel. Deferred so the badge's opening
|
||||
// click doesn't bubble up to the document and fire immediately. Skipped
|
||||
// for an auto-open: the user never clicked to open it, so their first
|
||||
// unrelated click must not dismiss it (it persists until Skip/×/leave).
|
||||
if (!auto) {
|
||||
if (_outsideClickClose) document.removeEventListener('click', _outsideClickClose);
|
||||
_outsideClickClose = () => { if (_state.enabled) disable(); };
|
||||
setTimeout(() => { if (_outsideClickClose) document.addEventListener('click', _outsideClickClose, { once: true }); }, 0);
|
||||
}
|
||||
|
||||
if (window.feedBack && !_onScreenChanged) {
|
||||
_onScreenChanged = () => { disable(); };
|
||||
// Auto-opened: close only when we actually LEAVE the song — a player
|
||||
// re-emit while staying put must not tear down the nudge. Manual:
|
||||
// unchanged (any screen change closes it).
|
||||
_onScreenChanged = () => {
|
||||
if (!_state.autoOpened || !document.getElementById('player')?.classList.contains('active')) disable();
|
||||
};
|
||||
_onSongReady = () => {
|
||||
_tunerUIApi.renderTuningOptions();
|
||||
if (_state.selectedTuningName === '_current') _syncCurrentTuning();
|
||||
@ -409,6 +434,7 @@
|
||||
const wasEnabled = _state.enabled;
|
||||
const onPlayer = document.getElementById('player')?.classList.contains('active');
|
||||
_state.enabled = false;
|
||||
_state.autoOpened = false;
|
||||
_state.manualTargetFreq = null;
|
||||
if (_outsideClickClose) { document.removeEventListener('click', _outsideClickClose); _outsideClickClose = null; }
|
||||
if (_state.activeViz) { _state.activeViz.destroy(); _state.activeViz = null; }
|
||||
|
||||
@ -10,6 +10,17 @@
|
||||
</label>
|
||||
</div>
|
||||
|
||||
<div class="flex items-center justify-between bg-dark-900/50 p-3 rounded-xl border border-gray-800/50">
|
||||
<div>
|
||||
<h3 class="text-sm font-medium text-gray-200">Auto-open on tuning change</h3>
|
||||
<p class="text-[11px] text-gray-500">When a song (or arrangement) needs a different tuning, pop the tuner open automatically. It stays open until you Skip or close it.</p>
|
||||
</div>
|
||||
<label class="relative inline-flex items-center cursor-pointer">
|
||||
<input type="checkbox" id="tuner-auto-open" class="sr-only peer" onchange="window._tunerToggleAutoOpen(this.checked)">
|
||||
<div class="w-9 h-5 bg-gray-700 peer-focus:outline-none rounded-full peer peer-checked:after:translate-x-full peer-checked:after:border-white after:content-[''] after:absolute after:top-[2px] after:left-[2px] after:bg-white after:border-gray-300 after:border after:rounded-full after:h-4 after:w-4 after:transition-all peer-checked:bg-accent"></div>
|
||||
</label>
|
||||
</div>
|
||||
|
||||
<script>
|
||||
if (window.feedBackDesktop && window.feedBackDesktop.isDesktop) {
|
||||
document.currentScript.insertAdjacentHTML('beforebegin', `
|
||||
@ -107,6 +118,9 @@
|
||||
const browserAudioToggle = document.getElementById('tuner-force-browser-audio');
|
||||
if (browserAudioToggle) browserAudioToggle.checked = config.audioInputMode === 'browser';
|
||||
|
||||
const autoOpenToggle = document.getElementById('tuner-auto-open');
|
||||
if (autoOpenToggle) autoOpenToggle.checked = config.autoOpenOnTuningChange === true;
|
||||
|
||||
render();
|
||||
} catch (e) { console.error('Tuner settings: load failed', e); }
|
||||
}
|
||||
@ -121,6 +135,11 @@
|
||||
save();
|
||||
};
|
||||
|
||||
window._tunerToggleAutoOpen = (enabled) => {
|
||||
config.autoOpenOnTuningChange = enabled;
|
||||
save();
|
||||
};
|
||||
|
||||
async function save(opts) {
|
||||
try {
|
||||
await fetch('/api/plugins/tuner/config', {
|
||||
|
||||
@ -565,6 +565,16 @@ window._tunerUI = function(state, actions) {
|
||||
title.textContent = 'TUNER';
|
||||
header.appendChild(title);
|
||||
|
||||
// Explicit close (the panel had no in-box dismiss before; persist mode
|
||||
// needs one). Mirrors the settings gear on the opposite side.
|
||||
const closeBtn = document.createElement('button');
|
||||
closeBtn.className = 'absolute left-0 text-fb-textDim hover:text-fb-text transition-colors text-lg leading-none';
|
||||
closeBtn.setAttribute('aria-label', 'Close tuner');
|
||||
closeBtn.title = 'Close';
|
||||
closeBtn.textContent = '×';
|
||||
closeBtn.onclick = () => actions.disable();
|
||||
header.appendChild(closeBtn);
|
||||
|
||||
const settingsBtn = document.createElement('button');
|
||||
settingsBtn.className = 'absolute right-0 text-fb-textDim hover:text-fb-text transition-colors';
|
||||
settingsBtn.innerHTML = `<svg xmlns="http://www.w3.org/2000/svg" class="h-4 w-4" fill="none" viewBox="0 0 24 24" stroke="currentColor"><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M10.325 4.317c.426-1.756 2.924-1.756 3.35 0a1.724 1.724 0 002.573 1.066c1.543-.94 3.31.826 2.37 2.37a1.724 1.724 0 001.065 2.572c1.756.426 1.756 2.924 0 3.35a1.724 1.724 0 00-1.066 2.573c.94 1.543-.826 3.31-2.37 2.37a1.724 1.724 0 00-2.572 1.065c-.426 1.756-2.924 1.756-3.35 0a1.724 1.724 0 00-2.573-1.066c-1.543.94-3.31-.826-2.37-2.37a1.724 1.724 0 00-1.065-2.572c-1.756-.426-1.756-2.924 0-3.35a1.724 1.724 0 001.066-2.573c-.94-1.543.826-3.31 2.37-2.37.996.608 2.296.07 2.572-1.065z"/><path stroke-linecap="round" stroke-linejoin="round" stroke-width="2" d="M15 12a3 3 0 11-6 0 3 3 0 016 0z"/></svg>`;
|
||||
@ -624,6 +634,16 @@ window._tunerUI = function(state, actions) {
|
||||
state.vizContainer.className = 'w-full';
|
||||
state.uiContainer.appendChild(state.vizContainer);
|
||||
|
||||
// Auto-open nudge's explicit dismiss (hidden unless auto-opened; enable()
|
||||
// toggles it). Closes the same way as the × — disable().
|
||||
const skipBtn = document.createElement('button');
|
||||
skipBtn.className = 'tuner-skip-btn hidden w-full mt-3 text-[11px] text-fb-textDim hover:text-fb-text border border-fb-border/40 hover:border-fb-border/70 rounded-lg py-1.5 transition-colors';
|
||||
skipBtn.textContent = 'Skip';
|
||||
skipBtn.title = 'Dismiss the tuner for this song';
|
||||
skipBtn.onclick = () => actions.disable();
|
||||
state.skipBtn = skipBtn;
|
||||
state.uiContainer.appendChild(skipBtn);
|
||||
|
||||
document.body.appendChild(state.uiContainer);
|
||||
state.uiContainer.addEventListener('click', (e) => e.stopPropagation());
|
||||
}
|
||||
@ -683,7 +703,11 @@ window._tunerUI = function(state, actions) {
|
||||
|
||||
const handlePlay = () => {
|
||||
updateFloatingButtonVisibility();
|
||||
if (state.enabled) actions.disable();
|
||||
// A manually-opened tuner closes when playback starts (you don't tune
|
||||
// while playing). An AUTO-opened tuner PERSISTS through the autoplay
|
||||
// song:play that immediately follows song entry — that auto-close was
|
||||
// the "opens then vanishes ~1s later" flash. It closes via Skip/×/leave.
|
||||
if (state.enabled && !state.autoOpened) actions.disable();
|
||||
};
|
||||
const handleStop = () => updateFloatingButtonVisibility();
|
||||
|
||||
|
||||
@ -26,7 +26,10 @@ function loadTuningHelpers() {
|
||||
|
||||
const feedBackHelpers = loadTuningHelpers();
|
||||
|
||||
function createTunerSandbox() {
|
||||
function createTunerSandbox(opts) {
|
||||
// Auto-open is opt-in (default off in prod). The sandbox defaults it ON so the
|
||||
// behaviour tests exercise the feature; pass { autoOpen: false } to gate it off.
|
||||
const autoOpen = !opts || opts.autoOpen !== false;
|
||||
const enableCalls = [];
|
||||
let playerActive = true;
|
||||
let songInfo = null;
|
||||
@ -44,6 +47,7 @@ function createTunerSandbox() {
|
||||
showFloatingButton: true,
|
||||
visualizationMode: 'default',
|
||||
audioInputMode: 'auto',
|
||||
autoOpenOnTuningChange: autoOpen,
|
||||
lastInstrument: 'guitar-6',
|
||||
lastTuning: 'Standard',
|
||||
freeTune: false,
|
||||
@ -155,9 +159,9 @@ function createTunerSandbox() {
|
||||
vm.runInContext(fs.readFileSync(TUNER_SCREEN_JS, 'utf8'), sandbox);
|
||||
|
||||
const realEnable = sandbox.window.tuner.enable.bind(sandbox.window.tuner);
|
||||
sandbox.window.tuner.enable = async () => {
|
||||
enableCalls.push(1);
|
||||
return realEnable();
|
||||
sandbox.window.tuner.enable = async (enableOpts) => {
|
||||
enableCalls.push(enableOpts || {});
|
||||
return realEnable(enableOpts);
|
||||
};
|
||||
|
||||
return sandbox;
|
||||
@ -315,6 +319,37 @@ test('song:loading clears dismiss state for next load', async () => {
|
||||
assert.equal(sandbox.__enableCalls.length, 2);
|
||||
});
|
||||
|
||||
test('auto-open is gated off when the setting is disabled (opt-in)', async () => {
|
||||
const sandbox = createTunerSandbox({ autoOpen: false });
|
||||
sandbox.window._tunerAutoOpen.resetState();
|
||||
await ready(sandbox, CUSTOM_GUITAR);
|
||||
await ready(sandbox, E_STANDARD); // a real tuning change, but the setting is off
|
||||
assert.equal(sandbox.__enableCalls.length, 0);
|
||||
});
|
||||
|
||||
test('auto-open enables in persist mode (passes { auto: true })', async () => {
|
||||
const sandbox = createTunerSandbox();
|
||||
sandbox.window._tunerAutoOpen.resetState();
|
||||
await ready(sandbox, CUSTOM_GUITAR);
|
||||
await ready(sandbox, E_STANDARD);
|
||||
assert.equal(sandbox.__enableCalls.length, 1);
|
||||
assert.equal(sandbox.__enableCalls[0].auto, true);
|
||||
});
|
||||
|
||||
test('persist: an auto-opened tuner is not torn down by autoplay / stray clicks', () => {
|
||||
const screenSrc = fs.readFileSync(TUNER_SCREEN_JS, 'utf8');
|
||||
const uiSrc = fs.readFileSync(
|
||||
path.join(__dirname, '..', '..', 'plugins', 'tuner', 'utils', 'ui.js'), 'utf8');
|
||||
// The gate is opt-in on the server config flag.
|
||||
assert.match(screenSrc, /autoOpenOnTuningChange/);
|
||||
// enable() records whether this was an auto-open …
|
||||
assert.match(screenSrc, /_state\.autoOpened\s*=\s*auto/);
|
||||
// … the outside-click dismiss is armed only for a manual open …
|
||||
assert.match(screenSrc, /if \(!auto\)[\s\S]*?addEventListener\('click'/);
|
||||
// … and the autoplay song:play closer ignores an auto-opened tuner (the flash fix).
|
||||
assert.match(uiSrc, /state\.enabled && !state\.autoOpened/);
|
||||
});
|
||||
|
||||
test('screen.js registers song:loading and song:ready auto-open listeners at boot', () => {
|
||||
const src = fs.readFileSync(TUNER_SCREEN_JS, 'utf8');
|
||||
assert.match(src, /function _installAutoOpenListeners/);
|
||||
|
||||
Loading…
Reference in New Issue
Block a user