diff --git a/architecture/seek.md b/architecture/seek.md new file mode 100644 index 0000000..dafa596 --- /dev/null +++ b/architecture/seek.md @@ -0,0 +1,224 @@ +# Seek within the playing track + +## Context + +Playback exposes track-level controls only: `TogglePlay`, `Next`, `Prev`, +`RestartTrack`. There is no way to move inside a track, which hurts most +where tracks are longest — the podcast providers (`/rss`, `/fyyd`) and +audiobooks (`/abs`), where "I missed that sentence" and "skip the ad" are +the two most common wishes. + +The audio engine can already do it: `PlayerEngine::seek_to` and +`PlayerEngineCommand::SeekTo` exist and are wired through +`Player::seek_to`. **Nothing calls them.** There is no RPC, no playback +command, no binding — and the one implementation that exists carries a +panic (D5). So this feature is almost entirely *wiring*, plus a decision +about where the arithmetic lives. + +Goal: a step of about 15 seconds forward and backward, from every client. + +## Assumptions + +Stated explicitly and settled by reading the code rather than by asking: + +- **A1 — Within the current track only.** A backward seek at 3 s lands at + 0, it does not step into the previous track; `Ctrl-p` is the + track-level control and stays that way. Crossing tracks would need the + queue lock and the previous track's duration, for a gesture nobody + expects to do that. +- **A2 — The server owns the position.** Clients render `TrackPosition` + from the update stream and never predict it, exactly as they do for + volume and play state. A seek therefore needs no optimistic UI and no + rollback when it is refused. +- **A3 — Fire-and-forget.** Like every other playback RPC, `Seek` returns + as soon as the command is queued. A refused seek is a server-side + warning, not a client-visible error (D7). +- **A4 — No autoplay.** Seeking with nothing loaded does nothing; it does + not start the queue. `TogglePlay` and `RestartTrack` are the controls + that resume an idle player. + +## Options + +The only real question is **where the arithmetic lives**: a seek is +relative ("15 seconds back"), but the engine seeks to an absolute +position. + +### Option A — the client computes the target + +`Seek(position_millis)`. Each client takes the last `TrackPosition` it +received, adds ±15 s, clamps against the duration it was told, and sends +an absolute target. + +- The wire is a plain absolute seek, which a click-on-the-progress-bar + gesture also wants. +- But the base is **stale**: positions are broadcast on a 250 ms tick and + then cross the network. The step is 15 s, so 250 ms of drift is not the + problem — **repetition** is. Press `.` three times quickly and all + three presses read the *same* broadcast position, compute the *same* + target, and the track advances 15 s instead of 45. That is the normal + way people use a seek key. +- Every client re-implements clamping, so the end-of-track and + unknown-duration edges have to be right in three places (TUI, web, + CLI) instead of one. +- While **paused** no position updates arrive at all (the tick loop skips + a paused sink), so a client's base position goes stale the moment you + pause, and a paused seek would be computed from wherever playback + stopped rather than from where the last seek left it. + +### Option B — the client sends a delta *(chosen)* + +`Seek(delta_millis)`, signed. The engine adds it to the live sink +position, clamps, and seeks. + +- Repeated presses compose exactly: each one reads the position the + previous one produced, because the engine is a single-threaded command + loop and `try_seek` updates the position before returning. +- Clamping policy lives in one place, next to the duration the engine + already tracks. +- Clients get simpler, not more complex: a constant and one RPC call. +- Cost: absolute seek is not on the wire. It is a compatible proto3 + addition when a click-to-seek gauge wants it (deferred, below). + +**Decision: Option B.** The composition argument decides it — Option A is +wrong in the ordinary case of pressing the key twice. + +## Decisions + +- **D1 — The delta crosses the wire; the engine accumulates.** Clients + send a signed offset, never a target. +- **D2 — One new RPC, relative only.** `Seek(SeekRequest) -> SeekResponse` + with `sint32 delta_millis = 1`. Milliseconds because `TrackPosition` + already speaks milliseconds, so a future sub-second step needs no wire + change; `sint32` because zigzag encoding keeps negatives at one byte. + Absolute seek stays deferred — adding a field later is compatible. +- **D3 — The step size is a client constant.** `SEEK_STEP` = 15 s, one + named constant per client. The wire carries milliseconds, so making the + step configurable later is a client-only change. +- **D4 — Clamping.** Backward past the start lands at 0. Forward past the + end clamps to `duration - 1 s` when the duration is known, so the track + runs out through the ordinary end-of-stream path and advances to the + next one; the engine never has to seek *to* the exact end, whose + behaviour differs per decoder. With an **unknown** duration (a + length-less stream reports 0) there is no upper clamp — the decoder + decides, and a refusal is just a warning. +- **D5 — Fix the panic in `seek_to`.** It currently does + `time.clamp(Duration::from_secs(1), duration)`. `Ord::clamp` asserts + `min <= max`, and `duration()` yields **0** whenever the duration is + unknown (HLS, some network streams) — so that call panics the engine + thread, taking audio down with it, on any duration-less source. It is + unreachable today only because nothing calls `seek_to`; wiring seek + makes it reachable from **user input**, which the hard rules forbid. + Replaced by saturating arithmetic with no assertions. The 1-second + floor also went: landing at 0 is exactly what a backward seek near the + start means. +- **D6 — The engine reports the new position immediately.** After a + successful seek it emits `PlayerMessage::Elapsed` rather than waiting + for the next 250 ms tick. This is not just about latency: `tick()` + returns early for a paused or empty sink, so without this a seek while + paused would leave every client showing the old position until playback + resumed. +- **D7 — Unseekable sources warn and change nothing.** HLS (SoundCloud) + is decoded with `seekable = false` precisely so symphonia never seeks + it, and `try_seek` reports `NotSupported`. The server logs a warning + and broadcasts nothing; clients keep showing the true position because + they never moved it (A2). Consistent with how `pause` and + `set_volume` failures are handled — no new status codes, no new + client-side error path. +- **D8 — Bindings: `Ctrl-b` back, `Ctrl-f` forward**, in the **global** + scope of both the TUI and the web client (the user's choice; an earlier + round used `,`/`.`). They join the existing control-chord family for + playback and movement — `Ctrl-n`/`Ctrl-p` for tracks, `Ctrl-d`/`Ctrl-u` + for paging — and read as vim's back/forward-a-screen pair. Neither chord + was bound in any scope of either client, and plain `f` (spectrum) is + untouched because the TUI's `lookup` compares every modifier except + `SHIFT` exactly. In the browser, `Ctrl-f` would otherwise open the find + bar; the web keydown handler already calls `prevent_default` on any + chord that resolves to an action, so binding it is enough to claim it. + The web transport bar also gets `⏪`/`⏩` buttons, and the CLI gets + `cbd global seek `, which accepts negatives. +- **D9 — No feature flag.** Seek is a few lines in the engine and one RPC + arm; it carries no dependency of its own, so by the rule in + `architecture/build-features.md` ("a feature must pay for itself in + dependencies") it does not earn one. + +## Structure + +```d2 +direction: right + +clients: Clients { + tui: cbd-tui\n`,` / `.` + web: cbd-web\n`,` / `.` / ⏪⏩ + cli: cbd-cli\nglobal seek N +} + +server: crabidy-server { + rpc: RpcService::seek + loop: playback loop\nPlaybackCommand::Seek + fwd: poll_play_bus +} + +engine: audio-player { + player: Player::seek_by + eng: PlayerEngine\nseek_by -> seek_clamped + sink: rodio Player\ntry_seek +} + +clients.tui -> server.rpc: Seek{delta_millis} +clients.web -> server.rpc: Seek{delta_millis} +clients.cli -> server.rpc: Seek{delta_millis} +server.rpc -> server.loop: bounded channel +server.loop -> engine.player: seek_by(delta) +engine.player -> engine.eng: SeekBy(delta, reply) +engine.eng -> engine.sink: try_seek(clamped target) +engine.eng -> server.fwd: PlayerMessage::Elapsed +server.fwd -> server.loop: PositionChanged +server.loop -> clients: TrackPosition broadcast +``` + +The delta stays a delta all the way down to the engine; the only place an +absolute position is computed is `PlayerEngine::seek_by`, which is also +the only place that knows the live sink position and the track duration. + +## Boundaries + +- **`audio-player`** gains `Player::seek_by(delta_millis: i64)` and + `PlayerEngineCommand::SeekBy`. The clamping helper is private; both + `seek_to` (absolute, kept for the deferred extension) and `seek_by` go + through it, so there is one clamping policy, not two. +- **`crabidy-core`** gains the `Seek` RPC and its two messages. +- **`crabidy-server`** gains `PlaybackCommand::Seek { delta_millis }` and + the RPC arm. The playback loop only forwards; it holds no seek state. +- **Clients** gain an action, a binding, an RPC wrapper, and a constant + each. No client-side position arithmetic anywhere (A2, D1). + +## Risks + +- **`try_seek` blocks the engine thread** for up to ~5 ms (it waits for + the audio callback to pick the order up). That thread already blocks + for up to 30 s opening a network stream, so this is not a new class of + stall — but it does mean seek is serialized behind an in-flight track + open, which is correct anyway. +- **Seeking while paused** relies on rodio's `periodic_access` sitting + *outside* `pausable` in the chain: a paused sink keeps being polled for + silence, so the seek order is still picked up. Verified in rodio 0.22.2 + (`src/player.rs`); if a future rodio inverts that order, a paused seek + would block until unpause. Worth re-checking on a rodio bump. +- **Network-backed sources** seek inside `stream_download`'s temp + storage. A seek outside the downloaded window triggers a fresh range + request, so a long forward seek can stall audio briefly. Bounded by the + existing HTTP timeouts; no new failure mode. +- **A 15-second step on a very short track** always lands in the clamp, + which is why the clamp has to be arithmetic that cannot assert (D5). + +## Deferred + +Recorded, not dropped: + +- **Absolute seek** (`position_millis`) and a click-to-seek progress + gauge in the web client. The gauge is already rendered; only the wire + field and a click handler are missing. A compatible proto3 addition. +- **Configurable step size** in the client configs, and a larger step on + `<`/`>` (same physical keys, shifted). Client-only once wanted. +- **Chapter-aware seek** for podcasts and audiobooks. No provider exposes + chapter marks through the library model today. diff --git a/audio-player/src/player.rs b/audio-player/src/player.rs index 6d9e550..51ab391 100644 --- a/audio-player/src/player.rs +++ b/audio-player/src/player.rs @@ -109,6 +109,22 @@ impl Player { rx.recv_async().await? } + /// Seeks `delta_millis` from the current position — negative seeks back — + /// and resolves to the position actually reached, clamped to the track. + /// + /// Relative rather than absolute on purpose: the engine adds the offset to + /// the live position, so pressing a seek key several times in a row + /// composes instead of repeatedly targeting the same place + /// (architecture/seek.md D1). Fails when nothing is loaded, and when the + /// source cannot seek at all (HLS). + pub async fn seek_by(&self, delta_millis: i64) -> Result { + let (tx, rx) = flume::bounded(1); + self.tx_engine + .send_async(PlayerEngineCommand::SeekBy(delta_millis, tx)) + .await?; + rx.recv_async().await? + } + pub async fn volume(&self) -> Result { let (tx, rx) = flume::bounded(1); self.tx_engine diff --git a/audio-player/src/player_engine.rs b/audio-player/src/player_engine.rs index 8745390..0325f15 100644 --- a/audio-player/src/player_engine.rs +++ b/audio-player/src/player_engine.rs @@ -38,6 +38,44 @@ fn display_source(source_str: &str) -> String { /// Interval between elapsed-position updates while playing. const TICK_INTERVAL: Duration = Duration::from_millis(250); +/// How close to the end of a track a forward seek may land. +/// +/// Seeking to the exact end is decoder-dependent — symphonia, the Opus +/// reader and a windowed http stream all disagree about whether that is an +/// error — so an overshooting forward seek stops here instead and lets the +/// track run out through the ordinary end-of-stream path, which advances the +/// queue (architecture/seek.md D4). +const SEEK_END_MARGIN: Duration = Duration::from_secs(1); + +/// The absolute position a seek of `delta_millis` from `position` should land +/// on, given the track's `duration` if it is known. +/// +/// Pure arithmetic, and deliberately free of assertions: it is driven +/// straight from user input, so every input — a delta larger than the track, +/// `i64::MIN`, a duration of zero, a track shorter than the step — has to +/// produce a position rather than a panic. (The previous implementation used +/// `clamp(Duration::from_secs(1), duration)`, which asserts `min <= max` and +/// therefore panicked on every duration-less stream.) +/// +/// Saturates at 0 going back — a backward seek near the start means "go to +/// the beginning", not "go to the previous track". Going forward it stops +/// [`SEEK_END_MARGIN`] short of the end when the duration is known; a +/// length-less stream reports no duration, and then there is nothing to clamp +/// against, so the target passes through and the decoder decides whether it +/// can honour it. +fn seek_target(position: Duration, delta_millis: i64, duration: Option) -> Duration { + let step = Duration::from_millis(delta_millis.unsigned_abs()); + let target = if delta_millis < 0 { + position.saturating_sub(step) + } else { + position.saturating_add(step) + }; + match duration { + Some(total) if !total.is_zero() => target.min(total.saturating_sub(SEEK_END_MARGIN)), + _ => target, + } +} + pub enum PlayerEngineCommand { Play(String, Sender>), SetVolume(f32, Sender), @@ -49,6 +87,10 @@ pub enum PlayerEngineCommand { GetDuration(Sender>), GetElapsed(Sender>), SeekTo(Duration, Sender>), + /// Seek by a signed offset in milliseconds from the live position + /// (negative seeks back). Relative rather than absolute so repeated + /// presses compose (architecture/seek.md D1). + SeekBy(i64, Sender>), GetVolume(Sender), ToggleMute(Sender), GetPaused(Sender>), @@ -243,6 +285,9 @@ impl PlayerEngine { PlayerEngineCommand::GetDuration(tx) => send_reply(tx, self.duration()), PlayerEngineCommand::GetElapsed(tx) => send_reply(tx, self.elapsed()), PlayerEngineCommand::SeekTo(time, tx) => send_reply(tx, self.seek_to(time)), + PlayerEngineCommand::SeekBy(delta_millis, tx) => { + send_reply(tx, self.seek_by(delta_millis)); + } PlayerEngineCommand::SetVolume(volume, tx) => { send_reply(tx, self.set_volume(volume)); } @@ -541,13 +586,57 @@ impl PlayerEngine { Ok(self.sink.get_pos()) } + /// Seeks to an absolute position, clamped by [`seek_target`]. Returns the + /// position actually reached. pub fn seek_to(&self, time: Duration) -> Result { - let duration = self.duration().unwrap_or_else(|_| self.sink.get_pos()); - let time = time.clamp(Duration::from_secs(1), duration); + if self.is_stopped() { + return Err(PlayerEngineError::NotPlaying.into()); + } + self.seek_sink(seek_target(time, 0, self.known_duration())) + } + + /// Seeks `delta_millis` from the **live** playback position (a negative + /// delta seeks back), clamped by [`seek_target`]. Returns the position + /// actually reached. + /// + /// The offset is applied here rather than by the caller so that repeated + /// presses compose: each seek starts from where the previous one landed, + /// not from a position update that was broadcast up to a tick ago + /// (architecture/seek.md D1). Seeking with nothing loaded is an error, not + /// a way to start playback. + pub fn seek_by(&self, delta_millis: i64) -> Result { + if self.is_stopped() { + return Err(PlayerEngineError::NotPlaying.into()); + } + let target = seek_target(self.sink.get_pos(), delta_millis, self.known_duration()); + self.seek_sink(target) + } + + /// Seeks the sink to an already-clamped absolute position and reports the + /// position reached. + /// + /// Reporting from here rather than leaving it to [`Self::tick`] is not + /// only about latency: the tick returns early for a paused sink, so + /// without this a seek while paused would leave every client showing the + /// pre-seek position until playback resumed (architecture/seek.md D6). + fn seek_sink(&self, target: Duration) -> Result { self.sink - .try_seek(time) + .try_seek(target) .map_err(|err| anyhow!("seek failed: {err}"))?; - Ok(self.sink.get_pos()) + let elapsed = self.sink.get_pos(); + debug!(?target, ?elapsed, "seeked"); + self.notify(PlayerMessage::Elapsed { + duration: self.known_duration().unwrap_or_default(), + elapsed, + }); + Ok(elapsed) + } + + /// The current track's total duration when the source reported one. + /// `None` covers both "no track loaded" and "a length-less stream", which + /// clamping treats alike. + fn known_duration(&self) -> Option { + self.media_info.as_ref().and_then(|info| info.duration) } /// The user's intended volume — the level playback would resume at, @@ -637,6 +726,107 @@ fn read_header(reader: &mut R, buf: &mut [u8]) -> Result { mod tests { use super::*; + /// A 10-minute track, the ordinary case. + fn ten_minutes() -> Option { + Some(Duration::from_secs(600)) + } + + #[test] + fn seek_target_moves_by_the_delta() { + let at = Duration::from_secs(100); + assert_eq!( + seek_target(at, 15_000, ten_minutes()), + Duration::from_secs(115) + ); + assert_eq!( + seek_target(at, -15_000, ten_minutes()), + Duration::from_secs(85) + ); + } + + /// Successive seeks compose because each one starts from the position the + /// previous one produced — the reason the delta is applied here and not by + /// the client (architecture/seek.md D1). + #[test] + fn successive_seeks_compose() { + let mut at = Duration::from_secs(100); + for _ in 0..3 { + at = seek_target(at, -15_000, ten_minutes()); + } + assert_eq!(at, Duration::from_secs(55)); + } + + #[test] + fn seek_target_saturates_at_the_start() { + // Back past the beginning lands at 0 — not below, and not in the + // previous track. + assert_eq!( + seek_target(Duration::from_secs(3), -15_000, ten_minutes()), + Duration::ZERO + ); + assert_eq!( + seek_target(Duration::ZERO, -15_000, ten_minutes()), + Duration::ZERO + ); + } + + /// Forward past the end stops a second short of it, so the track finishes + /// through the ordinary end-of-stream path and the queue advances. + #[test] + fn seek_target_saturates_near_the_end() { + assert_eq!( + seek_target(Duration::from_secs(595), 15_000, ten_minutes()), + Duration::from_secs(599) + ); + } + + /// A length-less stream reports no duration. The naive clamp turned that + /// into "seek to zero" (or panicked); the target has to pass through so + /// the decoder can decide. + #[test] + fn an_unknown_duration_does_not_clamp_forward() { + let at = Duration::from_secs(100); + assert_eq!(seek_target(at, 15_000, None), Duration::from_secs(115)); + // `MediaInfo` carries `Some(0)` for some sources; same meaning. + assert_eq!( + seek_target(at, 15_000, Some(Duration::ZERO)), + Duration::from_secs(115) + ); + } + + /// Every extreme has to yield a position rather than a panic: the deltas + /// come from user input, and `duration` from whatever a feed claimed. + #[test] + fn seek_target_never_panics_on_extremes() { + let positions = [Duration::ZERO, Duration::from_secs(1), Duration::MAX]; + let deltas = [i64::MIN, -1, 0, 1, i64::MAX]; + let durations = [ + None, + Some(Duration::ZERO), + // A track shorter than the end margin: the ceiling saturates to 0. + Some(Duration::from_millis(400)), + Some(Duration::from_secs(600)), + Some(Duration::MAX), + ]; + for position in positions { + for delta in deltas { + for duration in durations { + let _ = seek_target(position, delta, duration); + } + } + } + // The case that used to panic outright: unknown duration, and a + // sub-second track where the old floor exceeded the old ceiling. + assert_eq!( + seek_target(Duration::ZERO, 15_000, None), + Duration::from_secs(15) + ); + assert_eq!( + seek_target(Duration::ZERO, 15_000, Some(Duration::from_millis(400))), + Duration::ZERO + ); + } + #[test] fn logged_sources_never_carry_url_tokens() { // Stream URLs embed access tokens; only scheme and host may be diff --git a/cbd-cli/src/client.rs b/cbd-cli/src/client.rs index 3e25f9c..b0f8d07 100644 --- a/cbd-cli/src/client.rs +++ b/cbd-cli/src/client.rs @@ -20,8 +20,8 @@ use crabidy_core::proto::crabidy::{ ChangeVolumeRequest, ClearQueueRequest, CreateLibraryNodeRequest, DeleteLibraryNodeRequest, GetLibraryNodeRequest, InitRequest, InsertRequest, LibraryNode, NextRequest, PrevRequest, Queue, RemoveRequest, RenameLibraryNodeRequest, ReplaceRequest, RestartTrackRequest, - SaveQueueRequest, SetCurrentRequest, StopRequest, ToggleMuteRequest, TogglePlayRequest, - ToggleRepeatRequest, ToggleShuffleRequest, Track, + SaveQueueRequest, SeekRequest, SetCurrentRequest, StopRequest, ToggleMuteRequest, + TogglePlayRequest, ToggleRepeatRequest, ToggleShuffleRequest, Track, }; use crate::{Connection, GlobalCmd, LibraryCmd, QueueCmd, RemoteCmd}; @@ -281,6 +281,16 @@ async fn run_global(client: &mut Client, cmd: GlobalCmd) -> Result<(), Box { + // Milliseconds on the wire; the server clamps to the track. + client + .seek(SeekRequest { + delta_millis: seconds.saturating_mul(1_000), + }) + .await + .map_err(rpc_error)?; + println!("seeked {seconds:+} seconds"); + } GlobalCmd::Mute => { client .toggle_mute(ToggleMuteRequest {}) diff --git a/cbd-cli/src/lib.rs b/cbd-cli/src/lib.rs index 57bf0f4..ac58bfe 100644 --- a/cbd-cli/src/lib.rs +++ b/cbd-cli/src/lib.rs @@ -113,6 +113,13 @@ pub enum GlobalCmd { Prev, /// Restart the current track. Restart, + /// Seek inside the current track by a number of seconds (negative seeks + /// back, e.g. `-15`). Stays within the track: it saturates at the start, + /// and overshooting the end lets the track finish and the queue advance. + Seek { + #[arg(allow_negative_numbers = true)] + seconds: i32, + }, /// Toggle mute. Mute, /// Change the volume by a delta in [-1.0, 1.0] (e.g. `0.05`, `-0.1`). diff --git a/cbd-tui/src/app/bindings.rs b/cbd-tui/src/app/bindings.rs index bd14e49..e20994e 100644 --- a/cbd-tui/src/app/bindings.rs +++ b/cbd-tui/src/app/bindings.rs @@ -42,6 +42,14 @@ pub enum Action { ToggleRepeat, NextTrack, PrevTrack, + /// Move the playing position back inside the current track by + /// [`super::SEEK_STEP_MILLIS`]. Never leaves the track: near the start it + /// simply lands at 0 (architecture/seek.md A1). + SeekBackward, + /// Move the playing position forward inside the current track by + /// [`super::SEEK_STEP_MILLIS`]. Overshooting the end lets the track + /// finish, which advances the queue. + SeekForward, /// Show or hide the frequency-spectrum visualizer in the now-playing /// pane (client-side only; the server keeps streaming the bars). ToggleSpectrum, @@ -229,6 +237,20 @@ pub const BINDINGS: &[Binding] = &[ action: Action::PrevTrack, description: "Previous track", }, + Binding { + scope: Scope::Global, + mods: KeyModifiers::CONTROL, + code: KeyCode::Char('b'), + action: Action::SeekBackward, + description: "Seek back 15 seconds", + }, + Binding { + scope: Scope::Global, + mods: KeyModifiers::CONTROL, + code: KeyCode::Char('f'), + action: Action::SeekForward, + description: "Seek forward 15 seconds", + }, Binding { scope: Scope::Global, mods: KeyModifiers::NONE, @@ -858,6 +880,41 @@ mod tests { ); } + /// Seek rides the control chords in every focus, and taking `Ctrl-f` must + /// not disturb plain `f` — the modifier comparison is exact for anything + /// but `SHIFT`. + #[test] + fn seek_is_on_the_control_chords_in_any_focus() { + for focus in [UiFocus::Library, UiFocus::Queue] { + assert_eq!( + lookup(focus, false, key(KeyCode::Char('f'), KeyModifiers::CONTROL)), + Some(Action::SeekForward) + ); + assert_eq!( + lookup(focus, false, key(KeyCode::Char('b'), KeyModifiers::CONTROL)), + Some(Action::SeekBackward) + ); + // Unmodified, these keep their old meanings (or none at all). + assert_eq!( + lookup(focus, false, key(KeyCode::Char('f'), KeyModifiers::NONE)), + Some(Action::ToggleSpectrum) + ); + assert_eq!( + lookup(focus, false, key(KeyCode::Char('b'), KeyModifiers::NONE)), + None + ); + } + // And the help modal still swallows them. + assert_eq!( + lookup( + UiFocus::Library, + true, + key(KeyCode::Char('f'), KeyModifiers::CONTROL) + ), + None + ); + } + #[test] fn spectrum_moved_off_v_to_f() { // The frequency-spectrum toggle now lives on Global `f`, in any focus. diff --git a/cbd-tui/src/app/mod.rs b/cbd-tui/src/app/mod.rs index 342f6e0..22e46ef 100644 --- a/cbd-tui/src/app/mod.rs +++ b/cbd-tui/src/app/mod.rs @@ -79,6 +79,15 @@ pub const COLOR_GREEN: Color = Color::Rgb(163, 190, 140); /// provider). const CURRENT_QUEUE_PATH: &str = "/crabidy/current"; +/// How far one press of a seek key moves the playing position, in +/// milliseconds — the unit the wire speaks, so any step is expressible +/// (architecture/seek.md D3). +/// +/// The step lives here, in the client: the server is handed an offset and adds +/// it to the live position, which is what makes repeated presses compose. The +/// help text and the docs spell the number out, so change those too. +pub(crate) const SEEK_STEP_MILLIS: i64 = 15_000; + // FIXME: Rename this pub enum MessageToUi { Init(InitialData), @@ -127,6 +136,11 @@ pub enum MessageFromUi { NextTrack, PrevTrack, RestartTrack, + /// Move the position inside the playing track by a signed millisecond + /// offset. Relative: the server adds it to the live position, so holding + /// the key composes instead of aiming at the same spot every time + /// (architecture/seek.md D1). + Seek(i64), SetCurrentTrack(usize), TogglePlay, ChangeVolume(f32), @@ -525,6 +539,12 @@ impl App { } Action::NextTrack => self.queue.play_next(), Action::PrevTrack => self.queue.play_prev(), + Action::SeekBackward => { + let _ = self.tx.send(MessageFromUi::Seek(-SEEK_STEP_MILLIS)); + } + Action::SeekForward => { + let _ = self.tx.send(MessageFromUi::Seek(SEEK_STEP_MILLIS)); + } Action::ToggleSpectrum => self.now_playing.toggle_spectrum(), Action::LibraryFirst => self.library_move(|l| l.first()), Action::LibraryLast => self.library_move(|l| l.last()), @@ -834,6 +854,19 @@ mod tests { let _ = app.dispatch(Action::RestartTrack); assert!(matches!(rx.try_recv(), Ok(MessageFromUi::RestartTrack))); + // Seek sends the *step*, signed — never a target position: the server + // adds it to the live position (architecture/seek.md D1). + let _ = app.dispatch(Action::SeekForward); + assert!( + matches!(rx.try_recv(), Ok(MessageFromUi::Seek(d)) if d == SEEK_STEP_MILLIS), + "forward seek must send +one step" + ); + let _ = app.dispatch(Action::SeekBackward); + assert!( + matches!(rx.try_recv(), Ok(MessageFromUi::Seek(d)) if d == -SEEK_STEP_MILLIS), + "backward seek must send -one step" + ); + let _ = app.dispatch(Action::ToggleMute); assert!(matches!(rx.try_recv(), Ok(MessageFromUi::ToggleMute))); diff --git a/cbd-tui/src/lib.rs b/cbd-tui/src/lib.rs index 1a586c8..a4cdcc4 100644 --- a/cbd-tui/src/lib.rs +++ b/cbd-tui/src/lib.rs @@ -146,6 +146,9 @@ async fn poll( MessageFromUi::RestartTrack => { rpc_client.restart_track().await? } + MessageFromUi::Seek(delta_millis) => { + rpc_client.seek(delta_millis).await? + } MessageFromUi::SetCurrentTrack(pos) => { rpc_client.set_current_track(pos).await? } diff --git a/cbd-tui/src/rpc.rs b/cbd-tui/src/rpc.rs index 3fbb894..af36f6a 100644 --- a/cbd-tui/src/rpc.rs +++ b/cbd-tui/src/rpc.rs @@ -4,7 +4,7 @@ use crabidy_core::proto::crabidy::{ GetLibraryNodeRequest, GetUpdateStreamRequest, GetUpdateStreamResponse, InitRequest, InitResponse, InsertRequest, LibraryNode, NextRequest, PrevRequest, QueueRequest, RemoveRequest, RenameLibraryNodeRequest, ReplaceRequest, RestartTrackRequest, SaveQueueRequest, - SetCurrentRequest, ToggleMuteRequest, TogglePlayRequest, ToggleRepeatRequest, + SeekRequest, SetCurrentRequest, ToggleMuteRequest, TogglePlayRequest, ToggleRepeatRequest, ToggleShuffleRequest, }; @@ -325,6 +325,17 @@ impl RpcClient { Ok(()) } + /// Moves the playing position by a signed millisecond offset. The server + /// applies it to the live position and clamps it to the track; a source + /// that cannot seek leaves the position untouched. + pub async fn seek(&mut self, delta_millis: i64) -> Result<(), Box> { + let seek_request = Request::new(SeekRequest { + delta_millis: delta_millis as i32, + }); + self.client.seek(seek_request).await?; + Ok(()) + } + pub async fn set_current_track(&mut self, pos: usize) -> Result<(), Box> { let set_current_request = Request::new(SetCurrentRequest { position: pos as u32, diff --git a/cbd-web/src/app.rs b/cbd-web/src/app.rs index f7959a3..ec5ac0e 100644 --- a/cbd-web/src/app.rs +++ b/cbd-web/src/app.rs @@ -20,6 +20,12 @@ use crate::state::{ }; const VOLUME_STEP: f32 = 0.1; + +/// How far one seek press moves the playing position, in milliseconds — the +/// unit the wire speaks (architecture/seek.md D3). The step lives in the +/// client; the server only ever receives an offset and applies it to the live +/// position, which is what makes repeated presses compose. +pub(crate) const SEEK_STEP_MILLIS: i32 = 15_000; const JUMP: isize = 15; /// Stream reconnect backoff bounds, milliseconds. const BACKOFF_MIN_MS: u32 = 1_000; @@ -397,6 +403,10 @@ impl Store { Action::RestartTrack => self.call(async |mut rpc: Rpc| rpc.restart_track().await), Action::NextTrack => self.call(async |mut rpc: Rpc| rpc.next().await), Action::PrevTrack => self.call(async |mut rpc: Rpc| rpc.prev().await), + Action::SeekBackward => { + self.call(async |mut rpc: Rpc| rpc.seek(-SEEK_STEP_MILLIS).await) + } + Action::SeekForward => self.call(async |mut rpc: Rpc| rpc.seek(SEEK_STEP_MILLIS).await), Action::VolumeUp => { self.call(async |mut rpc: Rpc| rpc.change_volume(VOLUME_STEP).await) } @@ -1152,8 +1162,12 @@ fn Transport(store: Store) -> impl IntoView {
+ +