chore(rust): clear the clippy backlog and finish the poison-tolerant lock sweep
`cargo clippy --all-targets` went from 51 warnings (23 in the lib) to zero. Most were mechanical — needless borrows, `assert_eq!` against a bool literal, `vec!` where an array does, `or_insert_with(Vec::new)`, a loop index used only to index — and were applied with `clippy --fix`, then reviewed line by line. That review caught one auto-fix that was *not* semantically neutral: dropping the redundant `use hostname;` left its `#[cfg(target_os = "linux")]` orphaned directly above `SERVICE_NAME`, which would have silently cfg'd the constant out of every non-Linux build. Removed the stray attribute with the import. Where a lint asked for a risky change rather than a better one, it is suppressed with a comment saying why: - `too_many_arguments` on five `#[tauri::command]` handlers and `ThumbnailCache::save_thumbnail` — most of the arity is `State<'_, _>` injection, and a parameter struct would change the IPC contract and the generated TypeScript for no readability gain. - `large_enum_variant` on `PlayerStatusEvent` and `AutoplayDecision` — both are serde + specta wire types emitted a handful of times a second, never bulk allocated; boxing would have to stay invisible to the generated bindings while every match arm gained a deref. - `await_holding_lock` on the `hybrid`/`offline` test modules — the guard is a test-only serialisation lock for the process-global `INCLUDE_CATALOG_BROWSE` flag, and the await it spans *is* the critical section. Each `#[tokio::test]` gets its own single-threaded runtime, so this is not the production deadlock class the lint targets; restructuring would reintroduce the flag race. Real fixes elsewhere: `JellyfinItem::to_media_item` takes `self` by value, so it is now `into_media_item`; the five-tuple episode row in the download commands has a named `EpisodeRow` alias; the mpv `PropertyChange` arm matches `name: "pause"` instead of guarding on it. Also converted the last 27 raw `.lock().unwrap()` call sites to `lock_safe()`, completing the `MutexSafe`/`RwLockSafe` convention. All of them turned out to be in test modules — production code was already clean — so this is consistency rather than a fix. The two raw locks in `utils/lock.rs` stay raw on purpose: those tests deliberately poison a mutex to prove the helpers recover from it. Pure refactoring: all 698 tests still pass.
This commit is contained in:
@@ -418,13 +418,13 @@ mod tests {
|
||||
#[test]
|
||||
fn test_auth_manager_wrapper_structure() {
|
||||
// Verify wrapper type exists and has correct structure
|
||||
assert_eq!(std::mem::size_of::<AuthManagerWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<AuthManagerWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_session_verifier_wrapper_structure() {
|
||||
// Verify wrapper type exists and has correct structure
|
||||
assert_eq!(std::mem::size_of::<SessionVerifierWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<SessionVerifierWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -496,7 +496,7 @@ pub(crate) async fn requeue_mistyped_video_downloads(
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
|
||||
let query = Query::new(&format!(
|
||||
let query = Query::new(format!(
|
||||
"UPDATE downloads
|
||||
SET status = 'pending', stream_url = NULL, progress = 0,
|
||||
bytes_downloaded = 0, started_at = NULL, completed_at = NULL
|
||||
@@ -563,7 +563,7 @@ where
|
||||
),
|
||||
None => String::new(),
|
||||
};
|
||||
let rows_query = Query::new(&format!(
|
||||
let rows_query = Query::new(format!(
|
||||
"SELECT d.id, d.item_id,
|
||||
COALESCE(
|
||||
d.media_type,
|
||||
@@ -753,6 +753,7 @@ pub async fn resume_queued_downloads(
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::storage::db_service::RusqliteService;
|
||||
use crate::utils::lock::MutexSafe;
|
||||
use rusqlite::Connection;
|
||||
use std::sync::Mutex;
|
||||
|
||||
@@ -1012,14 +1013,14 @@ mod tests {
|
||||
resolve_pending_download_urls(&db, "/data", None, move |item_id, media_type, _q| {
|
||||
let seen = Arc::clone(&seen_c);
|
||||
async move {
|
||||
seen.lock().unwrap().push((item_id.clone(), media_type));
|
||||
seen.lock_safe().push((item_id.clone(), media_type));
|
||||
Some(format!("http://resolved/{item_id}"))
|
||||
}
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let seen = seen.lock().unwrap().clone();
|
||||
let seen = seen.lock_safe().clone();
|
||||
let of = |id: &str| {
|
||||
seen.iter()
|
||||
.find(|(i, _)| i == id)
|
||||
@@ -1045,14 +1046,14 @@ mod tests {
|
||||
resolve_pending_download_urls(&db, "/data", None, move |_id, media_type, _q| {
|
||||
let seen = Arc::clone(&seen_c);
|
||||
async move {
|
||||
*seen.lock().unwrap() = media_type;
|
||||
*seen.lock_safe() = media_type;
|
||||
Some("http://x".to_string())
|
||||
}
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(*seen.lock().unwrap(), "audio");
|
||||
assert_eq!(*seen.lock_safe(), "audio");
|
||||
}
|
||||
|
||||
/// An explicit `media_type` on the row always wins over the item's type.
|
||||
@@ -1069,14 +1070,14 @@ mod tests {
|
||||
resolve_pending_download_urls(&db, "/data", None, move |_id, media_type, _q| {
|
||||
let seen = Arc::clone(&seen_c);
|
||||
async move {
|
||||
*seen.lock().unwrap() = media_type;
|
||||
*seen.lock_safe() = media_type;
|
||||
Some("http://x".to_string())
|
||||
}
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(*seen.lock().unwrap(), "video");
|
||||
assert_eq!(*seen.lock_safe(), "video");
|
||||
}
|
||||
|
||||
/// Rows already downloaded under the audio default hold an audio-only
|
||||
|
||||
@@ -97,6 +97,6 @@ mod tests {
|
||||
// due to its dependencies, so we just test the wrapper type structure
|
||||
|
||||
// This verifies the wrapper type exists and can hold Arc<Mutex>
|
||||
assert_eq!(std::mem::size_of::<ConnectivityMonitorWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<ConnectivityMonitorWrapper>() > 0);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -19,6 +19,10 @@ mod smart_cache;
|
||||
pub use pinning::*;
|
||||
pub use smart_cache::*;
|
||||
|
||||
/// One row of the series episode listing used when queueing a whole series:
|
||||
/// `(id, name, season_name, index_number, parent_index_number)`.
|
||||
type EpisodeRow = (String, String, Option<String>, Option<i32>, Option<i32>);
|
||||
|
||||
/// Wrapper for DownloadManager to be used as Tauri state
|
||||
pub struct DownloadManagerWrapper(pub Mutex<DownloadManager>);
|
||||
|
||||
@@ -596,6 +600,10 @@ pub(crate) async fn queue_album_tracks(
|
||||
/// TRACES: UR-018, UR-055 | DR-173 | UT-170
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
// Three of the eight arguments are Tauri `State<'_, _>` injections plus the
|
||||
// `AppHandle`, not caller input. Folding the rest into a struct would change the
|
||||
// IPC contract and the generated TypeScript for no readability gain.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn download_album(
|
||||
db: State<'_, DatabaseWrapper>,
|
||||
repository: State<'_, crate::commands::repository::RepositoryManagerWrapper>,
|
||||
@@ -807,7 +815,7 @@ pub async fn download_series(
|
||||
vec![QueryParam::String(series_id)],
|
||||
);
|
||||
|
||||
let episodes: Vec<(String, String, Option<String>, Option<i32>, Option<i32>)> = db_service
|
||||
let episodes: Vec<EpisodeRow> = db_service
|
||||
.query_many(episodes_query, |row| {
|
||||
Ok((
|
||||
row.get(0)?,
|
||||
@@ -912,6 +920,10 @@ pub async fn download_series(
|
||||
/// Queue all episodes of a specific season for download
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
// One of the eight arguments is a Tauri `State<'_, _>` injection; the rest are
|
||||
// the season's identifying fields. Folding them into a struct would change the
|
||||
// IPC contract and the generated TypeScript for no readability gain.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn download_season(
|
||||
db: State<'_, DatabaseWrapper>,
|
||||
season_id: String,
|
||||
@@ -2307,7 +2319,7 @@ pub async fn delete_downloads_under(
|
||||
)";
|
||||
|
||||
let file_query = Query::with_params(
|
||||
&format!("SELECT d.file_path FROM downloads d WHERE {SCOPE}"),
|
||||
format!("SELECT d.file_path FROM downloads d WHERE {SCOPE}"),
|
||||
vec![
|
||||
QueryParam::String(user_id.clone()),
|
||||
QueryParam::String(item_id.clone()),
|
||||
@@ -2323,7 +2335,7 @@ pub async fn delete_downloads_under(
|
||||
.map_err(|e| e.to_string())?;
|
||||
|
||||
let delete_query = Query::with_params(
|
||||
&format!("DELETE FROM downloads WHERE id IN (SELECT d.id FROM downloads d WHERE {SCOPE})"),
|
||||
format!("DELETE FROM downloads WHERE id IN (SELECT d.id FROM downloads d WHERE {SCOPE})"),
|
||||
vec![
|
||||
QueryParam::String(user_id),
|
||||
QueryParam::String(item_id.clone()),
|
||||
|
||||
@@ -186,6 +186,7 @@ async fn run_drain(app: &tauri::AppHandle) -> Result<(), String> {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::utils::lock::MutexSafe;
|
||||
use rusqlite::Connection;
|
||||
use std::sync::Mutex;
|
||||
|
||||
@@ -211,7 +212,7 @@ mod tests {
|
||||
}
|
||||
|
||||
fn calls(&self) -> Vec<(String, bool)> {
|
||||
let mut calls = self.calls.lock().unwrap().clone();
|
||||
let mut calls = self.calls.lock_safe().clone();
|
||||
calls.sort();
|
||||
calls
|
||||
}
|
||||
|
||||
@@ -360,7 +360,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_playback_reporter_wrapper_structure() {
|
||||
// Verify wrapper type can hold Arc<TokioMutex<Option<T>>>
|
||||
assert_eq!(std::mem::size_of::<PlaybackReporterWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<PlaybackReporterWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -1480,6 +1480,10 @@ pub async fn player_seek_video(
|
||||
/// Note: Frontend should handle saving series preferences after this command succeeds
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
// Two of the eight arguments are Tauri `State<'_, _>` injections, not caller
|
||||
// input. Folding the rest into a struct would change the IPC contract and the
|
||||
// generated TypeScript for no readability gain.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn player_switch_audio_track(
|
||||
player: State<'_, PlayerStateWrapper>,
|
||||
repository_manager: State<'_, super::repository::RepositoryManagerWrapper>,
|
||||
@@ -1559,6 +1563,10 @@ pub async fn player_switch_audio_track(
|
||||
/// TRACES: UR-074 | DR-162
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
// Three of the nine arguments are Tauri `State<'_, _>` injections, not caller
|
||||
// input. Folding the rest into a struct would change the IPC contract and the
|
||||
// generated TypeScript for no readability gain.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn player_set_stream_quality(
|
||||
player: State<'_, PlayerStateWrapper>,
|
||||
repository_manager: State<'_, super::repository::RepositoryManagerWrapper>,
|
||||
@@ -1784,7 +1792,7 @@ pub async fn player_get_status(
|
||||
let local_media = {
|
||||
let queue_arc = controller.queue();
|
||||
let queue = queue_arc.lock().map_err(|e| e.to_string())?;
|
||||
queue.current().map(|item| MergedMediaItem::from(item))
|
||||
queue.current().map(MergedMediaItem::from)
|
||||
};
|
||||
|
||||
let local_is_playing = status.state.is_playing();
|
||||
@@ -1808,10 +1816,7 @@ pub async fn player_get_status(
|
||||
log::info!("[PlayerCommands] Merging remote session state");
|
||||
|
||||
// Merge media item
|
||||
status.merged_media = session
|
||||
.now_playing_item
|
||||
.as_ref()
|
||||
.map(|item| MergedMediaItem::from(item));
|
||||
status.merged_media = session.now_playing_item.as_ref().map(MergedMediaItem::from);
|
||||
|
||||
// Merge isPlaying (NOT isPaused!)
|
||||
status.merged_is_playing = session
|
||||
@@ -2741,6 +2746,8 @@ pub async fn player_disable_jellyfin(player: State<'_, PlayerStateWrapper>) -> R
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::utils::lock::MutexSafe;
|
||||
|
||||
/// The subtitle list the frontend resolved must survive the IPC hop and end
|
||||
/// up on the `MediaItem` the native backend loads.
|
||||
///
|
||||
@@ -3063,7 +3070,7 @@ mod tests {
|
||||
let database = Database::open_in_memory().unwrap();
|
||||
{
|
||||
let conn = database.connection();
|
||||
let conn = conn.lock().unwrap();
|
||||
let conn = conn.lock_safe();
|
||||
conn.execute_batch(&format!(
|
||||
r#"
|
||||
INSERT INTO servers (id, name, url) VALUES ('srv', 'Test', 'http://test');
|
||||
@@ -3119,7 +3126,7 @@ mod tests {
|
||||
assert_eq!(switched, 1, "only the download whose file exists switches");
|
||||
|
||||
let queue = controller.queue();
|
||||
let queue_lock = queue.lock().unwrap();
|
||||
let queue_lock = queue.lock_safe();
|
||||
match &queue_lock.items()[0].source {
|
||||
MediaSource::Local {
|
||||
file_path,
|
||||
@@ -3148,7 +3155,7 @@ mod tests {
|
||||
index_number: Option<i32>,
|
||||
}
|
||||
|
||||
let mut tracks = vec![
|
||||
let mut tracks = [
|
||||
MockTrack {
|
||||
id: "track1".to_string(),
|
||||
name: "Song 1".to_string(),
|
||||
@@ -3241,7 +3248,7 @@ mod tests {
|
||||
}
|
||||
|
||||
// Create tracks in random order (not sorted)
|
||||
let mut tracks = vec![
|
||||
let mut tracks = [
|
||||
MockTrack {
|
||||
id: "id5".to_string(),
|
||||
name: "Track 5".to_string(),
|
||||
|
||||
@@ -67,6 +67,10 @@ pub struct RepositoryManagerWrapper(pub RepositoryManager);
|
||||
/// Returns a handle (UUID) for accessing the repository
|
||||
#[tauri::command]
|
||||
#[specta::specta]
|
||||
// Four of the eight arguments are Tauri `State<'_, _>` injections, not caller
|
||||
// input. Folding the remaining four into a struct would change the IPC contract
|
||||
// and the generated TypeScript for no readability gain.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn repository_create(
|
||||
manager: State<'_, RepositoryManagerWrapper>,
|
||||
player: State<'_, crate::commands::player::PlayerStateWrapper>,
|
||||
@@ -1091,7 +1095,6 @@ mod tests {
|
||||
let handle = format!("{}", uuid);
|
||||
// UUID should convert to a non-empty string
|
||||
assert!(!handle.is_empty());
|
||||
assert!(handle.len() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -89,7 +89,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_session_poller_wrapper_structure() {
|
||||
// Test that wrapper type structure is correct
|
||||
assert_eq!(std::mem::size_of::<SessionPollerWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<SessionPollerWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -1658,19 +1658,19 @@ mod tests {
|
||||
#[test]
|
||||
fn test_database_wrapper_structure() {
|
||||
// Verify DatabaseWrapper can be created and holds Mutex<Database>
|
||||
assert_eq!(std::mem::size_of::<DatabaseWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<DatabaseWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_credential_store_wrapper_structure() {
|
||||
// Verify CredentialStoreWrapper can be created
|
||||
assert_eq!(std::mem::size_of::<CredentialStoreWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<CredentialStoreWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_thumbnail_cache_wrapper_structure() {
|
||||
// Verify ThumbnailCacheWrapper holds Arc<ThumbnailCache>
|
||||
assert_eq!(std::mem::size_of::<ThumbnailCacheWrapper>() > 0, true);
|
||||
assert!(std::mem::size_of::<ThumbnailCacheWrapper>() > 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -481,6 +481,7 @@ pub async fn sync_process_pending(app: tauri::AppHandle) -> Result<DrainReport,
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::utils::lock::MutexSafe;
|
||||
use rusqlite::Connection;
|
||||
use std::sync::Mutex;
|
||||
|
||||
@@ -517,7 +518,7 @@ mod tests {
|
||||
}
|
||||
|
||||
fn calls(&self) -> Vec<QueuedOp> {
|
||||
self.calls.lock().unwrap().clone()
|
||||
self.calls.lock_safe().clone()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -527,7 +528,7 @@ mod tests {
|
||||
if let Some(err) = &self.fail_with {
|
||||
return Err(err.clone());
|
||||
}
|
||||
self.calls.lock().unwrap().push(op.clone());
|
||||
self.calls.lock_safe().push(op.clone());
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -20,9 +20,6 @@ use sha2::{Digest, Sha256};
|
||||
use std::fs;
|
||||
use std::path::PathBuf;
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
use hostname;
|
||||
|
||||
#[cfg(not(target_os = "android"))]
|
||||
const SERVICE_NAME: &str = "com.dtourolle.jellytau";
|
||||
|
||||
@@ -203,15 +200,12 @@ impl CredentialStore {
|
||||
|
||||
// secret-tool doesn't support --version, so we test with a search command
|
||||
// that will succeed even if no items are found
|
||||
match Command::new("secret-tool")
|
||||
Command::new("secret-tool")
|
||||
.arg("search")
|
||||
.arg("service")
|
||||
.arg("__nonexistent_test__")
|
||||
.output()
|
||||
{
|
||||
Ok(_) => true, // If command runs (even with no results), secret-tool is available
|
||||
Err(_) => false, // Command not found or can't execute
|
||||
}
|
||||
.is_ok()
|
||||
}
|
||||
|
||||
#[cfg(all(not(target_os = "android"), not(target_os = "linux")))]
|
||||
|
||||
@@ -119,11 +119,12 @@ mod tests {
|
||||
use super::*;
|
||||
|
||||
fn item(name: &str, kind: MediaKind) -> MediaItem {
|
||||
let mut item = MediaItem::default();
|
||||
item.id = format!("id-{}-{:?}", name, kind);
|
||||
item.name = name.to_string();
|
||||
item.kind = kind;
|
||||
item
|
||||
MediaItem {
|
||||
id: format!("id-{}-{:?}", name, kind),
|
||||
name: name.to_string(),
|
||||
kind,
|
||||
..MediaItem::default()
|
||||
}
|
||||
}
|
||||
|
||||
fn names(items: &[MediaItem]) -> Vec<&str> {
|
||||
|
||||
@@ -389,14 +389,18 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_queue_precache_config() {
|
||||
let mut config = CacheConfig::default();
|
||||
config.queue_precache_enabled = false;
|
||||
let config = CacheConfig {
|
||||
queue_precache_enabled: false,
|
||||
..CacheConfig::default()
|
||||
};
|
||||
|
||||
let cache = SmartCache::new(config);
|
||||
assert!(!cache.should_precache_queue());
|
||||
|
||||
let mut new_config = CacheConfig::default();
|
||||
new_config.wifi_only = false;
|
||||
let new_config = CacheConfig {
|
||||
wifi_only: false,
|
||||
..CacheConfig::default()
|
||||
};
|
||||
cache.update_config(new_config);
|
||||
|
||||
assert!(cache.should_precache_queue());
|
||||
@@ -407,9 +411,11 @@ mod tests {
|
||||
// wifi_only must not short-circuit precaching: the network gate lives in
|
||||
// the download pump, which checks the *actual* transport. Enabling
|
||||
// WiFi-only while on WiFi should still precache.
|
||||
let mut config = CacheConfig::default();
|
||||
config.queue_precache_enabled = true;
|
||||
config.wifi_only = true;
|
||||
let config = CacheConfig {
|
||||
queue_precache_enabled: true,
|
||||
wifi_only: true,
|
||||
..CacheConfig::default()
|
||||
};
|
||||
|
||||
let cache = SmartCache::new(config);
|
||||
assert!(cache.should_precache_queue());
|
||||
@@ -422,7 +428,7 @@ mod tests {
|
||||
/// TRACES: UR-071 | DR-127 | UT-120
|
||||
#[tokio::test]
|
||||
async fn test_reclaim_expired_only_takes_expired_temporary_entries() {
|
||||
use crate::storage::db_service::{DatabaseService, RusqliteService};
|
||||
use crate::storage::db_service::RusqliteService;
|
||||
use rusqlite::Connection;
|
||||
use std::sync::{Arc, Mutex};
|
||||
|
||||
|
||||
@@ -620,7 +620,7 @@ fn create_player_backend(
|
||||
match MpvBackend::new(Some(_event_emitter), playback_reporter, position_throttler) {
|
||||
Ok(backend) => {
|
||||
info!("Successfully initialized MPV backend for Linux");
|
||||
return Box::new(backend);
|
||||
Box::new(backend)
|
||||
}
|
||||
Err(e) => {
|
||||
error!("\n========================================");
|
||||
@@ -645,7 +645,7 @@ fn create_player_backend(
|
||||
// still browse the library and manage downloads, and the frontend
|
||||
// can show a "playback unavailable" notice via this event.
|
||||
emit_backend_init_failed(&app_handle, "mpv", e.to_string());
|
||||
return Box::new(NullBackend::new());
|
||||
Box::new(NullBackend::new())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -624,7 +624,7 @@ impl PlaybackModeManager {
|
||||
);
|
||||
|
||||
// Log first few track IDs for debugging
|
||||
if queue_ids.len() > 0 {
|
||||
if !queue_ids.is_empty() {
|
||||
let preview: Vec<&str> = queue_ids.iter().take(3).map(|s| s.as_str()).collect();
|
||||
debug!("[PlaybackMode] First track IDs: {:?}...", preview);
|
||||
}
|
||||
@@ -914,7 +914,7 @@ mod tests {
|
||||
|
||||
impl PlayerEventEmitter for CapturingEmitter {
|
||||
fn emit(&self, event: PlayerStatusEvent) {
|
||||
self.events.lock().unwrap().push(event);
|
||||
self.events.lock_safe().push(event);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -942,7 +942,7 @@ mod tests {
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
manager.set_mode(PlaybackMode::Idle);
|
||||
|
||||
let events = emitter.events.lock().unwrap();
|
||||
let events = emitter.events.lock_safe();
|
||||
assert_eq!(events.len(), 3, "one event per real mode change");
|
||||
|
||||
match &events[0] {
|
||||
@@ -975,10 +975,10 @@ mod tests {
|
||||
|
||||
impl RemoteVolumeControl for RecordingVolumeControl {
|
||||
fn enable(&self, _initial_volume: i32) {
|
||||
self.calls.lock().unwrap().push("enable");
|
||||
self.calls.lock_safe().push("enable");
|
||||
}
|
||||
fn disable(&self) {
|
||||
self.calls.lock().unwrap().push("disable");
|
||||
self.calls.lock_safe().push("disable");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1014,7 +1014,7 @@ mod tests {
|
||||
manager.set_mode(PlaybackMode::Idle);
|
||||
|
||||
assert_eq!(
|
||||
*volume.calls.lock().unwrap(),
|
||||
*volume.calls.lock_safe(),
|
||||
vec!["enable", "disable"],
|
||||
"remote->idle must return volume control to the local speaker"
|
||||
);
|
||||
@@ -1033,7 +1033,7 @@ mod tests {
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
|
||||
assert_eq!(
|
||||
*volume.calls.lock().unwrap(),
|
||||
*volume.calls.lock_safe(),
|
||||
vec!["enable", "disable"],
|
||||
"remote->local must return volume control to the local speaker"
|
||||
);
|
||||
@@ -1053,7 +1053,7 @@ mod tests {
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
|
||||
assert!(
|
||||
volume.calls.lock().unwrap().is_empty(),
|
||||
volume.calls.lock_safe().is_empty(),
|
||||
"local/idle transitions must not touch remote volume routing"
|
||||
);
|
||||
}
|
||||
@@ -1074,7 +1074,7 @@ mod tests {
|
||||
});
|
||||
|
||||
assert_eq!(
|
||||
*volume.calls.lock().unwrap(),
|
||||
*volume.calls.lock_safe(),
|
||||
vec!["enable", "enable"],
|
||||
"remote->remote re-arms control without releasing it to local"
|
||||
);
|
||||
@@ -1091,7 +1091,7 @@ mod tests {
|
||||
manager.set_mode(PlaybackMode::Local);
|
||||
|
||||
assert_eq!(
|
||||
emitter.events.lock().unwrap().len(),
|
||||
emitter.events.lock_safe().len(),
|
||||
1,
|
||||
"repeated identical mode set emits only once"
|
||||
);
|
||||
|
||||
@@ -6,6 +6,12 @@ use serde::{Deserialize, Serialize};
|
||||
/// Autoplay decision result - determines what happens after playback ends
|
||||
#[derive(specta::Type, Debug, Clone, Serialize)]
|
||||
#[serde(tag = "action", rename_all = "camelCase")]
|
||||
// `ShowNextEpisodePopup` carries two `MediaItem`s, so it dwarfs the unit
|
||||
// variants. Boxing them is not worth it here: this enum is constructed once per
|
||||
// end-of-item (never in a hot loop or a large collection), and it is an IPC type
|
||||
// — the indirection would have to stay invisible to serde/specta while every
|
||||
// match arm gained a deref, for no measurable gain.
|
||||
#[allow(clippy::large_enum_variant)]
|
||||
pub enum AutoplayDecision {
|
||||
/// Stop playback (no next item or timer expired)
|
||||
Stop,
|
||||
|
||||
@@ -30,6 +30,12 @@ use super::{MediaSessionType, SleepTimerMode};
|
||||
// queue_changed never reach the frontend, so the mini player never appears).
|
||||
// Keep serde and specta agreeing: snake_case fields, snake_case variant tags.
|
||||
#[serde(tag = "type", rename_all = "snake_case")]
|
||||
// `ShowNextEpisodePopup` carries two `MediaItem`s, so it dwarfs the small
|
||||
// position/state variants. Boxing them is rejected deliberately: this is a
|
||||
// serde + specta wire type whose generated TypeScript must not shift, and the
|
||||
// events are emitted a few times a second at most — never bulk-allocated — so
|
||||
// the size difference costs nothing measurable.
|
||||
#[allow(clippy::large_enum_variant)]
|
||||
pub enum PlayerStatusEvent {
|
||||
/// Playback position updated (emitted periodically during playback)
|
||||
PositionUpdate {
|
||||
|
||||
@@ -243,7 +243,7 @@ impl MpvBackend {
|
||||
});
|
||||
}
|
||||
}
|
||||
libmpv::events::Event::PropertyChange { name, .. } if name == "pause" => {
|
||||
libmpv::events::Event::PropertyChange { name: "pause", .. } => {
|
||||
// Handle pause state changes
|
||||
if let Ok(is_paused) = mpv.get_property::<bool>("pause") {
|
||||
let media_id = state
|
||||
|
||||
@@ -1,14 +1,15 @@
|
||||
/// Tests for MpvBackend to prevent regressions
|
||||
///
|
||||
/// These tests are designed to catch common issues like:
|
||||
/// - Tokio runtime panics when spawning async tasks from std::thread
|
||||
/// - Position update thread failures
|
||||
/// - Event emission issues
|
||||
///
|
||||
/// TRACES: UR-003, UR-004 | IR-003 | IT-003, IT-004
|
||||
//! Tests for MpvBackend to prevent regressions
|
||||
//!
|
||||
//! These tests are designed to catch common issues like:
|
||||
//! - Tokio runtime panics when spawning async tasks from std::thread
|
||||
//! - Position update thread failures
|
||||
//! - Event emission issues
|
||||
//!
|
||||
//! TRACES: UR-003, UR-004 | IR-003 | IT-003, IT-004
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use crate::utils::lock::MutexSafe;
|
||||
use std::sync::{Arc, Mutex};
|
||||
use tokio::sync::Mutex as TokioMutex;
|
||||
|
||||
@@ -67,14 +68,14 @@ mod tests {
|
||||
if let Ok(handle) = tokio::runtime::Handle::try_current() {
|
||||
// Has runtime (shouldn't happen in this test)
|
||||
handle.spawn(async move {
|
||||
*counter_clone.lock().unwrap() += 1;
|
||||
*counter_clone.lock_safe() += 1;
|
||||
});
|
||||
} else {
|
||||
// No runtime - use fallback (should happen in this test)
|
||||
std::thread::spawn(move || {
|
||||
let rt = tokio::runtime::Runtime::new().unwrap();
|
||||
rt.block_on(async move {
|
||||
*counter_clone.lock().unwrap() += 1;
|
||||
*counter_clone.lock_safe() += 1;
|
||||
});
|
||||
});
|
||||
}
|
||||
@@ -85,7 +86,7 @@ mod tests {
|
||||
// Wait for async task to complete
|
||||
std::thread::sleep(std::time::Duration::from_millis(100));
|
||||
|
||||
let count = *counter.lock().unwrap();
|
||||
let count = *counter.lock_safe();
|
||||
assert_eq!(
|
||||
count, 1,
|
||||
"Fallback pattern should execute async code successfully"
|
||||
@@ -109,13 +110,13 @@ mod tests {
|
||||
let position = i as f64 * 0.25;
|
||||
|
||||
// Store position (simulating event emission)
|
||||
positions_clone.lock().unwrap().push(position);
|
||||
positions_clone.lock_safe().push(position);
|
||||
}
|
||||
});
|
||||
|
||||
handle.join().unwrap();
|
||||
|
||||
let recorded_positions = positions.lock().unwrap();
|
||||
let recorded_positions = positions.lock_safe();
|
||||
assert_eq!(
|
||||
recorded_positions.len(),
|
||||
5,
|
||||
|
||||
@@ -806,11 +806,10 @@ mod tests {
|
||||
assert_eq!(queue.current_index(), Some(first_shuffled_index));
|
||||
|
||||
// Move through shuffle order
|
||||
for i in 1..shuffle_order.len() {
|
||||
for &expected_index in &shuffle_order[1..] {
|
||||
assert!(queue.has_next());
|
||||
let result = queue.next();
|
||||
assert!(result.is_some());
|
||||
let expected_index = shuffle_order[i];
|
||||
assert_eq!(queue.current_index(), Some(expected_index));
|
||||
}
|
||||
|
||||
|
||||
@@ -159,6 +159,9 @@ mod tests {
|
||||
#[test]
|
||||
fn test_end_reason_clone() {
|
||||
let reason = EndReason::Finished;
|
||||
// Deliberately exercising the derived `Clone` impl, not a plain copy:
|
||||
// `EndReason` is also `Copy`, so clippy flags the call as redundant.
|
||||
#[allow(clippy::clone_on_copy)]
|
||||
let cloned = reason.clone();
|
||||
assert_eq!(reason, cloned);
|
||||
}
|
||||
|
||||
@@ -196,7 +196,7 @@ mod tests {
|
||||
|
||||
impl PlayerEventEmitter for RecordingEmitter {
|
||||
fn emit(&self, event: PlayerStatusEvent) {
|
||||
self.events.lock().unwrap().push(event);
|
||||
self.events.lock_safe().push(event);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -244,7 +244,7 @@ mod tests {
|
||||
let (mut b, events) = backend();
|
||||
b.load(&test_media()).unwrap();
|
||||
|
||||
let ev = events.lock().unwrap();
|
||||
let ev = events.lock_safe();
|
||||
let load = ev
|
||||
.iter()
|
||||
.find(|e| matches!(e, PlayerStatusEvent::WebviewAudioLoad { .. }))
|
||||
@@ -263,7 +263,7 @@ mod tests {
|
||||
b.pause().unwrap();
|
||||
b.seek(42.0).unwrap();
|
||||
|
||||
let ev = events.lock().unwrap();
|
||||
let ev = events.lock_safe();
|
||||
assert!(ev.iter().any(|e| matches!(
|
||||
e,
|
||||
PlayerStatusEvent::ControlCommand { action, .. } if action == "pause"
|
||||
|
||||
@@ -1134,6 +1134,16 @@ impl MediaRepository for HybridRepository {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
// `GATE_TEST_LOCK` below serialises the tests that flip the process-global
|
||||
// `INCLUDE_CATALOG_BROWSE` flag, so its guard is deliberately held across
|
||||
// the `.await` of the repository call under test — that await *is* the
|
||||
// critical section. This is not the production deadlock hazard the lint
|
||||
// targets: the lock is test-only, uncontended outside these tests, and each
|
||||
// `#[tokio::test]` runs on its own single-threaded runtime, so a held guard
|
||||
// cannot block another task on the same worker. Restructuring around it
|
||||
// would reintroduce the flag race the lock exists to prevent.
|
||||
#![allow(clippy::await_holding_lock)]
|
||||
|
||||
use super::*;
|
||||
use std::sync::Mutex;
|
||||
|
||||
|
||||
@@ -2510,6 +2510,16 @@ impl MediaRepository for OfflineRepository {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
// `CATALOG_BROWSE_LOCK` below serialises the tests that flip the
|
||||
// process-global `INCLUDE_CATALOG_BROWSE` flag, so its guard is deliberately
|
||||
// held across the `.await` of the query under test — that await *is* the
|
||||
// critical section. This is not the production deadlock hazard the lint
|
||||
// targets: the lock is test-only, uncontended outside these tests, and each
|
||||
// `#[tokio::test]` runs on its own single-threaded runtime, so a held guard
|
||||
// cannot block another task on the same worker. Restructuring around it
|
||||
// would reintroduce the flag race the lock exists to prevent.
|
||||
#![allow(clippy::await_holding_lock)]
|
||||
|
||||
use super::*;
|
||||
use crate::storage::db_service::RusqliteService;
|
||||
use rusqlite::Connection;
|
||||
@@ -2524,9 +2534,8 @@ mod tests {
|
||||
static CATALOG_BROWSE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
|
||||
|
||||
fn lock_catalog_browse() -> std::sync::MutexGuard<'static, ()> {
|
||||
CATALOG_BROWSE_LOCK
|
||||
.lock()
|
||||
.unwrap_or_else(|poisoned| poisoned.into_inner())
|
||||
use crate::utils::lock::MutexSafe;
|
||||
CATALOG_BROWSE_LOCK.lock_safe()
|
||||
}
|
||||
|
||||
/// TRACES: UR-065 | DR-108 | UT-111
|
||||
|
||||
@@ -984,7 +984,7 @@ struct JellyfinMediaSource {
|
||||
}
|
||||
|
||||
impl JellyfinItem {
|
||||
fn to_media_item(self, server_id: String) -> MediaItem {
|
||||
fn into_media_item(self, server_id: String) -> MediaItem {
|
||||
// Extract image tags before consuming self
|
||||
let primary_tag = self.image_tags.as_ref().and_then(|tags| tags.primary());
|
||||
let backdrop_tags = self.backdrop_image_tags;
|
||||
@@ -1123,7 +1123,7 @@ impl MediaRepository for OnlineRepository {
|
||||
items: response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect(),
|
||||
total_record_count: response.total_record_count,
|
||||
})
|
||||
@@ -1133,7 +1133,7 @@ impl MediaRepository for OnlineRepository {
|
||||
let endpoint = format!("/Users/{}/Items/{}?Fields=BackdropImageTags,ParentBackdropImageTags,People,MediaStreams,MediaSources,PremiereDate,UserData", self.user_id, item_id);
|
||||
|
||||
let item: JellyfinItem = self.get_json(&endpoint).await?;
|
||||
let media_item = item.to_media_item(self.user_id.clone());
|
||||
let media_item = item.into_media_item(self.user_id.clone());
|
||||
|
||||
Ok(media_item)
|
||||
}
|
||||
@@ -1148,7 +1148,7 @@ impl MediaRepository for OnlineRepository {
|
||||
let items: Vec<JellyfinItem> = self.get_json(&endpoint).await?;
|
||||
Ok(items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1171,7 +1171,7 @@ impl MediaRepository for OnlineRepository {
|
||||
Ok(response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1186,7 +1186,7 @@ impl MediaRepository for OnlineRepository {
|
||||
Ok(response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1206,7 +1206,7 @@ impl MediaRepository for OnlineRepository {
|
||||
let items: Vec<MediaItem> = response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect();
|
||||
|
||||
debug!("[get_recently_played_audio] Fetched {} items", items.len());
|
||||
@@ -1229,7 +1229,7 @@ impl MediaRepository for OnlineRepository {
|
||||
"[get_recently_played_audio] Grouping item '{}' into album '{}'",
|
||||
item.name, key
|
||||
);
|
||||
album_map.entry(key).or_insert_with(Vec::new).push(item);
|
||||
album_map.entry(key).or_default().push(item);
|
||||
} else {
|
||||
debug!(
|
||||
"[get_recently_played_audio] No album_id or album_name for item: '{}'",
|
||||
@@ -1344,7 +1344,7 @@ impl MediaRepository for OnlineRepository {
|
||||
Ok(response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1359,7 +1359,7 @@ impl MediaRepository for OnlineRepository {
|
||||
Ok(response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1461,7 +1461,7 @@ impl MediaRepository for OnlineRepository {
|
||||
items: response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect(),
|
||||
total_record_count: response.total_record_count,
|
||||
})
|
||||
@@ -1839,7 +1839,7 @@ impl MediaRepository for OnlineRepository {
|
||||
Ok(response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.server_url.clone()))
|
||||
.map(|item| item.into_media_item(self.server_url.clone()))
|
||||
.collect())
|
||||
}
|
||||
|
||||
@@ -1852,7 +1852,7 @@ impl MediaRepository for OnlineRepository {
|
||||
let items = response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.server_url.clone()))
|
||||
.map(|item| item.into_media_item(self.server_url.clone()))
|
||||
.collect();
|
||||
Ok(SearchResult {
|
||||
items,
|
||||
@@ -2189,7 +2189,7 @@ impl MediaRepository for OnlineRepository {
|
||||
items: response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect(),
|
||||
total_record_count: response.total_record_count,
|
||||
})
|
||||
@@ -2319,7 +2319,7 @@ impl MediaRepository for OnlineRepository {
|
||||
async fn get_person(&self, person_id: &str) -> Result<MediaItem, RepoError> {
|
||||
let endpoint = format!("/Users/{}/Items/{}", self.user_id, person_id);
|
||||
let item: JellyfinItem = self.get_json(&endpoint).await?;
|
||||
Ok(item.to_media_item(self.user_id.clone()))
|
||||
Ok(item.into_media_item(self.user_id.clone()))
|
||||
}
|
||||
|
||||
async fn get_items_by_person(
|
||||
@@ -2349,7 +2349,7 @@ impl MediaRepository for OnlineRepository {
|
||||
items: response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect(),
|
||||
total_record_count: response.total_record_count,
|
||||
})
|
||||
@@ -2373,7 +2373,7 @@ impl MediaRepository for OnlineRepository {
|
||||
items: response
|
||||
.items
|
||||
.into_iter()
|
||||
.map(|item| item.to_media_item(self.user_id.clone()))
|
||||
.map(|item| item.into_media_item(self.user_id.clone()))
|
||||
.collect(),
|
||||
total_record_count: response.total_record_count,
|
||||
})
|
||||
@@ -2461,7 +2461,7 @@ impl MediaRepository for OnlineRepository {
|
||||
.into_iter()
|
||||
.map(|pi| PlaylistEntry {
|
||||
playlist_item_id: pi.playlist_item_id,
|
||||
item: pi.item.to_media_item(self.user_id.clone()),
|
||||
item: pi.item.into_media_item(self.user_id.clone()),
|
||||
})
|
||||
.collect())
|
||||
}
|
||||
@@ -2959,7 +2959,7 @@ mod tests {
|
||||
}))
|
||||
.expect("fixture must deserialize");
|
||||
|
||||
let streams = item.to_media_item("server-1".to_string()).media_streams;
|
||||
let streams = item.into_media_item("server-1".to_string()).media_streams;
|
||||
let streams = streams.expect("the item carries streams");
|
||||
let deliverable = |index: i32| {
|
||||
streams
|
||||
@@ -3554,7 +3554,7 @@ mod tests {
|
||||
}"#;
|
||||
|
||||
let item: JellyfinItem = serde_json::from_str(json).expect("item should deserialize");
|
||||
let media = item.to_media_item("server1".to_string());
|
||||
let media = item.into_media_item("server1".to_string());
|
||||
|
||||
let user_data = media.user_data.expect("user data should be mapped");
|
||||
assert_eq!(user_data.is_favorite, Some(true));
|
||||
@@ -3574,7 +3574,7 @@ mod tests {
|
||||
let json = r#"{"Id": "x", "Name": "No User Data", "Type": "Movie"}"#;
|
||||
|
||||
let item: JellyfinItem = serde_json::from_str(json).expect("item should deserialize");
|
||||
let media = item.to_media_item("server1".to_string());
|
||||
let media = item.into_media_item("server1".to_string());
|
||||
|
||||
assert!(media.user_data.is_none());
|
||||
}
|
||||
@@ -3618,7 +3618,7 @@ mod tests {
|
||||
}"#;
|
||||
|
||||
let jellyfin_item: JellyfinItem = serde_json::from_str(json).expect("Failed to parse");
|
||||
let media_item = jellyfin_item.to_media_item("test-server-id".to_string());
|
||||
let media_item = jellyfin_item.into_media_item("test-server-id".to_string());
|
||||
|
||||
assert_eq!(media_item.id, "album456");
|
||||
assert_eq!(media_item.name, "Love and Theft");
|
||||
|
||||
@@ -155,6 +155,10 @@ impl ThumbnailCache {
|
||||
}
|
||||
|
||||
/// Save thumbnail to cache
|
||||
// The arguments are the cache key (item/type/tag) plus the payload and its
|
||||
// dimensions — all independent scalars borrowed from the caller. A parameter
|
||||
// struct would only move the same list one level down.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn save_thumbnail(
|
||||
&self,
|
||||
db: Arc<RusqliteService>,
|
||||
|
||||
Reference in New Issue
Block a user