diff --git a/docs/architecture/invariants.md b/docs/architecture/invariants.md index be905c31c..b5a2cb682 100644 --- a/docs/architecture/invariants.md +++ b/docs/architecture/invariants.md @@ -47,10 +47,14 @@ The hand-rolled copies had drifted apart, each carrying a different subset of th Converted so far — a sample, not a roster, and deliberately not kept exhaustive: `useVisualizerStyle`, `useEstimatedKaraoke`, `useLyricsLookupSettings`, `useDesktopLyricsStyle`, `useContrastMode`, `useTrackColumns`, `useScrollLongTitles`, `useArtistBioCollapsed`, `useHiddenKpis`, `useCoverSlideshow`, `useVisualizerColor`, `useArtistHero`, `useWrappedBannerVisibility` (two keys ⇒ two instances, each on its **own** broadcast channel — sharing one would make a write to either key re-read the other and clobber its in-flight optimistic value). -Still standalone, by shape rather than by oversight: `useWebRadioFavorites` (persists through the plugin-favorites commands, not `profile_setting`), `useHiResBadgeVisibility` (module-level store + subscription so a non-React consumer can read it), `useSortMemory` (many dynamic keys), [`useProfileScopedToggle`](../../src/hooks/useProfileScopedToggle.ts) (the engine toggles push their value to the audio threads on the way in, so they own their own commands rather than going through `profile_setting`), and the direct `setProfileSetting` calls in `ThemeContext` / `SkinContext` / `shortcuts.ts` / a few Settings cards. +Still standalone, by shape rather than by oversight: `useWebRadioFavorites` (persists through the plugin-favorites commands, not `profile_setting`), `useHiResBadgeVisibility` (module-level store + subscription so a non-React consumer can read it), `useSortMemory` (many dynamic keys), [`useProfileScopedToggle`](../../src/hooks/useProfileScopedToggle.ts) (the engine toggles push their value to the audio threads on the way in, so they own their own commands rather than going through `profile_setting`; it carries its own serialized write chain, below), and the direct `setProfileSetting` calls in `ThemeContext` / `SkinContext` / `shortcuts.ts` / a few Settings cards. The hook also passes the captured profile id to [`set_profile_setting` / `get_profile_setting`](../../src-tauri/crates/app/src/commands/profile.rs), which validate it through [`AppState::require_profile_pool_for`](../../src-tauri/crates/app/src/state.rs) **under the same lock `switch_profile` takes**. A JS-side "is this still my profile?" check runs before the IPC hop, so only the backend can stop a queued write from landing in the profile the user just switched to. Passing `None` opts out and targets whatever is active — correct for a fresh user action, wrong for anything that awaited first. +**The engine setting commands take the same id** (issue #996). Each `player_set_*` that saves a `profile_setting` row and then hands the value to the engine — every setter in [`commands/player/sound.rs`](../../src-tauri/crates/app/src/commands/player/sound.rs) but the volume, and the match-source-rate, pause-on-device-loss and keep-awake ones in `output.rs` — takes an optional `expected_profile_id`. The `sound.rs` ones go through [`save_then_apply`](../../src-tauri/crates/app/src/commands/player/settings.rs): the pool is leased through the same check (`state::lease_profile`), a profile that changed fails the command with `ProfileChanged` before anything is saved or applied, the rows are saved, and the engine is touched only while the profile they went into is still the active one, under the profile lock a switch takes for writing (`state::while_profile_is`). A switch that lands after the lease leaves the rows in the profile they were made for and skips the engine step, and the command still succeeds: the setting was saved where it belongs. A command that lost the race to a switch therefore never overwrites the values `switch_profile` restored for the next profile. `None` keeps the contract these commands always had: whatever profile is active, and with none at all the value is applied without being saved. The three `output.rs` ones keep requiring a profile instead: with none active they fail before anything is applied, with `NoActiveProfile` under `None` and with `ProfileChanged` (`active: None`) under an id. Every frontend caller passes the id it captured — the Settings handlers, the settings reset, the stereo-image card, the speed control — and the handlers check that the profile is still theirs before rolling back a failure, since a refused write now fails where it used to land in the wrong profile. Still on whatever is active: the exclusive output and the output device (applied by a blocking device rebuild), the EQ and the volume. + +[`useProfileScopedToggle`](../../src/hooks/useProfileScopedToggle.ts) also chains its writes (issue #995): one at a time, a queued write that a later click superseded is skipped, a write whose profile is no longer active is not sent, and a failure that is still the last decision rolls back to the value the backend last confirmed. Saved first, then applied, as each command does it — the chain only orders the commands. + ### Events The backend emits Tauri events; the frontend listens via `listen()` from `@tauri-apps/api/event`: diff --git a/src-tauri/crates/app/src/commands/player/mod.rs b/src-tauri/crates/app/src/commands/player/mod.rs index 5d87802a1..85a99dc2b 100644 --- a/src-tauri/crates/app/src/commands/player/mod.rs +++ b/src-tauri/crates/app/src/commands/player/mod.rs @@ -126,3 +126,6 @@ pub(crate) async fn fetch_replay_gain(pool: &sqlx::SqlitePool, track_id: i64) -> #[cfg(test)] mod album_ordering_tests; + +#[cfg(test)] +mod setting_write_tests; diff --git a/src-tauri/crates/app/src/commands/player/output.rs b/src-tauri/crates/app/src/commands/player/output.rs index e44e6ce85..2affdfb58 100644 --- a/src-tauri/crates/app/src/commands/player/output.rs +++ b/src-tauri/crates/app/src/commands/player/output.rs @@ -273,14 +273,19 @@ pub async fn player_set_match_source_rate( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { // Written before it is applied, and the error propagates. A // preference the UI believes it saved, and that comes back to its // old value at the next launch or the next profile switch, is worse // than a toggle that visibly refuses — and this one takes effect on // the next track, so there is nothing to undo in the engine when the - // write fails. - let pool = state.require_profile_pool().await?; + // write fails. Into the profile the toggle was flipped in, or + // nowhere (#996): see `save_then_apply`, whose rule this follows + // with a profile required. + let (pool, written_profile) = state + .require_profile_pool_with_id(expected_profile_id) + .await?; let now = chrono::Utc::now().timestamp_millis(); sqlx::query( "INSERT INTO profile_setting (key, value, value_type, updated_at) @@ -291,10 +296,14 @@ pub async fn player_set_match_source_rate( .bind(now) .execute(&*pool) .await?; - engine - .shared() - .match_source_rate - .store(enabled, std::sync::atomic::Ordering::Release); + drop(pool); + crate::state::while_profile_is(&state.profile, Some(written_profile), || { + engine + .shared() + .match_source_rate + .store(enabled, std::sync::atomic::Ordering::Release); + }) + .await; Ok(()) } @@ -349,11 +358,15 @@ pub async fn player_set_pause_on_device_loss( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { // Persisted first, same as `player_set_match_source_rate` and for // the same reason: a setting that only ever lived in memory is a - // setting the user will find undone. - let pool = state.require_profile_pool().await?; + // setting the user will find undone. Bound to the profile the same + // way too. + let (pool, written_profile) = state + .require_profile_pool_with_id(expected_profile_id) + .await?; let now = chrono::Utc::now().timestamp_millis(); let stored = if enabled { "1" } else { "0" }; sqlx::query( @@ -365,7 +378,11 @@ pub async fn player_set_pause_on_device_loss( .bind(now) .execute(&*pool) .await?; - engine.set_pause_on_device_loss(enabled); + drop(pool); + crate::state::while_profile_is(&state.profile, Some(written_profile), || { + engine.set_pause_on_device_loss(enabled) + }) + .await; Ok(()) } @@ -409,10 +426,10 @@ pub async fn player_set_keep_awake( // applied while this is still the active profile, or not at all — a // switch that got in first has run, or will run, the next profile's // own restore, which must not be overwritten with this one's value. - let active = state.profile.read().await; - if active.as_ref().map(|p| p.profile_id) == Some(written_profile) { - crate::power::set_enabled(enabled); - } + crate::state::while_profile_is(&state.profile, Some(written_profile), || { + crate::power::set_enabled(enabled) + }) + .await; Ok(()) } diff --git a/src-tauri/crates/app/src/commands/player/setting_write_tests.rs b/src-tauri/crates/app/src/commands/player/setting_write_tests.rs new file mode 100644 index 000000000..9644228d8 --- /dev/null +++ b/src-tauri/crates/app/src/commands/player/setting_write_tests.rs @@ -0,0 +1,134 @@ +//! A setting command writes into the profile its control was acted on, or +//! nowhere (issue #996). Driven through AutoMix's command body, on a +//! profile slot of the test's own: an `AppState` cannot be built here. + +use sqlx::SqlitePool; +use tokio::sync::RwLock; + +use super::*; +use crate::{ + audio::state::SharedPlayback, + error::AppError, + state::{while_profile_is, ActiveProfile}, +}; + +/// A profile database with the repo's migrations, `foreign_keys` on as on +/// every profile pool. +async fn profile_db(dir: &std::path::Path, name: &str) -> SqlitePool { + use sqlx::sqlite::SqliteConnectOptions; + use std::str::FromStr; + let opts = SqliteConnectOptions::from_str(&format!("sqlite://{}", dir.join(name).display())) + .unwrap() + .create_if_missing(true) + .foreign_keys(true); + let pool = SqlitePool::connect_with(opts).await.unwrap(); + sqlx::migrate!("../../migrations/profile") + .run(&pool) + .await + .unwrap(); + pool +} + +async fn automix_row(pool: &SqlitePool) -> Option { + sqlx::query_scalar("SELECT value FROM profile_setting WHERE key = 'audio.automix'") + .fetch_optional(pool) + .await + .unwrap() +} + +#[tokio::test] +async fn a_toggle_lands_in_the_profile_it_was_flipped_in() { + let dir = tempfile::tempdir().unwrap(); + let a = profile_db(dir.path(), "a.db").await; + let slot = RwLock::new(Some(ActiveProfile::for_test(1, a.clone()))); + let shared = SharedPlayback::new(); + + set_automix(&slot, &shared, true, Some(1)).await.unwrap(); + + assert_eq!(automix_row(&a).await.as_deref(), Some("true")); + assert!(shared.automix.is_enabled()); +} + +#[tokio::test] +async fn a_toggle_for_a_profile_since_left_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let a = profile_db(dir.path(), "a.db").await; + let b = profile_db(dir.path(), "b.db").await; + let slot = RwLock::new(Some(ActiveProfile::for_test(1, a.clone()))); + let shared = SharedPlayback::new(); + + // Flipped in profile 1; the switch to profile 2 lands before the + // command reaches the pool. + *slot.write().await = Some(ActiveProfile::for_test(2, b.clone())); + let err = set_automix(&slot, &shared, true, Some(1)) + .await + .unwrap_err(); + + assert!( + matches!( + err, + AppError::ProfileChanged { + expected: 1, + active: Some(2) + } + ), + "unexpected error: {err}", + ); + // Nothing written anywhere, and the engine keeps what profile 2's + // restore put there. + assert_eq!(automix_row(&b).await, None); + assert_eq!(automix_row(&a).await, None); + assert!(!shared.automix.is_enabled()); +} + +#[tokio::test] +async fn a_toggle_for_a_profile_since_closed_is_refused() { + let slot = RwLock::new(None); + let shared = SharedPlayback::new(); + + let err = set_automix(&slot, &shared, true, Some(1)) + .await + .unwrap_err(); + + assert!( + matches!( + err, + AppError::ProfileChanged { + expected: 1, + active: None + } + ), + "unexpected error: {err}", + ); + assert!(!shared.automix.is_enabled()); +} + +#[tokio::test] +async fn no_profile_in_mind_and_none_active_applies_without_saving() { + // The contract the setters always had: a caller with no profile in + // mind, with none open, still changes what plays. + let slot = RwLock::new(None); + let shared = SharedPlayback::new(); + + set_automix(&slot, &shared, true, None).await.unwrap(); + + assert!(shared.automix.is_enabled()); +} + +#[tokio::test] +async fn the_engine_step_runs_only_for_the_active_profile() { + // The guard `save_then_apply` puts around the engine step, after the + // save: once profile 2 is active, a step for profile 1 is skipped, so + // a command that saved into profile 1 just before the switch cannot + // overwrite profile 2's restore. + let dir = tempfile::tempdir().unwrap(); + let a = profile_db(dir.path(), "a.db").await; + let b = profile_db(dir.path(), "b.db").await; + let slot = RwLock::new(Some(ActiveProfile::for_test(1, a))); + + *slot.write().await = Some(ActiveProfile::for_test(2, b)); + + assert_eq!(while_profile_is(&slot, Some(1), || ()).await, None); + assert_eq!(while_profile_is(&slot, Some(2), || 7).await, Some(7)); + assert_eq!(while_profile_is(&slot, None, || ()).await, None); +} diff --git a/src-tauri/crates/app/src/commands/player/settings.rs b/src-tauri/crates/app/src/commands/player/settings.rs index 884ce4282..3863633a5 100644 --- a/src-tauri/crates/app/src/commands/player/settings.rs +++ b/src-tauri/crates/app/src/commands/player/settings.rs @@ -3,11 +3,13 @@ use serde::Serialize; use tauri::{AppHandle, Emitter}; +use tokio::sync::RwLock; use crate::{ audio::{replay_gain::GainMode, AudioEngine}, - error::AppResult, + error::{AppError, AppResult}, queue::{self}, + state::{lease_profile, while_profile_is, ActiveProfile}, }; use super::*; @@ -130,6 +132,60 @@ pub(super) async fn persist_setting( Ok(()) } +/// The `profile_setting` row of an on/off setting, spelled the way the +/// setters have always written it. +pub(super) fn bool_row(key: &'static str, enabled: bool) -> (&'static str, String, SettingType) { + ( + key, + if enabled { "true" } else { "false" }.to_string(), + SettingType::Bool, + ) +} + +/// Save a setting into the profile its control was acted on, then apply +/// it to the engine (issue #996). +/// +/// `expected` is that profile's id, as the frontend captured it when the +/// user acted: +/// +/// - **`Some(id)`** — the pool is leased only if `id` is still the active +/// profile, checked under the lock a switch takes; otherwise the command +/// fails with [`AppError::ProfileChanged`](crate::error::AppError) and +/// neither saves nor applies anything. A switch landing between the +/// click and the command can no longer write one profile's choice into +/// the next one. +/// - **`None`** — whatever profile is active, for a caller with no profile +/// in mind. With none active at all the setting is applied without being +/// saved, the contract these commands have always had. +/// +/// Saved first, then applied: a value the database refuses never reaches +/// the engine. The lease is released before the engine is touched, and +/// the engine is touched only while the profile the rows went into is +/// still the active one ([`while_profile_is`]): a switch that got in +/// between has restored, or is about to restore, the next profile's own +/// values, and a stale command must not overwrite them. The rows are saved +/// either way — they belong to that profile. +pub(super) async fn save_then_apply( + profile: &RwLock>, + expected: Option, + rows: &[(&str, String, SettingType)], + apply: impl FnOnce() -> AppResult<()>, +) -> AppResult<()> { + let written = match lease_profile(profile, expected).await { + Ok((pool, profile_id)) => { + for (key, value, value_type) in rows { + persist_setting(&pool, key, value, *value_type).await?; + } + Some(profile_id) + } + Err(AppError::NoActiveProfile) => None, + Err(err) => return Err(err), + }; + while_profile_is(profile, written, apply) + .await + .unwrap_or(Ok(())) +} + /// Read a boolean `profile_setting`. `"true"` / `"1"` read as true and /// `"false"` / `"0"` as false; a row that is missing, unreadable or /// holds anything else resolves to `default`. diff --git a/src-tauri/crates/app/src/commands/player/sound.rs b/src-tauri/crates/app/src/commands/player/sound.rs index 51054b6f8..d4d19f1bc 100644 --- a/src-tauri/crates/app/src/commands/player/sound.rs +++ b/src-tauri/crates/app/src/commands/player/sound.rs @@ -6,11 +6,12 @@ use std::sync::Arc; use serde::Serialize; use tauri::AppHandle; +use tokio::sync::RwLock; use crate::{ - audio::{engine::AudioCmd, replay_gain::GainMode, AudioEngine}, + audio::{engine::AudioCmd, replay_gain::GainMode, state::SharedPlayback, AudioEngine}, error::AppResult, - state::AppState, + state::{ActiveProfile, AppState}, }; use super::*; @@ -42,18 +43,15 @@ pub async fn player_set_normalize( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.normalize", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine.send(AudioCmd::SetNormalize(enabled))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.normalize", enabled)], + || engine.send(AudioCmd::SetNormalize(enabled)), + ) + .await } /// Allowed DSD → PCM FIR tap counts. 256 is the historical default @@ -71,6 +69,7 @@ pub async fn player_set_dsd_precision( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, taps: u32, + expected_profile_id: Option, ) -> AppResult<()> { // Reject an out-of-set value rather than silently clamping — keeps // the persisted row in lockstep with the three UI options. @@ -79,20 +78,19 @@ pub async fn player_set_dsd_precision( } else { 256 }; - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.dsd_precision", - taps.to_string(), - SettingType::Int, - ) - .await?; - } - engine - .shared() - .dsd_taps - .store(taps, std::sync::atomic::Ordering::Release); - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[("audio.dsd_precision", taps.to_string(), SettingType::Int)], + || { + engine + .shared() + .dsd_taps + .store(taps, std::sync::atomic::Ordering::Release); + Ok(()) + }, + ) + .await } /// Toggle native DSD output via DoP (DSD over PCM), #495. When on AND an @@ -109,21 +107,21 @@ pub async fn player_set_dsd_dop( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.dsd_dop", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine - .shared() - .dsd_dop_enabled - .store(enabled, std::sync::atomic::Ordering::Release); - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.dsd_dop", enabled)], + || { + engine + .shared() + .dsd_dop_enabled + .store(enabled, std::sync::atomic::Ordering::Release); + Ok(()) + }, + ) + .await } /// Toggle ReplayGain — multiply each track by its analyzed gain to @@ -134,18 +132,15 @@ pub async fn player_set_replaygain( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.replaygain", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine.send(AudioCmd::SetReplayGain(enabled))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.replaygain", enabled)], + || engine.send(AudioCmd::SetReplayGain(enabled)), + ) + .await } /// Bound on the pre-amp and on the fallback gain, in dB. Wide enough @@ -171,47 +166,46 @@ pub async fn player_set_replaygain_options( preamp_db: f32, fallback_db: f32, prevent_clipping: bool, + expected_profile_id: Option, ) -> AppResult<()> { let preamp = clamp_replaygain_adjust(preamp_db); let fallback = clamp_replaygain_adjust(fallback_db); - if let Ok(pool) = state.require_profile_pool().await { - // The two gains are decimals, which the table has no marker for: - // stored as text like `ui.zoom_level`, and parsed back on restore. - // They were written as `'float'`, which the CHECK refuses, so - // neither ever survived a restart. - for (key, value, value_type) in [ - ( - "audio.replaygain_preamp", - preamp.to_string(), - SettingType::Text, - ), - ( - "audio.replaygain_fallback", - fallback.to_string(), - SettingType::Text, - ), - ( - "audio.replaygain_prevent_clipping", - prevent_clipping.to_string(), - SettingType::Bool, - ), - ] { - persist_setting(&pool, key, value, value_type).await?; - } - } - - let shared = engine.shared(); - shared - .replaygain_preamp_db_bits - .store(preamp.to_bits(), std::sync::atomic::Ordering::Relaxed); - shared - .replaygain_fallback_db_bits - .store(fallback.to_bits(), std::sync::atomic::Ordering::Relaxed); - shared - .replaygain_prevent_clipping - .store(prevent_clipping, std::sync::atomic::Ordering::Relaxed); - Ok(()) + // The two gains are decimals, which the table has no marker for: + // stored as text like `ui.zoom_level`, and parsed back on restore. + // They were written as `'float'`, which the CHECK refuses, so + // neither ever survived a restart. + let rows = [ + ( + "audio.replaygain_preamp", + preamp.to_string(), + SettingType::Text, + ), + ( + "audio.replaygain_fallback", + fallback.to_string(), + SettingType::Text, + ), + ( + "audio.replaygain_prevent_clipping", + prevent_clipping.to_string(), + SettingType::Bool, + ), + ]; + save_then_apply(&state.profile, expected_profile_id, &rows, || { + let shared = engine.shared(); + shared + .replaygain_preamp_db_bits + .store(preamp.to_bits(), std::sync::atomic::Ordering::Relaxed); + shared + .replaygain_fallback_db_bits + .store(fallback.to_bits(), std::sync::atomic::Ordering::Relaxed); + shared + .replaygain_prevent_clipping + .store(prevent_clipping, std::sync::atomic::Ordering::Relaxed); + Ok(()) + }) + .await } /// Choose which of a file's two ReplayGain measurements to apply: @@ -232,23 +226,26 @@ pub async fn player_set_replaygain_mode( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, mode: String, + expected_profile_id: Option, ) -> AppResult<()> { let mode = GainMode::from_setting(&mode); - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, + save_then_apply( + &state.profile, + expected_profile_id, + &[( "audio.replaygain_mode", - mode.as_setting(), + mode.as_setting().to_string(), SettingType::Text, - ) - .await?; - } - engine - .shared() - .replaygain_mode_bits - .store(mode.as_bits(), std::sync::atomic::Ordering::Relaxed); - - Ok(()) + )], + || { + engine + .shared() + .replaygain_mode_bits + .store(mode.as_bits(), std::sync::atomic::Ordering::Relaxed); + Ok(()) + }, + ) + .await } /// Keep a pre-amp / fallback value inside the supported range, and @@ -268,18 +265,15 @@ pub async fn player_set_mono( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.mono", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine.send(AudioCmd::SetMono(enabled))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.mono", enabled)], + || engine.send(AudioCmd::SetMono(enabled)), + ) + .await } /// Set the left / right balance, in percent: `-100` is hard left, `100` @@ -291,20 +285,17 @@ pub async fn player_set_balance( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, value: i32, + expected_profile_id: Option, ) -> AppResult<()> { let limit = i32::from(crate::audio::stereo::BALANCE_LIMIT); let clamped = value.clamp(-limit, limit); - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.balance", - clamped.to_string(), - SettingType::Int, - ) - .await?; - } - engine.send(AudioCmd::SetBalance(clamped))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[("audio.balance", clamped.to_string(), SettingType::Int)], + || engine.send(AudioCmd::SetBalance(clamped)), + ) + .await } /// Set the stereo width, in percent: `0` is mono, `100` the source as it @@ -316,19 +307,16 @@ pub async fn player_set_stereo_width( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, value: i32, + expected_profile_id: Option, ) -> AppResult<()> { let clamped = value.clamp(0, i32::from(crate::audio::stereo::WIDTH_MAX)); - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.stereo_width", - clamped.to_string(), - SettingType::Int, - ) - .await?; - } - engine.send(AudioCmd::SetStereoWidth(clamped))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[("audio.stereo_width", clamped.to_string(), SettingType::Int)], + || engine.send(AudioCmd::SetStereoWidth(clamped)), + ) + .await } /// Toggle smart crossfade. When ON, same-album transitions skip @@ -341,21 +329,21 @@ pub async fn player_set_smart_crossfade( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.smart_crossfade", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine - .shared() - .smart_crossfade_enabled - .store(enabled, std::sync::atomic::Ordering::Release); - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.smart_crossfade", enabled)], + || { + engine + .shared() + .smart_crossfade_enabled + .store(enabled, std::sync::atomic::Ordering::Release); + Ok(()) + }, + ) + .await } #[tauri::command] @@ -380,29 +368,29 @@ pub async fn player_set_dynamic_crossfade( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.dynamic_crossfade", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine - .shared() - .dynamic_crossfade_enabled - .store(enabled, std::sync::atomic::Ordering::Release); - if !enabled { - // Drop any in-flight override so the next transition snaps - // back to the static crossfade immediately. - engine - .shared() - .pending_next_crossfade_ms - .store(0, std::sync::atomic::Ordering::Release); - } - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.dynamic_crossfade", enabled)], + || { + engine + .shared() + .dynamic_crossfade_enabled + .store(enabled, std::sync::atomic::Ordering::Release); + if !enabled { + // Drop any in-flight override so the next transition snaps + // back to the static crossfade immediately. + engine + .shared() + .pending_next_crossfade_ms + .store(0, std::sync::atomic::Ordering::Release); + } + Ok(()) + }, + ) + .await } #[tauri::command] @@ -426,26 +414,43 @@ pub async fn player_set_automix( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.automix", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - let automix = &engine.shared().automix; - automix - .enabled - .store(enabled, std::sync::atomic::Ordering::Release); - if !enabled { - // A plan already handed over would still be honoured: drop it - // so the next transition is the plain one at once. - automix.clear_plan(); - } - Ok(()) + set_automix( + &state.profile, + engine.shared(), + enabled, + expected_profile_id, + ) + .await +} + +/// [`player_set_automix`] on the profile slot and the shared playback +/// state rather than on Tauri's managed state, so a test can drive it. +pub(super) async fn set_automix( + profile: &RwLock>, + shared: &SharedPlayback, + enabled: bool, + expected_profile_id: Option, +) -> AppResult<()> { + save_then_apply( + profile, + expected_profile_id, + &[bool_row("audio.automix", enabled)], + || { + let automix = &shared.automix; + automix + .enabled + .store(enabled, std::sync::atomic::Ordering::Release); + if !enabled { + // A plan already handed over would still be honoured: drop + // it so the next transition is the plain one at once. + automix.clear_plan(); + } + Ok(()) + }, + ) + .await } #[tauri::command] @@ -462,21 +467,21 @@ pub async fn player_set_visualizer( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "ui.visualizer", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine - .shared() - .visualizer_enabled - .store(enabled, std::sync::atomic::Ordering::Release); - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("ui.visualizer", enabled)], + || { + engine + .shared() + .visualizer_enabled + .store(enabled, std::sync::atomic::Ordering::Release); + Ok(()) + }, + ) + .await } #[tauri::command] @@ -487,12 +492,11 @@ pub async fn player_get_visualizer(engine: tauri::State<'_, Arc>) - .load(std::sync::atomic::Ordering::Relaxed)) } -/// Update the playback speed multiplier. Pushes the new value live -/// to the audio engine so the active stream's resampler is rebuilt -/// against the new effective input rate, then persists to -/// `profile_setting['audio.playback_speed']`. Clamped to `[0.5, 2.0]` -/// on the engine side — values outside that range are saturated, not -/// rejected. +/// Update the playback speed multiplier. Clamped to `[0.5, 2.0]` — +/// values outside that range are saturated, not rejected — saved to +/// `profile_setting['audio.playback_speed']`, then pushed live to the +/// audio engine so the active stream's resampler is rebuilt against the +/// new effective input rate. The engine gets the value that was saved. /// /// Pitch is NOT preserved (1.5× → ~ +7 semitones). Proper pitch-locked /// time-stretching would need a phase vocoder; this is intentionally @@ -503,19 +507,20 @@ pub async fn player_set_speed( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, value: f32, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - let clamped = value.clamp(0.5, 2.0); - persist_setting( - &pool, + let clamped = value.clamp(0.5, 2.0); + save_then_apply( + &state.profile, + expected_profile_id, + &[( "audio.playback_speed", clamped.to_string(), SettingType::Text, - ) - .await?; - } - engine.send(AudioCmd::SetSpeed(value))?; - Ok(()) + )], + || engine.send(AudioCmd::SetSpeed(clamped)), + ) + .await } #[tauri::command] @@ -530,18 +535,15 @@ pub async fn player_set_gapless( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, enabled: bool, + expected_profile_id: Option, ) -> AppResult<()> { - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.gapless", - if enabled { "true" } else { "false" }, - SettingType::Bool, - ) - .await?; - } - engine.send(AudioCmd::SetGapless(enabled))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[bool_row("audio.gapless", enabled)], + || engine.send(AudioCmd::SetGapless(enabled)), + ) + .await } /// Update the crossfade duration. Pushes the new value live to the @@ -552,19 +554,16 @@ pub async fn player_set_crossfade( state: tauri::State<'_, AppState>, engine: tauri::State<'_, Arc>, seconds: f64, + expected_profile_id: Option, ) -> AppResult<()> { let ms = (seconds.max(0.0) * 1000.0).round() as i64; - if let Ok(pool) = state.require_profile_pool().await { - persist_setting( - &pool, - "audio.crossfade_ms", - ms.to_string(), - SettingType::Int, - ) - .await?; - } - engine.send(AudioCmd::SetCrossfade(ms.max(0) as u32))?; - Ok(()) + save_then_apply( + &state.profile, + expected_profile_id, + &[("audio.crossfade_ms", ms.to_string(), SettingType::Int)], + || engine.send(AudioCmd::SetCrossfade(ms.max(0) as u32)), + ) + .await } /// Return current audio settings so the Settings view can hydrate. diff --git a/src-tauri/crates/app/src/state.rs b/src-tauri/crates/app/src/state.rs index bae2198b3..c9ff121d8 100644 --- a/src-tauri/crates/app/src/state.rs +++ b/src-tauri/crates/app/src/state.rs @@ -191,6 +191,13 @@ impl ActiveProfile { } } + /// An active profile over `pool`, for a test outside this module that + /// drives a command body against a profile slot of its own. + #[cfg(test)] + pub(crate) fn for_test(profile_id: i64, pool: SqlitePool) -> Self { + Self::new(profile_id, pool) + } + fn lease(&self) -> ProfilePool { ProfilePool { pool: self.pool.clone(), @@ -698,12 +705,7 @@ impl AppState { &self, expected: Option, ) -> AppResult<(ProfilePool, i64)> { - let guard = self.profile.read().await; - check_expected_profile(guard.as_ref().map(|p| p.profile_id), expected)?; - guard - .as_ref() - .map(|p| (p.lease(), p.profile_id)) - .ok_or(AppError::NoActiveProfile) + lease_profile(&self.profile, expected).await } /// Return the active profile id, or an error if none is active. @@ -809,6 +811,40 @@ fn check_expected_profile(active: Option, expected: Option) -> AppResu } } +/// [`AppState::require_profile_pool_with_id`] on the profile slot itself, +/// so a caller that is handed the slot rather than the whole `AppState` +/// — a setting command's body, driven by a test — takes the same check +/// under the same lock. +pub(crate) async fn lease_profile( + slot: &RwLock>, + expected: Option, +) -> AppResult<(ProfilePool, i64)> { + let guard = slot.read().await; + check_expected_profile(guard.as_ref().map(|p| p.profile_id), expected)?; + guard + .as_ref() + .map(|p| (p.lease(), p.profile_id)) + .ok_or(AppError::NoActiveProfile) +} + +/// Run `f` only while `profile_id` is the active profile (`None`: while +/// none is), under the profile lock a switch takes for writing. +/// +/// For a setting that is applied to process-wide state after it was saved +/// into a profile (issue #996): `switch_profile` swaps the pool under the +/// write half of this lock and restores the next profile's settings after +/// it, so either the pool has not been swapped yet and `f` runs before +/// that restore, or it has and `f` is skipped — it never lands on top of +/// the next profile's values. Returns `None` when skipped. +pub(crate) async fn while_profile_is( + slot: &RwLock>, + profile_id: Option, + f: impl FnOnce() -> R, +) -> Option { + let guard = slot.read().await; + (guard.as_ref().map(|p| p.profile_id) == profile_id).then(f) +} + #[cfg(test)] mod expected_profile_tests; diff --git a/src/components/views/SettingsView.tsx b/src/components/views/SettingsView.tsx index 9255617da..3cb8504dc 100644 --- a/src/components/views/SettingsView.tsx +++ b/src/components/views/SettingsView.tsx @@ -1587,39 +1587,69 @@ function SettingsViewContent({ }; }, [activeProfile?.id, audioSettingsNonce]); + // Which profile is live, readable from inside a callback that was + // scheduled under a different one. Declared here rather than beside + // its first user further down: every handler below captures the id + // at click time and compares against this, and `react-hooks` + // reasons about the whole component — a ref first referenced above + // its own declaration makes the rule treat it as frozen. + // + // Each handler also hands that id to its command, which refuses the + // write once another profile is active (#996): the comparison here + // runs before the IPC hop, so only the backend can close the window + // after it. A refused write then fails, and its rollback must not + // land on the profile the user switched to — hence the same + // comparison before every rollback. Kept in sync from a layout effect, + // so a callback that resolves between the commit and the passive + // effects already sees the profile on screen. + const activeProfileIdRef = useRef(activeProfile?.id); + useLayoutEffect(() => { + activeProfileIdRef.current = activeProfile?.id; + }, [activeProfile?.id]); + const handleToggleNormalize = useCallback(() => { const next = !normalize; + const profileId = activeProfile?.id; setNormalize(next); - playerSetNormalize(next).catch((err) => { + playerSetNormalize(next, profileId ?? null).catch((err) => { console.error("[Settings] set normalize failed", err); + if (activeProfileIdRef.current !== profileId) return; setNormalize(!next); // rollback }); - }, [normalize]); + }, [normalize, activeProfile?.id]); const handleToggleReplayGain = useCallback(() => { const next = !replayGain; + const profileId = activeProfile?.id; setReplayGain(next); - playerSetReplayGain(next).catch((err) => { + playerSetReplayGain(next, profileId ?? null).catch((err) => { console.error("[Settings] set replaygain failed", err); + if (activeProfileIdRef.current !== profileId) return; setReplayGain(!next); // rollback }); - }, [replayGain]); + }, [replayGain, activeProfile?.id]); // The sliders fire on every pixel of drag. The audio side is a pair // of atomics and would happily take all of them, but each push also // writes three rows to SQLite — so the round trip is debounced and - // local state drives the UI in the meantime. + // local state drives the UI in the meantime. The profile is the one + // the slider moved in, not the one active when the timer fires. const pushReplayGainOptions = useCallback( (preampDb: number, fallbackDb: number, preventClipping: boolean) => { if (replayGainOptionsDebounce.current != null) { window.clearTimeout(replayGainOptionsDebounce.current); } + const profileId = activeProfileIdRef.current; replayGainOptionsDebounce.current = window.setTimeout(() => { - playerSetReplayGainOptions({ - preampDb, - fallbackDb, - preventClipping, - }).catch((err) => { + if (activeProfileIdRef.current !== profileId) return; + playerSetReplayGainOptions( + { + preampDb, + fallbackDb, + preventClipping, + }, + profileId ?? null, + ).catch((err) => { console.error("[Settings] set replaygain options failed", err); // Not saved, so not applied either: show what the engine // still holds instead of the slider's position. @@ -1659,17 +1689,6 @@ function SettingsViewContent({ [pushReplayGainOptions, replayGainPreamp, replayGainPreventClipping], ); - // Which profile is live, readable from inside a callback that was - // scheduled under a different one. Declared here rather than beside - // its first user further down: every handler below captures the id - // at click time and compares against this, and `react-hooks` - // reasons about the whole component — a ref first referenced above - // its own declaration makes the rule treat it as frozen. - const activeProfileIdRef = useRef(activeProfile?.id); - useEffect(() => { - activeProfileIdRef.current = activeProfile?.id; - }, [activeProfile?.id]); - const handleReplayGainModeChange = useCallback( (next: ReplayGainMode) => { const previous = replayGainMode; @@ -1681,7 +1700,7 @@ function SettingsViewContent({ // cannot land on top of a choice the user has already made. audioSettingsTouched.current.add("replayGainMode"); setReplayGainMode(next); - playerSetReplayGainMode(next).catch((err) => { + playerSetReplayGainMode(next, profileId ?? null).catch((err) => { console.error("[Settings] set replaygain mode failed", err); if (activeProfileIdRef.current !== profileId) return; // Only roll back if no newer selection superseded this one — @@ -1708,15 +1727,17 @@ function SettingsViewContent({ (next: DsdPrecisionTaps) => { const prev = dsdTaps; if (prev === next) return; + const profileId = activeProfile?.id; setDsdTaps(next); // optimistic - playerSetDsdPrecision(next).catch((err) => { + playerSetDsdPrecision(next, profileId ?? null).catch((err) => { console.error("[Settings] set DSD precision failed", err); + if (activeProfileIdRef.current !== profileId) return; // Only roll back if no newer selection superseded this one — // a stale failure must not clobber a later successful click. setDsdTaps((cur) => (cur === next ? prev : cur)); }); }, - [dsdTaps], + [dsdTaps, activeProfile?.id], ); // Writes are chained rather than fired in parallel: two fast clicks @@ -1738,7 +1759,7 @@ function SettingsViewContent({ .catch(() => {}) .then(() => { if (activeProfileIdRef.current !== profileId) return; - return playerSetDsdDop(next); + return playerSetDsdDop(next, profileId ?? null); }) .catch((err) => { console.error("[Settings] set DSD DoP failed", err); @@ -1795,21 +1816,25 @@ function SettingsViewContent({ const handleToggleGapless = useCallback(() => { const next = !gapless; + const profileId = activeProfile?.id; setGapless(next); - playerSetGapless(next).catch((err) => { + playerSetGapless(next, profileId ?? null).catch((err) => { console.error("[Settings] set gapless failed", err); + if (activeProfileIdRef.current !== profileId) return; setGapless(!next); // rollback }); - }, [gapless]); + }, [gapless, activeProfile?.id]); const handleToggleMono = useCallback(() => { const next = !mono; + const profileId = activeProfile?.id; setMono(next); - playerSetMono(next).catch((err) => { + playerSetMono(next, profileId ?? null).catch((err) => { console.error("[Settings] set mono failed", err); + if (activeProfileIdRef.current !== profileId) return; setMono(!next); }); - }, [mono]); + }, [mono, activeProfile?.id]); // Both of these are profile-scoped settings written by a backend // command, so they take the same discipline `handleToggleDsdDop` @@ -1830,7 +1855,7 @@ function SettingsViewContent({ .catch(() => {}) .then(() => { if (activeProfileIdRef.current !== profileId) return; - return playerSetMatchSourceRate(next); + return playerSetMatchSourceRate(next, profileId ?? null); }) .catch((err) => { console.error("[Settings] set match source rate failed", err); @@ -1854,7 +1879,7 @@ function SettingsViewContent({ .catch(() => {}) .then(() => { if (activeProfileIdRef.current !== profileId) return; - return playerSetPauseOnDeviceLoss(next); + return playerSetPauseOnDeviceLoss(next, profileId ?? null); }) .catch((err) => { console.error("[Settings] set pause on device loss failed", err); @@ -1888,14 +1913,17 @@ function SettingsViewContent({ }, [keepAwake, activeProfile?.id]); // Debounce crossfade slider changes to avoid spamming the backend. + // Saved into the profile the slider moved in, like the ReplayGain ones. const crossfadeTimerRef = useRef(null); const handleCrossfadeChange = useCallback((sec: number) => { setCrossfadeSec(sec); if (crossfadeTimerRef.current != null) { window.clearTimeout(crossfadeTimerRef.current); } + const profileId = activeProfileIdRef.current; crossfadeTimerRef.current = window.setTimeout(() => { - playerSetCrossfade(sec).catch((err) => { + if (activeProfileIdRef.current !== profileId) return; + playerSetCrossfade(sec, profileId ?? null).catch((err) => { console.error("[Settings] set crossfade failed", err); // Same as the ReplayGain sliders: re-read what is in force. setAudioSettingsNonce((n) => n + 1); diff --git a/src/components/views/settings/StereoImageCard.tsx b/src/components/views/settings/StereoImageCard.tsx index 044852e11..a368f721f 100644 --- a/src/components/views/settings/StereoImageCard.tsx +++ b/src/components/views/settings/StereoImageCard.tsx @@ -20,7 +20,10 @@ import { type Knob = "balance" | "width"; -const WRITERS: Record Promise> = { +const WRITERS: Record< + Knob, + (value: number, expectedProfileId: number | null) => Promise +> = { balance: playerSetBalance, width: playerSetStereoWidth, }; @@ -117,7 +120,9 @@ export function StereoImageCard() { chain.current = chain.current.then(async () => { try { if (profileIdRef.current !== profileId) return; - await WRITERS[knob](value); + // The command refuses it too once another profile is active: this + // check runs before the IPC hop, the backend's after it (#996). + await WRITERS[knob](value, profileId ?? null); } catch (err) { console.error(`[StereoImageCard] set ${knob} failed`, err); // Not saved, so not applied: show what the engine still holds — diff --git a/src/components/views/settings/settingsReset.ts b/src/components/views/settings/settingsReset.ts index f2d47d947..e2db8f987 100644 --- a/src/components/views/settings/settingsReset.ts +++ b/src/components/views/settings/settingsReset.ts @@ -316,6 +316,17 @@ function command( }; } +/** + * An engine setter that names the profile it writes into (#996), bound to + * the profile being reset: the command refuses the write once another + * profile is active, rather than putting this one's default there. + */ +function inProfile( + write: (value: T, expectedProfileId: number | null) => Promise, +): (value: T, ctx: SettingsResetContext) => Promise { + return (value, ctx) => write(value, ctx.profileId); +} + /** * A `profile_setting` row the frontend writes itself, with the window * event its hook re-reads on. The reset writes the default the way the @@ -405,7 +416,10 @@ function profileBool(key: BooleanSettingKey, event?: string): SettingAccessor { */ function engineSwitch( key: "audio.normalize" | "audio.mono" | "audio.gapless" | "audio.replaygain", - write: (enabled: boolean) => Promise, + write: ( + enabled: boolean, + expectedProfileId: number | null, + ) => Promise, ): SettingAccessor { const fallback: boolean = SETTINGS[key].defaultValue; return { @@ -419,8 +433,8 @@ function engineSwitch( : fallback; return labelIf(value !== fallback, key); }, - reset: async () => { - await write(fallback); + reset: async (ctx) => { + await write(fallback, ctx.profileId); }, }; } @@ -488,24 +502,27 @@ function replayGainOption( key, ); }, - reset: async () => { + reset: async (ctx) => { // Fresh, not from the inspection: an earlier reset in the same pass // may have put one of the other two back already. const s = await playerGetAudioSettings(); - await playerSetReplayGainOptions({ - preampDb: - key === "audio.replaygain_preamp" - ? SETTINGS[key].defaultValue - : s.replaygain_preamp_db, - fallbackDb: - key === "audio.replaygain_fallback" - ? SETTINGS[key].defaultValue - : s.replaygain_fallback_db, - preventClipping: - key === "audio.replaygain_prevent_clipping" - ? SETTINGS[key].defaultValue - : s.replaygain_prevent_clipping, - }); + await playerSetReplayGainOptions( + { + preampDb: + key === "audio.replaygain_preamp" + ? SETTINGS[key].defaultValue + : s.replaygain_preamp_db, + fallbackDb: + key === "audio.replaygain_fallback" + ? SETTINGS[key].defaultValue + : s.replaygain_fallback_db, + preventClipping: + key === "audio.replaygain_prevent_clipping" + ? SETTINGS[key].defaultValue + : s.replaygain_prevent_clipping, + }, + ctx.profileId, + ); }, }; } @@ -667,23 +684,23 @@ const ACCESSORS: Record = { "audio.crossfade_ms", async (reads) => (await reads.audio()).crossfade_ms, // The command takes seconds; the slider shows whole ones. - (ms) => playerSetCrossfade(Math.round(ms / 1000)), + (ms, ctx) => playerSetCrossfade(Math.round(ms / 1000), ctx.profileId), ), "audio.smart_crossfade": command( "audio.smart_crossfade", getSmartCrossfade, - setSmartCrossfade, + inProfile(setSmartCrossfade), ), "audio.dynamic_crossfade": command( "audio.dynamic_crossfade", getDynamicCrossfade, - setDynamicCrossfade, + inProfile(setDynamicCrossfade), ), - "audio.automix": command("audio.automix", getAutoMix, setAutoMix), + "audio.automix": command("audio.automix", getAutoMix, inProfile(setAutoMix)), "audio.pause_on_device_loss": command( "audio.pause_on_device_loss", async (reads) => (await reads.audio()).pause_on_device_loss, - playerSetPauseOnDeviceLoss, + inProfile(playerSetPauseOnDeviceLoss), ), "audio.normalize": engineSwitch("audio.normalize", playerSetNormalize), "audio.replaygain": engineSwitch("audio.replaygain", playerSetReplayGain), @@ -699,7 +716,7 @@ const ACCESSORS: Record = { command( "audio.replaygain_mode", async (reads) => (await reads.audio()).replaygain_mode, - playerSetReplayGainMode, + inProfile(playerSetReplayGainMode), ), whileReplayGainOn, ), @@ -733,25 +750,25 @@ const ACCESSORS: Record = { "audio.match_source_rate": command( "audio.match_source_rate", async (reads) => (await reads.audio()).match_source_rate, - playerSetMatchSourceRate, + inProfile(playerSetMatchSourceRate), ), "audio.mono": engineSwitch("audio.mono", playerSetMono), "audio.balance": command( "audio.balance", async (reads) => (await reads.audio()).balance, - playerSetBalance, + inProfile(playerSetBalance), ), "audio.stereo_width": command( "audio.stereo_width", async (reads) => (await reads.audio()).stereo_width, - playerSetStereoWidth, + inProfile(playerSetStereoWidth), ), "audio.dsd_precision": command( "audio.dsd_precision", async (reads) => (await reads.audio()) .dsd_taps as (typeof SETTINGS)["audio.dsd_precision"]["defaultValue"], - playerSetDsdPrecision, + inProfile(playerSetDsdPrecision), ), "audio.dsd_dop": { inspect: async (_ctx, reads) => @@ -761,7 +778,8 @@ const ACCESSORS: Record = { SETTINGS["audio.dsd_dop"].defaultValue, "audio.dsd_dop", ), - reset: () => playerSetDsdDop(SETTINGS["audio.dsd_dop"].defaultValue), + reset: (ctx) => + playerSetDsdDop(SETTINGS["audio.dsd_dop"].defaultValue, ctx.profileId), }, // ---- Appearance ------------------------------------------------------ @@ -888,7 +906,7 @@ const ACCESSORS: Record = { "ui.visualizer": command( "ui.visualizer", getVisualizerEnabled, - setVisualizerEnabled, + inProfile(setVisualizerEnabled), ), "immersive.merged_lyrics": immersivePref("immersive.merged_lyrics"), "immersive.use_native_fullscreen": immersivePref( diff --git a/src/contexts/PlayerContext.tsx b/src/contexts/PlayerContext.tsx index e2dfa11e1..8ac8d835d 100644 --- a/src/contexts/PlayerContext.tsx +++ b/src/contexts/PlayerContext.tsx @@ -672,18 +672,23 @@ export function PlayerProvider({ children }: { children: ReactNode }) { }, 60); }, []); - const setPlaybackSpeed = useCallback((value: number) => { - const clamped = Math.max(0.5, Math.min(2.0, value)); - setPlaybackSpeedState(clamped); - playerSetSpeed(clamped).catch((err) => { - console.error("[PlayerContext] set speed failed", err); - // The backend saves before it applies, so a refused speed never - // reached the engine: show the one it still plays at. - playerGetSpeed() - .then(setPlaybackSpeedState) - .catch(() => {}); - }); - }, []); + const activeProfileId = activeProfile?.id; + const setPlaybackSpeed = useCallback( + (value: number) => { + const clamped = Math.max(0.5, Math.min(2.0, value)); + setPlaybackSpeedState(clamped); + // Saved into the profile it was chosen in, or refused (#996). + playerSetSpeed(clamped, activeProfileId ?? null).catch((err) => { + console.error("[PlayerContext] set speed failed", err); + // The backend saves before it applies, so a refused speed never + // reached the engine: show the one it still plays at. + playerGetSpeed() + .then(setPlaybackSpeedState) + .catch(() => {}); + }); + }, + [activeProfileId], + ); const toggleMute = useCallback(() => { lastLocalVolumeWriteRef.current = Date.now(); diff --git a/src/hooks/useProfileScopedToggle.ts b/src/hooks/useProfileScopedToggle.ts index 04819d0fe..39ad73546 100644 --- a/src/hooks/useProfileScopedToggle.ts +++ b/src/hooks/useProfileScopedToggle.ts @@ -22,19 +22,30 @@ import { useProfile } from "./useProfile"; * the new profile. * - **A read already in flight never lands on a click.** `touched` * drops the answer once the user has decided. + * - **Writes go one at a time, and the last click wins** (issue #995). + * Each write waits for the one before it, so two fast clicks can no + * longer reach the backend in the wrong order and leave the first + * value saved; a queued write that a later click has superseded is + * skipped, the later one carrying the value the user ended on. A + * failed write does not stop the chain. + * - **A queued write belongs to the profile it was made in.** It is not + * sent once that profile is no longer active, and when it is sent it + * names that profile to the command, which refuses it if a switch got + * in after this check (issue #996) — only the backend can close that + * window, the check here runs before the IPC hop. * - **A failed write rolls back only while it is still the last - * decision.** Each click and each profile change bumps a revision; - * a failure from an older one is dropped, because a later click has - * already decided something else — and comparing values instead - * would miss the case where three clicks bring the switch back to - * what the failed write was setting. + * decision**, and to the value the backend last confirmed. Each click + * and each profile change bumps a revision; a failure from an older + * one is dropped, because a later click has already decided something + * else — and comparing values instead would miss the case where three + * clicks bring the switch back to what the failed write was setting. * * A failed read resolves to `fallback` rather than leaving the control * disabled forever. */ export function useProfileScopedToggle( read: () => Promise, - write: (value: boolean) => Promise, + write: (value: boolean, expectedProfileId: number | null) => Promise, options: { label: string; fallback?: boolean }, ): { enabled: boolean; hydrated: boolean; toggle: () => void } { const { label, fallback = false } = options; @@ -48,6 +59,23 @@ export function useProfileScopedToggle( // Bumped by every click and by every profile change. A write that // fails belongs to one revision; anything newer has already decided. const revision = useRef(0); + // Bumped by clicks alone: a queued write is skipped once a later click + // has queued its own. + const clicks = useRef(0); + // The profile active now, for a write that waited in the chain to + // check it still belongs to the profile on screen. + const liveProfileId = useRef(activeProfileId); + // What the backend last held for the profile on screen: the read's + // answer, then each write it accepted. A failed write rolls back to + // it, not to the value its own click replaced — after a queue of + // clicks, that one may never have reached the backend. + const confirmed = useRef<{ profileId: number | undefined; on: boolean }>({ + profileId: undefined, + on: fallback, + }); + // The writes, one after the other. Never rejects: every link catches + // its own failure. + const chain = useRef>(Promise.resolve()); // The read effect is keyed on the profile alone, so the commands are // reached through refs: a caller passing an inline arrow would // otherwise re-issue the read on every render. Kept in sync from an @@ -63,17 +91,23 @@ export function useProfileScopedToggle( useEffect(() => { let stale = false; const profileId = activeProfileId; + liveProfileId.current = profileId; touched.current = false; revision.current += 1; readRef .current() .then((on) => { + // Once the user has clicked, the writes are what the backend + // holds: an answer read before them must not stand in for them. if (stale || touched.current) return; + confirmed.current = { profileId, on }; setState({ profileId, on }); }) .catch((err) => { console.error(`[${label}] read failed`, err); - if (!stale) setState({ profileId, on: fallback }); + if (stale || touched.current) return; + confirmed.current = { profileId, on: fallback }; + setState({ profileId, on: fallback }); }); return () => { stale = true; @@ -93,21 +127,39 @@ export function useProfileScopedToggle( touched.current = true; revision.current += 1; const writeRevision = revision.current; + clicks.current += 1; + const click = clicks.current; setState({ profileId, on: next }); - writeRef.current(next).catch((err) => { - console.error(`[${label}] write failed`, err); - // Undo this write only while it is still the last decision made. - // Comparing the value instead would not be enough: three clicks - // bring the switch back to what this write was setting, and the - // rollback would then undo the click the user actually ended on. - // A profile change bumps the revision too, so a late failure - // cannot re-tag the value with a profile we have left. - if (revision.current !== writeRevision) return; - setState((cur) => - cur !== null && cur.profileId === profileId - ? { profileId, on: !next } - : cur, - ); + chain.current = chain.current.then(async () => { + // Superseded while it waited: a later click is queued behind this + // one and carries the value the user ended on. + if (clicks.current !== click) return; + // Made in a profile that is no longer active: not this one's to + // save. The command would refuse it anyway; this spares the trip. + if (liveProfileId.current !== profileId) return; + try { + await writeRef.current(next, profileId ?? null); + if (liveProfileId.current === profileId) { + confirmed.current = { profileId, on: next }; + } + } catch (err) { + console.error(`[${label}] write failed`, err); + // Undo this write only while it is still the last decision made. + // Comparing the value instead would not be enough: three clicks + // bring the switch back to what this write was setting, and the + // rollback would then undo the click the user actually ended on. + // A profile change bumps the revision too, so a late failure + // cannot re-tag the value with a profile we have left. + if (revision.current !== writeRevision) return; + const held = confirmed.current; + setState((cur) => + cur !== null && + cur.profileId === profileId && + held.profileId === profileId + ? { profileId, on: held.on } + : cur, + ); + } }); }, [enabled, hydrated, label, activeProfileId]); diff --git a/src/lib/tauri/player.ts b/src/lib/tauri/player.ts index 46cc1dcce..46734abf7 100644 --- a/src/lib/tauri/player.ts +++ b/src/lib/tauri/player.ts @@ -501,14 +501,22 @@ export function playerGetAudioSettings(): Promise { return invoke("player_get_audio_settings"); } -export function playerSetNormalize(enabled: boolean): Promise { +export function playerSetNormalize( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_normalize", { enabled }), + invoke("player_set_normalize", { enabled, expectedProfileId }), ); } -export function playerSetMono(enabled: boolean): Promise { - return announceSettingWrite(invoke("player_set_mono", { enabled })); +export function playerSetMono( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { + return announceSettingWrite( + invoke("player_set_mono", { enabled, expectedProfileId }), + ); } /** Balance end stop, in percent either side of the centre. */ @@ -521,9 +529,15 @@ export const STEREO_WIDTH_MAX = 200; * is attenuated, the near one never raised. Saved before it is applied: * a refused write rejects and leaves the engine where it was. */ -export function playerSetBalance(value: number): Promise { +export function playerSetBalance( + value: number, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_balance", { value: Math.round(value) }), + invoke("player_set_balance", { + value: Math.round(value), + expectedProfileId, + }), ); } @@ -531,9 +545,15 @@ export function playerSetBalance(value: number): Promise { * Stereo width in whole percents: 0 is mono, 100 the source as it is, * 200 the side doubled. Saved before it is applied, like the balance. */ -export function playerSetStereoWidth(value: number): Promise { +export function playerSetStereoWidth( + value: number, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_stereo_width", { value: Math.round(value) }), + invoke("player_set_stereo_width", { + value: Math.round(value), + expectedProfileId, + }), ); } @@ -550,9 +570,15 @@ export function playerSetStereoWidth(value: number): Promise { * costs an audible gap, so every rate change becomes a break in the * music. Takes effect on the next track. */ -export function playerSetMatchSourceRate(enabled: boolean): Promise { +export function playerSetMatchSourceRate( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_match_source_rate", { enabled }), + invoke("player_set_match_source_rate", { + enabled, + expectedProfileId, + }), ); } @@ -564,9 +590,15 @@ export function playerSetMatchSourceRate(enabled: boolean): Promise { * that flaps and comes back is still picked up where it was; only a * fallback onto a different endpoint parks the session. */ -export function playerSetPauseOnDeviceLoss(enabled: boolean): Promise { +export function playerSetPauseOnDeviceLoss( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_pause_on_device_loss", { enabled }), + invoke("player_set_pause_on_device_loss", { + enabled, + expectedProfileId, + }), ); } @@ -600,15 +632,21 @@ export function playerHoldDisplay(held: boolean): Promise { return request; } -export function playerSetCrossfade(seconds: number): Promise { +export function playerSetCrossfade( + seconds: number, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_crossfade", { seconds }), + invoke("player_set_crossfade", { seconds, expectedProfileId }), ); } -export function playerSetReplayGain(enabled: boolean): Promise { +export function playerSetReplayGain( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_replaygain", { enabled }), + invoke("player_set_replaygain", { enabled, expectedProfileId }), ); } @@ -627,12 +665,14 @@ export interface ReplayGainOptions { export function playerSetReplayGainOptions( options: ReplayGainOptions, + expectedProfileId: number | null = null, ): Promise { return announceSettingWrite( invoke("player_set_replaygain_options", { preampDb: options.preampDb, fallbackDb: options.fallbackDb, preventClipping: options.preventClipping, + expectedProfileId, }), ); } @@ -655,14 +695,22 @@ export const REPLAYGAIN_MODES: readonly ReplayGainMode[] = [ "album", ]; -export function playerSetReplayGainMode(mode: ReplayGainMode): Promise { +export function playerSetReplayGainMode( + mode: ReplayGainMode, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_replaygain_mode", { mode }), + invoke("player_set_replaygain_mode", { mode, expectedProfileId }), ); } -export function playerSetGapless(enabled: boolean): Promise { - return announceSettingWrite(invoke("player_set_gapless", { enabled })); +export function playerSetGapless( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { + return announceSettingWrite( + invoke("player_set_gapless", { enabled, expectedProfileId }), + ); } /** @@ -671,9 +719,12 @@ export function playerSetGapless(enabled: boolean): Promise { * the next track open. An out-of-set value is coerced to 256 by the * backend. Persisted in `profile_setting['audio.dsd_precision']`. */ -export function playerSetDsdPrecision(taps: DsdPrecisionTaps): Promise { +export function playerSetDsdPrecision( + taps: DsdPrecisionTaps, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_dsd_precision", { taps }), + invoke("player_set_dsd_precision", { taps, expectedProfileId }), ); } @@ -687,8 +738,13 @@ export function playerSetDsdPrecision(taps: DsdPrecisionTaps): Promise { * DSD → PCM, so it's safe to leave on. Persisted in * `profile_setting['audio.dsd_dop']`. */ -export function playerSetDsdDop(enabled: boolean): Promise { - return announceSettingWrite(invoke("player_set_dsd_dop", { enabled })); +export function playerSetDsdDop( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { + return announceSettingWrite( + invoke("player_set_dsd_dop", { enabled, expectedProfileId }), + ); } /** @@ -697,8 +753,11 @@ export function playerSetDsdDop(enabled: boolean): Promise { * lifts the pitch by ~7 semitones (resampler-shift, same as VLC's * default playback rate). */ -export function playerSetSpeed(value: number): Promise { - return invoke("player_set_speed", { value }); +export function playerSetSpeed( + value: number, + expectedProfileId: number | null = null, +): Promise { + return invoke("player_set_speed", { value, expectedProfileId }); } export function playerGetSpeed(): Promise { diff --git a/src/lib/tauri/smartCrossfade.ts b/src/lib/tauri/smartCrossfade.ts index 53938b74e..fb412b699 100644 --- a/src/lib/tauri/smartCrossfade.ts +++ b/src/lib/tauri/smartCrossfade.ts @@ -6,9 +6,12 @@ export function getSmartCrossfade(): Promise { return invoke("player_get_smart_crossfade"); } -export function setSmartCrossfade(enabled: boolean): Promise { +export function setSmartCrossfade( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_smart_crossfade", { enabled }), + invoke("player_set_smart_crossfade", { enabled, expectedProfileId }), ); } @@ -16,9 +19,15 @@ export function getDynamicCrossfade(): Promise { return invoke("player_get_dynamic_crossfade"); } -export function setDynamicCrossfade(enabled: boolean): Promise { +export function setDynamicCrossfade( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_dynamic_crossfade", { enabled }), + invoke("player_set_dynamic_crossfade", { + enabled, + expectedProfileId, + }), ); } @@ -26,6 +35,11 @@ export function getAutoMix(): Promise { return invoke("player_get_automix"); } -export function setAutoMix(enabled: boolean): Promise { - return announceSettingWrite(invoke("player_set_automix", { enabled })); +export function setAutoMix( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { + return announceSettingWrite( + invoke("player_set_automix", { enabled, expectedProfileId }), + ); } diff --git a/src/lib/tauri/visualizer.ts b/src/lib/tauri/visualizer.ts index 2d8ab344a..b5bad1ee2 100644 --- a/src/lib/tauri/visualizer.ts +++ b/src/lib/tauri/visualizer.ts @@ -6,8 +6,11 @@ export function getVisualizerEnabled(): Promise { return invoke("player_get_visualizer"); } -export function setVisualizerEnabled(enabled: boolean): Promise { +export function setVisualizerEnabled( + enabled: boolean, + expectedProfileId: number | null = null, +): Promise { return announceSettingWrite( - invoke("player_set_visualizer", { enabled }), + invoke("player_set_visualizer", { enabled, expectedProfileId }), ); }