From 384dafec57269c5142abed6c1131e52d76bb30f7 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:37:12 -0400 Subject: [PATCH 1/6] Follow the SDK's move to selections and filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Rust SDK on this branch takes an `AssetSelection` where it used to take a list of ids, asks for a `TimelineQuery` where `timeline()` took nothing, and has grown a `person_id` on `AssetQuery`. None of that is a decision this crate gets to make, and until it followed, the browser did not compile at all — which would have made the rest of this task impossible to even test. Co-Authored-By: Claude with claude-opus-5[1m] --- src/commands/albums.rs | 6 +++--- src/commands/assets.rs | 8 ++++---- src/commands/upload.rs | 4 ++-- src/context.rs | 1 + src/tui/app.rs | 1 + src/tui/mod.rs | 6 +++--- 6 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/commands/albums.rs b/src/commands/albums.rs index a979ca0..96ad6a0 100644 --- a/src/commands/albums.rs +++ b/src/commands/albums.rs @@ -1,7 +1,7 @@ //! Albums, and the links that publish them. use anyhow::Result; -use imogen_sdk::{AlbumCreate, AlbumUpdate}; +use imogen_sdk::{AlbumCreate, AlbumUpdate, AssetSelection}; use serde_json::json; use crate::cli::{AlbumCommand, QueryArgs}; @@ -193,7 +193,7 @@ async fn add(ctx: &Context, reference: &str, assets: &[String], query: &QueryArg let mut skipped = 0u64; let mut count = 0u64; for chunk in ids.chunks(500) { - let result = ctx.client.albums.add_assets(&album.id, chunk).await?; + let result = ctx.client.albums.add_assets(&album.id, &AssetSelection::ids(chunk)).await?; added += result.added; skipped += result.skipped; count = result.asset_count; @@ -222,7 +222,7 @@ async fn add(ctx: &Context, reference: &str, assets: &[String], query: &QueryArg async fn remove(ctx: &Context, reference: &str, assets: &[String]) -> Result<()> { let album = ctx.find_album(reference).await?; - let result = ctx.client.albums.remove_assets(&album.id, assets).await?; + let result = ctx.client.albums.remove_assets(&album.id, &AssetSelection::ids(assets)).await?; if ctx.out.is_json() { return ctx.out.json(&result); } diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 8e8578b..47150a7 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -1,7 +1,7 @@ //! Listing, showing, editing and trashing photographs. use anyhow::{bail, Result}; -use imogen_sdk::{Asset, AssetStatus, AssetType, AssetUpdate, GeoPoint}; +use imogen_sdk::{Asset, AssetSelection, AssetStatus, AssetType, AssetUpdate, GeoPoint, TimelineQuery}; use serde_json::json; use crate::cli::{EditArgs, ListArgs, RestoreArgs, SearchArgs, ShowArgs, TrashArgs}; @@ -287,7 +287,7 @@ pub async fn stats(ctx: &Context) -> Result<()> { } pub async fn timeline(ctx: &Context, after: Option<&str>, before: Option<&str>) -> Result<()> { - let timeline = ctx.client.assets.timeline().await?; + let timeline = ctx.client.assets.timeline(&TimelineQuery::default()).await?; let buckets: Vec<_> = timeline .buckets .into_iter() @@ -435,7 +435,7 @@ pub async fn trash(ctx: &Context, args: &TrashArgs) -> Result<()> { return Ok(()); } - let result = ctx.client.assets.trash(&targets).await?; + let result = ctx.client.assets.trash(&AssetSelection::ids(&targets)).await?; if ctx.out.is_json() { return ctx.out.json(&result); } @@ -475,7 +475,7 @@ pub async fn restore(ctx: &Context, args: &RestoreArgs) -> Result<()> { args.ids.clone() }; - let result = ctx.client.assets.restore(&targets).await?; + let result = ctx.client.assets.restore(&AssetSelection::ids(&targets)).await?; if ctx.out.is_json() { return ctx.out.json(&result); } diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 5e4b54e..678eba4 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -13,7 +13,7 @@ use std::sync::Arc; use anyhow::{bail, Context as _, Result}; use futures::stream::{self, StreamExt}; -use imogen_sdk::{AssetUploadMetadata, GeoPoint, UploadOptions}; +use imogen_sdk::{AssetSelection, AssetUploadMetadata, GeoPoint, UploadOptions}; use indicatif::{ProgressBar, ProgressStyle}; use serde::Deserialize; use serde_json::json; @@ -198,7 +198,7 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { for (album_id, asset_ids) in &by_album { // Album membership goes on in chunks: an imported album can hold thousands. for chunk in asset_ids.chunks(500) { - match ctx.client.albums.add_assets(album_id, chunk).await { + match ctx.client.albums.add_assets(album_id, &AssetSelection::ids(chunk)).await { Ok(result) => added += result.added, Err(error) => ctx.out.warn(format!("Could not fill an album: {error}")), } diff --git a/src/context.rs b/src/context.rs index ff86ae5..9081f6b 100644 --- a/src/context.rs +++ b/src/context.rs @@ -123,6 +123,7 @@ impl Context { MediaType::Video => AssetType::Video, }), album_id, + person_id: None, favorite: args.favorite.then_some(true), archived: args.archived.then_some(true), trashed: args.trashed.then_some(true), diff --git a/src/tui/app.rs b/src/tui/app.rs index 11e209d..8f91d9e 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -198,6 +198,7 @@ impl App { q: (!self.query.is_empty()).then(|| self.query.clone()), r#type: None, album_id: self.album.as_ref().map(|album| album.id.clone()), + person_id: None, favorite: (self.scope == Scope::Favorites).then_some(true), archived: (self.scope == Scope::Archived).then_some(true), trashed: (self.scope == Scope::Trash).then_some(true), diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 6cb6d38..bcbe5c3 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -274,7 +274,7 @@ async fn upload_one(ctx: &Context, path: PathBuf) -> Loaded { async fn fill_album(ctx: &Context, album_id: String, ids: Vec) -> Loaded { let mut added = 0u64; for chunk in ids.chunks(500) { - match ctx.client.albums.add_assets(&album_id, chunk).await { + match ctx.client.albums.add_assets(&album_id, &imogen_sdk::AssetSelection::ids(chunk)).await { Ok(result) => added += result.added, Err(error) => return Loaded::Filed(Err(error.into())), } @@ -549,7 +549,7 @@ async fn handle_key<'a>( if matches!(key.code, KeyCode::Char('y') | KeyCode::Char('Y')) { match action { Action::Trash(ids) => { - match ctx.client.assets.trash(&ids).await { + match ctx.client.assets.trash(&imogen_sdk::AssetSelection::ids(&ids)).await { Ok(result) => { app.note(format!("{} moved to the trash.", result.count)); reload(ctx, app, work); @@ -558,7 +558,7 @@ async fn handle_key<'a>( }; } Action::Restore(ids) => { - match ctx.client.assets.restore(&ids).await { + match ctx.client.assets.restore(&imogen_sdk::AssetSelection::ids(&ids)).await { Ok(result) => { app.note(format!("{} restored.", result.count)); reload(ctx, app, work); From 74f60ba5974f2ad815d6ce9613354444cdcf53cd Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:54:27 -0400 Subject: [PATCH 2/6] Hold a window of the library, not all of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The browser kept every asset it had paged in and every thumbnail it had ever decoded, the second of those in a map that was never evicted from. On a twenty-year library that is a leak with a picture in it. And there was no way to reach 2009 except paging back from today, a screen at a time. It now learns the shape of the timeline from the day buckets — one entry a day, small enough to hold for twenty years — and holds only the tiles the viewport covers, dropping the periods it has scrolled away from. Decoded thumbnails are capped the way previews already were, and eviction also forgets that a picture was ever asked for: dropping the image without dropping that record would leave a tile that could never be filled again. `g` jumps to a date, landing on the nearest day that has photographs when the one asked for has none. It walks the sorted buckets rather than the calendar, so a gap of eleven years costs exactly what a gap of two days costs — the web version of this searched month by month, gave up, and threw the reader at the end of the library. `[` and `]` step whole years the same way, through the years the library actually has. A rail down the right edge says where in the twenty years you are, which a screen of tiles cannot. The viewer and the details panel still need a whole record, so one is read for the photograph being looked at and no others; a tile carries only what the grid draws. Co-Authored-By: Claude with claude-opus-5[1m] --- src/commands/albums.rs | 12 +- src/commands/assets.rs | 22 +- src/commands/upload.rs | 7 +- src/dates.rs | 134 ++++++- src/tui/app.rs | 567 +++++++++++++++++++++++----- src/tui/mod.rs | 836 +++++++++++++++++++++++++++++++++++++---- src/tui/ui.rs | 270 +++++++++++-- 7 files changed, 1634 insertions(+), 214 deletions(-) diff --git a/src/commands/albums.rs b/src/commands/albums.rs index 96ad6a0..3edd859 100644 --- a/src/commands/albums.rs +++ b/src/commands/albums.rs @@ -193,7 +193,11 @@ async fn add(ctx: &Context, reference: &str, assets: &[String], query: &QueryArg let mut skipped = 0u64; let mut count = 0u64; for chunk in ids.chunks(500) { - let result = ctx.client.albums.add_assets(&album.id, &AssetSelection::ids(chunk)).await?; + let result = ctx + .client + .albums + .add_assets(&album.id, &AssetSelection::ids(chunk)) + .await?; added += result.added; skipped += result.skipped; count = result.asset_count; @@ -222,7 +226,11 @@ async fn add(ctx: &Context, reference: &str, assets: &[String], query: &QueryArg async fn remove(ctx: &Context, reference: &str, assets: &[String]) -> Result<()> { let album = ctx.find_album(reference).await?; - let result = ctx.client.albums.remove_assets(&album.id, &AssetSelection::ids(assets)).await?; + let result = ctx + .client + .albums + .remove_assets(&album.id, &AssetSelection::ids(assets)) + .await?; if ctx.out.is_json() { return ctx.out.json(&result); } diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 47150a7..cd231a7 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -1,7 +1,9 @@ //! Listing, showing, editing and trashing photographs. use anyhow::{bail, Result}; -use imogen_sdk::{Asset, AssetSelection, AssetStatus, AssetType, AssetUpdate, GeoPoint, TimelineQuery}; +use imogen_sdk::{ + Asset, AssetSelection, AssetStatus, AssetType, AssetUpdate, GeoPoint, TimelineQuery, +}; use serde_json::json; use crate::cli::{EditArgs, ListArgs, RestoreArgs, SearchArgs, ShowArgs, TrashArgs}; @@ -287,7 +289,11 @@ pub async fn stats(ctx: &Context) -> Result<()> { } pub async fn timeline(ctx: &Context, after: Option<&str>, before: Option<&str>) -> Result<()> { - let timeline = ctx.client.assets.timeline(&TimelineQuery::default()).await?; + let timeline = ctx + .client + .assets + .timeline(&TimelineQuery::default()) + .await?; let buckets: Vec<_> = timeline .buckets .into_iter() @@ -435,7 +441,11 @@ pub async fn trash(ctx: &Context, args: &TrashArgs) -> Result<()> { return Ok(()); } - let result = ctx.client.assets.trash(&AssetSelection::ids(&targets)).await?; + let result = ctx + .client + .assets + .trash(&AssetSelection::ids(&targets)) + .await?; if ctx.out.is_json() { return ctx.out.json(&result); } @@ -475,7 +485,11 @@ pub async fn restore(ctx: &Context, args: &RestoreArgs) -> Result<()> { args.ids.clone() }; - let result = ctx.client.assets.restore(&AssetSelection::ids(&targets)).await?; + let result = ctx + .client + .assets + .restore(&AssetSelection::ids(&targets)) + .await?; if ctx.out.is_json() { return ctx.out.json(&result); } diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 678eba4..2f6961b 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -198,7 +198,12 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { for (album_id, asset_ids) in &by_album { // Album membership goes on in chunks: an imported album can hold thousands. for chunk in asset_ids.chunks(500) { - match ctx.client.albums.add_assets(album_id, &AssetSelection::ids(chunk)).await { + match ctx + .client + .albums + .add_assets(album_id, &AssetSelection::ids(chunk)) + .await + { Ok(result) => added += result.added, Err(error) => ctx.out.warn(format!("Could not fill an album: {error}")), } diff --git a/src/dates.rs b/src/dates.rs index adbdad7..54dcf1d 100644 --- a/src/dates.rs +++ b/src/dates.rs @@ -24,8 +24,10 @@ fn widen(input: &str, suffix: &str) -> String { return trimmed.to_string(); } match trimmed.len() { - // A bare year or year-month is a range too: 2024 means all of 2024. - 4 => format!("{trimmed}-01-01{suffix}"), + // A bare year or year-month is a range too: 2024 means all of 2024, which ends on + // the last day of December rather than the first day of January. + 4 if suffix.starts_with("T00") => format!("{trimmed}-01-01{suffix}"), + 4 => format!("{trimmed}-12-31{suffix}"), 7 if suffix.starts_with("T00") => format!("{trimmed}-01{suffix}"), 7 => format!("{trimmed}-{}{suffix}", last_day_of(trimmed)), 10 => format!("{trimmed}{suffix}"), @@ -46,6 +48,94 @@ fn last_day_of(year_month: &str) -> String { format!("{:02}", month.length(year)) } +/// Month names, so the browser's jump prompt takes "aug 2011" as well as "2011-08". A +/// date filter never needed these — a person typing a command line writes the ISO shape — +/// but somebody scrubbing a twenty-year timeline is thinking in months, not in hyphens. +const MONTHS: [&str; 12] = [ + "january", + "february", + "march", + "april", + "may", + "june", + "july", + "august", + "september", + "october", + "november", + "december", +]; + +/// A date somebody typed, and how much of one they actually said. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Day { + /// The day to land on, `YYYY-MM-DD`. + pub date: String, + /// How many characters of `date` were named rather than filled in: 4 for a year, 7 for + /// a month, 10 for a day. Landing anywhere inside what was named is landing where they + /// asked, so a caller knows when it has nothing to apologise for. + pub named: usize, +} + +/// The day somebody means, or `None` when what they typed is not a date at all. +/// +/// Anything less than a whole day widens to that period's *last* day, because the timeline +/// runs newest first: a jump into August 2011 should land at the top of August, not at the +/// bottom of it. +pub fn to_day(input: &str) -> Option { + let trimmed = input.trim(); + if trimmed.is_empty() { + return None; + } + if let Some((year, month)) = year_and_named_month(trimmed) { + return Some(Day { + date: format!( + "{year}-{month:02}-{}", + last_day_of(&format!("{year}-{month:02}")) + ), + named: 7, + }); + } + // Otherwise it is one of the shapes `--before` already takes, and the end of whatever + // period it names is the day to land on. + let widened = to_end_of_day(trimmed); + let date = widened.split('T').next().unwrap_or_default(); + let format = time::macros::format_description!("[year]-[month]-[day]"); + Date::parse(date, &format).ok()?; + Some(Day { + date: date.to_string(), + named: match trimmed.len() { + 4 => 4, + 7 => 7, + _ => 10, + }, + }) +} + +/// `aug 2011`, `August 2011`, `2011 sept`. Returns `None` for anything carrying a token +/// that is neither a four-digit year nor a month name, so an ISO date falls through to be +/// parsed properly rather than being guessed at here. +fn year_and_named_month(input: &str) -> Option<(i32, u8)> { + let lowered = input.to_lowercase(); + let mut year = None; + let mut month = None; + for word in lowered + .split(|c: char| !c.is_ascii_alphanumeric()) + .filter(|word| !word.is_empty()) + { + if word.len() == 4 && word.bytes().all(|byte| byte.is_ascii_digit()) { + year = Some(word.parse().ok()?); + } else if word.len() >= 3 { + // Three letters is enough to name a month, and is what people type. + let index = MONTHS.iter().position(|name| name.starts_with(word))?; + month = Some(index as u8 + 1); + } else { + return None; + } + } + Some((year?, month?)) +} + /// A capture time somebody typed, as the instant the API wants. A bare date becomes noon /// rather than midnight: a photograph with an unknown time sorts among that day's others /// instead of ahead of all of them. @@ -94,6 +184,46 @@ mod tests { assert_eq!(to_end_of_day("2024-06-01"), "2024-06-01T23:59:59.999Z"); } + #[test] + fn a_bare_year_ends_in_december() { + // It used to end on the first of January, which made `--before 2011` mean + // "before the second day of 2011" and quietly hid the whole year. + assert_eq!(to_end_of_day("2011"), "2011-12-31T23:59:59.999Z"); + assert_eq!(to_start_of_day("2011"), "2011-01-01T00:00:00.000Z"); + } + + fn day(input: &str) -> Option { + to_day(input).map(|day| day.date) + } + + #[test] + fn a_day_to_jump_to_widens_to_the_top_of_whatever_period_was_named() { + assert_eq!(day("2011-08-14").as_deref(), Some("2011-08-14")); + assert_eq!(day("2011-08").as_deref(), Some("2011-08-31")); + assert_eq!(day("2011").as_deref(), Some("2011-12-31")); + assert_eq!(day("aug 2011").as_deref(), Some("2011-08-31")); + assert_eq!(day(" September 2011 ").as_deref(), Some("2011-09-30")); + assert_eq!(day("2011 feb").as_deref(), Some("2011-02-28")); + assert_eq!(day("2012 feb").as_deref(), Some("2012-02-29")); + } + + /// What was filled in, and what was actually said. + #[test] + fn a_date_remembers_how_much_of_it_was_named() { + assert_eq!(to_day("2011").unwrap().named, 4); + assert_eq!(to_day("2011-08").unwrap().named, 7); + assert_eq!(to_day("aug 2011").unwrap().named, 7); + assert_eq!(to_day("2011-08-14").unwrap().named, 10); + } + + #[test] + fn something_that_is_not_a_date_is_not_guessed_at() { + assert_eq!(to_day("not a date"), None); + assert_eq!(to_day(""), None); + assert_eq!(to_day("2011-13-40"), None); + assert_eq!(to_day("beach"), None); + } + #[test] fn a_month_ends_on_its_own_last_day() { assert_eq!(to_end_of_day("2024-02"), "2024-02-29T23:59:59.999Z"); diff --git a/src/tui/app.rs b/src/tui/app.rs index 8f91d9e..53dfa21 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -5,7 +5,7 @@ use std::path::PathBuf; use std::sync::Arc; use image::DynamicImage; -use imogen_sdk::{Album, Asset, AssetQuery, AssetSort, AssetStatus, SortOrder}; +use imogen_sdk::{Album, Asset, AssetFilter, AssetStatus, TimelineBucket, TimelineTile}; use ratatui::layout::Rect; /// Which set of photographs is on screen. The trash and the archive are not filters @@ -44,6 +44,8 @@ pub enum Mode { Picker, /// Typing a path to jump the picker to. Escape abandons it. PickerPath(String), + /// Typing a date to jump the timeline to. Applied on Enter, abandoned on Escape. + JumpDate(String), /// Waiting for a yes or a no before doing something that cannot be undone. Confirm { prompt: String, @@ -71,9 +73,54 @@ pub struct Tile { /// photographs is instant, few enough that the memory stays bounded. pub const PREVIEW_CACHE: usize = 8; +/// How many decoded grid thumbnails to keep. Enough that scrolling back a screen or two is +/// instant; few enough that a twenty-year library does not become a memory leak. The map +/// used to grow without bound, and it holds decoded images. +pub const THUMBNAIL_CACHE: usize = 256; + +/// The stretch of the timeline actually held in memory: the tiles the viewport covers, and +/// the global index the first of them sits at. +/// +/// Everything outside it is described by the day buckets alone, which cost a few dozen +/// bytes a day rather than a decoded photograph apiece. Twenty years of buckets is a list +/// small enough to hold whole; twenty years of tiles is not. +#[derive(Debug, Default)] +pub struct TileWindow { + pub base: usize, + pub tiles: Vec, +} + +impl TileWindow { + /// The tile at a global index, or `None` when the window does not reach that far. + pub fn get(&self, index: usize) -> Option<&TimelineTile> { + self.tiles.get(index.checked_sub(self.base)?) + } +} + pub struct App { - pub assets: Vec, - pub cursor: Option, + /// The shape of the whole library: one entry a day, newest first, as the server + /// orders it. This is what makes 2009 reachable without paging through 2010. + pub buckets: Vec, + /// Every photograph the buckets account for. Recomputed by [`App::recount`]. + pub total: usize, + /// Tiles keyed by the `YYYY-MM` period they came in, which is the unit the server + /// serves and so the unit this keeps and drops. + pub periods: HashMap>, + /// Periods with more tiles still to come, and the cursor to ask for them with. A month + /// of five thousand photographs arrives in more than one answer. + pub period_more: HashMap, + /// Periods already asked for, so a slow answer is not asked for twice. + pub period_inflight: HashSet, + /// The contiguous run of tiles the grid indexes into, rebuilt as periods arrive. + pub window: TileWindow, + /// What the viewport wanted last pass, so the window is only rebuilt when it moves. + pub held: Vec, + /// The full record of the selected photograph — everything a tile deliberately does + /// not carry. Fetched when the viewer or the details panel needs it, not per keypress. + pub detail: Option, + /// Whose record was last asked for. A request that fails would otherwise be made again + /// on the very next pass of the loop, turning a broken network into a busy loop. + pub detail_asked: Option, pub selected: usize, /// The first grid row on screen. pub scroll: usize, @@ -85,6 +132,8 @@ pub struct App { pub album_selected: usize, pub thumbnails: HashMap>, + /// Insertion order for the thumbnails, for evicting the least recently wanted. + pub thumb_order: VecDeque, pub wanted: HashSet, /// Assets whose status is being re-checked, so the same one is not asked for twice /// while an answer is still on its way. @@ -111,6 +160,9 @@ pub struct App { pub picker_preview_area: Rect, pub grid_area: Rect, + /// The gutter down the right of the grid where the years are drawn. Empty in every + /// mode that is not the grid. + pub rail_area: Rect, pub tiles: Vec, pub columns: usize, pub tile_width: u16, @@ -136,15 +188,21 @@ pub struct App { pub loading: bool, pub images_dirty: bool, pub should_quit: bool, - pub total: Option, pub show_info: bool, } impl App { pub fn new() -> Self { Self { - assets: Vec::new(), - cursor: None, + buckets: Vec::new(), + total: 0, + periods: HashMap::new(), + period_more: HashMap::new(), + period_inflight: HashSet::new(), + window: TileWindow::default(), + held: Vec::new(), + detail: None, + detail_asked: None, selected: 0, scroll: 0, mode: Mode::Grid, @@ -154,12 +212,14 @@ impl App { albums: Vec::new(), album_selected: 0, thumbnails: HashMap::new(), + thumb_order: VecDeque::new(), wanted: HashSet::new(), refreshing: HashSet::new(), previews: HashMap::new(), preview_order: VecDeque::new(), preview_inflight: HashSet::new(), grid_area: Rect::default(), + rail_area: Rect::default(), tiles: Vec::new(), columns: 1, tile_width: 20, @@ -181,54 +241,334 @@ impl App { loading: false, images_dirty: true, should_quit: false, - total: None, show_info: false, } } - pub fn selected_asset(&self) -> Option<&Asset> { - self.assets.get(self.selected) + /// The tile under the cursor. A tile carries what the grid draws and nothing else; the + /// whole record lives in [`App::detail`], and only for the one being looked at. + pub fn selected_tile(&self) -> Option<&TimelineTile> { + self.window.get(self.selected) + } + + pub fn selected_id(&self) -> Option { + self.selected_tile().map(|tile| tile.id.clone()) } - /// The query the current scope means. - pub fn to_query(&self) -> AssetQuery { - AssetQuery { - cursor: None, - limit: Some(100), + /// The full record of the selected photograph, when the one held is still the one + /// selected. A detail that belongs to the photograph you have moved on from is worse + /// than none: it would draw somebody else's filename under this picture. + pub fn detail(&self) -> Option<&Asset> { + let held = self.detail.as_ref()?; + (self.selected_id().as_deref() == Some(held.id.as_str())).then_some(held) + } + + /// The filter the current scope means. The spine and every windowed fetch go through + /// this one function, so the trash cannot be counted while the library is shown. + pub fn to_filter(&self) -> AssetFilter { + AssetFilter { q: (!self.query.is_empty()).then(|| self.query.clone()), - r#type: None, album_id: self.album.as_ref().map(|album| album.id.clone()), - person_id: None, favorite: (self.scope == Scope::Favorites).then_some(true), archived: (self.scope == Scope::Archived).then_some(true), trashed: (self.scope == Scope::Trash).then_some(true), - taken_after: None, - taken_before: None, - bbox: None, - sort: Some(AssetSort::CapturedAt), - order: Some(SortOrder::Desc), + ..Default::default() } } /// Forgets the current results without forgetting the pictures already decoded: the /// same photograph in a different scope does not need fetching twice. pub fn reset_results(&mut self) { - self.assets.clear(); - self.cursor = None; + self.buckets.clear(); + self.total = 0; + self.periods.clear(); + self.period_more.clear(); + self.period_inflight.clear(); + self.window = TileWindow::default(); + self.held.clear(); + self.detail = None; + self.detail_asked = None; self.selected = 0; self.scroll = 0; self.images_dirty = true; } + /// How many photographs the spine accounts for. Everything that maps an index to a + /// place on the timeline is arithmetic over this and the bucket counts. + pub fn recount(&mut self) { + self.total = self + .buckets + .iter() + .map(|bucket| bucket.count as usize) + .sum(); + if self.selected >= self.total { + self.selected = self.total.saturating_sub(1); + } + } + pub fn visible_rows(&self) -> usize { (self.grid_area.height / self.tile_height.max(1)).max(1) as usize } + /// The first global index of the given day, or of the nearest older day that has + /// photographs when that one has none. + /// + /// One walk of the sorted spine, so a gap of a month and a gap of eleven years cost + /// the same. The web's first attempt at this stepped through calendar months looking + /// for one that existed, gave up after a bounded number of steps, and threw the reader + /// at the end of the library; on a twenty-year collection a gap of years is ordinary. + pub fn index_for_date(&self, wanted: &str) -> Option { + if self.buckets.is_empty() { + return None; + } + let mut start = 0usize; + for bucket in &self.buckets { + if bucket.date.as_str() <= wanted { + return Some(start.min(self.total.saturating_sub(1))); + } + start += bucket.count as usize; + } + // Older than anything here. The oldest photograph is as far back as it goes. + Some(self.total.saturating_sub(1)) + } + + /// The day an index falls on — the inverse of [`App::index_for_date`]. + pub fn date_at_index(&self, index: usize) -> Option { + let mut start = 0usize; + for bucket in &self.buckets { + let end = start + bucket.count as usize; + if index < end { + return Some(bucket.date.clone()); + } + start = end; + } + None + } + + /// The `YYYY-MM` an index falls in. + pub fn period_at_index(&self, index: usize) -> Option { + let date = self.date_at_index(index)?; + (date.len() >= 7).then(|| date[..7].to_string()) + } + + /// Every period the library has, newest first. + pub fn periods_in_order(&self) -> Vec { + let mut periods: Vec = Vec::new(); + for bucket in &self.buckets { + if bucket.date.len() < 7 { + continue; + } + let period = &bucket.date[..7]; + if periods.last().map(String::as_str) != Some(period) { + periods.push(period.to_string()); + } + } + periods + } + + /// Where a period's first photograph sits in the global order. + pub fn period_start(&self, period: &str) -> Option { + let mut start = 0usize; + for bucket in &self.buckets { + if bucket.date.len() >= 7 && &bucket.date[..7] == period { + return Some(start); + } + start += bucket.count as usize; + } + None + } + + /// The periods the viewport covers, plus one either side so stepping down a row does + /// not stall on a fetch. + /// + /// Derived from the buckets alone, so this answers before a single tile has arrived + /// and before a single frame has been drawn. Nothing here may wait on a measurement + /// that only a rendered frame provides — that circle is how the web version of this + /// shipped twice with a grid that never rendered. + pub fn periods_for_viewport(&self) -> Vec { + if self.total == 0 { + return Vec::new(); + } + let columns = self.columns.max(1); + let last_index = self.total - 1; + let first = (self.scroll * columns).min(last_index); + let last = ((self.scroll + self.visible_rows()) * columns) + .saturating_sub(1) + .min(last_index); + + let all = self.periods_in_order(); + let (Some(from), Some(to)) = (self.period_at_index(first), self.period_at_index(last)) + else { + return Vec::new(); + }; + let (Some(from), Some(to)) = ( + all.iter().position(|held| *held == from), + all.iter().position(|held| *held == to), + ) else { + return Vec::new(); + }; + let low = from.saturating_sub(1); + let high = (to + 1).min(all.len() - 1); + all[low..=high].to_vec() + } + + /// What still needs asking for: `None` when the period is held whole or already on its + /// way, otherwise the cursor to continue from (`None` for a first request). + pub fn period_wanted(&self, period: &str) -> Option> { + if self.period_inflight.contains(period) { + return None; + } + match ( + self.periods.contains_key(period), + self.period_more.get(period), + ) { + (true, None) => None, + (_, more) => Some(more.cloned()), + } + } + + /// Drops the periods the viewport has moved away from. This is the other half of the + /// bound on memory: the thumbnail cache caps the pictures, this caps the tiles. + pub fn forget_periods_outside(&mut self, keep: &[String]) { + self.periods.retain(|period, _| keep.contains(period)); + self.period_more.retain(|period, _| keep.contains(period)); + } + + /// Lays the held periods end to end into the run of tiles the grid indexes into. + /// + /// Stops at the first hole. Two periods with a third still on its way are not + /// adjacent, and joining them would silently draw July's photographs at August's + /// indices — wrong pictures, with nothing on screen to say so. + pub fn rebuild_window(&mut self) { + let mut ordered: Vec<(usize, String)> = self + .periods + .keys() + .filter_map(|period| Some((self.period_start(period)?, period.clone()))) + .collect(); + ordered.sort(); + + let mut base = None; + let mut tiles: Vec = Vec::new(); + for (start, period) in ordered { + let held = &self.periods[&period]; + match base { + None => { + base = Some(start); + tiles.extend_from_slice(held); + } + Some(first) if first + tiles.len() == start => tiles.extend_from_slice(held), + Some(_) => break, + } + } + self.window = TileWindow { + base: base.unwrap_or(0), + tiles, + }; + } + + /// Every year the library has, newest first. + pub fn years(&self) -> Vec { + let mut years: Vec = Vec::new(); + for bucket in &self.buckets { + if bucket.date.len() < 4 { + continue; + } + let year = &bucket.date[..4]; + if years.last().map(String::as_str) != Some(year) { + years.push(year.to_string()); + } + } + years + } + + pub fn index_for_year(&self, year: &str) -> Option { + let mut start = 0usize; + for bucket in &self.buckets { + if bucket.date.len() >= 4 && &bucket.date[..4] == year { + return Some(start); + } + start += bucket.count as usize; + } + None + } + + /// Steps a whole year — positive is older, the direction the timeline runs. + /// + /// Through the years the library actually has, not through the calendar: that is what + /// makes crossing an eleven-year hole cost one press rather than eleven. + pub fn step_year(&mut self, delta: isize) { + let years = self.years(); + if years.is_empty() { + return; + } + let here = self + .date_at_index(self.selected) + .filter(|date| date.len() >= 4) + .map(|date| date[..4].to_string()); + let at = here + .and_then(|year| years.iter().position(|held| *held == year)) + .unwrap_or(0); + let next = (at as isize + delta).clamp(0, years.len() as isize - 1) as usize; + if next == at { + return; + } + if let Some(index) = self.index_for_year(&years[next]) { + self.go_to(index); + } + } + + /// Puts the cursor on a global index and the screen around it. + pub fn go_to(&mut self, index: usize) { + if self.total == 0 { + return; + } + self.selected = index.min(self.total - 1); + // A jump puts its day at the top of the screen rather than leaving it wherever the + // old scroll happened to sit. + self.scroll = self.selected / self.columns.max(1); + self.images_dirty = true; + } + + /// Where each year sits on a rail `height` rows tall. + /// + /// A year whose row a newer one already claimed is left off rather than drawn over it: + /// a rail is a map, and two labels in one place is worse than one. + pub fn year_marks(&self, height: u16) -> Vec<(u16, String)> { + if self.total == 0 || height == 0 { + return Vec::new(); + } + let mut marks: Vec<(u16, String)> = Vec::new(); + for year in self.years() { + let Some(index) = self.index_for_year(&year) else { + continue; + }; + let row = self.rail_row_for(index, height); + if marks.iter().any(|(taken, _)| *taken == row) { + continue; + } + marks.push((row, year)); + } + marks + } + + /// Where the cursor sits on a rail `height` rows tall. + pub fn rail_row(&self, height: u16) -> u16 { + self.rail_row_for(self.selected, height) + } + + fn rail_row_for(&self, index: usize, height: u16) -> u16 { + if self.total <= 1 || height <= 1 { + return 0; + } + let along = index as f64 / (self.total - 1) as f64; + (along * (height - 1) as f64).round() as u16 + } + pub fn move_by(&mut self, delta: isize) { - if self.assets.is_empty() { + if self.total == 0 { return; } - let last = self.assets.len() as isize - 1; + let last = self.total as isize - 1; let next = (self.selected as isize + delta).clamp(0, last) as usize; if next != self.selected { self.selected = next; @@ -250,16 +590,6 @@ impl App { } } - /// True when the grid is close enough to the end of what has been fetched that the - /// next page should be asked for. - pub fn wants_more(&self) -> bool { - if self.cursor.is_none() || self.loading { - return false; - } - let visible_end = (self.scroll + self.visible_rows() + 2) * self.columns.max(1); - visible_end >= self.assets.len() - } - /// The photographs on screen, which are the ones worth fetching a thumbnail for. pub fn visible_ids(&self) -> Vec { self.tiles.iter().map(|tile| tile.id.clone()).collect() @@ -271,29 +601,44 @@ impl App { pub fn pending_on_screen(&self) -> Vec { self.tiles .iter() - .filter_map(|tile| self.assets.get(tile.index)) - .filter(|asset| !matches!(asset.status, AssetStatus::Ready | AssetStatus::Failed)) - .map(|asset| asset.id.clone()) + .filter_map(|tile| self.window.get(tile.index)) + .filter(|tile| !matches!(tile.status, AssetStatus::Ready | AssetStatus::Failed)) + .map(|tile| tile.id.clone()) .collect() } - /// Takes a re-checked asset. Becoming ready is what makes the picture worth asking for - /// again: the first request for it was answered with a 404 and cached as "asked for", - /// so without forgetting that the tile stays empty however long you wait. + /// Takes a re-read asset: the full record for the details panel, and whatever of it + /// the tile also carries. + /// + /// Becoming ready is what makes the picture worth asking for again: the first request + /// for it was answered with a 404 and cached as "asked for", so without forgetting + /// that the tile stays empty however long you wait. pub fn apply_refreshed(&mut self, asset: Asset) { self.refreshing.remove(&asset.id); - let Some(slot) = self.assets.iter_mut().find(|held| held.id == asset.id) else { + if self.selected_id().as_deref() == Some(asset.id.as_str()) { + self.detail = Some(asset.clone()); + } + + let became_ready; + if let Some(tile) = self + .window + .tiles + .iter_mut() + .find(|held| held.id == asset.id) + { + became_ready = asset.status == AssetStatus::Ready && tile.status != AssetStatus::Ready; + tile.status = asset.status; + tile.favorite = asset.favorite; + } else { + // Reloaded out from under an answer that was still on its way. return; - }; - let was_ready = slot.status == AssetStatus::Ready; - let now_ready = asset.status == AssetStatus::Ready; - let id = asset.id.clone(); - *slot = asset; - - if now_ready && !was_ready { - self.thumbnails.remove(&id); - self.wanted.remove(&id); - self.forget_preview(&id); + } + + if became_ready { + self.thumbnails.remove(&asset.id); + self.thumb_order.retain(|held| *held != asset.id); + self.wanted.remove(&asset.id); + self.forget_preview(&asset.id); self.images_dirty = true; } } @@ -316,6 +661,37 @@ impl App { } } + /// Keeps a decoded grid thumbnail, dropping the oldest once the cap is reached — the + /// same discipline `previews` has always had, applied to the map that never had it. + /// This one holds decoded images and used to grow for the whole session; on a + /// twenty-year library that is a memory leak with a picture in it. + /// + /// Two things make the eviction safe. Dropping a picture also forgets that it was ever + /// asked for: `wanted` is the record of "a request has gone out for this", and leaving + /// it behind would leave a tile that can never be filled again however far back you + /// scroll. And a thumbnail the grid is drawing right now is never the one chosen, + /// because evicting what is on screen is a hole that refills only to be evicted again. + pub fn remember_thumbnail(&mut self, id: String) { + // Looking at it again buys it time, rather than leaving it where it first landed. + self.thumb_order.retain(|held| *held != id); + self.thumb_order.push_back(id); + + let on_screen: HashSet = self.visible_ids().into_iter().collect(); + while self.thumb_order.len() > THUMBNAIL_CACHE { + let Some(oldest) = self + .thumb_order + .iter() + .position(|held| !on_screen.contains(held)) + .and_then(|at| self.thumb_order.remove(at)) + else { + // Everything held is on screen. The cap yields rather than the grid. + break; + }; + self.thumbnails.remove(&oldest); + self.wanted.remove(&oldest); + } + } + pub fn forget_preview(&mut self, id: &str) { self.previews.remove(id); self.preview_order.retain(|held| held != id); @@ -372,9 +748,47 @@ mod tests { } } - fn waiting_on(id: &str, status: AssetStatus) -> App { + /// A one-day library with one photograph in it, laid out. + fn holding(ids: &[(&str, AssetStatus)]) -> App { let mut app = App::new(); - app.assets = vec![asset(id, status)]; + app.buckets = vec![TimelineBucket { + date: "2024-06-01".into(), + count: ids.len() as u64, + cover_asset_id: None, + }]; + app.recount(); + app.periods.insert( + "2024-06".into(), + ids.iter() + .map(|(id, status)| TimelineTile { + id: (*id).into(), + captured_at: "2024-06-01T09:30:00.000Z".into(), + width: None, + height: None, + r#type: AssetType::Image, + status: *status, + favorite: false, + duration: None, + placeholder_color: None, + live_photo_video_id: None, + }) + .collect(), + ); + app.rebuild_window(); + app.tiles = ids + .iter() + .enumerate() + .map(|(index, (id, _))| Tile { + id: (*id).into(), + inner: Rect::default(), + index, + }) + .collect(); + app + } + + fn waiting_on(id: &str, status: AssetStatus) -> App { + let mut app = holding(&[(id, status)]); app.tiles = vec![Tile { id: id.into(), inner: Rect::default(), @@ -419,7 +833,10 @@ mod tests { ); assert!(!app.refreshing.contains("a")); assert!(app.images_dirty, "the grid has to be redrawn to show it"); - assert_eq!(app.assets[0].status, AssetStatus::Ready); + assert_eq!( + app.window.get(0).map(|tile| tile.status), + Some(AssetStatus::Ready) + ); } #[test] @@ -445,7 +862,7 @@ mod tests { /// What the event loop does each pass: ask for the selected photograph's preview if it /// is not already held and not already on its way. Returns whether a request was made. fn tick_viewer(app: &mut App) -> bool { - let id = app.selected_asset().map(|a| a.id.clone()).unwrap(); + let id = app.selected_id().unwrap(); if app.preview_for(&id).is_none() && app.preview_inflight.insert(id) { return true; } @@ -461,23 +878,7 @@ mod tests { fn looking_at_a_photograph_a_second_time_loads_it_again() { // The bug: the record of "already asked for" outlived the picture itself, so // coming back to a photograph left the viewer saying "Loading…" for ever. - let mut app = App::new(); - app.assets = vec![ - asset("a", AssetStatus::Ready), - asset("b", AssetStatus::Ready), - ]; - app.tiles = vec![ - Tile { - id: "a".into(), - inner: Rect::default(), - index: 0, - }, - Tile { - id: "b".into(), - inner: Rect::default(), - index: 1, - }, - ]; + let mut app = holding(&[("a", AssetStatus::Ready), ("b", AssetStatus::Ready)]); app.mode = Mode::Viewer; assert!(tick_viewer(&mut app), "asks for the first one"); @@ -499,13 +900,7 @@ mod tests { #[test] fn a_photograph_dropped_from_the_cache_is_asked_for_again() { - let mut app = App::new(); - app.assets = vec![asset("a", AssetStatus::Ready)]; - app.tiles = vec![Tile { - id: "a".into(), - inner: Rect::default(), - index: 0, - }]; + let mut app = holding(&[("a", AssetStatus::Ready)]); app.mode = Mode::Viewer; assert!(tick_viewer(&mut app)); @@ -554,13 +949,7 @@ mod tests { fn a_request_that_fails_can_be_made_again() { // Every answer clears the in-flight mark, including a refusal, so a photograph // that failed once is not written off for the rest of the session. - let mut app = App::new(); - app.assets = vec![asset("a", AssetStatus::Ready)]; - app.tiles = vec![Tile { - id: "a".into(), - inner: Rect::default(), - index: 0, - }]; + let mut app = holding(&[("a", AssetStatus::Ready)]); app.mode = Mode::Viewer; assert!(tick_viewer(&mut app)); @@ -578,7 +967,7 @@ mod tests { // The grid can be reloaded while a refresh is in flight. let mut app = waiting_on("a", AssetStatus::Pending); app.apply_refreshed(asset("gone", AssetStatus::Ready)); - assert_eq!(app.assets.len(), 1); - assert_eq!(app.assets[0].id, "a"); + assert_eq!(app.window.tiles.len(), 1); + assert_eq!(app.window.tiles[0].id, "a"); } } diff --git a/src/tui/mod.rs b/src/tui/mod.rs index bcbe5c3..28a9c89 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -23,7 +23,7 @@ use crossterm::terminal::{ use crossterm::{execute, queue}; use futures::stream::{FuturesUnordered, StreamExt}; use image::DynamicImage; -use imogen_sdk::{AssetUpdate, AssetVariant, UploadOptions}; +use imogen_sdk::{AssetUpdate, AssetVariant, TimelineBucketQuery, TimelineQuery, UploadOptions}; use ratatui::backend::CrosstermBackend; use ratatui::Terminal; @@ -40,7 +40,12 @@ type Job<'a> = std::pin::Pin + 'a>> enum Loaded { Thumbnail(String, Result>), Preview(String, Result>), - Page(Result), + /// The shape of the whole library: one entry a day. Small enough to hold for twenty + /// years, which is the whole point of it. + Spine(Result>), + /// One `YYYY-MM` of tiles, named so a slow answer is filed under the period it was + /// asked for rather than under wherever the viewport has since moved. + Bucket(String, Result), Albums(Result>), /// Boxed: an upload result carries a whole `Asset`, which would otherwise make /// every variant of this enum as large as the largest one. @@ -93,7 +98,7 @@ async fn event_loop(ctx: &Context) -> Result<()> { poll.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); let mut work: FuturesUnordered> = FuturesUnordered::new(); - work.push(Box::pin(load_page(ctx, app.to_query(), None))); + work.push(Box::pin(load_spine(ctx, spine_query(&app)))); app.loading = true; work.push(Box::pin(load_albums(ctx))); @@ -118,9 +123,27 @@ async fn event_loop(ctx: &Context) -> Result<()> { if let Some(id) = viewer_preview_wanted(&mut app) { work.push(Box::pin(load_bytes(ctx, id, AssetVariant::Preview, true))); } - if app.wants_more() { - app.loading = true; - work.push(Box::pin(load_page(ctx, app.to_query(), app.cursor.clone()))); + // Which periods the viewport covers, rather than how close the cursor is to the + // end of what has been paged in. Moving the viewport is the only thing that + // changes the answer, so the window is only rebuilt when it does. + let periods = app.periods_for_viewport(); + if periods != app.held { + app.held = periods.clone(); + app.forget_periods_outside(&periods); + app.rebuild_window(); + app.images_dirty = true; + } + for period in &periods { + if let Some(cursor) = app.period_wanted(period) { + app.period_inflight.insert(period.clone()); + work.push(Box::pin(load_bucket( + ctx, + bucket_query(period, &app, cursor), + ))); + } + } + if let Some(id) = detail_wanted(&mut app) { + work.push(Box::pin(load_asset(ctx, id))); } if let Some(path) = app.picker.as_ref().and_then(wants_preview) { if !app.local_previews.contains_key(&path) && app.local_wanted.insert(path.clone()) { @@ -198,13 +221,47 @@ fn key_stream() -> tokio::sync::mpsc::Receiver { receiver } -fn load_page( - ctx: &Context, - mut query: imogen_sdk::AssetQuery, - cursor: Option, -) -> impl std::future::Future + '_ { - query.cursor = cursor; - async move { Loaded::Page(ctx.client.assets.list(&query).await.map_err(Into::into)) } +/// The whole timeline's shape, under the current scope's filter. +fn spine_query(app: &App) -> TimelineQuery { + TimelineQuery { + covers: None, + filter: app.to_filter(), + } +} + +/// One period of tiles, under the same filter the spine was counted with. A window that +/// fetched a different filter from the one it was sized by would show the wrong +/// photographs at the right indices, with nothing on screen to say so. +fn bucket_query(period: &str, app: &App, cursor: Option) -> TimelineBucketQuery { + TimelineBucketQuery { + period: period.to_string(), + cursor, + // Left unset so the server's own default applies rather than a client-side guess. + limit: None, + filter: app.to_filter(), + } +} + +async fn load_spine(ctx: &Context, query: TimelineQuery) -> Loaded { + Loaded::Spine( + ctx.client + .assets + .timeline(&query) + .await + .map(|timeline| timeline.buckets) + .map_err(Into::into), + ) +} + +async fn load_bucket(ctx: &Context, query: TimelineBucketQuery) -> Loaded { + let period = query.period.clone(); + let page = ctx + .client + .assets + .timeline_bucket(&query) + .await + .map_err(Into::into); + Loaded::Bucket(period, page) } async fn load_asset(ctx: &Context, id: String) -> Loaded { @@ -222,7 +279,7 @@ fn viewer_preview_wanted(app: &mut App) -> Option { if app.mode != Mode::Viewer { return None; } - let id = app.selected_asset()?.id.clone(); + let id = app.selected_id()?; if app.preview_for(&id).is_some() { return None; } @@ -232,6 +289,23 @@ fn viewer_preview_wanted(app: &mut App) -> Option { Some(id) } +/// The whole record the viewer's title and the details panel need, when what is held is +/// not the photograph being looked at. A tile deliberately carries only what the grid +/// draws, so the exif, the place and the filename are read once, on the one being looked +/// at, rather than for every tile that scrolls past. +fn detail_wanted(app: &mut App) -> Option { + if app.mode != Mode::Viewer && !app.show_info { + return None; + } + let id = app.selected_id()?; + if app.detail().is_some() || app.detail_asked.as_deref() == Some(id.as_str()) { + return None; + } + app.detail_asked = Some(id.clone()); + app.refreshing.insert(id.clone()); + Some(id) +} + /// The file under the picker's cursor, when it is one worth trying to draw. fn wants_preview(picker: &Picker) -> Option { let entry = picker.current()?; @@ -274,7 +348,12 @@ async fn upload_one(ctx: &Context, path: PathBuf) -> Loaded { async fn fill_album(ctx: &Context, album_id: String, ids: Vec) -> Loaded { let mut added = 0u64; for chunk in ids.chunks(500) { - match ctx.client.albums.add_assets(&album_id, &imogen_sdk::AssetSelection::ids(chunk)).await { + match ctx + .client + .albums + .add_assets(&album_id, &imogen_sdk::AssetSelection::ids(chunk)) + .await + { Ok(result) => added += result.added, Err(error) => return Loaded::Filed(Err(error.into())), } @@ -329,7 +408,8 @@ fn absorb(app: &mut App, loaded: Loaded) { match loaded { Loaded::Thumbnail(id, Ok(bytes)) => { if let Ok(image) = media::decode(&bytes) { - app.thumbnails.insert(id, Arc::new(image)); + app.thumbnails.insert(id.clone(), Arc::new(image)); + app.remember_thumbnail(id); app.images_dirty = true; } } @@ -351,17 +431,36 @@ fn absorb(app: &mut App, loaded: Loaded) { app.preview_inflight.remove(&id); app.note(format!("Could not load: {error}")); } - Loaded::Page(Ok(page)) => { + Loaded::Spine(Ok(buckets)) => { app.loading = false; - if app.total.is_none() { - app.total = page.total; - } - app.cursor = page.next_cursor; - app.assets.extend(page.items); + app.buckets = buckets; + app.recount(); + // The window was sized against the old spine; whatever is held may now sit at + // different indices, so it is laid out again before anything draws. + app.held.clear(); + app.rebuild_window(); app.images_dirty = true; } - Loaded::Page(Err(error)) => { + Loaded::Spine(Err(error)) => { app.loading = false; + app.note(format!("Could not load the timeline: {error}")); + } + Loaded::Bucket(period, Ok(page)) => { + app.period_inflight.remove(&period); + match page.next_cursor { + Some(cursor) => { + app.period_more.insert(period.clone(), cursor); + } + None => { + app.period_more.remove(&period); + } + } + app.periods.entry(period).or_default().extend(page.items); + app.rebuild_window(); + app.images_dirty = true; + } + Loaded::Bucket(period, Err(error)) => { + app.period_inflight.remove(&period); app.note(format!("Could not load photographs: {error}")); } Loaded::Albums(Ok(albums)) => app.albums = albums, @@ -425,8 +524,8 @@ fn place_images(app: &App) -> Result<()> { } else if app.mode == Mode::Viewer { // Matched against the asset on screen: a preview that arrives after you have moved // on belongs to the photograph it was asked for, not to whichever one is showing. - if let (Some(asset), Some(tile)) = (app.selected_asset(), app.tiles.first()) { - if let Some(image) = app.preview_for(&asset.id) { + if let (Some(id), Some(tile)) = (app.selected_id(), app.tiles.first()) { + if let Some(image) = app.preview_for(&id) { let (cols, rows) = fit(image, tile.inner.width, tile.inner.height); // Centre it in the pane rather than pinning it to the corner. let x = tile.inner.x + (tile.inner.width.saturating_sub(cols)) / 2; @@ -504,6 +603,28 @@ async fn handle_key<'a>( return Ok(()); } + // Typing a date to jump the timeline to. + if let Mode::JumpDate(current) = &app.mode { + let mut input = current.clone(); + match key.code { + KeyCode::Esc => app.mode = Mode::Grid, + KeyCode::Enter => { + app.mode = Mode::Grid; + jump_to(app, &input); + } + KeyCode::Backspace => { + input.pop(); + app.mode = Mode::JumpDate(input); + } + KeyCode::Char(c) => { + input.push(c); + app.mode = Mode::JumpDate(input); + } + _ => {} + } + return Ok(()); + } + // Typing a path to jump the picker to. if let Mode::PickerPath(current) = &app.mode { let mut input = current.clone(); @@ -549,7 +670,12 @@ async fn handle_key<'a>( if matches!(key.code, KeyCode::Char('y') | KeyCode::Char('Y')) { match action { Action::Trash(ids) => { - match ctx.client.assets.trash(&imogen_sdk::AssetSelection::ids(&ids)).await { + match ctx + .client + .assets + .trash(&imogen_sdk::AssetSelection::ids(&ids)) + .await + { Ok(result) => { app.note(format!("{} moved to the trash.", result.count)); reload(ctx, app, work); @@ -558,7 +684,12 @@ async fn handle_key<'a>( }; } Action::Restore(ids) => { - match ctx.client.assets.restore(&imogen_sdk::AssetSelection::ids(&ids)).await { + match ctx + .client + .assets + .restore(&imogen_sdk::AssetSelection::ids(&ids)) + .await + { Ok(result) => { app.note(format!("{} restored.", result.count)); reload(ctx, app, work); @@ -639,7 +770,7 @@ async fn handle_key<'a>( app.images_dirty = true; } KeyCode::Enter => { - if app.selected_asset().is_some() { + if app.selected_tile().is_some() { app.mode = Mode::Viewer; app.images_dirty = true; } @@ -650,16 +781,14 @@ async fn handle_key<'a>( KeyCode::Down | KeyCode::Char('j') => app.move_by(columns), KeyCode::PageUp => app.move_by(-columns * app.visible_rows() as isize), KeyCode::PageDown => app.move_by(columns * app.visible_rows() as isize), - KeyCode::Char('g') => { - app.selected = 0; - app.scroll = 0; - app.images_dirty = true; - } - KeyCode::Char('G') => { - app.selected = app.assets.len().saturating_sub(1); - app.keep_selection_visible(); - app.images_dirty = true; - } + // `g` used to mean "the top", which on a twenty-year library is the one place you + // can already reach. It now asks where you want to be; Home still means the top. + KeyCode::Char('g') => app.mode = Mode::JumpDate(String::new()), + KeyCode::Home => app.go_to(0), + KeyCode::End | KeyCode::Char('G') => app.go_to(app.total.saturating_sub(1)), + // Whole years, through the years the library has rather than through the calendar. + KeyCode::Char('[') => app.step_year(1), + KeyCode::Char(']') => app.step_year(-1), KeyCode::Char('/') => app.mode = Mode::Search(app.query.clone()), KeyCode::Char('i') => { app.show_info = !app.show_info; @@ -687,24 +816,33 @@ async fn handle_key<'a>( KeyCode::Char('f') => toggle(ctx, app, Favorite).await, KeyCode::Char('e') => toggle(ctx, app, Archive).await, KeyCode::Char('d') => { - if let Some(asset) = app.selected_asset() { - app.mode = Mode::Confirm { - prompt: format!( - "Move “{}” to the trash?", + if let Some(id) = app.selected_id() { + // Named where the name is known, dated otherwise: a tile does not carry a + // filename, and a prompt about something unnamed is worse than one about + // a day. + let what = match app.detail() { + Some(asset) => format!( + "“{}”", crate::output::truncate(&asset.original_filename, 40) ), - action: Action::Trash(vec![asset.id.clone()]), + None => match app.date_at_index(app.selected) { + Some(date) => format!("the photograph from {date}"), + None => "it".to_string(), + }, + }; + app.mode = Mode::Confirm { + prompt: format!("Move {what} to the trash?"), + action: Action::Trash(vec![id]), }; } } - KeyCode::Char('r') => { - if let Some(asset) = app.selected_asset() { - if asset.deleted_at.is_some() { - app.mode = Mode::Confirm { - prompt: "Restore it from the trash?".into(), - action: Action::Restore(vec![asset.id.clone()]), - }; - } + // The trash is a place, not a flag, and being in it is what the scope says. + KeyCode::Char('r') if app.scope == Scope::Trash => { + if let Some(id) = app.selected_id() { + app.mode = Mode::Confirm { + prompt: "Restore it from the trash?".into(), + action: Action::Restore(vec![id]), + }; } } _ => {} @@ -817,10 +955,36 @@ fn expand(input: &str) -> PathBuf { fn reload<'a>(ctx: &'a Context, app: &mut App, work: &mut FuturesUnordered>) { app.reset_results(); - app.total = None; app.wanted.clear(); app.loading = true; - work.push(Box::pin(load_page(ctx, app.to_query(), None))); + work.push(Box::pin(load_spine(ctx, spine_query(app)))); +} + +/// Puts the cursor on the day somebody typed, or on the nearest day that has photographs +/// when that day has none — and says so when it has landed somewhere else, because a +/// silent landing looks like a jump that did not work. +fn jump_to(app: &mut App, input: &str) { + let Some(day) = crate::dates::to_day(input) else { + app.note(format!( + "“{}” is not a date. Try 2011, aug 2011, or 2011-08-14.", + input.trim() + )); + return; + }; + let Some(index) = app.index_for_date(&day.date) else { + app.note("There is nothing here to jump to."); + return; + }; + app.go_to(index); + + // Only a landing outside what was actually asked for is a surprise. Somebody who + // typed "march 2000" and arrived on the second of March asked for that. + let Some(landed) = app.date_at_index(index) else { + return; + }; + if landed.get(..day.named) != day.date.get(..day.named) { + app.note(format!("Nothing on {}. This is {landed}.", day.date)); + } } fn switch<'a>(ctx: &'a Context, app: &mut App, work: &mut FuturesUnordered>, scope: Scope) { @@ -871,15 +1035,26 @@ impl Toggle for Archive { } async fn toggle(ctx: &Context, app: &mut App, which: impl Toggle) { - let Some(asset) = app.selected_asset().cloned() else { + let Some(id) = app.selected_id() else { return; }; + // A patch that inverts a flag needs to know what the flag is, and a tile does not + // carry all of them. Read the record first when it is not already held — one request, + // on a key somebody pressed deliberately. + let asset = match app.detail().cloned() { + Some(held) => held, + None => match ctx.client.assets.get(&id).await { + Ok(asset) => asset, + Err(error) => { + app.note(format!("Could not read it: {error}")); + return; + } + }, + }; let (patch, message) = which.patch(&asset); - match ctx.client.assets.update(&asset.id, &patch).await { + match ctx.client.assets.update(&id, &patch).await { Ok(updated) => { - if let Some(slot) = app.assets.iter_mut().find(|a| a.id == updated.id) { - *slot = updated; - } + app.apply_refreshed(updated); app.note(message); } Err(error) => app.note(format!("Could not change it: {error}")), @@ -889,8 +1064,8 @@ async fn toggle(ctx: &Context, app: &mut App, which: impl Toggle) { #[cfg(test)] mod tests { use super::*; - use crate::tui::app::Tile; - use imogen_sdk::{Asset, AssetStatus, AssetType}; + use crate::tui::app::{Tile, THUMBNAIL_CACHE}; + use imogen_sdk::{AssetStatus, AssetType, TimelineBucket, TimelineTile}; use ratatui::layout::Rect; fn png_bytes() -> Vec { @@ -902,40 +1077,48 @@ mod tests { out } - fn asset(id: &str) -> Asset { - Asset { + fn test_image() -> DynamicImage { + DynamicImage::ImageRgba8(image::RgbaImage::new(2, 2)) + } + + fn bucket(date: &str, count: u64) -> TimelineBucket { + TimelineBucket { + date: date.into(), + count, + cover_asset_id: None, + } + } + + fn tile(id: &str, captured_at: &str) -> TimelineTile { + TimelineTile { id: id.into(), - owner_id: "owner".into(), - r#type: AssetType::Image, - status: AssetStatus::Ready, - original_filename: "photo.jpg".into(), - mime_type: "image/jpeg".into(), - checksum: "c".repeat(64), - size_bytes: 1, + captured_at: captured_at.into(), width: None, height: None, - duration: None, - captured_at: "2024-06-01T09:30:00.000Z".into(), - captured_at_is_exact: false, - captured_at_original: None, - captured_at_original_is_exact: None, - created_at: "2024-06-01T09:30:00.000Z".into(), - updated_at: "2024-06-01T09:30:00.000Z".into(), - deleted_at: None, + r#type: AssetType::Image, + status: AssetStatus::Ready, favorite: false, - archived: false, - description: None, - exif: None, - location: None, + duration: None, placeholder_color: None, live_photo_video_id: None, - device_asset_id: None, } } + /// An app as the loop leaves it once one day's bucket has arrived: a spine that knows + /// the shape of the library, a window of tiles, and a layout over them. fn viewing(ids: &[&str]) -> App { let mut app = App::new(); - app.assets = ids.iter().map(|id| asset(id)).collect(); + if !ids.is_empty() { + app.buckets = vec![bucket("2024-06-01", ids.len() as u64)]; + app.recount(); + app.periods.insert( + "2024-06".into(), + ids.iter() + .map(|id| tile(id, "2024-06-01T09:30:00.000Z")) + .collect(), + ); + app.rebuild_window(); + } app.tiles = ids .iter() .enumerate() @@ -949,6 +1132,493 @@ mod tests { app } + /// A library with an eleven-year hole in the middle, which is what an ordinary + /// twenty-year library looks like: a burst, a gap, another burst. + fn library_with_a_gap() -> App { + let mut app = App::new(); + app.buckets = vec![ + bucket("2011-08-15", 10), + bucket("2011-08-14", 10), + bucket("2000-03-02", 10), + bucket("2000-03-01", 10), + ]; + app.recount(); + app + } + + #[test] + fn thumbnails_are_evicted_once_the_cap_is_reached() { + let mut app = viewing(&[]); + app.grid_area = Rect::new(0, 0, 80, 24); + for n in 0..(THUMBNAIL_CACHE + 10) { + let id = format!("asset-{n}"); + app.thumbnails.insert(id.clone(), Arc::new(test_image())); + app.remember_thumbnail(id); + } + assert!(app.thumbnails.len() <= THUMBNAIL_CACHE); + assert!(app.thumb_order.len() <= THUMBNAIL_CACHE); + } + + /// The other half of eviction. `wanted` is the record of "a request has gone out for + /// this"; dropping the picture without dropping that record leaves a tile that can + /// never be filled again, however far back you scroll. + #[test] + fn an_evicted_thumbnail_is_asked_for_again() { + let mut app = viewing(&[]); + for n in 0..(THUMBNAIL_CACHE + 10) { + let id = format!("asset-{n}"); + app.wanted.insert(id.clone()); + app.thumbnails.insert(id.clone(), Arc::new(test_image())); + app.remember_thumbnail(id); + } + assert!(!app.thumbnails.contains_key("asset-0"), "evicted"); + assert!( + !app.wanted.contains("asset-0"), + "and so must be asked for again rather than left blank for ever" + ); + } + + #[test] + fn the_thumbnail_on_screen_is_never_the_one_evicted() { + // "keep" is both the oldest thing in the cache and the one the grid is drawing. + let mut app = viewing(&["keep"]); + app.thumbnails.insert("keep".into(), Arc::new(test_image())); + app.remember_thumbnail("keep".into()); + for n in 0..(THUMBNAIL_CACHE + 10) { + let id = format!("asset-{n}"); + app.thumbnails.insert(id.clone(), Arc::new(test_image())); + app.remember_thumbnail(id); + } + assert!( + app.thumbnails.contains_key("keep"), + "the picture on screen must survive its own eviction" + ); + } + + #[test] + fn jumping_to_a_date_lands_on_it() { + let mut app = viewing(&[]); + app.buckets = vec![bucket("2011-08-15", 10), bucket("2011-08-14", 10)]; + app.recount(); + assert_eq!(app.index_for_date("2011-08-14"), Some(10)); + } + + #[test] + fn jumping_to_a_day_with_no_photographs_lands_on_the_nearest_older_one() { + let mut app = viewing(&[]); + app.buckets = vec![bucket("2011-08-15", 10), bucket("2011-08-10", 10)]; + app.recount(); + assert_eq!(app.index_for_date("2011-08-12"), Some(10)); + } + + /// The bug the web shipped: the search for the nearest day gave up after a bounded + /// number of steps and threw the reader at the end of the library. A gap of eleven + /// years must cost exactly what a gap of two days costs. + #[test] + fn a_gap_of_years_costs_no_more_than_a_gap_of_days() { + let app = library_with_a_gap(); + // Into the hole: the nearest day with photographs, going older, is 2000-03-02. + assert_eq!(app.index_for_date("2006-06-15"), Some(20)); + // And not the end of the library, which is where giving up would land. + assert_ne!(app.index_for_date("2006-06-15"), Some(app.total - 1)); + } + + #[test] + fn jumping_ahead_of_the_library_lands_on_the_newest_photograph() { + let app = library_with_a_gap(); + assert_eq!(app.index_for_date("2030-01-01"), Some(0)); + } + + #[test] + fn jumping_past_the_end_lands_on_the_oldest_photograph() { + let mut app = viewing(&[]); + app.buckets = vec![bucket("2011-08-15", 10)]; + app.recount(); + assert_eq!(app.index_for_date("1999-01-01"), Some(9)); + } + + #[test] + fn an_empty_library_has_nowhere_to_jump_to() { + let app = viewing(&[]); + assert_eq!(app.index_for_date("2011-08-14"), None); + } + + #[test] + fn the_day_at_an_index_is_the_day_the_index_was_found_for() { + let app = library_with_a_gap(); + for (index, expected) in [ + (0, "2011-08-15"), + (9, "2011-08-15"), + (10, "2011-08-14"), + (20, "2000-03-02"), + (39, "2000-03-01"), + ] { + assert_eq!(app.date_at_index(index).as_deref(), Some(expected)); + } + assert_eq!(app.date_at_index(40), None, "past the end of the library"); + } + + /// Stepping through the years the library has, not through the calendar: one press + /// crosses the whole hole. + #[test] + fn stepping_a_year_crosses_a_gap_of_years_in_one_press() { + let mut app = library_with_a_gap(); + app.step_year(1); + assert_eq!( + app.date_at_index(app.selected).as_deref(), + Some("2000-03-02") + ); + app.step_year(-1); + assert_eq!( + app.date_at_index(app.selected).as_deref(), + Some("2011-08-15") + ); + // And neither end runs off. + app.step_year(-1); + assert_eq!(app.selected, 0); + app.step_year(1); + app.step_year(1); + assert_eq!( + app.date_at_index(app.selected).as_deref(), + Some("2000-03-02") + ); + } + + #[test] + fn the_viewport_asks_only_for_the_periods_it_covers() { + let mut app = viewing(&[]); + app.buckets = vec![ + bucket("2011-09-01", 200), + bucket("2011-08-14", 200), + bucket("2011-07-01", 200), + ]; + app.recount(); + app.columns = 4; + app.grid_area = Rect::new(0, 0, 80, 24); + app.selected = 0; + let periods = app.periods_for_viewport(); + assert!(periods.contains(&"2011-09".to_string())); + assert!(!periods.contains(&"2011-07".to_string())); + } + + /// Nothing here may wait on a drawn frame to become useful. The web shipped a grid + /// that refused to render until it had been measured, so there was never anything to + /// measure; an unmeasured App must still be able to say what to fetch. + #[test] + fn the_periods_wanted_are_known_before_anything_has_been_measured() { + let mut app = App::new(); + app.buckets = vec![bucket("2011-09-01", 200)]; + app.recount(); + assert_eq!(app.grid_area, Rect::default(), "not yet laid out"); + assert!( + app.periods_for_viewport().contains(&"2011-09".to_string()), + "the first fetch cannot wait for a frame that needs it to have happened" + ); + } + + /// The trash, the archive and the favourites are separate places, and a windowed + /// fetch carrying the wrong filter would show the wrong photographs with nothing on + /// screen to say so. + #[test] + fn each_scope_asks_for_its_own_photographs() { + let mut app = App::new(); + assert_eq!(app.to_filter(), imogen_sdk::AssetFilter::default()); + + app.scope = Scope::Trash; + assert_eq!(app.to_filter().trashed, Some(true)); + assert_eq!(app.to_filter().archived, None); + + app.scope = Scope::Archived; + assert_eq!(app.to_filter().archived, Some(true)); + assert_eq!(app.to_filter().trashed, None); + + app.scope = Scope::Favorites; + assert_eq!(app.to_filter().favorite, Some(true)); + + app.scope = Scope::Library; + app.query = "beach".into(); + assert_eq!(app.to_filter().q.as_deref(), Some("beach")); + } + + /// The same filter reaches the spine and every bucket, so what is counted and what is + /// shown cannot disagree about which place you are in. + #[test] + fn the_spine_and_the_bucket_are_filtered_the_same_way() { + let mut app = App::new(); + app.scope = Scope::Trash; + assert_eq!( + bucket_query("2011-08", &app, None).filter, + spine_query(&app).filter + ); + assert_eq!( + bucket_query("2011-08", &app, None).filter.trashed, + Some(true) + ); + } + + #[test] + fn the_window_answers_for_the_indices_it_holds_and_no_others() { + let mut app = App::new(); + app.buckets = vec![bucket("2011-09-02", 2), bucket("2011-08-14", 2)]; + app.recount(); + app.periods + .insert("2011-08".into(), vec![tile("c", "x"), tile("d", "x")]); + app.rebuild_window(); + + assert_eq!(app.window.base, 2); + assert!(app.window.get(1).is_none(), "not held"); + assert_eq!(app.window.get(2).map(|t| t.id.as_str()), Some("c")); + assert_eq!(app.window.get(3).map(|t| t.id.as_str()), Some("d")); + assert!(app.window.get(4).is_none(), "past the end"); + } + + /// Two periods with a third still on its way are not adjacent, and joining them would + /// silently put July's photographs at August's indices. + #[test] + fn a_hole_in_what_has_arrived_does_not_join_tiles_across_it() { + let mut app = App::new(); + app.buckets = vec![ + bucket("2011-09-01", 1), + bucket("2011-08-14", 1), + bucket("2011-07-01", 1), + ]; + app.recount(); + app.periods.insert("2011-09".into(), vec![tile("sep", "x")]); + app.periods.insert("2011-07".into(), vec![tile("jul", "x")]); + app.rebuild_window(); + + assert_eq!(app.window.get(0).map(|t| t.id.as_str()), Some("sep")); + assert!( + app.window.get(1).is_none(), + "August has not arrived, so index 1 is nobody's" + ); + assert_ne!(app.window.get(2).map(|t| t.id.as_str()), Some("jul")); + } + + #[test] + fn a_month_by_name_is_a_date_somebody_may_type() { + let day = |input| crate::dates::to_day(input).map(|day| day.date); + assert_eq!(day("aug 2011").as_deref(), Some("2011-08-31")); + assert_eq!(day("August 2011").as_deref(), Some("2011-08-31")); + assert_eq!(day("2011").as_deref(), Some("2011-12-31")); + assert_eq!(day("2011-08-14").as_deref(), Some("2011-08-14")); + assert_eq!(day("2011-08").as_deref(), Some("2011-08-31")); + assert_eq!(day("not a date"), None); + } + + /// Typed, jumped, landed — through the same path the key handler takes. + #[test] + fn typing_a_month_lands_in_that_month() { + let mut app = library_with_a_gap(); + jump_to(&mut app, "march 2000"); + assert_eq!( + app.date_at_index(app.selected).as_deref(), + Some("2000-03-02") + ); + assert!( + app.status.is_none(), + "it landed on the day it was asked for" + ); + + jump_to(&mut app, "june 2006"); + assert_eq!( + app.date_at_index(app.selected).as_deref(), + Some("2000-03-02") + ); + assert!( + app.status.is_some(), + "and says so when it lands somewhere else" + ); + } + + /// The year rail is a map of the whole library, and two labels in one place is worse + /// than one. + #[test] + fn the_year_rail_never_stacks_two_years_on_one_row() { + let mut app = App::new(); + app.buckets = (0..20) + .map(|n| bucket(&format!("{}-06-01", 2024 - n), 100)) + .collect(); + app.recount(); + let marks = app.year_marks(8); + let rows: Vec = marks.iter().map(|(row, _)| *row).collect(); + let mut sorted = rows.clone(); + sorted.sort_unstable(); + sorted.dedup(); + assert_eq!(rows.len(), sorted.len(), "{marks:?}"); + assert!(marks.iter().all(|(row, _)| *row < 8)); + assert_eq!(marks.first().map(|(_, year)| year.as_str()), Some("2024")); + } + + /// One pass of the event loop's window management, without the network: settle the + /// window on where the viewport is, and answer every period it asks for. + fn pass(app: &mut App, served: &mut usize) { + let periods = app.periods_for_viewport(); + if periods != app.held { + app.held = periods.clone(); + app.forget_periods_outside(&periods); + app.rebuild_window(); + } + for period in &periods { + if app.period_wanted(period).is_none() { + continue; + } + let start = app.period_start(period).unwrap(); + let count = app + .buckets + .iter() + .filter(|bucket| &bucket.date[..7] == period) + .map(|bucket| bucket.count as usize) + .sum::(); + *served += 1; + absorb( + app, + Loaded::Bucket( + period.clone(), + Ok(imogen_sdk::TilePage { + items: (start..start + count) + .map(|n| tile(&format!("a{n}"), "2011-01-01T00:00:00.000Z")) + .collect(), + next_cursor: None, + total: None, + }), + ), + ); + } + } + + /// A library of twenty years, one month at a time. + fn twenty_years() -> App { + let mut app = App::new(); + app.buckets = (0..240) + .map(|n| bucket(&format!("{}-{:02}-01", 2024 - n / 12, 12 - n % 12), 40)) + .collect(); + app.recount(); + app.columns = 4; + app.grid_area = Rect::new(0, 0, 80, 27); + app + } + + /// The other half of the leak. Tiles were the `Vec` that grew with every page + /// fetched and was never trimmed; scrolling the length of a twenty-year library must + /// now cost a fixed amount of memory, not a rising one. + #[test] + fn scrolling_the_whole_library_does_not_accumulate_tiles() { + let mut app = twenty_years(); + let mut served = 0usize; + let mut worst = 0usize; + + while app.selected + 1 < app.total { + pass(&mut app, &mut served); + worst = worst.max(app.periods.values().map(Vec::len).sum::()); + app.move_by(app.columns as isize * 3); + } + + assert_eq!(app.total, 9600, "the whole library was walked"); + // Five periods of forty is the most the viewport plus one either side can cover. + assert!(worst <= 5 * 40, "held {worst} tiles at once"); + assert!( + app.window.tiles.len() <= 5 * 40, + "the window is the viewport's, not the library's" + ); + assert!( + served > 200, + "and every period was really fetched: {served}" + ); + } + + /// Scrolling back does refetch — that is the trade the bound buys — but the window + /// must land on the same tiles, not on tiles shifted by whatever was dropped. + #[test] + fn scrolling_back_lands_on_the_same_photographs() { + let mut app = twenty_years(); + let mut served = 0usize; + pass(&mut app, &mut served); + let first = app.window.get(0).map(|tile| tile.id.clone()); + + app.go_to(5000); + pass(&mut app, &mut served); + app.go_to(0); + pass(&mut app, &mut served); + + assert_eq!(app.window.get(0).map(|tile| tile.id.clone()), first); + assert_eq!(app.window.base, 0); + } + + /// The spine arriving is what makes the library reachable at all: before it, nothing + /// knows there is a 2009 to ask for. + #[test] + fn the_spine_is_what_makes_the_far_end_reachable() { + let mut app = App::new(); + app.columns = 4; + app.grid_area = Rect::new(0, 0, 80, 27); + assert!(app.periods_for_viewport().is_empty(), "nothing known yet"); + + absorb( + &mut app, + Loaded::Spine(Ok(vec![bucket("2024-06-01", 40), bucket("2009-03-01", 40)])), + ); + assert_eq!(app.total, 80); + assert!(!app.loading); + // And 2009 is one jump away, with nothing paged through to get there. + assert_eq!(app.index_for_date("2009-03-01"), Some(40)); + } + + /// A reload swaps the spine underneath tiles that were held against the old one. + /// Their indices are not the same indices any more. + #[test] + fn a_new_spine_relays_the_window_rather_than_trusting_the_old_indices() { + let mut app = App::new(); + app.buckets = vec![bucket("2024-06-01", 40)]; + app.recount(); + app.periods + .insert("2024-06".into(), vec![tile("a", "x"), tile("b", "x")]); + app.rebuild_window(); + assert_eq!(app.window.base, 0); + + // Something older arrived, so June is no longer at the top of the library. + absorb( + &mut app, + Loaded::Spine(Ok(vec![bucket("2024-07-01", 10), bucket("2024-06-01", 40)])), + ); + assert_eq!( + app.window.base, 10, + "June's tiles moved down by all of July" + ); + } + + /// A record is read once for the photograph being looked at, not on every pass of the + /// loop. A fetch that fails must not become a busy loop against a broken network. + #[test] + fn the_record_of_one_photograph_is_asked_for_once() { + let mut app = viewing(&["a", "b"]); + assert_eq!(detail_wanted(&mut app).as_deref(), Some("a")); + assert_eq!(detail_wanted(&mut app), None, "not asked for twice"); + + // The request fails. Still not asked for again, however many passes go by. + absorb( + &mut app, + Loaded::Refreshed("a".into(), Box::new(Err(anyhow::anyhow!("no network")))), + ); + assert_eq!(detail_wanted(&mut app), None); + assert_eq!(detail_wanted(&mut app), None); + + // Moving on asks for the one you moved on to. + app.move_by(1); + assert_eq!(detail_wanted(&mut app).as_deref(), Some("b")); + } + + /// And nothing is read for a grid nobody has asked for details about: that would be a + /// request per tile scrolled past. + #[test] + fn scrolling_the_grid_reads_no_records_at_all() { + let mut app = viewing(&["a", "b"]); + app.mode = Mode::Grid; + assert_eq!(detail_wanted(&mut app), None); + app.show_info = true; + assert_eq!(detail_wanted(&mut app).as_deref(), Some("a")); + } + /// The reported bug, through the real request decision and the real absorb: view one, /// move to the next, come back — and the first must still appear. It did not, because /// the note that it had been asked for outlived the picture it was asked for. diff --git a/src/tui/ui.rs b/src/tui/ui.rs index 0db8877..482e8ab 100644 --- a/src/tui/ui.rs +++ b/src/tui/ui.rs @@ -18,6 +18,9 @@ use crate::tui::app::{App, Mode, Tile}; const ACCENT: Color = Color::Rgb(0xE0, 0xA1, 0x62); const MUTED: Color = Color::Rgb(0x90, 0x96, 0xA0); +/// The gutter down the right of the grid: four cells for a year and one for the marker. +const RAIL: u16 = 5; + /// Works out where everything goes, including the holes the photographs are placed into. /// Called before drawing so the placement pass and the draw pass agree. pub fn layout(app: &mut App, area: Rect) { @@ -42,6 +45,7 @@ pub fn layout(app: &mut App, area: Rect) { }; app.grid_area = content; + app.rail_area = Rect::default(); app.tiles.clear(); if matches!(app.mode, Mode::Picker | Mode::PickerPath(_)) { @@ -61,7 +65,7 @@ pub fn layout(app: &mut App, area: Rect) { } if app.mode == Mode::Viewer { - if let Some(asset) = app.selected_asset() { + if let Some(id) = app.selected_id() { let inner = Rect { x: content.x + 1, y: content.y + 1, @@ -69,7 +73,7 @@ pub fn layout(app: &mut App, area: Rect) { height: content.height.saturating_sub(2), }; app.tiles.push(Tile { - id: asset.id.clone(), + id, inner, index: app.selected, }); @@ -80,6 +84,23 @@ pub fn layout(app: &mut App, area: Rect) { return; } + // The year rail only earns its gutter if what is left still fits a tile. + let content = if content.width > RAIL + app.tile_width { + app.rail_area = Rect { + x: content.x + content.width - RAIL, + y: content.y, + width: RAIL, + height: content.height, + }; + Rect { + width: content.width - RAIL, + ..content + } + } else { + content + }; + app.grid_area = content; + let columns = (content.width / app.tile_width.max(1)).max(1) as usize; app.columns = columns; let visible = (content.height / app.tile_height.max(1)).max(1) as usize; @@ -87,13 +108,13 @@ pub fn layout(app: &mut App, area: Rect) { for row in 0..visible { for column in 0..columns { let index = (app.scroll + row) * columns + column; - let Some(asset) = app.assets.get(index) else { + let Some(tile) = app.window.get(index) else { continue; }; let x = content.x + column as u16 * app.tile_width; let y = content.y + row as u16 * app.tile_height; app.tiles.push(Tile { - id: asset.id.clone(), + id: tile.id.clone(), // One cell of border all round, and the last row of the tile is the caption. inner: Rect { x: x + 1, @@ -128,6 +149,7 @@ pub fn draw(frame: &mut Frame, app: &App) { Mode::Viewer => draw_viewer(frame, app, chunks[1]), _ => { draw_grid(frame, app); + draw_rail(frame, app); if app.show_info { let split = Layout::default() .direction(Direction::Horizontal) @@ -144,11 +166,9 @@ fn draw_title(frame: &mut Frame, app: &App, area: Rect) { (Some(album), _) => format!("album · {}", album.name), (None, scope) => scope.label().to_string(), }; - let counted = match app.total { - Some(total) => format!("{total} photographs"), - None if app.cursor.is_some() => format!("{}+ loaded", app.assets.len()), - None => format!("{} photographs", app.assets.len()), - }; + // The buckets know the whole count before a single picture has been fetched, so this + // never has to say "so many loaded so far". + let counted = format!("{} photographs", app.total); let mut left = vec![ Span::styled( " imogen ", @@ -180,6 +200,7 @@ fn draw_title(frame: &mut Frame, app: &App, area: Rect) { fn draw_footer(frame: &mut Frame, app: &App, area: Rect) { let text = match &app.mode { Mode::Search(input) => format!(" search: {input}▏"), + Mode::JumpDate(input) => format!(" jump to: {input}▏"), Mode::PickerPath(input) => format!(" go to: {input}▏"), Mode::Picker => { let picker = app.picker.as_ref(); @@ -208,21 +229,47 @@ fn draw_footer(frame: &mut Frame, app: &App, area: Rect) { ), _ => match &app.status { Some(status) => format!(" {status}"), - None => match app.selected_asset() { - Some(asset) => format!( - " {} · {} · {}{}", - output::truncate(&asset.original_filename, 40), - output::date(&asset.captured_at), - output::bytes(asset.size_bytes), - if asset.favorite { " ★" } else { "" } - ), - None => " nothing here".to_string(), + // The filename and the size come from the whole record, which is only read + // for the photograph being looked at; the day comes from the tile, which is + // always there. Waiting for the record before saying anything would leave the + // footer blank for every photograph somebody merely scrolled past. + None => match app.selected_tile() { + Some(tile) => { + let named = match app.detail() { + Some(asset) => format!( + "{} · {} · ", + output::truncate(&asset.original_filename, 40), + output::bytes(asset.size_bytes) + ), + None => String::new(), + }; + format!( + " {named}{} · {} of {}{}", + output::date(&tile.captured_at), + app.selected + 1, + app.total, + if tile.favorite { " ★" } else { "" } + ) + } + // The buckets know what day this is even before its tiles arrive, so a + // stretch still on its way says where it is rather than "nothing here" — + // which is what an empty library says, and means something else. + None => match app.date_at_index(app.selected) { + Some(date) => format!( + " {} · {} of {}", + output::date(&format!("{date}T00:00:00.000Z")), + app.selected + 1, + app.total + ), + None if app.loading => " loading…".to_string(), + None => " nothing here".to_string(), + }, }, }, }; let style = match &app.mode { - Mode::Search(_) | Mode::PickerPath(_) => Style::default().fg(ACCENT), + Mode::Search(_) | Mode::PickerPath(_) | Mode::JumpDate(_) => Style::default().fg(ACCENT), Mode::Picker => Style::default().fg(MUTED), Mode::Confirm { .. } => Style::default().fg(Color::Rgb(0xE0, 0x7A, 0x5F)), _ if app.uploading() => Style::default().fg(ACCENT), @@ -247,7 +294,7 @@ fn draw_footer(frame: &mut Frame, app: &App, area: Rect) { } fn draw_grid(frame: &mut Frame, app: &App) { - if app.assets.is_empty() { + if app.total == 0 { let message = if app.loading { "Loading…" } else if app.query.is_empty() { @@ -265,8 +312,23 @@ fn draw_grid(frame: &mut Frame, app: &App) { return; } + if app.tiles.is_empty() { + // The spine says there are photographs here; their tiles are still on their way. + let where_at = app + .date_at_index(app.selected) + .map(|date| format!("Fetching {date}…")) + .unwrap_or_else(|| "Fetching…".to_string()); + frame.render_widget( + Paragraph::new(where_at) + .style(Style::default().fg(MUTED)) + .alignment(Alignment::Center), + centred(app.grid_area, 30, 1), + ); + return; + } + for tile in &app.tiles { - let Some(asset) = app.assets.get(tile.index) else { + let Some(held) = app.window.get(tile.index) else { continue; }; let selected = tile.index == app.selected; @@ -295,18 +357,18 @@ fn draw_grid(frame: &mut Frame, app: &App) { height: 1, }; let mut marks = String::new(); - if asset.favorite { + if held.favorite { marks.push('★'); } - if asset.r#type == AssetType::Video { + if held.r#type == AssetType::Video { marks.push('▶'); } - if asset.status != AssetStatus::Ready { + if held.status != AssetStatus::Ready { marks.push('·'); } let label = format!( "{}{}", - output::date(&asset.captured_at), + output::date(&held.captured_at), if marks.is_empty() { String::new() } else { @@ -327,11 +389,48 @@ fn draw_grid(frame: &mut Frame, app: &App) { } } +/// The year rail: where you are in twenty years, in five cells. +/// +/// The grid itself cannot say this — a screen of tiles looks the same in 2009 as in 2024 — +/// so the rail is the only thing on screen that answers "how far in am I". +fn draw_rail(frame: &mut Frame, app: &App) { + let area = app.rail_area; + if area.width == 0 || area.height == 0 || app.total == 0 { + return; + } + let here = app.rail_row(area.height); + let marks = app.year_marks(area.height); + + let lines: Vec = (0..area.height) + .map(|row| { + let year = marks + .iter() + .find(|(at, _)| *at == row) + .map(|(_, year)| year.as_str()) + .unwrap_or(""); + let style = if row == here { + Style::default().fg(ACCENT).add_modifier(Modifier::BOLD) + } else { + Style::default().fg(MUTED) + }; + Line::from(Span::styled( + format!("{}{year:>4}", if row == here { "\u{203a}" } else { " " }), + style, + )) + }) + .collect(); + + frame.render_widget(Paragraph::new(lines), area); +} + fn draw_viewer(frame: &mut Frame, app: &App, area: Rect) { - let title = app - .selected_asset() - .map(|asset| format!(" {} ", asset.original_filename)) - .unwrap_or_default(); + // The filename once the record has arrived, the day until then: a viewer with no + // title at all reads as a viewer that has failed. + let title = match (app.detail(), app.selected_tile()) { + (Some(asset), _) => format!(" {} ", asset.original_filename), + (None, Some(tile)) => format!(" {} ", output::date(&tile.captured_at)), + (None, None) => String::new(), + }; frame.render_widget( Block::default() .borders(Borders::ALL) @@ -340,8 +439,8 @@ fn draw_viewer(frame: &mut Frame, app: &App, area: Rect) { area, ); let showing = app - .selected_asset() - .map(|asset| app.preview_for(&asset.id).is_some()) + .selected_id() + .map(|id| app.preview_for(&id).is_some()) .unwrap_or(false); if !showing { frame.render_widget( @@ -517,7 +616,7 @@ fn draw_albums(frame: &mut Frame, app: &App, area: Rect) { } fn draw_info(frame: &mut Frame, app: &App, area: Rect) { - let Some(asset) = app.selected_asset() else { + let Some(asset) = app.detail() else { return; }; let mut lines = vec![ @@ -592,7 +691,9 @@ fn draw_help(frame: &mut Frame, area: Rect, picking: bool) { ("a", "albums"), ("u", "pick files to upload"), ("1 2 3 4", "library · favourites · archive · trash"), - ("g G", "first · last"), + ("g", "jump to a date — 2011, aug 2011, 2011-08-14"), + ("[ ]", "a year older · a year newer"), + ("home end", "first · last"), ("R", "reload"), ("?", "this"), ("q", "quit"), @@ -655,6 +756,7 @@ fn centred(area: Rect, width: u16, height: u16) -> Rect { mod tests { use super::*; use crate::tui::picker::Picker; + use imogen_sdk::{AssetStatus, AssetType}; use ratatui::backend::TestBackend; use ratatui::Terminal; @@ -780,6 +882,108 @@ mod tests { assert!(render(&mut app, 100, 20).contains(&name)); } + /// Not an assertion — prints the grid so the rail can be looked at. + #[test] + #[ignore = "for looking at, not for CI"] + fn show_grid() { + let mut app = browsing(); + app.selected = 500; + app.keep_selection_visible(); + println!("{}", render(&mut app, 90, 24)); + } + + /// A grid over twenty years, so the rail has something to say. + fn browsing() -> App { + let mut app = App::new(); + app.buckets = (0..20) + .map(|n| imogen_sdk::TimelineBucket { + date: format!("{}-06-01", 2024 - n), + count: 40, + cover_asset_id: None, + }) + .collect(); + app.recount(); + app.periods.insert( + "2024-06".into(), + (0..40) + .map(|n| imogen_sdk::TimelineTile { + id: format!("a{n}"), + captured_at: "2024-06-01T09:30:00.000Z".into(), + width: None, + height: None, + r#type: AssetType::Image, + status: AssetStatus::Ready, + favorite: false, + duration: None, + placeholder_color: None, + live_photo_video_id: None, + }) + .collect(), + ); + app.rebuild_window(); + app + } + + /// The grid itself looks the same in 2009 as in 2024. The rail is the only thing on + /// screen that says how far into twenty years you are. + #[test] + fn the_year_rail_says_where_in_the_library_you_are() { + let mut app = browsing(); + let screen = render(&mut app, 100, 30); + assert!(screen.contains("2024"), "{screen}"); + assert!( + screen.contains("›"), + "the cursor has a place on the rail: {screen}" + ); + // And the rail is in its gutter, not over the tiles. + assert!(app.rail_area.width > 0); + assert_eq!( + app.rail_area.x + app.rail_area.width, + app.grid_area.x + app.grid_area.width + RAIL + ); + } + + /// A narrow window keeps the photographs and loses the rail, rather than the other way + /// round. + #[test] + fn a_window_too_narrow_for_both_keeps_the_photographs() { + let mut app = browsing(); + render(&mut app, 22, 30); + assert_eq!(app.rail_area.width, 0); + assert!(app.columns >= 1); + } + + /// "Nothing here" is what an empty library says. A stretch whose tiles have not + /// arrived is a different thing, and saying the wrong one of the two reads as a + /// browser that has broken. + #[test] + fn a_stretch_still_on_its_way_says_where_it_is_not_that_there_is_nothing() { + let mut app = browsing(); + app.selected = 500; + app.keep_selection_visible(); + let screen = render(&mut app, 90, 24); + assert!(!screen.contains("nothing here"), "{screen}"); + assert!(screen.contains("2012"), "{screen}"); + assert!(screen.contains("501 of 800"), "{screen}"); + } + + #[test] + fn the_jump_prompt_reads_like_the_search_prompt() { + let mut app = browsing(); + app.mode = Mode::JumpDate("aug 2011".into()); + let screen = render(&mut app, 100, 30); + assert!(screen.contains("jump to: aug 2011"), "{screen}"); + } + + /// The buckets know the whole count before a picture has been fetched, so the header + /// never has to hedge with "so many loaded so far". + #[test] + fn the_header_counts_the_whole_library_not_what_has_arrived() { + let mut app = browsing(); + let screen = render(&mut app, 100, 30); + assert!(screen.contains("800 photographs"), "{screen}"); + } + #[test] fn an_upload_in_progress_outranks_the_last_message() { let mut app = App::new(); From 4314ca3be71f356ae73819e4d859e5bdf33103da Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:10:40 -0400 Subject: [PATCH 3/6] Drop an answer that belongs to results already thrown away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing drains the in-flight requests when the scope changes, so a bucket fetched under the library's filter could land after the trash's spine had replaced it. Filed, it put library photographs at trash indices — the wrong pictures, at plausible places, with nothing on screen to say so. Worse, the period then counted as held whole, so the correct tiles were never asked for and the wrong ones stayed until the viewport moved off. Every request now carries the epoch it was issued under, and an answer stamped with a stale one is dropped before it touches anything — including the in-flight mark, which belongs to the request the current results have out for the same period. `imogen timeline --before 2011` also hid all of 2011. It compared bucket dates against the bound as raw strings, and "2011-08-15" <= "2011" is false. The bounds now widen the way every other filter's do, so a bound less than a whole day means the whole of that period. And the thumbnail cache is arrival-ordered, which for a grid is already recency — thumbnails arrive as they scroll into view. It said it was doing something cleverer than that and was not. Co-Authored-By: Claude with claude-opus-5[1m] --- README.md | 30 ++++++-- src/commands/assets.rs | 121 ++++++++++++++++++++++++++++-- src/tui/app.rs | 18 ++++- src/tui/mod.rs | 165 +++++++++++++++++++++++++++++++++++++---- 4 files changed, 306 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index b557f9f..cf5ad27 100644 --- a/README.md +++ b/README.md @@ -169,23 +169,43 @@ With no arguments, imogen draws your library. ``` ┌ imogen · library ────────────────────────── 12,431 photographs ─┐ -│ ┌────────┐ ┌────────┐ ┌────────┐ ┌────────┐ │ -│ │ photo │ │ photo │ │ photo │ │ photo │ │ -│ │2019-07 │ │2019-07 │ │2019-07★│ │2019-07 │ │ -│ └────────┘ └────────┘ └────────┘ └────────┘ │ +│ ┌────────┐ ┌────────┐ ┌────────┐ ┌────────┐ 2024 │ +│ │ photo │ │ photo │ │ photo │ │ photo │ 2022 │ +│ │2019-07 │ │2019-07 │ │2019-07★│ │2019-07 │ ›2019 │ +│ └────────┘ └────────┘ └────────┘ └────────┘ 2016 │ └ harbour.jpg · 2019-07-14 · 2.4 MiB ★ ? help q quit ─────┘ ``` +The rail down the right edge is the whole library, however many years of it there are, with +`›` marking where you are. imogen holds the stretch you are looking at rather than +everything you have scrolled past, so the far end costs the same as the near one. + | | | |---|---| | `↑ ↓ ← →` `h j k l` | move | | `enter` · `escape` | look at it · back | | `/` | search | +| `g` | jump to a date | +| `[` · `]` | a year older · a year newer | +| `home` · `end` | first · last | | `f` · `e` · `d` · `r` | favourite · archive · trash · restore | | `i` · `a` | details · albums | | `u` | pick files to upload | | `1` `2` `3` `4` | library · favourites · archive · trash | -| `g` `G` · `R` · `?` | first · last · reload · keys | +| `R` · `?` | reload · keys | + +### Going to a date + +`g` asks where you want to be, and takes the same vocabulary as `--after` does, plus month +names: + +``` + jump to: aug 2011▏ +``` + +`2011`, `aug 2011`, `august 2011` and `2011-08-14` all work. If the day you name has no +photographs — and on a library of any age most days do not — it lands on the nearest day +that does, however many years away that is, and says where it put you. ### Picking files diff --git a/src/commands/assets.rs b/src/commands/assets.rs index cd231a7..0151c98 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -8,6 +8,7 @@ use serde_json::json; use crate::cli::{EditArgs, ListArgs, RestoreArgs, SearchArgs, ShowArgs, TrashArgs}; use crate::context::Context; +use crate::dates; use crate::output::{self, GREEN, RED, YELLOW}; pub async fn list(ctx: &Context, args: &ListArgs) -> Result<()> { @@ -288,18 +289,45 @@ pub async fn stats(ctx: &Context) -> Result<()> { Ok(()) } +/// The days inside a pair of bounds, widened the way every other filter widens them. +/// +/// A bucket's date is a whole day, and a bound may be less than one: `--before 2011` means +/// the end of 2011, not the string "2011". Compared raw, `"2011-08-15" <= "2011"` is false +/// and the command answers with nothing — it hid whole years in silence, which on a +/// twenty-year library is the one answer that looks like a working command. +fn within( + buckets: Vec, + after: Option<&str>, + before: Option<&str>, +) -> Vec { + let from = after.map(|bound| day_of(&dates::to_start_of_day(bound))); + let until = before.map(|bound| day_of(&dates::to_end_of_day(bound))); + buckets + .into_iter() + .filter(|bucket| { + from.as_deref() + .is_none_or(|from| bucket.date.as_str() >= from) + }) + .filter(|bucket| { + until + .as_deref() + .is_none_or(|until| bucket.date.as_str() <= until) + }) + .collect() +} + +/// The `YYYY-MM-DD` out of an instant, which is the grain a day bucket is keyed by. +fn day_of(timestamp: &str) -> String { + timestamp.split('T').next().unwrap_or_default().to_string() +} + pub async fn timeline(ctx: &Context, after: Option<&str>, before: Option<&str>) -> Result<()> { let timeline = ctx .client .assets .timeline(&TimelineQuery::default()) .await?; - let buckets: Vec<_> = timeline - .buckets - .into_iter() - .filter(|bucket| after.map(|a| bucket.date.as_str() >= a).unwrap_or(true)) - .filter(|bucket| before.map(|b| bucket.date.as_str() <= b).unwrap_or(true)) - .collect(); + let buckets = within(timeline.buckets, after, before); if ctx.out.is_json() { return ctx.out.json(&json!({ "buckets": buckets })); @@ -502,3 +530,84 @@ pub async fn restore(ctx: &Context, args: &RestoreArgs) -> Result<()> { )); Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use imogen_sdk::TimelineBucket; + + fn days(dates: &[&str]) -> Vec { + dates + .iter() + .map(|date| TimelineBucket { + date: (*date).into(), + count: 1, + cover_asset_id: None, + }) + .collect() + } + + fn kept(after: Option<&str>, before: Option<&str>) -> Vec { + within( + days(&[ + "2012-01-04", + "2011-12-31", + "2011-08-15", + "2011-01-01", + "2010-12-31", + ]), + after, + before, + ) + .into_iter() + .map(|bucket| bucket.date) + .collect() + } + + /// A bound less than a whole day means the whole of that period. Compared raw, + /// `"2011-08-15" <= "2011"` is false, so `--before 2011` answered with nothing at all + /// and looked like a library with no photographs in it. + #[test] + fn a_bare_year_covers_the_whole_year() { + assert_eq!( + kept(None, Some("2011")), + vec!["2011-12-31", "2011-08-15", "2011-01-01", "2010-12-31"] + ); + assert_eq!( + kept(Some("2011"), None), + vec!["2012-01-04", "2011-12-31", "2011-08-15", "2011-01-01"] + ); + assert_eq!( + kept(Some("2011"), Some("2011")), + vec!["2011-12-31", "2011-08-15", "2011-01-01"], + "both bounds together are the whole year and nothing else" + ); + } + + /// And the same for a bare month, whose last day is not the same in every month. + #[test] + fn a_bare_month_covers_the_whole_month() { + assert_eq!( + kept(None, Some("2011-08")), + vec!["2011-08-15", "2011-01-01", "2010-12-31"] + ); + assert_eq!(kept(Some("2011-08"), Some("2011-08")), vec!["2011-08-15"]); + assert_eq!( + within(days(&["2011-02-28"]), None, Some("2011-02")) + .into_iter() + .map(|bucket| bucket.date) + .collect::>(), + vec!["2011-02-28"], + "February ends on its own last day" + ); + } + + #[test] + fn a_whole_day_is_still_taken_at_its_word() { + assert_eq!( + kept(Some("2011-08-15"), Some("2011-08-15")), + vec!["2011-08-15"] + ); + assert_eq!(kept(None, None).len(), 5, "no bounds keeps everything"); + } +} diff --git a/src/tui/app.rs b/src/tui/app.rs index 53dfa21..d1860a0 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -115,6 +115,12 @@ pub struct App { pub window: TileWindow, /// What the viewport wanted last pass, so the window is only rebuilt when it moves. pub held: Vec, + /// Which set of results is the current one. Every request is stamped with the epoch it + /// was issued under, and an answer that arrives after a reload carries a stale stamp. + /// Nothing drains the in-flight requests when the scope changes, so without this a + /// bucket fetched under the library's filter can be filed under the trash's spine — + /// the wrong photographs, at plausible indices, with nothing on screen to say so. + pub epoch: u64, /// The full record of the selected photograph — everything a tile deliberately does /// not carry. Fetched when the viewer or the details panel needs it, not per keypress. pub detail: Option, @@ -201,6 +207,7 @@ impl App { period_inflight: HashSet::new(), window: TileWindow::default(), held: Vec::new(), + epoch: 0, detail: None, detail_asked: None, selected: 0, @@ -279,6 +286,8 @@ impl App { /// Forgets the current results without forgetting the pictures already decoded: the /// same photograph in a different scope does not need fetching twice. pub fn reset_results(&mut self) { + // Everything already asked for belongs to the results being thrown away. + self.epoch = self.epoch.wrapping_add(1); self.buckets.clear(); self.total = 0; self.periods.clear(); @@ -672,8 +681,13 @@ impl App { /// scroll. And a thumbnail the grid is drawing right now is never the one chosen, /// because evicting what is on screen is a hole that refills only to be evicted again. pub fn remember_thumbnail(&mut self, id: String) { - // Looking at it again buys it time, rather than leaving it where it first landed. - self.thumb_order.retain(|held| *held != id); + // Arrival order, exactly as `preview_order` is, and for a grid that is already + // recency: thumbnails are fetched as they scroll into view, so the order they + // arrived in and the order they were last wanted in are the same order. An id + // already held keeps its place rather than gaining a second one. + if self.thumb_order.contains(&id) { + return; + } self.thumb_order.push_back(id); let on_screen: HashSet = self.visible_ids().into_iter().collect(); diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 28a9c89..aae9442 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -41,11 +41,12 @@ enum Loaded { Thumbnail(String, Result>), Preview(String, Result>), /// The shape of the whole library: one entry a day. Small enough to hold for twenty - /// years, which is the whole point of it. - Spine(Result>), + /// years, which is the whole point of it. Carries the epoch it was asked under. + Spine(u64, Result>), /// One `YYYY-MM` of tiles, named so a slow answer is filed under the period it was - /// asked for rather than under wherever the viewport has since moved. - Bucket(String, Result), + /// asked for rather than under wherever the viewport has since moved, and stamped so + /// an answer for results that no longer exist is dropped rather than believed. + Bucket(u64, String, Result), Albums(Result>), /// Boxed: an upload result carries a whole `Asset`, which would otherwise make /// every variant of this enum as large as the largest one. @@ -98,7 +99,7 @@ async fn event_loop(ctx: &Context) -> Result<()> { poll.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay); let mut work: FuturesUnordered> = FuturesUnordered::new(); - work.push(Box::pin(load_spine(ctx, spine_query(&app)))); + work.push(Box::pin(load_spine(ctx, spine_query(&app), app.epoch))); app.loading = true; work.push(Box::pin(load_albums(ctx))); @@ -139,6 +140,7 @@ async fn event_loop(ctx: &Context) -> Result<()> { work.push(Box::pin(load_bucket( ctx, bucket_query(period, &app, cursor), + app.epoch, ))); } } @@ -242,8 +244,9 @@ fn bucket_query(period: &str, app: &App, cursor: Option) -> TimelineBuck } } -async fn load_spine(ctx: &Context, query: TimelineQuery) -> Loaded { +async fn load_spine(ctx: &Context, query: TimelineQuery, epoch: u64) -> Loaded { Loaded::Spine( + epoch, ctx.client .assets .timeline(&query) @@ -253,7 +256,7 @@ async fn load_spine(ctx: &Context, query: TimelineQuery) -> Loaded { ) } -async fn load_bucket(ctx: &Context, query: TimelineBucketQuery) -> Loaded { +async fn load_bucket(ctx: &Context, query: TimelineBucketQuery, epoch: u64) -> Loaded { let period = query.period.clone(); let page = ctx .client @@ -261,7 +264,7 @@ async fn load_bucket(ctx: &Context, query: TimelineBucketQuery) -> Loaded { .timeline_bucket(&query) .await .map_err(Into::into); - Loaded::Bucket(period, page) + Loaded::Bucket(epoch, period, page) } async fn load_asset(ctx: &Context, id: String) -> Loaded { @@ -431,7 +434,10 @@ fn absorb(app: &mut App, loaded: Loaded) { app.preview_inflight.remove(&id); app.note(format!("Could not load: {error}")); } - Loaded::Spine(Ok(buckets)) => { + // A spine for results that have been thrown away is dropped whole. Not even + // `loading` is cleared by it: the spine that replaced it is still on its way. + Loaded::Spine(epoch, _) if epoch != app.epoch => {} + Loaded::Spine(_, Ok(buckets)) => { app.loading = false; app.buckets = buckets; app.recount(); @@ -441,11 +447,16 @@ fn absorb(app: &mut App, loaded: Loaded) { app.rebuild_window(); app.images_dirty = true; } - Loaded::Spine(Err(error)) => { + Loaded::Spine(_, Err(error)) => { app.loading = false; app.note(format!("Could not load the timeline: {error}")); } - Loaded::Bucket(period, Ok(page)) => { + // Tiles fetched under a filter that is no longer the one on screen. Dropped before + // anything is touched — clearing the in-flight mark here would cancel the request + // the *current* results have out for the same period, and then the correct tiles + // would never arrive either, because a period held whole is never asked for again. + Loaded::Bucket(epoch, _, _) if epoch != app.epoch => {} + Loaded::Bucket(_, period, Ok(page)) => { app.period_inflight.remove(&period); match page.next_cursor { Some(cursor) => { @@ -459,7 +470,7 @@ fn absorb(app: &mut App, loaded: Loaded) { app.rebuild_window(); app.images_dirty = true; } - Loaded::Bucket(period, Err(error)) => { + Loaded::Bucket(_, period, Err(error)) => { app.period_inflight.remove(&period); app.note(format!("Could not load photographs: {error}")); } @@ -957,7 +968,7 @@ fn reload<'a>(ctx: &'a Context, app: &mut App, work: &mut FuturesUnordered imogen_sdk::TilePage { + imogen_sdk::TilePage { + items: ids + .iter() + .map(|id| tile(id, "2011-08-14T00:00:00.000Z")) + .collect(), + next_cursor: None, + total: None, + } + } + + /// Nothing drains the in-flight requests when the scope changes, so an answer fetched + /// under the library's filter can land after the trash's spine has replaced it. Filed, + /// it would put library photographs at trash indices — the wrong pictures, at plausible + /// places, with nothing on screen to say so. That is the one outcome a photo library + /// must not have. + #[test] + fn a_bucket_fetched_under_the_old_scope_is_dropped_not_shown() { + let mut app = App::new(); + app.buckets = vec![bucket("2011-08-14", 2)]; + app.recount(); + let stale = app.epoch; + app.period_inflight.insert("2011-08".into()); + + // The scope changes: results thrown away, a new spine asked for and arrived. + app.reset_results(); + assert_ne!(app.epoch, stale, "a reload is a new set of results"); + app.scope = Scope::Trash; + let now = app.epoch; + absorb( + &mut app, + Loaded::Spine(now, Ok(vec![bucket("2011-08-14", 2)])), + ); + // The current results have their own request out for the same period. + app.period_inflight.insert("2011-08".into()); + + // Now the library's answer turns up. + absorb( + &mut app, + Loaded::Bucket( + stale, + "2011-08".into(), + Ok(page(&["library-a", "library-b"])), + ), + ); + + assert!( + app.periods.is_empty(), + "the wrong scope's tiles must not be filed" + ); + assert!( + app.window.tiles.is_empty(), + "and so must not be placed in the window" + ); + // And the mark for the request the *current* results have out is untouched — else + // the right tiles would never arrive either, because a period held whole is never + // asked for a second time. + assert!(app.period_inflight.contains("2011-08")); + assert_eq!(app.period_wanted("2011-08"), None, "still in flight"); + + // The current scope's own answer is filed as normal. + let now = app.epoch; + absorb( + &mut app, + Loaded::Bucket(now, "2011-08".into(), Ok(page(&["trash-a", "trash-b"]))), + ); + assert_eq!( + app.window.get(0).map(|tile| tile.id.as_str()), + Some("trash-a") + ); + } + + /// The same for the spine. A stale one must not even clear `loading`: the spine that + /// replaced it has not arrived yet, and saying otherwise reads as a finished load. + #[test] + fn a_spine_for_results_that_were_thrown_away_is_dropped_whole() { + let mut app = App::new(); + let stale = app.epoch; + app.reset_results(); + app.loading = true; + + absorb( + &mut app, + Loaded::Spine(stale, Ok(vec![bucket("2011-08-14", 500)])), + ); + assert_eq!(app.total, 0, "the old library's shape must not be adopted"); + assert!(app.buckets.is_empty()); + assert!( + app.loading, + "the spine that replaced it is still on its way" + ); + + let now = app.epoch; + absorb( + &mut app, + Loaded::Spine(now, Ok(vec![bucket("2011-08-14", 2)])), + ); + assert_eq!(app.total, 2); + assert!(!app.loading); + } + /// The reported bug, through the real request decision and the real absorb: view one, /// move to the next, come back — and the first must still appear. It did not, because /// the note that it had been asked for outlived the picture it was asked for. From abba75cae1c01c2e0bb9e3a2c133ee5b5c5564f1 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:26:01 -0400 Subject: [PATCH 4/6] File a page only if it answers what the period is asking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A month over the server's five-thousand default arrives in more than one answer, and a scanned-archive import can land forty thousand photographs on one date. Scroll away from such a month mid-fetch and its second page came back to a period the viewport had already forgotten — where, appended to nothing, it became that period's first page: the third photograph of August drawn at August's first index, the month then counting as held whole and so never asked for again. Wrong photographs at plausible places, silently and permanently, reached without any scope change, so the epoch could not see it. A period's tiles are only meaningful as a complete prefix from its first page, so the two kinds of page are not interchangeable. A first page always answers something — it starts the prefix. A continuation answers "what follows this cursor", which is only a question while the period is still asking it, and `period_more` is already where that question is written down. So a page now carries the cursor it was asked from, and is filed only when that matches what the period wants. A dropped page leaves the period with no tiles and no cursor, which is exactly what "not held" looks like, so the next pass asks again from the first page. The forget still leaves `period_inflight` alone. That is what keeps at most one request out per period, which is in turn what lets an answer clear the mark without having to wonder whose request it belongs to. Clearing it there would un-strand the period by asking for it twice. Co-Authored-By: Claude with claude-opus-5[1m] --- src/tui/app.rs | 26 ++++++ src/tui/mod.rs | 240 ++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 256 insertions(+), 10 deletions(-) diff --git a/src/tui/app.rs b/src/tui/app.rs index d1860a0..d6c85f5 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -438,11 +438,37 @@ impl App { /// Drops the periods the viewport has moved away from. This is the other half of the /// bound on memory: the thumbnail cache caps the pictures, this caps the tiles. + /// + /// `period_inflight` is deliberately left alone. It is what makes "at most one request + /// is ever out for a period" true, and that in turn is what lets an answer clear the + /// mark without having to wonder whose request it belongs to. Forgetting a period does + /// not un-send the request already out for it; it only stops wanting the answer, which + /// is [`App::accepts_page`]'s job rather than this one's. pub fn forget_periods_outside(&mut self, keep: &[String]) { self.periods.retain(|period, _| keep.contains(period)); self.period_more.retain(|period, _| keep.contains(period)); } + /// Whether a page answers the question this period is currently asking. + /// + /// A period's tiles are only meaningful as a complete prefix from its first page, so + /// the two kinds of page are not interchangeable. A first page always answers + /// something — it *starts* the prefix. A continuation answers "what follows this + /// cursor", which is only a question while the period is still asking it, and + /// `period_more` is where that question is written down. + /// + /// Without this, a month big enough to paginate that the viewport scrolled away from + /// mid-fetch would take its second page as its first: the third photograph of August + /// drawn at August's first index, the month then counting as held whole, and so never + /// asked for again. Wrong photographs at plausible places, silently and permanently — + /// the same shape as a stale scope's answer, reached without any scope change. + pub fn accepts_page(&self, period: &str, asked_from: Option<&str>) -> bool { + match asked_from { + None => true, + Some(cursor) => self.period_more.get(period).map(String::as_str) == Some(cursor), + } + } + /// Lays the held periods end to end into the run of tiles the grid indexes into. /// /// Stops at the first hole. Two periods with a third still on its way are not diff --git a/src/tui/mod.rs b/src/tui/mod.rs index aae9442..0644585 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -43,10 +43,11 @@ enum Loaded { /// The shape of the whole library: one entry a day. Small enough to hold for twenty /// years, which is the whole point of it. Carries the epoch it was asked under. Spine(u64, Result>), - /// One `YYYY-MM` of tiles, named so a slow answer is filed under the period it was - /// asked for rather than under wherever the viewport has since moved, and stamped so - /// an answer for results that no longer exist is dropped rather than believed. - Bucket(u64, String, Result), + /// One `YYYY-MM` of tiles: the epoch it was asked under, the period it belongs to, the + /// cursor it was asked from (`None` for a first page), and the answer. The period and + /// the cursor together are the question it answers, so an answer to a question nobody + /// is asking any more can be told apart from one that is still wanted. + Bucket(u64, String, Option, Result), Albums(Result>), /// Boxed: an upload result carries a whole `Asset`, which would otherwise make /// every variant of this enum as large as the largest one. @@ -258,13 +259,14 @@ async fn load_spine(ctx: &Context, query: TimelineQuery, epoch: u64) -> Loaded { async fn load_bucket(ctx: &Context, query: TimelineBucketQuery, epoch: u64) -> Loaded { let period = query.period.clone(); + let asked_from = query.cursor.clone(); let page = ctx .client .assets .timeline_bucket(&query) .await .map_err(Into::into); - Loaded::Bucket(epoch, period, page) + Loaded::Bucket(epoch, period, asked_from, page) } async fn load_asset(ctx: &Context, id: String) -> Loaded { @@ -455,9 +457,20 @@ fn absorb(app: &mut App, loaded: Loaded) { // anything is touched — clearing the in-flight mark here would cancel the request // the *current* results have out for the same period, and then the correct tiles // would never arrive either, because a period held whole is never asked for again. - Loaded::Bucket(epoch, _, _) if epoch != app.epoch => {} - Loaded::Bucket(_, period, Ok(page)) => { + Loaded::Bucket(epoch, _, _, _) if epoch != app.epoch => {} + Loaded::Bucket(_, period, asked_from, Ok(page)) => { + // The request is over either way, and the mark can only be this request's own: + // at most one is ever out for a period, which is why the forget leaves it be. app.period_inflight.remove(&period); + + if !app.accepts_page(&period, asked_from.as_deref()) { + // A continuation of a period the viewport has since scrolled away from. + // Dropped, which leaves that period with no tiles and no cursor — exactly + // what "not held at all" looks like — so the next pass asks again from the + // first page rather than treating page two as page one. + return; + } + match page.next_cursor { Some(cursor) => { app.period_more.insert(period.clone(), cursor); @@ -466,11 +479,19 @@ fn absorb(app: &mut App, loaded: Loaded) { app.period_more.remove(&period); } } - app.periods.entry(period).or_default().extend(page.items); + match asked_from { + // A first page starts the prefix rather than adding to one. + None => { + app.periods.insert(period, page.items); + } + Some(_) => { + app.periods.entry(period).or_default().extend(page.items); + } + } app.rebuild_window(); app.images_dirty = true; } - Loaded::Bucket(_, period, Err(error)) => { + Loaded::Bucket(_, period, _, Err(error)) => { app.period_inflight.remove(&period); app.note(format!("Could not load photographs: {error}")); } @@ -1500,6 +1521,7 @@ mod tests { Loaded::Bucket( epoch, period.clone(), + None, Ok(imogen_sdk::TilePage { items: (start..start + count) .map(|n| tile(&format!("a{n}"), "2011-01-01T00:00:00.000Z")) @@ -1652,6 +1674,198 @@ mod tests { assert_eq!(detail_wanted(&mut app).as_deref(), Some("a")); } + /// One page of a period, as the server would answer it. + fn part(ids: &[&str], next: Option<&str>) -> imogen_sdk::TilePage { + imogen_sdk::TilePage { + items: ids + .iter() + .map(|id| tile(id, "2011-08-14T00:00:00.000Z")) + .collect(), + next_cursor: next.map(str::to_string), + total: None, + } + } + + /// A month too big for one answer — a scanned-archive import can land forty thousand + /// photographs on one date — with its first page in and its second page out. + fn mid_pagination() -> App { + let mut app = App::new(); + app.buckets = vec![bucket("2011-08-14", 4)]; + app.recount(); + let epoch = app.epoch; + absorb( + &mut app, + Loaded::Bucket( + epoch, + "2011-08".into(), + None, + Ok(part(&["a0", "a1"], Some("cursor-1"))), + ), + ); + assert_eq!( + app.period_wanted("2011-08"), + Some(Some("cursor-1".into())), + "the month is not held whole yet" + ); + app.period_inflight.insert("2011-08".into()); + app + } + + /// The residual the epoch could never see, because it needs no scope change. Scroll + /// away from a month mid-pagination and its second page arrives into a period that + /// has been forgotten — where, filed, it becomes that period's *first* page: the third + /// photograph of August drawn at August's first index, the month counting as held + /// whole, and so never asked for again. + #[test] + fn a_page_arriving_into_a_forgotten_period_is_dropped_not_taken_for_its_first() { + let mut app = mid_pagination(); + let epoch = app.epoch; + + // The viewport scrolls off August entirely. + app.forget_periods_outside(&[]); + + // And page two turns up. + absorb( + &mut app, + Loaded::Bucket( + epoch, + "2011-08".into(), + Some("cursor-1".into()), + Ok(part(&["a2", "a3"], None)), + ), + ); + + assert!( + app.periods.is_empty(), + "page two is not page one, and must not become it" + ); + app.rebuild_window(); + assert_ne!( + app.window.get(0).map(|tile| tile.id.as_str()), + Some("a2"), + "the third photograph must not be drawn at the month's first index" + ); + assert!(app.window.get(0).is_none()); + assert_eq!( + app.period_wanted("2011-08"), + Some(None), + "and the month is asked for again from its first page, not stranded" + ); + } + + /// The tempting wrong version of the fix is to clear `period_inflight` in the forget. + /// It un-strands the period, but it also breaks the one-request-at-a-time invariant + /// that lets an answer clear the mark without wondering whose request it is — so the + /// month gets asked for twice while the first answer is still coming. + #[test] + fn forgetting_a_period_does_not_ask_again_while_its_answer_is_still_coming() { + let mut app = mid_pagination(); + app.forget_periods_outside(&[]); + + assert_eq!( + app.period_wanted("2011-08"), + None, + "one request at a time per period: the answer is still on its way" + ); + + // Only once that answer has landed — and been dropped — is it asked for again. + let epoch = app.epoch; + absorb( + &mut app, + Loaded::Bucket( + epoch, + "2011-08".into(), + Some("cursor-1".into()), + Ok(part(&["a2", "a3"], None)), + ), + ); + assert_eq!(app.period_wanted("2011-08"), Some(None)); + } + + /// And the guard must not simply refuse every continuation: a page that answers what + /// the period is actually asking still extends it, and completes it. + #[test] + fn a_page_that_answers_what_the_period_is_asking_is_still_filed() { + let mut app = mid_pagination(); + let epoch = app.epoch; + absorb( + &mut app, + Loaded::Bucket( + epoch, + "2011-08".into(), + Some("cursor-1".into()), + Ok(part(&["a2", "a3"], None)), + ), + ); + + assert_eq!(app.periods["2011-08"].len(), 4, "both pages, in order"); + assert_eq!(app.window.get(0).map(|tile| tile.id.as_str()), Some("a0")); + assert_eq!(app.window.get(3).map(|tile| tile.id.as_str()), Some("a3")); + assert_eq!( + app.period_wanted("2011-08"), + None, + "and now the month really is held whole" + ); + } + + /// Belt and braces, and deliberately so. Nothing today can deliver a first page into a + /// period that already holds tiles — `period_wanted` only asks for one when the period + /// is held not at all — so this cannot happen as the code stands. It is pinned because + /// the reason it cannot happen lives in a different function from the one that would + /// suffer for it: a first page *starts* a period's tiles rather than adding to them, + /// and that should stay true by construction rather than by an argument about which + /// requests can be in flight. + #[test] + fn a_first_page_starts_a_period_rather_than_adding_to_it() { + let mut app = mid_pagination(); + let epoch = app.epoch; + absorb( + &mut app, + Loaded::Bucket( + epoch, + "2011-08".into(), + None, + Ok(part(&["a0", "a1"], Some("cursor-1"))), + ), + ); + assert_eq!( + app.periods["2011-08"].len(), + 2, + "two photographs, not the same two twice" + ); + } + + /// A continuation quoting a cursor the period has moved past is answering a question + /// asked two fetches ago. The cursor is the identity of the page wanted, not merely a + /// flag that some page is wanted. + #[test] + fn a_continuation_from_the_wrong_cursor_is_not_filed() { + let mut app = mid_pagination(); + let epoch = app.epoch; + absorb( + &mut app, + Loaded::Bucket( + epoch, + "2011-08".into(), + Some("some-older-cursor".into()), + Ok(part(&["wrong-a", "wrong-b"], None)), + ), + ); + assert_eq!( + app.periods["2011-08"], + vec![ + tile("a0", "2011-08-14T00:00:00.000Z"), + tile("a1", "2011-08-14T00:00:00.000Z") + ], + "page one is untouched" + ); + assert_eq!( + app.period_wanted("2011-08"), + Some(Some("cursor-1".into())), + "and the page actually wanted is still wanted" + ); + } + /// A page of tiles as the server would answer for one period. fn page(ids: &[&str]) -> imogen_sdk::TilePage { imogen_sdk::TilePage { @@ -1695,6 +1909,7 @@ mod tests { Loaded::Bucket( stale, "2011-08".into(), + None, Ok(page(&["library-a", "library-b"])), ), ); @@ -1717,7 +1932,12 @@ mod tests { let now = app.epoch; absorb( &mut app, - Loaded::Bucket(now, "2011-08".into(), Ok(page(&["trash-a", "trash-b"]))), + Loaded::Bucket( + now, + "2011-08".into(), + None, + Ok(page(&["trash-a", "trash-b"])), + ), ); assert_eq!( app.window.get(0).map(|tile| tile.id.as_str()), From 1e4db0c1d9168f2ec4afcdec04e26af7bf8420df Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:44:08 -0400 Subject: [PATCH 5/6] Let the spine decide how many photographs a period contributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rebuild_window` laid each held period's tiles down at the index the spine gave that period, but took as many tiles as the period happened to be holding. A period holding more than its bucket says therefore ran into the next period's indices: September's third photograph drawn under August's first, and everything after it one place along. That is reachable — a new spine is absorbed over tiles that were filed against the old one, so a period that shrank between the two leaves the window holding more than it has room for. The spine defines every index in the browser: `total`, `date_at_index`, `index_for_date` and `period_start` all derive from it. So a period contributes at most the photographs the spine says it has, and the window is a view onto spine-defined indices rather than a concatenation that happens to start in the right place. Pinned exhaustively rather than by example: over every combination of which periods are held, any tile the window answers with must be the tile at that global index, and must fall on the day `date_at_index` independently says that index falls on. Co-Authored-By: Claude with claude-opus-5[1m] --- src/tui/app.rs | 17 ++++++++ src/tui/mod.rs | 116 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+) diff --git a/src/tui/app.rs b/src/tui/app.rs index d6c85f5..88e39ba 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -387,6 +387,16 @@ impl App { None } + /// How many photographs the spine says a period holds. The spine is authoritative for + /// every index in the browser, so this is what a period is allowed to contribute. + pub fn period_count(&self, period: &str) -> usize { + self.buckets + .iter() + .filter(|bucket| bucket.date.len() >= 7 && &bucket.date[..7] == period) + .map(|bucket| bucket.count as usize) + .sum() + } + /// The periods the viewport covers, plus one either side so stepping down a row does /// not stall on a fetch. /// @@ -485,7 +495,14 @@ impl App { let mut base = None; let mut tiles: Vec = Vec::new(); for (start, period) in ordered { + // The spine is what defines an index, so a period contributes at most the + // photographs the spine says it has. A tile past that is not at any valid + // index of its own period, and letting it through would push every photograph + // after it one place along — the next period's first photograph drawn under + // the previous period's last index. A held period can outrun its bucket when a + // new spine arrives over tiles that were filed against the old one. let held = &self.periods[&period]; + let held = &held[..held.len().min(self.period_count(&period))]; match base { None => { base = Some(start); diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 0644585..a292045 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -1400,6 +1400,122 @@ mod tests { ); } + /// Exhaustive over which periods happen to be held: the window's whole contract is + /// that `get(i)` is the photograph at global index `i` — the same `i` the cursor, the + /// caption, the viewer and `date_at_index` all use. An off-by-one anywhere in the base + /// arithmetic draws every tile of a partially held window one place out. + #[test] + fn every_held_tile_answers_at_its_own_global_index() { + // Three periods, uneven, with September split over two days so a period is not a + // bucket. Global indices: Sep 0-1, Aug 2-4, Jul 5. + let spine = vec![ + bucket("2011-09-02", 1), + bucket("2011-09-01", 1), + bucket("2011-08-14", 3), + bucket("2011-07-01", 1), + ]; + let contents = [ + ("2011-09", vec![(0, "2011-09-02"), (1, "2011-09-01")]), + ( + "2011-08", + vec![(2, "2011-08-14"), (3, "2011-08-14"), (4, "2011-08-14")], + ), + ("2011-07", vec![(5, "2011-07-01")]), + ]; + + for held in 0..(1u8 << 3) { + let mut app = App::new(); + app.buckets = spine.clone(); + app.recount(); + assert_eq!(app.total, 6); + + for (slot, (period, items)) in contents.iter().enumerate() { + if held & (1 << slot) == 0 { + continue; + } + app.periods.insert( + (*period).into(), + items + .iter() + .map(|(index, day)| { + tile(&format!("t{index}"), &format!("{day}T00:00:00.000Z")) + }) + .collect(), + ); + } + app.rebuild_window(); + + for index in 0..app.total { + let Some(found) = app.window.get(index) else { + continue; + }; + assert_eq!( + found.id, + format!("t{index}"), + "held={held:04b}: index {index} drew {}", + found.id + ); + // And the window agrees with the timeline arithmetic, which walks the + // buckets independently of it. + assert_eq!( + found.captured_at.split('T').next().unwrap(), + app.date_at_index(index).unwrap(), + "held={held:04b}: index {index} is on a different day than the spine says" + ); + } + // The test cannot pass by holding nothing: whenever the newest period is held, + // its tiles must really be reachable. + if held & 1 != 0 { + assert_eq!(app.window.base, 0); + assert_eq!(app.window.get(0).map(|t| t.id.as_str()), Some("t0")); + } + } + } + + /// A period cannot be allowed to outrun its bucket. The spine defines every index in + /// the browser, so tiles beyond what the spine says a period holds are at no valid + /// index of their own — and left in, they push the next period's photographs one place + /// along, which is the wrong photograph at a plausible position all over again. + /// + /// Reachable when a new spine arrives over tiles filed against the old one: a period + /// that shrank between the two leaves the window holding more than it has room for. + #[test] + fn a_period_cannot_outrun_what_the_spine_says_it_holds() { + let mut app = App::new(); + app.buckets = vec![bucket("2011-09-02", 2), bucket("2011-08-14", 2)]; + app.recount(); + app.periods.insert( + "2011-09".into(), + vec![ + tile("t0", "2011-09-02T00:00:00.000Z"), + tile("t1", "2011-09-02T00:00:00.000Z"), + tile("spill", "2011-09-02T00:00:00.000Z"), + ], + ); + app.periods.insert( + "2011-08".into(), + vec![ + tile("t2", "2011-08-14T00:00:00.000Z"), + tile("t3", "2011-08-14T00:00:00.000Z"), + ], + ); + app.rebuild_window(); + + assert_eq!(app.window.get(0).map(|t| t.id.as_str()), Some("t0")); + assert_eq!(app.window.get(1).map(|t| t.id.as_str()), Some("t1")); + assert_ne!( + app.window.get(2).map(|t| t.id.as_str()), + Some("spill"), + "September's third photograph must not be drawn at August's first index" + ); + assert_eq!( + app.window.get(2).map(|t| t.id.as_str()), + Some("t2"), + "August's own photograph belongs there" + ); + assert_eq!(app.window.get(3).map(|t| t.id.as_str()), Some("t3")); + } + #[test] fn the_window_answers_for_the_indices_it_holds_and_no_others() { let mut app = App::new(); From 2b9e039b205f4af3759842a229ea3748b61692a8 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:58:04 -0400 Subject: [PATCH 6/6] Page the timeline for album and person listings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server caps `AlbumWithAssets.assets` and `PersonWithPhotos.photos` at sixty now — both became a cover sample, and the grid was moved to paging the timeline under an `albumId` or `personId` filter. The CLI was not, so `imogen album show holidays --ids` printed sixty ids where it used to print every one, and `imogen album show holidays --ids | xargs imogen trash` trashed sixty of thirty thousand photographs without saying so. The header made it worse rather than better: `photographs: 30000` sat directly above sixty rows. So both commands walk `GET /assets/timeline/bucket` instead — the day buckets say which months exist, and each month is followed by its cursor to the end. Nothing caps or limits: a list feeding `xargs` is either all of it or it is wrong. The header now counts what was actually fetched, so the number above the rows is the number of rows. For a person it also stops calling faces photographs: `photoCount` on the wire is the server's `faceCount`, which counts faces, so one photograph holding two of this person's faces counted twice. It is printed as "faces" beside a "photographs" count that is the rows, and the list column is headed FACES for the same reason. A tile is not an `Asset`. The bucket endpoint returns the lean projection, which carries no filename, no size, and no archived or trashed mark, so those columns are dropped from these two tables rather than printed empty — filling them would mean one request per photograph. `imogen ls --album --all` still walks `GET /assets` and prints the full row. The JSON changes shape with it: `album show` now emits `{album, items, count}` and `people show` `{person, items, count, faceCount}`, where `items` are tiles rather than assets. The old shape flattened the record and hung a capped `assets`/`photos` array off it, so nothing that read it was reading the truth anyway. Co-Authored-By: Claude with claude-opus-5[1m] --- src/commands/albums.rs | 51 ++++++---- src/commands/assets.rs | 218 ++++++++++++++++++++++++++++++++++++++++- src/commands/people.rs | 46 ++++++--- 3 files changed, 284 insertions(+), 31 deletions(-) diff --git a/src/commands/albums.rs b/src/commands/albums.rs index 3edd859..240e545 100644 --- a/src/commands/albums.rs +++ b/src/commands/albums.rs @@ -1,7 +1,7 @@ //! Albums, and the links that publish them. use anyhow::Result; -use imogen_sdk::{AlbumCreate, AlbumUpdate, AssetSelection}; +use imogen_sdk::{AlbumCreate, AlbumUpdate, AssetFilter, AssetSelection}; use serde_json::json; use crate::cli::{AlbumCommand, QueryArgs}; @@ -74,30 +74,47 @@ async fn list(ctx: &Context) -> Result<()> { Ok(()) } +/// The album, and every photograph in it. +/// +/// The assets on `GET /albums/{id}` are a capped cover sample now — sixty of them, +/// however many the album holds — so this pages the timeline under an `albumId` filter +/// instead. It has to be every one: `imogen album show holidays --ids | xargs imogen +/// trash` is a real pipeline, and a list quietly cut to sixty would trash sixty. +/// +/// The header counts what was actually fetched rather than the album's own +/// `assetCount`, so the number above the rows is the number of rows. The two agree in +/// the ordinary case — both leave out the trashed, the archived and the vaulted — and +/// where they would not, the honest number is the one belonging to the list printed. async fn show(ctx: &Context, reference: &str, ids_only: bool) -> Result<()> { let album = ctx.find_album(reference).await?; - let full = ctx.client.albums.get(&album.id).await?; + let tiles = crate::commands::assets::all_tiles( + ctx, + &AssetFilter { + album_id: Some(album.id.clone()), + ..Default::default() + }, + ) + .await?; + if ctx.out.is_json() { - return ctx.out.json(&full); + return ctx.out.json(&json!({ + "album": album, + "items": tiles, + "count": tiles.len(), + })); } if ids_only { - for asset in &full.assets { - ctx.out.value(&asset.id); - } - return Ok(()); + return crate::commands::assets::print_tiles(ctx, &tiles, true); } - ctx.out.heading(&full.album.name); + ctx.out.heading(&album.name); ctx.out.fields(&[ - ("id", full.album.id.clone()), - ( - "description", - full.album.description.clone().unwrap_or_default(), - ), - ("photographs", full.album.asset_count.to_string()), - ("created", crate::output::date(&full.album.created_at)), + ("id", album.id.clone()), + ("description", album.description.clone().unwrap_or_default()), + ("photographs", tiles.len().to_string()), + ("created", crate::output::date(&album.created_at)), ( "public link", - full.album + album .share_slug .as_ref() .map(|slug| format!("{}/share/{slug}", ctx.server)) @@ -105,7 +122,7 @@ async fn show(ctx: &Context, reference: &str, ids_only: bool) -> Result<()> { ), ]); ctx.out.line(""); - crate::commands::assets::print_assets(ctx, &full.assets, false) + crate::commands::assets::print_tiles(ctx, &tiles, false) } async fn create( diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 0151c98..f0af1d7 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -2,7 +2,8 @@ use anyhow::{bail, Result}; use imogen_sdk::{ - Asset, AssetSelection, AssetStatus, AssetType, AssetUpdate, GeoPoint, TimelineQuery, + Asset, AssetFilter, AssetSelection, AssetStatus, AssetType, AssetUpdate, GeoPoint, TilePage, + TimelineBucket, TimelineBucketQuery, TimelineQuery, TimelineTile, }; use serde_json::json; @@ -88,6 +89,138 @@ fn flags(ctx: &Context, asset: &Asset) -> String { marks.join(" ") } +/// Every photograph under a filter, to the last one. +/// +/// `GET /albums/{id}` and `GET /people/{id}` hand back a capped cover sample now, so +/// anything that has to enumerate a whole album or a whole person walks the timeline +/// instead — the same surface the web grid pages, under an `albumId` or `personId` +/// filter. The day buckets say which periods exist; each period is then followed by its +/// cursor to the end. Nothing here caps or limits: a short list feeding `xargs` is a +/// silently wrong list. +pub async fn all_tiles(ctx: &Context, filter: &AssetFilter) -> Result> { + let timeline = ctx + .client + .assets + .timeline(&TimelineQuery { + covers: None, + filter: filter.clone(), + }) + .await?; + collect_tiles(&timeline.buckets, |period, cursor| async move { + let page = ctx + .client + .assets + .timeline_bucket(&TimelineBucketQuery { + period, + cursor, + // Unset, so the server's own page size applies rather than a guess here. + limit: None, + filter: filter.clone(), + }) + .await?; + Ok(page) + }) + .await +} + +/// The walk itself, over whatever fetches a page — the network in earnest, a fake under +/// test. +async fn collect_tiles( + buckets: &[TimelineBucket], + mut fetch: F, +) -> Result> +where + F: FnMut(String, Option) -> Fut, + Fut: std::future::Future>, +{ + let mut tiles = Vec::new(); + for period in periods_of(buckets) { + let mut cursor = None; + loop { + let page = fetch(period.clone(), cursor).await?; + tiles.extend(page.items); + match page.next_cursor { + Some(next) => cursor = Some(next), + None => break, + } + } + } + Ok(tiles) +} + +/// The months the day buckets fall in, each once, in the order the timeline gave them. +/// +/// Months rather than days because the bucket endpoint takes either, and a month is one +/// round trip where its days would be thirty. +fn periods_of(buckets: &[TimelineBucket]) -> Vec { + let mut periods: Vec = Vec::new(); + for bucket in buckets { + let period: String = bucket.date.chars().take(7).collect(); + if !periods.contains(&period) { + periods.push(period); + } + } + periods +} + +/// The rows a timeline tile can fill. +/// +/// A tile is the grid's lean projection, and it carries no filename, no size, and no +/// archived or trashed mark. Those columns are left out rather than printed empty: +/// hydrating a whole album into `Asset`s to fill them would be one request per +/// photograph. `imogen ls --album --all` still walks `GET /assets` and prints the +/// full row for anyone who wants it. +pub fn print_tiles(ctx: &Context, tiles: &[TimelineTile], ids_only: bool) -> Result<()> { + if ids_only { + for tile in tiles { + ctx.out.value(&tile.id); + } + return Ok(()); + } + if tiles.is_empty() { + ctx.out.note("Nothing matched."); + return Ok(()); + } + + let rows: Vec> = tiles + .iter() + .map(|tile| { + vec![ + tile.id.clone(), + output::date(&tile.captured_at), + match tile.r#type { + AssetType::Image => "photo".into(), + AssetType::Video => match tile.duration { + Some(seconds) => format!("video, {seconds:.0}s"), + None => "video".into(), + }, + }, + tile_flags(ctx, tile), + ] + }) + .collect(); + + ctx.out.table(&["ID", "TAKEN", "KIND", ""], &rows); + ctx.out.note(format!("\n{} shown", tiles.len())); + Ok(()) +} + +/// The marks a tile can carry. Archived and trashed are absent by construction: the +/// timeline excludes both unless asked for them, and a tile could not say so anyway. +fn tile_flags(ctx: &Context, tile: &TimelineTile) -> String { + let mut marks = Vec::new(); + if tile.favorite { + marks.push(ctx.out.paint("★", YELLOW)); + } + match tile.status { + AssetStatus::Ready => {} + AssetStatus::Failed => marks.push(ctx.out.paint("failed", RED)), + AssetStatus::Pending => marks.push(ctx.out.dim("pending")), + AssetStatus::Processing => marks.push(ctx.out.dim("processing")), + } + marks.join(" ") +} + pub async fn show(ctx: &Context, args: &ShowArgs) -> Result<()> { let asset = ctx.client.assets.get(&args.id).await?; let faces = if args.faces { @@ -535,6 +668,7 @@ pub async fn restore(ctx: &Context, args: &RestoreArgs) -> Result<()> { mod tests { use super::*; use imogen_sdk::TimelineBucket; + use std::collections::HashMap; fn days(dates: &[&str]) -> Vec { dates @@ -610,4 +744,86 @@ mod tests { ); assert_eq!(kept(None, None).len(), 5, "no bounds keeps everything"); } + + fn tile(id: &str) -> TimelineTile { + TimelineTile { + id: id.into(), + captured_at: "2011-08-15T00:00:00.000Z".into(), + width: None, + height: None, + r#type: AssetType::Image, + status: AssetStatus::Ready, + favorite: false, + duration: None, + placeholder_color: None, + live_photo_video_id: None, + } + } + + /// A stand-in for `GET /assets/timeline/bucket`: months, each holding one or more + /// pages, with the page index standing in for the opaque cursor. + fn pages_of(month: &str) -> Vec> { + let library: HashMap<&str, Vec>> = HashMap::from([ + ("2011-08", vec![vec!["a", "b"]]), + // Three pages: a month heavy enough that one round trip is not the whole of it. + ("2011-07", vec![vec!["c", "d"], vec!["e", "f"], vec!["g"]]), + ("2010-12", vec![vec!["h"]]), + ]); + library + .get(month) + .unwrap_or_else(|| panic!("asked for {month}, which the timeline never listed")) + .clone() + } + + /// The whole point of the walk: an album or a person is enumerated to the last + /// photograph, because the ids feed `xargs` and a short list is a silently wrong one. + /// + /// Two ways to get this wrong are both pinned here — stopping at the first page of a + /// month, and stopping at the first month — because either leaves a list that looks + /// perfectly well-formed. + #[tokio::test] + async fn every_page_of_every_month_is_walked() { + let buckets = days(&[ + "2011-08-15", + "2011-08-02", + "2011-07-30", + "2011-07-01", + "2010-12-31", + ]); + + let tiles = collect_tiles(&buckets, |period, cursor| async move { + let pages = pages_of(&period); + let index: usize = cursor.map(|c| c.parse().unwrap()).unwrap_or(0); + Ok(TilePage { + items: pages[index].iter().map(|id| tile(id)).collect(), + next_cursor: (index + 1 < pages.len()).then(|| (index + 1).to_string()), + total: None, + }) + }) + .await + .unwrap(); + + assert_eq!( + tiles.iter().map(|t| t.id.as_str()).collect::>(), + vec!["a", "b", "c", "d", "e", "f", "g", "h"], + "every tile in every month, in timeline order" + ); + } + + /// Days are what the timeline hands back and months are what the bucket endpoint + /// takes, so the walk asks for each month once rather than for each day. + #[test] + fn the_months_are_asked_for_once_each_newest_first() { + assert_eq!( + periods_of(&days(&[ + "2011-08-15", + "2011-08-02", + "2011-07-30", + "2011-07-01", + "2010-12-31", + ])), + vec!["2011-08", "2011-07", "2010-12"] + ); + assert!(periods_of(&[]).is_empty()); + } } diff --git a/src/commands/people.rs b/src/commands/people.rs index f2d267f..9e1bfc4 100644 --- a/src/commands/people.rs +++ b/src/commands/people.rs @@ -1,7 +1,7 @@ //! People, as grouped by face recognition. use anyhow::Result; -use imogen_sdk::PersonUpdate; +use imogen_sdk::{AssetFilter, PersonUpdate}; use serde_json::json; use crate::cli::PeopleCommand; @@ -49,30 +49,50 @@ async fn list(ctx: &Context, hidden: bool) -> Result<()> { ] }) .collect(); - ctx.out.table(&["ID", "NAME", "PHOTOS", ""], &rows); + // FACES, not PHOTOS: `photoCount` on the wire is the server's `faceCount`, and one + // photograph holding two faces of the same person counts twice in it. + ctx.out.table(&["ID", "NAME", "FACES", ""], &rows); Ok(()) } +/// The person, and every photograph they appear in. +/// +/// Two things were wrong here at once. The photos on `GET /people/{id}` are a capped +/// cover sample, so this pages the timeline under a `personId` filter instead — every +/// one, because `imogen people show alice --ids` feeds a pipeline. And `photoCount` on +/// the wire is the server's `faceCount`: it counts faces, so a photograph with two of +/// this person's faces in it counts twice. It is still worth showing, but only under its +/// own name. "photographs" is the number of rows below it; "faces" is the other number. async fn show(ctx: &Context, reference: &str, ids_only: bool) -> Result<()> { let person = ctx.find_person(reference).await?; - let full = ctx.client.people.get(&person.id).await?; + let tiles = crate::commands::assets::all_tiles( + ctx, + &AssetFilter { + person_id: Some(person.id.clone()), + ..Default::default() + }, + ) + .await?; + if ctx.out.is_json() { - return ctx.out.json(&full); + return ctx.out.json(&json!({ + "person": person, + "items": tiles, + "count": tiles.len(), + "faceCount": person.photo_count, + })); } if ids_only { - for photo in &full.photos { - ctx.out.value(&photo.id); - } - return Ok(()); + return crate::commands::assets::print_tiles(ctx, &tiles, true); } - ctx.out - .heading(full.person.name.as_deref().unwrap_or("unnamed")); + ctx.out.heading(person.name.as_deref().unwrap_or("unnamed")); ctx.out.fields(&[ - ("id", full.person.id.clone()), - ("photographs", full.person.photo_count.to_string()), + ("id", person.id.clone()), + ("photographs", tiles.len().to_string()), + ("faces", person.photo_count.to_string()), ]); ctx.out.line(""); - crate::commands::assets::print_assets(ctx, &full.photos, false) + crate::commands::assets::print_tiles(ctx, &tiles, false) } async fn rename(ctx: &Context, reference: &str, name: &str) -> Result<()> {