queue, tui: dedup by title, and keep foreign writes off the screen
Four findings from a day's cbd.log and a 121-entry queue. **Dedup by title.** The provider-item identity removed nothing from that queue — 121 entries, 121 distinct ids — while three "Sink Into The Hips" sat in it: two remixes and the album version, 171/189/228s. Those are different recordings and merging them by default would silently discard a version the user chose, so the default stands and `DedupQueue` gains a `by_title` flag: lowercased (artist, title), survivor = the playing entry else the *longest* take. Own key everywhere (`U`, `queue dedup --titles`) because it throws recordings away. Duration-tolerant matching was the third option and is not worth it: every same-title group in that queue differed by tens of seconds, so a safe tolerance caught nothing. **stderr no longer points at the terminal.** tracing goes to a file because the TUI owns the screen, but fd 2 did not — and in the bundled `cbd` the ALSA C library shares the process, so its "underrun occurred" printed straight onto the interface, scrolled the terminal a line and left the layout looking shifted (the queue appearing to bleed into the now-playing pane; ratatui repaints only changed cells, so it persisted). The message was lost too. One dup2 before the alternate screen sends fd 2 to `<log dir>/cbd.stderr.log`, so those diagnostics are kept instead. **The spectrum flapped ~1/s during playback**, 3664 times in one log. tokio's default MissedTickBehavior::Burst keeps the absolute schedule, so once the per-tick FFT lateness reaches a whole period two ticks fire back to back and the second necessarily sees no new frames — read as silence, which zeroed the bars. Now `Delay`, plus a FlowDetector that wants two consecutive empty ticks before declaring idle. **The one ERROR in the log was a shutdown race**, mislabelled: "request to server failed: sending on a closed channel" was the orchestrator's send to the UI channel after the UI thread exited. It now reports the UI closing at info and stops the loop instead of spinning on a stream nobody reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Test User
parent
001abcc35c
commit
bb084dd62b
|
|
@ -706,6 +706,7 @@ dependencies = [
|
|||
"crossterm",
|
||||
"dirs",
|
||||
"flume",
|
||||
"libc",
|
||||
"mpris-server",
|
||||
"notify-rust",
|
||||
"ratatui",
|
||||
|
|
|
|||
|
|
@ -49,6 +49,9 @@ futures = "0.3"
|
|||
gloo-timers = { version = "0.3", features = ["futures"] }
|
||||
http = "1"
|
||||
include_dir = "0.7"
|
||||
# `dup2` only: the TUI points stderr at a file so C-library writes (ALSA
|
||||
# underrun messages) cannot scribble on the interface (cbd-tui/src/stderr.rs).
|
||||
libc = "0.2"
|
||||
leptos = { version = "0.8", default-features = false, features = ["csr"] }
|
||||
notify-rust = "4"
|
||||
# The MPRIS D-Bus surface of cbd-tui. Built on zbus, which speaks the
|
||||
|
|
|
|||
|
|
@ -90,13 +90,14 @@ the playback loop like every other queue verb.
|
|||
3. **Same artist and title**, normalized. Would also catch the same song
|
||||
from two different providers.
|
||||
|
||||
**Decision: 2, falling back to 1** (D3). 3 is rejected: identical
|
||||
artist/title is routinely a *different recording* — a live take, a remaster,
|
||||
a radio edit, the studio version — and the server cannot tell which. Dropping
|
||||
one would be a silent, unrecoverable edit of the user's queue, and the false
|
||||
positives land exactly on the collections (greatest-hits, live albums) where
|
||||
a user is most deliberate. A fuzzy dedup belongs behind a client-side preview
|
||||
where the user confirms each pair; that is Deferred.
|
||||
**Decision: 2 by default, falling back to 1** (D3) — and **3 behind an
|
||||
explicit flag** (D3a). 3 cannot be the default: identical artist/title is
|
||||
routinely a *different recording* — a live take, a remaster, a radio edit, the
|
||||
studio version — and the server cannot tell which, so a default that merged
|
||||
them would silently and unrecoverably edit the queue, with the false positives
|
||||
landing exactly on the collections (greatest-hits, live albums, remix albums)
|
||||
where a user is most deliberate. But it is what a listener sometimes means, so
|
||||
it is reachable in one keypress and never by accident.
|
||||
|
||||
## Decisions
|
||||
|
||||
|
|
@ -115,8 +116,8 @@ where the user confirms each pair; that is Deferred.
|
|||
bounded result channel, the way `SaveQueue` already reports.
|
||||
`SortQueueResponse` stays empty: the new order *is* the answer, and it
|
||||
arrives on the stream.
|
||||
- **D3 — Duplicate identity is `(provider, provider_item_id)`, or the whole
|
||||
path.** The provider is the first path segment; ids are provider-internal
|
||||
- **D3 — The default duplicate identity is `(provider, provider_item_id)`, or
|
||||
the whole path.** The provider is the first path segment; ids are provider-internal
|
||||
(the content store keys them the same way, `by_provider_id(provider, id)`),
|
||||
so leaving them unscoped would let two providers' numeric ids collide and
|
||||
merge two unrelated tracks. When the id is empty — most providers, and
|
||||
|
|
@ -125,6 +126,20 @@ where the user confirms each pair; that is Deferred.
|
|||
streaming original are **not** duplicates, because they are different
|
||||
providers. Conservative on purpose: a missed duplicate is a keypress, a
|
||||
wrong merge is lost queue state.
|
||||
- **D3a — …and an opt-in `by_title` identity beside it.** Measured against a
|
||||
real queue, D3 alone removed nothing from 121 entries — every entry was a
|
||||
distinct provider item — while the *user* saw duplicates: four "Referees
|
||||
Don't Fall In Love", three "Sink Into The Hips". They were remixes, edits and
|
||||
album versions of the same songs, which D3 is right to keep and which a
|
||||
listener may still not want three of. So `DedupQueue` takes a flag: with
|
||||
`by_title` the identity is the lowercased `(artist, title)` pair, and the
|
||||
survivor of a group is the **longest** take (the full version rather than a
|
||||
radio edit), still yielding to the playing entry. It stays opt-in and on its
|
||||
own key, because it *discards recordings that differ* — the exact loss D3
|
||||
refuses to make the default. Duration-tolerant matching was the third option
|
||||
and is not worth its complexity: on the queue that motivated this, every
|
||||
same-title group differed by tens of seconds, so any tolerance narrow enough
|
||||
to be safe caught nothing.
|
||||
- **D4 — The survivor is the current track, else the earliest.** Within a
|
||||
group of duplicates, the entry at the current position survives if it is
|
||||
in the group; otherwise the earliest one does, and every later copy goes.
|
||||
|
|
@ -187,8 +202,10 @@ where the user confirms each pair; that is Deferred.
|
|||
`play_order`/`current_offset` bookkeeping, because that invariant is
|
||||
theirs. The split is what keeps D3/D7/D8 testable as data instead of
|
||||
through a queue.
|
||||
- **D14 — TUI: `u` dedups, `S` opens a sort menu.** `u` is "unique" and is
|
||||
free in the queue scope. Sorting needs a choice, so `S` opens a modal
|
||||
- **D14 — TUI: `u` dedups, `U` dedups by title, `S` opens a sort menu.** `u`
|
||||
is "unique" and is free in the queue scope; the shifted form is the shifted
|
||||
*behaviour* (D3a), which is the pattern `w`/`W` and `c`/`C` already use here
|
||||
for "the same verb, more of it". Sorting needs a choice, so `S` opens a modal
|
||||
overlay listing the five strategies with their letters (`a` artist, `l`
|
||||
album, `t` title, `d` duration, `r` reverse; the capital of each sorts
|
||||
descending), `Esc` closes. A menu rather than a chord sequence: the
|
||||
|
|
@ -323,8 +340,9 @@ loop -> client: SortQueueResponse {}
|
|||
|
||||
## Deferred
|
||||
|
||||
- **Fuzzy dedup** (artist/title matching) behind a client-side confirmation
|
||||
view — the only safe home for it (see the options above).
|
||||
- **A preview for `by_title`** (D3a): a confirmation view listing which
|
||||
recording of each song would survive, so the aggressive identity can be
|
||||
inspected before it removes anything rather than only undone by re-queueing.
|
||||
- **`ReorderQueue`/move**: drag-and-drop in the web client and `K`/`J` row
|
||||
moves in the TUI, on an RPC that identifies rows by path rather than
|
||||
index (Option C).
|
||||
|
|
|
|||
|
|
@ -241,14 +241,19 @@ async fn run_queue(client: &mut Client, cmd: QueueCmd) -> Result<(), Box<dyn std
|
|||
.map_err(rpc_error)?;
|
||||
println!("cleared the queue");
|
||||
}
|
||||
QueueCmd::Dedup => {
|
||||
QueueCmd::Dedup { titles } => {
|
||||
let response = client
|
||||
.dedup_queue(DedupQueueRequest {})
|
||||
.dedup_queue(DedupQueueRequest { by_title: titles })
|
||||
.await
|
||||
.map_err(rpc_error)?;
|
||||
// 0 is a real answer — "there were no duplicates" — so it is
|
||||
// printed like any other count (architecture/queue-order.md D2).
|
||||
println!("removed {} duplicate(s)", response.into_inner().removed);
|
||||
let removed = response.into_inner().removed;
|
||||
if titles {
|
||||
println!("removed {removed} same-title duplicate(s)");
|
||||
} else {
|
||||
println!("removed {removed} duplicate(s)");
|
||||
}
|
||||
}
|
||||
QueueCmd::Sort { key, desc } => {
|
||||
client
|
||||
|
|
|
|||
|
|
@ -107,7 +107,13 @@ pub enum QueueCmd {
|
|||
},
|
||||
/// Drop duplicate entries, keeping one copy of each track. Prints how
|
||||
/// many were removed; never removes the playing track.
|
||||
Dedup,
|
||||
Dedup {
|
||||
/// Collapse same artist+title instead of the provably-same item: one
|
||||
/// entry per *song*, keeping the longest take. Aggressive — a remix and
|
||||
/// the album version count as the same song.
|
||||
#[arg(long)]
|
||||
titles: bool,
|
||||
},
|
||||
/// Reorder the queue by one strategy (architecture/queue-order.md).
|
||||
Sort {
|
||||
#[arg(value_enum)]
|
||||
|
|
@ -431,12 +437,19 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn queue_dedup_takes_no_arguments() {
|
||||
fn queue_dedup_takes_an_optional_titles_flag() {
|
||||
let cli = TuiCli::try_parse_from(["cbd-tui", "queue", "dedup"]).expect("parse");
|
||||
assert!(matches!(
|
||||
cli.command,
|
||||
Some(TuiCommand::Queue(QueueCmd::Dedup))
|
||||
));
|
||||
match cli.command {
|
||||
Some(TuiCommand::Queue(QueueCmd::Dedup { titles })) => {
|
||||
assert!(!titles, "the safe identity unless asked")
|
||||
}
|
||||
other => panic!("unexpected: {other:?}"),
|
||||
}
|
||||
let cli = TuiCli::try_parse_from(["cbd-tui", "queue", "dedup", "--titles"]).expect("parse");
|
||||
match cli.command {
|
||||
Some(TuiCommand::Queue(QueueCmd::Dedup { titles })) => assert!(titles),
|
||||
other => panic!("unexpected: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
/// The strategy is a value enum, so it is spelled lowercase, completes in
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ clap.workspace = true
|
|||
dirs.workspace = true
|
||||
toml.workspace = true
|
||||
flume.workspace = true
|
||||
libc.workspace = true
|
||||
mpris-server = { workspace = true, optional = true }
|
||||
notify-rust = { workspace = true, optional = true }
|
||||
ratatui.workspace = true
|
||||
|
|
|
|||
|
|
@ -127,6 +127,10 @@ pub enum Action {
|
|||
/// removed appears in the pane title for a few seconds
|
||||
/// (architecture/queue-order.md).
|
||||
QueueDedup,
|
||||
/// Drop entries with the same artist and title, keeping the longest take —
|
||||
/// one entry per *song* rather than per recording. Aggressive and opt-in:
|
||||
/// it discards remixes and edits (architecture/queue-order.md D3a).
|
||||
QueueDedupTitles,
|
||||
/// Open the sort menu: one keypress there reorders the whole queue
|
||||
/// (architecture/queue-order.md D14). Modal — while it is open, this
|
||||
/// table is unreachable.
|
||||
|
|
@ -568,6 +572,13 @@ pub const BINDINGS: &[Binding] = &[
|
|||
action: Action::QueueDedup,
|
||||
description: "Unique: drop duplicate tracks from the queue",
|
||||
},
|
||||
Binding {
|
||||
scope: Scope::Queue,
|
||||
mods: KeyModifiers::SHIFT,
|
||||
code: KeyCode::Char('U'),
|
||||
action: Action::QueueDedupTitles,
|
||||
description: "Unique by title: one entry per song, keeping the longest",
|
||||
},
|
||||
Binding {
|
||||
scope: Scope::Queue,
|
||||
mods: KeyModifiers::SHIFT,
|
||||
|
|
@ -1100,7 +1111,15 @@ mod tests {
|
|||
),
|
||||
Some(Action::QueueSortMenu)
|
||||
);
|
||||
for code in [KeyCode::Char('u'), KeyCode::Char('S')] {
|
||||
assert_eq!(
|
||||
lookup(
|
||||
UiFocus::Queue,
|
||||
false,
|
||||
key(KeyCode::Char('U'), KeyModifiers::SHIFT)
|
||||
),
|
||||
Some(Action::QueueDedupTitles)
|
||||
);
|
||||
for code in [KeyCode::Char('u'), KeyCode::Char('U'), KeyCode::Char('S')] {
|
||||
assert_eq!(
|
||||
lookup(UiFocus::Library, false, key(code, KeyModifiers::NONE)),
|
||||
None,
|
||||
|
|
|
|||
|
|
@ -156,7 +156,10 @@ pub enum MessageFromUi {
|
|||
ClearQueue(bool),
|
||||
/// Drop duplicate queue entries; the reply comes back as
|
||||
/// [`MessageToUi::QueueDeduped`] (architecture/queue-order.md).
|
||||
DedupQueue,
|
||||
/// `by_title` asks for the aggressive same-song identity (D3a).
|
||||
DedupQueue {
|
||||
by_title: bool,
|
||||
},
|
||||
/// Reorder the queue by one strategy. Fire-and-forget: the new order
|
||||
/// arrives on the update stream like every other queue change.
|
||||
SortQueue {
|
||||
|
|
@ -746,12 +749,13 @@ impl App {
|
|||
self.set_register(dropped);
|
||||
let _ = self.tx.send(MessageFromUi::ClearQueue(false));
|
||||
}
|
||||
Action::QueueDedup => {
|
||||
Action::QueueDedup | Action::QueueDedupTitles => {
|
||||
// Server-side (architecture/queue-order.md D1): the count
|
||||
// comes back as `MessageToUi::QueueDeduped`. Nothing to do on
|
||||
// an empty queue.
|
||||
if !self.queue.is_empty() {
|
||||
let _ = self.tx.send(MessageFromUi::DedupQueue);
|
||||
let by_title = action == Action::QueueDedupTitles;
|
||||
let _ = self.tx.send(MessageFromUi::DedupQueue { by_title });
|
||||
}
|
||||
}
|
||||
Action::QueueSortMenu => {
|
||||
|
|
@ -1830,7 +1834,17 @@ mod tests {
|
|||
|
||||
app.queue.update_queue(one_track_queue());
|
||||
let _ = app.dispatch(Action::QueueDedup);
|
||||
assert!(matches!(rx.try_recv(), Ok(MessageFromUi::DedupQueue)));
|
||||
assert!(matches!(
|
||||
rx.try_recv(),
|
||||
Ok(MessageFromUi::DedupQueue { by_title: false })
|
||||
));
|
||||
|
||||
// `U` is the same verb with the aggressive identity (D3a).
|
||||
let _ = app.dispatch(Action::QueueDedupTitles);
|
||||
assert!(matches!(
|
||||
rx.try_recv(),
|
||||
Ok(MessageFromUi::DedupQueue { by_title: true })
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@
|
|||
pub mod app;
|
||||
pub mod config;
|
||||
pub mod rpc;
|
||||
pub mod stderr;
|
||||
|
||||
#[cfg(feature = "mpris")]
|
||||
pub mod mpris;
|
||||
|
|
@ -96,7 +97,13 @@ async fn orchestrate(
|
|||
let mut rpc_client = rpc::RpcClient::connect(&config.server).await?;
|
||||
|
||||
if let Some(root_node) = rpc_client.get_library_node(crabidy_core::ROOT_PATH).await? {
|
||||
tx.send(MessageToUi::ReplaceLibraryNode(root_node.clone()))?;
|
||||
if tx
|
||||
.send(MessageToUi::ReplaceLibraryNode(root_node.clone()))
|
||||
.is_err()
|
||||
{
|
||||
info!("the ui closed before the library root arrived");
|
||||
return Ok(());
|
||||
}
|
||||
}
|
||||
|
||||
// A desktop that cannot host the player (no session bus) leaves this
|
||||
|
|
@ -108,21 +115,40 @@ async fn orchestrate(
|
|||
if let Some(mpris) = &mpris {
|
||||
mpris.publish_init(&init_data);
|
||||
}
|
||||
tx.send_async(MessageToUi::Init(init_data)).await?;
|
||||
if tx.send_async(MessageToUi::Init(init_data)).await.is_err() {
|
||||
info!("the ui closed before the initial state arrived");
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
loop {
|
||||
if let Err(err) = poll(&mut rpc_client, &rx, &tx, &mpris).await {
|
||||
error!("request to server failed: {err}");
|
||||
match poll(&mut rpc_client, &rx, &tx, &mpris).await {
|
||||
Ok(Flow::Continue) => {}
|
||||
// The UI thread owns the other end; quitting drops it. That is a
|
||||
// shutdown, not a failed request — reporting it as one produced an
|
||||
// ERROR line ("sending on a closed channel") on every clean exit,
|
||||
// and the loop then spun on a stream nobody was listening to.
|
||||
Ok(Flow::UiGone) => {
|
||||
info!("the ui closed, stopping the orchestrator");
|
||||
return Ok(());
|
||||
}
|
||||
Err(err) => error!("request to server failed: {err}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// What the orchestrator does after one poll.
|
||||
enum Flow {
|
||||
Continue,
|
||||
/// The `MessageToUi` receiver is gone: the terminal UI has exited.
|
||||
UiGone,
|
||||
}
|
||||
|
||||
async fn poll(
|
||||
rpc_client: &mut RpcClient,
|
||||
rx: &Receiver<MessageFromUi>,
|
||||
tx: &Sender<MessageToUi>,
|
||||
mpris: &Option<mpris::Feed>,
|
||||
) -> Result<(), Box<dyn Error>> {
|
||||
) -> Result<Flow, Box<dyn Error>> {
|
||||
select! {
|
||||
Ok(msg) = &mut rx.recv_async() => {
|
||||
match msg {
|
||||
|
|
@ -218,11 +244,11 @@ async fn poll(
|
|||
MessageFromUi::ClearQueue(exclude_current) => {
|
||||
rpc_client.clear_queue(exclude_current).await?
|
||||
}
|
||||
MessageFromUi::DedupQueue => {
|
||||
MessageFromUi::DedupQueue { by_title } => {
|
||||
// The count is the whole point (architecture/queue-order.md
|
||||
// D2); a failure is logged and the queue simply stays as it
|
||||
// is — it must not tear down the poll loop.
|
||||
match rpc_client.dedup_queue().await {
|
||||
match rpc_client.dedup_queue(by_title).await {
|
||||
Ok(removed) => {
|
||||
let _ = tx.send(MessageToUi::QueueDeduped { removed });
|
||||
}
|
||||
|
|
@ -262,7 +288,9 @@ async fn poll(
|
|||
if let Some(mpris) = mpris {
|
||||
mpris.publish(&update);
|
||||
}
|
||||
tx.send_async(MessageToUi::Update(update)).await?;
|
||||
if tx.send_async(MessageToUi::Update(update)).await.is_err() {
|
||||
return Ok(Flow::UiGone);
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(err) => {
|
||||
|
|
@ -275,7 +303,7 @@ async fn poll(
|
|||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
Ok(Flow::Continue)
|
||||
}
|
||||
|
||||
fn run_ui(
|
||||
|
|
|
|||
|
|
@ -18,14 +18,12 @@ const CONFIG_FILE: &str = "cbd-tui.toml";
|
|||
static CONFIG: OnceLock<Config> = OnceLock::new();
|
||||
|
||||
/// Logs to a file: the terminal is owned by the TUI, so writing log lines to
|
||||
/// stdout/stderr would corrupt the interface.
|
||||
/// stdout/stderr would corrupt the interface. The same reason stderr itself is
|
||||
/// redirected — see [`cbd_tui::stderr`].
|
||||
fn init_tracing() -> Option<tracing_appender::non_blocking::WorkerGuard> {
|
||||
use tracing_subscriber::{prelude::*, EnvFilter};
|
||||
|
||||
let log_dir = dirs::state_dir()
|
||||
.or_else(dirs::cache_dir)
|
||||
.unwrap_or_else(std::env::temp_dir)
|
||||
.join("crabidy");
|
||||
let log_dir = cbd_tui::stderr::log_dir();
|
||||
if let Err(err) = std::fs::create_dir_all(&log_dir) {
|
||||
eprintln!(
|
||||
"could not create log directory {}: {err}",
|
||||
|
|
@ -56,6 +54,16 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
|
|||
// No subcommand: load config, apply overrides, run the TUI.
|
||||
None => {
|
||||
let _log_guard = init_tracing();
|
||||
// Before the alternate screen: from here on, a write to fd 2 that
|
||||
// did not go through tracing (a panic message, ALSA's underrun
|
||||
// chatter) would land on the interface.
|
||||
let stderr_log = cbd_tui::stderr::log_dir().join("cbd-tui.stderr.log");
|
||||
if let Err(err) = cbd_tui::stderr::capture_into(&stderr_log) {
|
||||
eprintln!(
|
||||
"could not redirect stderr to {}: {err}",
|
||||
stderr_log.display()
|
||||
);
|
||||
}
|
||||
let mut config = config::load_first_run(CONFIG_FILE);
|
||||
config::apply_overrides(
|
||||
&mut config,
|
||||
|
|
|
|||
|
|
@ -284,10 +284,10 @@ impl RpcClient {
|
|||
/// (architecture/queue-order.md D2) — the one queue verb with an answer
|
||||
/// worth showing, because `0` means "no duplicates", not "nothing
|
||||
/// happened".
|
||||
pub async fn dedup_queue(&mut self) -> Result<u32, Box<dyn Error>> {
|
||||
pub async fn dedup_queue(&mut self, by_title: bool) -> Result<u32, Box<dyn Error>> {
|
||||
let response = self
|
||||
.client
|
||||
.dedup_queue(Request::new(DedupQueueRequest {}))
|
||||
.dedup_queue(Request::new(DedupQueueRequest { by_title }))
|
||||
.await?;
|
||||
Ok(response.into_inner().removed)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,112 @@
|
|||
//! Keeping foreign writes off the TUI's screen.
|
||||
//!
|
||||
//! `tracing` goes to a log file precisely because the terminal belongs to the
|
||||
//! interface — but that only covers what *we* log. File descriptor 2 still
|
||||
//! points at the terminal, and plenty in this process never goes through
|
||||
//! `tracing`:
|
||||
//!
|
||||
//! - ALSA prints from C (`ALSA lib pcm.c:…: underrun occurred`), which the
|
||||
//! bundled `cbd` sees because the audio stack shares its process.
|
||||
//! - the default panic hook, and anything a dependency decides to
|
||||
//! `eprintln!`.
|
||||
//!
|
||||
//! Those bytes land wherever the cursor is, scroll the terminal by a line and
|
||||
//! leave the whole layout looking shifted — the queue appearing to bleed into
|
||||
//! the now-playing pane. Because ratatui repaints only the cells that changed,
|
||||
//! the damage persists until something forces a full redraw. Worse, the
|
||||
//! message is then *lost*: it never reaches the log, so afterwards there is no
|
||||
//! record of the underrun that caused it.
|
||||
//!
|
||||
//! So the fix is one `dup2`: point fd 2 at a file next to the log before the
|
||||
//! alternate screen is entered. The diagnostics are kept, just not on screen.
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
/// The directory both binaries log into: `crabidy/` under the state dir,
|
||||
/// falling back to the cache dir and then to the temp dir.
|
||||
///
|
||||
/// Shared so the log file and the captured stderr always land together.
|
||||
pub fn log_dir() -> PathBuf {
|
||||
dirs::state_dir()
|
||||
.or_else(dirs::cache_dir)
|
||||
.unwrap_or_else(std::env::temp_dir)
|
||||
.join("crabidy")
|
||||
}
|
||||
|
||||
/// Points file descriptor 2 at `path` (appending), so writes that bypass
|
||||
/// `tracing` are recorded instead of scribbling on the interface.
|
||||
///
|
||||
/// Call once, before the terminal is put into raw mode. Errors are returned
|
||||
/// rather than handled: a client that cannot open its log file should still
|
||||
/// start — it just keeps the noisy stderr it has always had.
|
||||
#[cfg(unix)]
|
||||
pub fn capture_into(path: &Path) -> std::io::Result<()> {
|
||||
use std::os::fd::AsRawFd;
|
||||
|
||||
let file = std::fs::OpenOptions::new()
|
||||
.create(true)
|
||||
.append(true)
|
||||
.open(path)?;
|
||||
// `dup2` makes fd 2 a second reference to this file, so dropping `file`
|
||||
// (and its own descriptor) at the end of this function leaves stderr
|
||||
// pointing at it.
|
||||
if unsafe { libc::dup2(file.as_raw_fd(), libc::STDERR_FILENO) } == -1 {
|
||||
return Err(std::io::Error::last_os_error());
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Nothing to do off Unix: there is no `dup2`, and the crabidy clients are
|
||||
/// terminal programs on Linux and macOS.
|
||||
#[cfg(not(unix))]
|
||||
pub fn capture_into(_path: &Path) -> std::io::Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// The redirect must actually move the process's stderr, and appending
|
||||
/// must not truncate what an earlier run wrote.
|
||||
///
|
||||
/// The write goes to the descriptor directly rather than through
|
||||
/// `eprintln!`: that is what a C library does (the case this exists for),
|
||||
/// and the test harness intercepts the macro but not the descriptor.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn captured_stderr_reaches_the_file_and_appends() {
|
||||
use std::io::Write;
|
||||
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
let path = dir.path().join("cbd.stderr.log");
|
||||
std::fs::write(&path, "earlier run\n").expect("seed");
|
||||
|
||||
// Keep the real stderr so the rest of the suite still has one. Briefly
|
||||
// process-wide, so a parallel test writing to fd 2 in this window would
|
||||
// land in the file too — harmless here, and the alternative is a child
|
||||
// process for one assertion.
|
||||
let saved = unsafe { libc::dup(libc::STDERR_FILENO) };
|
||||
assert!(saved >= 0, "could not save stderr");
|
||||
capture_into(&path).expect("redirect");
|
||||
let wrote = std::io::stderr().write_all(b"underrun occurred\n");
|
||||
let flushed = std::io::stderr().flush();
|
||||
let restored = unsafe { libc::dup2(saved, libc::STDERR_FILENO) };
|
||||
assert!(restored >= 0, "could not restore stderr");
|
||||
unsafe { libc::close(saved) };
|
||||
wrote.expect("write to the redirected stderr");
|
||||
flushed.expect("flush");
|
||||
|
||||
let captured = std::fs::read_to_string(&path).expect("read back");
|
||||
assert!(captured.starts_with("earlier run\n"), "{captured:?}");
|
||||
assert!(captured.contains("underrun occurred"), "{captured:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_log_dir_is_under_crabidy() {
|
||||
assert_eq!(
|
||||
log_dir().file_name().and_then(|n| n.to_str()),
|
||||
Some("crabidy")
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
@ -601,12 +601,13 @@ impl Store {
|
|||
}));
|
||||
}
|
||||
}
|
||||
Action::QueueDedup => {
|
||||
Action::QueueDedup | Action::QueueDedupTitles => {
|
||||
if self.queue.with_untracked(|q| !q.is_empty()) {
|
||||
let by_title = action == Action::QueueDedupTitles;
|
||||
let this = *self;
|
||||
let Some(mut rpc) = self.rpc() else { return };
|
||||
spawn_local(async move {
|
||||
match rpc.dedup_queue().await {
|
||||
match rpc.dedup_queue(by_title).await {
|
||||
// 0 is an answer, not a non-event
|
||||
// (architecture/queue-order.md D2).
|
||||
Ok(removed) => this.notify(format!("removed {removed} duplicate(s)")),
|
||||
|
|
|
|||
|
|
@ -69,6 +69,9 @@ pub enum Action {
|
|||
/// Drop duplicate queue entries server-side; the count lands in a toast
|
||||
/// (architecture/queue-order.md D16).
|
||||
QueueDedup,
|
||||
/// Drop entries with the same artist and title, keeping the longest take.
|
||||
/// Aggressive and opt-in (architecture/queue-order.md D3a).
|
||||
QueueDedupTitles,
|
||||
/// Open the sort menu dialog.
|
||||
QueueSortMenu,
|
||||
/// Close it without sorting (`Escape`/`q`/`S`).
|
||||
|
|
@ -355,6 +358,11 @@ pub const HELP: &[HelpEntry] = &[
|
|||
key: "u",
|
||||
description: "Unique: drop duplicate tracks from the queue",
|
||||
},
|
||||
HelpEntry {
|
||||
scope: "Queue",
|
||||
key: "U",
|
||||
description: "Unique by title: one entry per song, keeping the longest",
|
||||
},
|
||||
HelpEntry {
|
||||
scope: "Queue",
|
||||
key: "S",
|
||||
|
|
@ -460,6 +468,7 @@ pub fn lookup(focus: Focus, help_open: bool, key: &str, ctrl: bool) -> Option<Ac
|
|||
"C" => Some(Action::QueueClearAll),
|
||||
"w" => Some(Action::QueueSaveAs),
|
||||
"u" => Some(Action::QueueDedup),
|
||||
"U" => Some(Action::QueueDedupTitles),
|
||||
"S" => Some(Action::QueueSortMenu),
|
||||
_ => None,
|
||||
},
|
||||
|
|
@ -574,7 +583,12 @@ mod tests {
|
|||
lookup(Focus::Queue, false, "S", false),
|
||||
Some(Action::QueueSortMenu)
|
||||
);
|
||||
assert_eq!(
|
||||
lookup(Focus::Queue, false, "U", false),
|
||||
Some(Action::QueueDedupTitles)
|
||||
);
|
||||
assert_eq!(lookup(Focus::Library, false, "u", false), None);
|
||||
assert_eq!(lookup(Focus::Library, false, "U", false), None);
|
||||
assert_eq!(lookup(Focus::Library, false, "S", false), None);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -232,10 +232,10 @@ impl Rpc {
|
|||
|
||||
/// Drops duplicate queue entries; returns how many went
|
||||
/// (architecture/queue-order.md D2).
|
||||
pub async fn dedup_queue(&mut self) -> Result<u32, Status> {
|
||||
pub async fn dedup_queue(&mut self, by_title: bool) -> Result<u32, Status> {
|
||||
let response = self
|
||||
.client
|
||||
.dedup_queue(Request::new(DedupQueueRequest {}))
|
||||
.dedup_queue(Request::new(DedupQueueRequest { by_title }))
|
||||
.await?;
|
||||
Ok(response.into_inner().removed)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -53,6 +53,16 @@ async fn run_bundle(cli: CbdCli) -> Result<(), Box<dyn Error>> {
|
|||
// Both halves share one file-based subscriber: the terminal belongs
|
||||
// to the TUI, so the server's usual stderr logging would corrupt it.
|
||||
let _log_guard = init_tracing();
|
||||
// Tracing is not the only writer, though. The audio stack runs *in this
|
||||
// process* here, so ALSA's "underrun occurred" prints — and any panic
|
||||
// message — would go straight onto the interface (cbd_tui::stderr).
|
||||
let stderr_log = cbd_tui::stderr::log_dir().join("cbd.stderr.log");
|
||||
if let Err(err) = cbd_tui::stderr::capture_into(&stderr_log) {
|
||||
eprintln!(
|
||||
"could not redirect stderr to {}: {err}",
|
||||
stderr_log.display()
|
||||
);
|
||||
}
|
||||
// `cbd` reads its OWN config (`cbd.toml`), separate from the
|
||||
// standalone `cbd-tui`'s `cbd-tui.toml`. The two run side by side on
|
||||
// one machine — `cbd` self-contained against its in-process server,
|
||||
|
|
@ -182,10 +192,7 @@ async fn wait_for_server(
|
|||
fn init_tracing() -> Option<tracing_appender::non_blocking::WorkerGuard> {
|
||||
use tracing_subscriber::{prelude::*, EnvFilter};
|
||||
|
||||
let log_dir = dirs::state_dir()
|
||||
.or_else(dirs::cache_dir)
|
||||
.unwrap_or_else(std::env::temp_dir)
|
||||
.join("crabidy");
|
||||
let log_dir = cbd_tui::stderr::log_dir();
|
||||
if let Err(err) = std::fs::create_dir_all(&log_dir) {
|
||||
eprintln!(
|
||||
"could not create log directory {}: {err}",
|
||||
|
|
|
|||
|
|
@ -195,7 +195,16 @@ message ClearQueueResponse {}
|
|||
// under the same provider (the first path segment) or — when that id is
|
||||
// empty — the same `path`. Deliberately *not* metadata matching: the same
|
||||
// artist and title is routinely a different recording (D3).
|
||||
message DedupQueueRequest {}
|
||||
message DedupQueueRequest {
|
||||
// Collapse entries with the same artist and title instead — the same song
|
||||
// whatever the recording, so a remix, a radio edit and the album version
|
||||
// reduce to one. Aggressive by construction: it discards versions the user
|
||||
// may have queued deliberately, which is why it is opt-in and why the
|
||||
// default above only merges what is provably the same item. The survivor is
|
||||
// the playing entry, else the *longest* take (the full version rather than
|
||||
// an edit), else the earliest.
|
||||
bool by_title = 1;
|
||||
}
|
||||
message DedupQueueResponse {
|
||||
// How many entries were dropped. 0 says the queue held no duplicates,
|
||||
// which is not the same answer as "nothing happened" — hence a response
|
||||
|
|
|
|||
|
|
@ -184,32 +184,36 @@ fn spawn_spectrum_task(
|
|||
const FPS: u64 = 20;
|
||||
tokio::spawn(async move {
|
||||
let mut analyzer = spectrum::SpectrumAnalyzer::new(audio_player::SPECTRUM_WINDOW);
|
||||
let mut last_count = tap.frame_count();
|
||||
let mut was_active = false;
|
||||
let mut flow = spectrum::FlowDetector::new(tap.frame_count());
|
||||
let mut interval = tokio::time::interval(std::time::Duration::from_millis(1000 / FPS));
|
||||
// Each tick costs a little (an FFT and a broadcast), and the default
|
||||
// `Burst` behaviour keeps the *absolute* schedule: once that cost has
|
||||
// accumulated to a whole period, two ticks fire back to back and the
|
||||
// second one necessarily sees no new frames. That is what made the bars
|
||||
// flick to the floor roughly once a second during playback. `Delay`
|
||||
// measures each period from the previous tick instead, so a tick is
|
||||
// never spent catching up.
|
||||
interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay);
|
||||
loop {
|
||||
interval.tick().await;
|
||||
// Nobody watching: do no work.
|
||||
if update_tx.receiver_count() == 0 {
|
||||
continue;
|
||||
}
|
||||
let count = tap.frame_count();
|
||||
if count != last_count {
|
||||
last_count = count;
|
||||
if !was_active {
|
||||
debug!("spectrum: audio flowing, streaming bars");
|
||||
}
|
||||
was_active = true;
|
||||
match flow.observe(tap.frame_count()) {
|
||||
spectrum::Tick::Bars => {
|
||||
let bins = analyzer.analyze(&tap.snapshot());
|
||||
let _ = update_tx.send(Update::Spectrum(SpectrumFrame { bins }));
|
||||
} else if was_active {
|
||||
// Playback just went idle: drop the bars to the floor once.
|
||||
}
|
||||
spectrum::Tick::Silence => {
|
||||
// Playback went idle: drop the bars to the floor once.
|
||||
debug!("spectrum: audio idle, bars to zero");
|
||||
was_active = false;
|
||||
let _ = update_tx.send(Update::Spectrum(SpectrumFrame {
|
||||
bins: vec![0.0; spectrum::SPECTRUM_BINS],
|
||||
}));
|
||||
}
|
||||
spectrum::Tick::Nothing => {}
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
|
|
@ -668,12 +672,15 @@ impl QueueManager {
|
|||
/// Drops duplicate entries and returns how many were removed
|
||||
/// (architecture/queue-order.md D3–D5).
|
||||
///
|
||||
/// Two entries are the same track by [`queue_order::track_id`]. The
|
||||
/// playing entry always survives its group, so this never changes what is
|
||||
/// playing: the removal goes through [`Self::remove_tracks`], which
|
||||
/// therefore reports no successor to start.
|
||||
pub fn dedup(&mut self) -> usize {
|
||||
let positions = queue_order::duplicate_positions(&self.tracks, self.current_position());
|
||||
/// Two entries are the same track by [`queue_order::track_id`] under
|
||||
/// `identity` — the provably-same provider item by default, or the same
|
||||
/// artist and title when the caller opts in (D3a). The playing entry always
|
||||
/// survives its group, so this never changes what is playing: the removal
|
||||
/// goes through [`Self::remove_tracks`], which therefore reports no
|
||||
/// successor to start.
|
||||
pub fn dedup(&mut self, identity: queue_order::Identity) -> usize {
|
||||
let positions =
|
||||
queue_order::duplicate_positions(&self.tracks, self.current_position(), identity);
|
||||
if positions.is_empty() {
|
||||
return 0;
|
||||
}
|
||||
|
|
@ -1014,7 +1021,7 @@ mod tests {
|
|||
#[test]
|
||||
fn dedup_removes_later_copies_and_reports_the_count() {
|
||||
let mut q = queue_of(vec![track(0), track(1), track(0), track(2), track(1)]);
|
||||
assert_eq!(q.dedup(), 2);
|
||||
assert_eq!(q.dedup(queue_order::Identity::ProviderItem), 2);
|
||||
assert_eq!(titles(&q), vec!["track 0", "track 1", "track 2"]);
|
||||
assert_order_is_a_permutation(&q);
|
||||
}
|
||||
|
|
@ -1026,7 +1033,7 @@ mod tests {
|
|||
let mut q = queue_of(vec![track(0), track(1), track(0)]);
|
||||
assert!(q.set_current_position(2));
|
||||
let playing = q.current_track().expect("current");
|
||||
assert_eq!(q.dedup(), 1);
|
||||
assert_eq!(q.dedup(queue_order::Identity::ProviderItem), 1);
|
||||
assert_eq!(
|
||||
q.current_track().expect("still a current track").path,
|
||||
playing.path
|
||||
|
|
@ -1038,17 +1045,17 @@ mod tests {
|
|||
#[test]
|
||||
fn dedup_of_a_clean_queue_changes_nothing() {
|
||||
let mut q = queue_of(vec![track(0), track(1)]);
|
||||
assert_eq!(q.dedup(), 0);
|
||||
assert_eq!(q.dedup(queue_order::Identity::ProviderItem), 0);
|
||||
assert_eq!(titles(&q), vec!["track 0", "track 1"]);
|
||||
// And an empty queue is not a panic.
|
||||
let mut empty = QueueManager::new();
|
||||
assert_eq!(empty.dedup(), 0);
|
||||
assert_eq!(empty.dedup(queue_order::Identity::ProviderItem), 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dedup_leaves_the_queue_playable() {
|
||||
let mut q = queue_of(vec![track(0), track(0), track(1)]);
|
||||
q.dedup();
|
||||
q.dedup(queue_order::Identity::ProviderItem);
|
||||
assert_eq!(q.next_track().expect("advances").title, "track 1");
|
||||
assert!(q.next_track().is_none());
|
||||
}
|
||||
|
|
@ -1291,6 +1298,9 @@ pub enum PlaybackCommand {
|
|||
/// queue mutation; the count travels back through `result_tx` because it
|
||||
/// cannot be recovered from the resulting snapshot.
|
||||
DedupQueue {
|
||||
/// Collapse same artist+title instead of the provably-same item — the
|
||||
/// aggressive, opt-in identity (architecture/queue-order.md D3a).
|
||||
by_title: bool,
|
||||
result_tx: flume::Sender<usize>,
|
||||
},
|
||||
/// Reorders the queue by one strategy (architecture/queue-order.md D6).
|
||||
|
|
|
|||
|
|
@ -265,7 +265,10 @@ impl Playback {
|
|||
}
|
||||
}
|
||||
|
||||
PlaybackCommand::DedupQueue { result_tx } => {
|
||||
PlaybackCommand::DedupQueue {
|
||||
by_title,
|
||||
result_tx,
|
||||
} => {
|
||||
// Never removes the playing entry (architecture/queue-order.md
|
||||
// D4), so unlike `Remove` there is no successor to start.
|
||||
let removed = {
|
||||
|
|
@ -273,13 +276,18 @@ impl Playback {
|
|||
error!("queue lock poisoned");
|
||||
return;
|
||||
};
|
||||
let removed = queue.dedup();
|
||||
let identity = if by_title {
|
||||
crate::queue_order::Identity::ArtistTitle
|
||||
} else {
|
||||
crate::queue_order::Identity::ProviderItem
|
||||
};
|
||||
let removed = queue.dedup(identity);
|
||||
if removed > 0 {
|
||||
self.broadcast_queue(&queue);
|
||||
}
|
||||
removed
|
||||
};
|
||||
debug!(removed, "de-duplicated the queue");
|
||||
debug!(removed, by_title, "de-duplicated the queue");
|
||||
if let Err(err) = result_tx.send(removed) {
|
||||
// The caller gave up (client gone); the queue is already
|
||||
// deduped and broadcast, so this is only a lost count.
|
||||
|
|
@ -1221,7 +1229,10 @@ mod tests {
|
|||
|
||||
let (result_tx, result_rx) = flume::bounded(1);
|
||||
playback
|
||||
.handle_command(PlaybackCommand::DedupQueue { result_tx })
|
||||
.handle_command(PlaybackCommand::DedupQueue {
|
||||
by_title: false,
|
||||
result_tx,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(result_rx.recv_async().await.expect("a reply"), 1);
|
||||
|
|
@ -1247,7 +1258,10 @@ mod tests {
|
|||
|
||||
let (result_tx, result_rx) = flume::bounded(1);
|
||||
playback
|
||||
.handle_command(PlaybackCommand::DedupQueue { result_tx })
|
||||
.handle_command(PlaybackCommand::DedupQueue {
|
||||
by_title: false,
|
||||
result_tx,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert_eq!(result_rx.recv_async().await.expect("a reply"), 0);
|
||||
|
|
@ -1272,7 +1286,10 @@ mod tests {
|
|||
let (result_tx, result_rx) = flume::bounded(1);
|
||||
drop(result_rx);
|
||||
playback
|
||||
.handle_command(PlaybackCommand::DedupQueue { result_tx })
|
||||
.handle_command(PlaybackCommand::DedupQueue {
|
||||
by_title: false,
|
||||
result_tx,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(queue_titles(&playback), vec!["track 0"]);
|
||||
}
|
||||
|
|
@ -1324,7 +1341,10 @@ mod tests {
|
|||
|
||||
let (result_tx, _result_rx) = flume::bounded(1);
|
||||
playback
|
||||
.handle_command(PlaybackCommand::DedupQueue { result_tx })
|
||||
.handle_command(PlaybackCommand::DedupQueue {
|
||||
by_title: false,
|
||||
result_tx,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(
|
||||
rx.borrow().clone().expect("snapshot sent").tracks.len(),
|
||||
|
|
|
|||
|
|
@ -21,23 +21,44 @@ use crabidy_core::proto::crabidy::{QueueSort, Track};
|
|||
///
|
||||
/// Cheap to build (two borrows, no allocation) so a dedup pass over a long
|
||||
/// queue stays a single hash-set walk.
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
|
||||
#[derive(Clone, Debug, PartialEq, Eq, Hash)]
|
||||
pub enum TrackId<'a> {
|
||||
/// `(provider, provider_item_id)` — the same item however it was reached
|
||||
/// (through an album, a playlist, a search result).
|
||||
ProviderItem { provider: &'a str, id: &'a str },
|
||||
/// The full library path, for tracks whose provider reports no id.
|
||||
Path(&'a str),
|
||||
/// Lowercased artist and title: the same *song*, whatever the recording
|
||||
/// ([`Identity::ArtistTitle`]).
|
||||
ArtistTitle(String, String),
|
||||
}
|
||||
|
||||
/// The de-duplication identity of `track` (D3).
|
||||
pub fn track_id(track: &Track) -> TrackId<'_> {
|
||||
if track.provider_item_id.is_empty() {
|
||||
return TrackId::Path(&track.path);
|
||||
/// What makes two queue entries "the same track" for de-duplication.
|
||||
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)]
|
||||
pub enum Identity {
|
||||
/// The provably-same item: the provider's own id under the same provider,
|
||||
/// or the same path (D3). The default, because it cannot discard a version
|
||||
/// the user chose.
|
||||
#[default]
|
||||
ProviderItem,
|
||||
/// The same artist and title, compared case-insensitively — a remix, a
|
||||
/// radio edit and the album version collapse to one entry. Opt-in: it
|
||||
/// throws away recordings that differ (D3a).
|
||||
ArtistTitle,
|
||||
}
|
||||
TrackId::ProviderItem {
|
||||
|
||||
/// The de-duplication identity of `track` under `identity` (D3, D3a).
|
||||
pub fn track_id(track: &Track, identity: Identity) -> TrackId<'_> {
|
||||
match identity {
|
||||
Identity::ArtistTitle => TrackId::ArtistTitle(
|
||||
track.artist.trim().to_lowercase(),
|
||||
track.title.trim().to_lowercase(),
|
||||
),
|
||||
Identity::ProviderItem if track.provider_item_id.is_empty() => TrackId::Path(&track.path),
|
||||
Identity::ProviderItem => TrackId::ProviderItem {
|
||||
provider: provider_of(&track.path),
|
||||
id: &track.provider_item_id,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -59,28 +80,39 @@ fn provider_of(path: &str) -> &str {
|
|||
/// never in the returned list and a dedup cannot interrupt playback.
|
||||
/// `current` being out of range (an empty queue) simply means "no protected
|
||||
/// entry".
|
||||
pub fn duplicate_positions(tracks: &[Track], current: usize) -> Vec<usize> {
|
||||
// The survivor of each group, decided before anything is dropped: the
|
||||
// playing entry when it is in the group, otherwise the first one seen.
|
||||
// `current` is looked up once, so a group whose survivor is the playing
|
||||
// entry can still drop entries that come *before* it (D4).
|
||||
let playing = tracks.get(current).map(track_id);
|
||||
let mut survivors: HashMap<TrackId<'_>, usize> = HashMap::new();
|
||||
for (pos, track) in tracks.iter().enumerate() {
|
||||
let id = track_id(track);
|
||||
let survivor = survivors.entry(id).or_insert(pos);
|
||||
if Some(id) == playing {
|
||||
pub fn duplicate_positions(tracks: &[Track], current: usize, identity: Identity) -> Vec<usize> {
|
||||
let keys: Vec<TrackId<'_>> = tracks.iter().map(|t| track_id(t, identity)).collect();
|
||||
// The survivor of each group, decided before anything is dropped, so a
|
||||
// group whose survivor is the playing entry can still drop entries that
|
||||
// come *before* it (D4).
|
||||
let mut survivors: HashMap<&TrackId<'_>, usize> = HashMap::new();
|
||||
for (pos, key) in keys.iter().enumerate() {
|
||||
let survivor = survivors.entry(key).or_insert(pos);
|
||||
if keys.get(current) == Some(key) {
|
||||
// The playing entry always wins its group: a dedup that stops the
|
||||
// music is a bug, not a policy.
|
||||
*survivor = current;
|
||||
} else if identity == Identity::ArtistTitle && *survivor != current {
|
||||
// Same song, different recordings: keep the longest take — the
|
||||
// full version rather than a radio edit (D3a). Unknown lengths
|
||||
// lose, and equal lengths keep the earlier entry.
|
||||
let better = duration_of(tracks, pos) > duration_of(tracks, *survivor);
|
||||
if better {
|
||||
*survivor = pos;
|
||||
}
|
||||
}
|
||||
tracks
|
||||
.iter()
|
||||
.enumerate()
|
||||
.filter(|(pos, track)| survivors.get(&track_id(track)) != Some(pos))
|
||||
.map(|(pos, _)| pos)
|
||||
}
|
||||
(0..tracks.len())
|
||||
.filter(|pos| survivors.get(&keys[*pos]) != Some(pos))
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// The known duration of a queue entry in seconds; `0` when unknown, which
|
||||
/// makes an unknown length lose the "longest take" comparison above.
|
||||
fn duration_of(tracks: &[Track], pos: usize) -> u32 {
|
||||
tracks.get(pos).and_then(|t| t.duration).unwrap_or(0)
|
||||
}
|
||||
|
||||
/// The permutation that `sort` puts `tracks` in: `perm[new] = old`, i.e. the
|
||||
/// new queue is `perm.iter().map(|&old| tracks[old])`.
|
||||
///
|
||||
|
|
@ -216,7 +248,10 @@ mod tests {
|
|||
// The same Tidal track through an album and through a playlist.
|
||||
let via_album = track("/tidal/albums/7/99", "99");
|
||||
let via_playlist = track("/tidal/playlists/p/99", "99");
|
||||
assert_eq!(track_id(&via_album), track_id(&via_playlist));
|
||||
assert_eq!(
|
||||
track_id(&via_album, Identity::ProviderItem),
|
||||
track_id(&via_playlist, Identity::ProviderItem)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -225,7 +260,10 @@ mod tests {
|
|||
// unrelated track (D3).
|
||||
let tidal = track("/tidal/albums/7/1234", "1234");
|
||||
let jamendo = track("/jamendo/tracks/1234", "1234");
|
||||
assert_ne!(track_id(&tidal), track_id(&jamendo));
|
||||
assert_ne!(
|
||||
track_id(&tidal, Identity::ProviderItem),
|
||||
track_id(&jamendo, Identity::ProviderItem)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -234,7 +272,10 @@ mod tests {
|
|||
// provider, so it stays a separate entry (D3, stated risk).
|
||||
let source = track("/tidal/albums/7/99", "99");
|
||||
let capture = track("/crabidy/mix/song.cbd-track.toml", "99");
|
||||
assert_ne!(track_id(&source), track_id(&capture));
|
||||
assert_ne!(
|
||||
track_id(&source, Identity::ProviderItem),
|
||||
track_id(&capture, Identity::ProviderItem)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -242,9 +283,18 @@ mod tests {
|
|||
let a = track("/fs/music/song.flac", "");
|
||||
let same = track("/fs/music/song.flac", "");
|
||||
let other = track("/fs/music/other.flac", "");
|
||||
assert_eq!(track_id(&a), track_id(&same));
|
||||
assert_ne!(track_id(&a), track_id(&other));
|
||||
assert!(matches!(track_id(&a), TrackId::Path(_)));
|
||||
assert_eq!(
|
||||
track_id(&a, Identity::ProviderItem),
|
||||
track_id(&same, Identity::ProviderItem)
|
||||
);
|
||||
assert_ne!(
|
||||
track_id(&a, Identity::ProviderItem),
|
||||
track_id(&other, Identity::ProviderItem)
|
||||
);
|
||||
assert!(matches!(
|
||||
track_id(&a, Identity::ProviderItem),
|
||||
TrackId::Path(_)
|
||||
));
|
||||
}
|
||||
|
||||
// ---- which copies go (D4) -----------------------------------------
|
||||
|
|
@ -257,7 +307,10 @@ mod tests {
|
|||
track("/tidal/b/1", "1"),
|
||||
track("/tidal/a/2", "2"),
|
||||
];
|
||||
assert_eq!(duplicate_positions(&tracks, 0), vec![2, 3]);
|
||||
assert_eq!(
|
||||
duplicate_positions(&tracks, 0, Identity::ProviderItem),
|
||||
vec![2, 3]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -269,25 +322,31 @@ mod tests {
|
|||
track("/tidal/a/2", "2"),
|
||||
track("/tidal/b/1", "1"),
|
||||
];
|
||||
assert_eq!(duplicate_positions(&tracks, 2), vec![0]);
|
||||
assert_eq!(
|
||||
duplicate_positions(&tracks, 2, Identity::ProviderItem),
|
||||
vec![0]
|
||||
);
|
||||
// With another entry playing, the ordinary rule applies again.
|
||||
assert_eq!(duplicate_positions(&tracks, 1), vec![2]);
|
||||
assert_eq!(
|
||||
duplicate_positions(&tracks, 1, Identity::ProviderItem),
|
||||
vec![2]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn nothing_to_do_is_an_empty_list() {
|
||||
let tracks = [track("/tidal/a/1", "1"), track("/tidal/a/2", "2")];
|
||||
assert!(duplicate_positions(&tracks, 0).is_empty());
|
||||
assert!(duplicate_positions(&[], 0).is_empty());
|
||||
assert!(duplicate_positions(&tracks, 0, Identity::ProviderItem).is_empty());
|
||||
assert!(duplicate_positions(&[], 0, Identity::ProviderItem).is_empty());
|
||||
// An out-of-range current (an empty or freshly cleared queue) is not
|
||||
// a panic and protects nothing.
|
||||
assert!(duplicate_positions(&[], 9).is_empty());
|
||||
assert!(duplicate_positions(&[], 9, Identity::ProviderItem).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn positions_come_back_ascending_and_are_unique() {
|
||||
let tracks: Vec<Track> = (0..6).map(|_| track("/tidal/a/1", "1")).collect();
|
||||
let positions = duplicate_positions(&tracks, 3);
|
||||
let positions = duplicate_positions(&tracks, 3, Identity::ProviderItem);
|
||||
let mut sorted = positions.clone();
|
||||
sorted.sort_unstable();
|
||||
sorted.dedup();
|
||||
|
|
@ -297,6 +356,74 @@ mod tests {
|
|||
assert!(!positions.contains(&3));
|
||||
}
|
||||
|
||||
// ---- the opt-in title identity (D3a) -------------------------------
|
||||
|
||||
/// The case that motivated the mode, from a real queue: two remixes and the
|
||||
/// album version of one song, three distinct provider items with three
|
||||
/// different lengths. The default identity keeps all three; the title
|
||||
/// identity keeps the longest.
|
||||
#[test]
|
||||
fn the_title_identity_collapses_different_recordings() {
|
||||
// Faithful to the queue this came from: distinct ISRC-style ids under
|
||||
// one provider, three lengths, two of them from the remixes album.
|
||||
let with_id = |id: &str, title: &str, secs: u32| Track {
|
||||
provider_item_id: id.to_string(),
|
||||
path: format!("/crabidy/Mindchatter/{id}.cbd-track.toml"),
|
||||
..meta("Mindchatter", None, title, Some(secs))
|
||||
};
|
||||
let tracks = vec![
|
||||
with_id("QZES72134712", "Sink Into The Hips", 189),
|
||||
with_id("QZES72136149", "Sink Into The Hips", 171),
|
||||
with_id("QZMEM2002285", "Sink Into the Hips", 228),
|
||||
];
|
||||
assert!(
|
||||
duplicate_positions(&tracks, 0, Identity::ProviderItem).is_empty(),
|
||||
"three distinct items are not duplicates by default"
|
||||
);
|
||||
// Nothing playing (an out-of-range current), so the length rule decides:
|
||||
// case in the title is not a difference, and the 228s take survives.
|
||||
assert_eq!(
|
||||
duplicate_positions(&tracks, 9, Identity::ArtistTitle),
|
||||
vec![0, 1]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_title_identity_keeps_the_playing_entry_over_the_longest() {
|
||||
let tracks = vec![
|
||||
meta("artist", None, "song", Some(300)),
|
||||
meta("artist", None, "song", Some(120)),
|
||||
];
|
||||
// Position 1 is playing: it survives even though it is the shorter
|
||||
// take — playback outranks the length rule (D4).
|
||||
assert_eq!(
|
||||
duplicate_positions(&tracks, 1, Identity::ArtistTitle),
|
||||
vec![0]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unknown_length_loses_to_a_known_one() {
|
||||
let tracks = vec![
|
||||
meta("artist", None, "song", None),
|
||||
meta("artist", None, "song", Some(120)),
|
||||
];
|
||||
assert_eq!(
|
||||
duplicate_positions(&tracks, 5, Identity::ArtistTitle),
|
||||
vec![0],
|
||||
"the entry with a known length survives"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_title_identity_does_not_merge_different_artists() {
|
||||
let tracks = vec![
|
||||
meta("Aretha Franklin", None, "Respect", Some(147)),
|
||||
meta("Otis Redding", None, "Respect", Some(128)),
|
||||
];
|
||||
assert!(duplicate_positions(&tracks, 0, Identity::ArtistTitle).is_empty());
|
||||
}
|
||||
|
||||
// ---- sorting (D6–D9) ----------------------------------------------
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -356,16 +356,21 @@ impl CrabidyService for RpcService {
|
|||
Ok(Response::new(ClearQueueResponse {}))
|
||||
}
|
||||
|
||||
#[instrument(skip(self, _request))]
|
||||
#[instrument(skip(self, request), fields(by_title))]
|
||||
async fn dedup_queue(
|
||||
&self,
|
||||
_request: Request<DedupQueueRequest>,
|
||||
request: Request<DedupQueueRequest>,
|
||||
) -> Result<Response<DedupQueueResponse>, Status> {
|
||||
let by_title = request.into_inner().by_title;
|
||||
tracing::Span::current().record("by_title", by_title);
|
||||
debug!("received dedup_queue request");
|
||||
// The count comes back from the loop (architecture/queue-order.md D2);
|
||||
// the dedup itself has already been broadcast by then.
|
||||
let (result_tx, result_rx) = flume::bounded(1);
|
||||
self.send_playback(PlaybackCommand::DedupQueue { result_tx })
|
||||
self.send_playback(PlaybackCommand::DedupQueue {
|
||||
by_title,
|
||||
result_tx,
|
||||
})
|
||||
.await?;
|
||||
let removed = result_rx.recv_async().await.map_err(|err| {
|
||||
error!("no reply from playback loop: {err}");
|
||||
|
|
@ -759,14 +764,14 @@ mod tests {
|
|||
.expect("command sent")
|
||||
.command
|
||||
{
|
||||
PlaybackCommand::DedupQueue { result_tx } => {
|
||||
PlaybackCommand::DedupQueue { result_tx, .. } => {
|
||||
result_tx.send(7).expect("reply accepted");
|
||||
}
|
||||
other => panic!("expected a dedup command, got {}", other.name()),
|
||||
}
|
||||
});
|
||||
let response = service
|
||||
.dedup_queue(Request::new(DedupQueueRequest {}))
|
||||
.dedup_queue(Request::new(DedupQueueRequest { by_title: false }))
|
||||
.await
|
||||
.expect("accepted");
|
||||
assert_eq!(response.into_inner().removed, 7);
|
||||
|
|
|
|||
|
|
@ -102,12 +102,112 @@ fn log_bin_edges(mag_len: usize, bins: usize) -> Vec<usize> {
|
|||
.collect()
|
||||
}
|
||||
|
||||
/// How many consecutive ticks may see no new samples before the bars are
|
||||
/// declared idle and dropped to zero.
|
||||
///
|
||||
/// One is too few. The tap counts *frames*, so during playback every tick
|
||||
/// normally sees thousands more — but a tick that arrives early (or two that
|
||||
/// arrive together) sees none, and calling that silence made the bars flick to
|
||||
/// the floor about once a second and wrote two log lines each time. Two ticks
|
||||
/// is 100 ms at 20 fps: still immediate to the eye when playback really stops.
|
||||
const IDLE_TICKS_BEFORE_ZERO: u8 = 2;
|
||||
|
||||
/// Decides, tick by tick, whether audio is flowing — the only state the
|
||||
/// spectrum task keeps.
|
||||
///
|
||||
/// Split out from the task so the flapping that motivated it is testable
|
||||
/// without a runtime or an audio device (architecture/spectrum.md D2).
|
||||
pub struct FlowDetector {
|
||||
last_count: u64,
|
||||
idle_ticks: u8,
|
||||
active: bool,
|
||||
}
|
||||
|
||||
/// What the spectrum task should send after one tick.
|
||||
#[derive(Debug, PartialEq, Eq)]
|
||||
pub enum Tick {
|
||||
/// Audio is flowing: analyze the window and broadcast bars.
|
||||
Bars,
|
||||
/// Playback just went idle: broadcast a single zeroed frame so the bars
|
||||
/// fall instead of freezing.
|
||||
Silence,
|
||||
/// Nothing to say.
|
||||
Nothing,
|
||||
}
|
||||
|
||||
impl FlowDetector {
|
||||
pub fn new(count: u64) -> Self {
|
||||
Self {
|
||||
last_count: count,
|
||||
idle_ticks: 0,
|
||||
active: false,
|
||||
}
|
||||
}
|
||||
|
||||
/// Folds this tick's frame count into the decision.
|
||||
pub fn observe(&mut self, count: u64) -> Tick {
|
||||
if count != self.last_count {
|
||||
self.last_count = count;
|
||||
self.idle_ticks = 0;
|
||||
self.active = true;
|
||||
return Tick::Bars;
|
||||
}
|
||||
// No new frames. Only after enough of them in a row is this silence
|
||||
// rather than a tick that merely landed between two callbacks.
|
||||
self.idle_ticks = self.idle_ticks.saturating_add(1);
|
||||
if self.active && self.idle_ticks >= IDLE_TICKS_BEFORE_ZERO {
|
||||
self.active = false;
|
||||
return Tick::Silence;
|
||||
}
|
||||
Tick::Nothing
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
const WINDOW: usize = 2048;
|
||||
|
||||
/// The flapping this exists to stop: a single tick with no new frames is
|
||||
/// not silence, so it must not zero the bars (which is what the log showed
|
||||
/// happening ~1/s during playback).
|
||||
#[test]
|
||||
fn one_empty_tick_does_not_zero_the_bars() {
|
||||
let mut flow = FlowDetector::new(0);
|
||||
assert_eq!(flow.observe(1000), Tick::Bars);
|
||||
assert_eq!(
|
||||
flow.observe(1000),
|
||||
Tick::Nothing,
|
||||
"one empty tick is not silence"
|
||||
);
|
||||
assert_eq!(flow.observe(2000), Tick::Bars);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sustained_silence_zeroes_the_bars_exactly_once() {
|
||||
let mut flow = FlowDetector::new(0);
|
||||
assert_eq!(flow.observe(1000), Tick::Bars);
|
||||
assert_eq!(flow.observe(1000), Tick::Nothing);
|
||||
assert_eq!(flow.observe(1000), Tick::Silence);
|
||||
// And then stays quiet rather than re-sending zero frames forever.
|
||||
for _ in 0..5 {
|
||||
assert_eq!(flow.observe(1000), Tick::Nothing);
|
||||
}
|
||||
// Resuming reports flowing again.
|
||||
assert_eq!(flow.observe(1001), Tick::Bars);
|
||||
}
|
||||
|
||||
/// A player that never started must not emit a zero frame just because
|
||||
/// nothing is happening.
|
||||
#[test]
|
||||
fn an_idle_player_says_nothing() {
|
||||
let mut flow = FlowDetector::new(0);
|
||||
for _ in 0..10 {
|
||||
assert_eq!(flow.observe(0), Tick::Nothing);
|
||||
}
|
||||
}
|
||||
|
||||
fn sine(freq: f32, sample_rate: f32, len: usize) -> Vec<f32> {
|
||||
(0..len)
|
||||
.map(|i| (2.0 * std::f32::consts::PI * freq * i as f32 / sample_rate).sin())
|
||||
|
|
|
|||
|
|
@ -48,11 +48,13 @@ cbd global volume -- -0.1 # lower the volume
|
|||
[--keep-current]`, `queue set-current <POS>`, `queue save`/`capture
|
||||
<NAME>`, `queue shuffle`, and `queue repeat` change it.
|
||||
- `queue dedup` drops duplicate entries and prints how many went (`0` when
|
||||
there were none); `queue sort <artist|album|title|duration|reverse>
|
||||
[--desc]` reorders it. Both are described under [Order operations:
|
||||
dedup and sort](../queue.md#order-operations-dedup-and-sort) — the
|
||||
strategy is a fixed choice, so a shell completes it and a typo fails
|
||||
before anything reaches the server.
|
||||
there were none); `--titles` switches to the blunter same-song identity
|
||||
(keeping the longest take, discarding remixes and edits);
|
||||
`queue sort <artist|album|title|duration|reverse> [--desc]` reorders it.
|
||||
Both are described under [Order operations: dedup and
|
||||
sort](../queue.md#order-operations-dedup-and-sort) — the strategy is a
|
||||
fixed choice, so a shell completes it and a typo fails before anything
|
||||
reaches the server.
|
||||
- `global play`/`stop`/`next`/`prev`/`restart`/`mute`, `global
|
||||
volume <DELTA>`, and `global seek <SECONDS>` (negative seeks back, e.g.
|
||||
`global seek -15`) control playback.
|
||||
|
|
|
|||
|
|
@ -308,6 +308,7 @@ pane).
|
|||
| Queue | `c` | Clear queue except current (to register) |
|
||||
| Queue | `C` | Clear entire queue (to register) |
|
||||
| Queue | `u` | Unique: drop duplicate tracks |
|
||||
| Queue | `U` | Unique by title: one entry per song |
|
||||
| Queue | `S` | Sort the queue (opens a strategy menu) |
|
||||
| Queue | `w` | Save queue under a name |
|
||||
| Queue | `W` | Capture the queue into /crabidy (audio) |
|
||||
|
|
|
|||
|
|
@ -182,6 +182,15 @@ your queue. Two consequences:
|
|||
The RPC answers with the number of entries removed; `0` is a real answer, and
|
||||
the clients show it ("no duplicates") rather than nothing.
|
||||
|
||||
```admonish warning title="Dedup by title"
|
||||
The same verb with a blunter identity: **same artist and title**, whatever the
|
||||
recording, keeping the longest take (still yielding to the playing entry). One
|
||||
entry per *song* rather than per recording — which means it discards remixes,
|
||||
live takes and radio edits you may have queued on purpose. It is on its own key
|
||||
for that reason (`U` in either client, `queue dedup --titles` on the command
|
||||
line); `u` never does this.
|
||||
```
|
||||
|
||||
**Sort** (`S` opens a menu in either client, `cbd queue sort <key> [--desc]`)
|
||||
reorders by one strategy:
|
||||
|
||||
|
|
|
|||
|
|
@ -11,10 +11,15 @@ are the things a reader has to check by reading the code. Each is pass/fail.
|
|||
the group, otherwise the earliest one (D4). Check the survivor selection
|
||||
directly, not only through a test: a "keep the first" shortcut is a silent
|
||||
playback stop the moment the playing copy is a later one.
|
||||
- [ ] **G2 — Duplicate identity is provider-scoped.** The key is
|
||||
- [ ] **G2 — The default duplicate identity is provider-scoped.** The key is
|
||||
`(first path segment, provider_item_id)` when the id is non-empty and the
|
||||
whole path otherwise (D3). No comparison of artist/title anywhere in the
|
||||
dedup path.
|
||||
whole path otherwise (D3). Artist/title matching exists **only** under the
|
||||
opt-in `by_title` identity (D3a), never on the default path, and the flag is
|
||||
never defaulted to true anywhere between key and wire.
|
||||
- [ ] **G2a — `by_title` keeps the longest take** and still yields to the
|
||||
playing entry (D3a), and every client surface that reaches it is distinct
|
||||
from the safe one (its own key, its own flag) — a user cannot get it by
|
||||
mistyping.
|
||||
- [ ] **G3 — Sort is stable and total.** `sort_permutation` returns a
|
||||
permutation of `0..len` for every strategy and direction, equal keys keep
|
||||
their queue order, and blanks/unknown durations sort last in *both*
|
||||
|
|
|
|||
Loading…
Reference in New Issue