From deabac0923a11fc1a3855401ab33d3854486dac2 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Fri, 21 Aug 2026 20:31:48 +0200 Subject: [PATCH] Carry a photograph's thumbnail across a reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Work in progress found uncommitted in the tree, committed as its own change so the fixes that follow can be read separately. Not authored in this session; the description below is written from the diff. `load_window` rebuilds every row, and a scroll reloads once the view has travelled a quarter of the loaded window — so three quarters of the cells being rebuilt are the same photographs already on screen. Rebuilding them empty blanked the grid to `Theme.ground` and refilled it a beat later, once a worker had re-read and re-decoded each one from the store. That is the black flash on every screenful of scrolling, and on a column change, a zoom step, a filter and a return from develop. Thumbnails are now held by `image_id` across the swap — a refcount per cell, no pixels move — along with the "no preview" verdict, which is an answer about the file worth keeping for the same reason. `requested` is rebuilt from what the new model actually holds rather than cleared, so a carried cell is not fetched again while one newly scrolled in still is. The size class each cell's pixels came from is tracked alongside, so a grid zoomed past that class still asks for the sharper one. Also guards the whole `scrolled` and `columns-changed` handlers on `show-library` rather than just the resume latch: a Flickable being torn down passes its viewport through zero, which was indistinguishable from a fling to the top and reloaded the window against the first rows of the catalog every time an image was opened. --- ui/dr-ui/src/library_ui.rs | 342 +++++++++++++++++++++++++++++++++---- 1 file changed, 307 insertions(+), 35 deletions(-) diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index a5ceee3..9320495 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -61,6 +61,17 @@ pub struct LibraryController { /// Catalog row ids and whether each still needs its EXIF read. image_ids: RefCell>, needs_metadata: RefCell>, + /// The size class of the pixels each row is currently showing, `None` for a + /// row still waiting. + /// + /// Parallel to the model, and the reason [`load_window`] can carry a + /// thumbnail across a reload without re-deciding whether it is sharp + /// enough: a cell holding the 256px class while the grid has since been + /// zoomed past that must keep showing what it has *and* still ask for the + /// large one. Recording the class the pixels came from is what tells those + /// two states apart — without it a carried thumbnail either blocks the + /// sharper fetch forever or is re-fetched on every scroll. + thumb_class: RefCell>>, /// Where in the catalog the current window starts. Scrubbing moves this. offset: RefCell, /// The first visible ordinal, kept so returning from the develop view lands @@ -252,6 +263,7 @@ impl LibraryController { sizes: RefCell::new(Vec::new()), image_ids: RefCell::new(Vec::new()), needs_metadata: RefCell::new(Vec::new()), + thumb_class: RefCell::new(Vec::new()), offset: RefCell::new(0), resume_at: std::cell::Cell::new(0), window: RefCell::new(INITIAL_WINDOW), @@ -1574,6 +1586,83 @@ fn open_catalog_for_offline( } } +/// What one cell of the outgoing model is worth keeping. +#[derive(Clone)] +struct Held { + thumbnail: slint::Image, + has_thumb: bool, + /// A completed fetch that found no preview. Worth carrying for the same + /// reason the pixels are: it is an answer about the file, and re-asking it + /// on every scroll is a fetch that will fail again. + unavailable: bool, + /// Which size class the pixels came from, so the reload can tell a cell + /// that is showing what it should from one that is showing the small class + /// while the grid has since been zoomed past it. + class: Option, +} + +/// **What the outgoing model is still holding, keyed on the photograph.** +/// +/// A reload replaces every row, and a scroll reloads once the view has +/// travelled a quarter of the loaded window — so three quarters of the cells +/// being rebuilt are the *same* photographs the user is looking at right now. +/// Rebuilding them empty blanked the whole grid to `Theme.ground` and refilled +/// it a beat later, once a worker had re-read and re-decoded every one of them +/// from the thumbnail store. That is the black flash that punctuated every +/// screenful of scrolling, and the one visible on a column change, a zoom step, +/// a filter and a return from develop. +/// +/// Keyed on `image_id` rather than on the row, because the row is exactly what +/// a reload changes. Cheap: the values are `slint::Image` handles, so this is a +/// refcount per cell and no pixels move. +fn hold_thumbnails( + previous: &slint::ModelRc, + ids: &[i64], + classes: &[Option], +) -> std::collections::HashMap { + ids.iter() + .enumerate() + .filter_map(|(row, id)| { + let cell = previous.row_data(row)?; + // Nothing to carry: a row still waiting is equally blank either + // way, and holding a default image would claim otherwise. + if !cell.has_thumb && !cell.unavailable { + return None; + } + Some(( + *id, + Held { + thumbnail: cell.thumbnail, + has_thumb: cell.has_thumb, + unavailable: cell.unavailable, + class: classes.get(row).copied().flatten(), + }, + )) + }) + .collect() +} + +/// The fetches a reloaded window does **not** have to make. +/// +/// The set used to be cleared on every load, which said "every cell here is +/// still to be fetched" — true when every row came back empty, and false now +/// that the overlap keeps its pixels. Re-requesting them re-read and re-decoded +/// three quarters of the window from the store on every scroll. +/// +/// Rebuilt rather than merely kept, and that is the half that is easy to get +/// wrong: an image served into a window the user has since scrolled away from +/// is no longer on screen, and a set that remembered it would leave that cell +/// permanently blank when they scrolled back. Only what the new model actually +/// holds counts as served — and only at the class it holds it at, so zooming +/// past the grid class still asks for the large one. +fn already_served( + held: &std::collections::HashMap, + ids: impl Iterator, +) -> std::collections::HashSet<(i64, dr_thumbs::ThumbSize)> { + ids.filter_map(|id| Some((id, held.get(&id)?.class?))) + .collect() +} + /// Fill the model from the catalog and start fetching thumbnails. /// /// Reads the window starting at the controller's current offset, which the @@ -1695,26 +1784,37 @@ fn load_window(window: &AppWindow, ctl: &Rc) { }) .collect(); + // What the outgoing model is still holding — see [`hold_thumbnails`]. + let held = { + let previous = window.get_library_cells(); + let ids = ctl.image_ids.borrow(); + let classes = ctl.thumb_class.borrow(); + hold_thumbnails(&previous, &ids, &classes) + }; + let rows: Vec = cells .iter() .zip(headings) - .map(|(c, heading)| LibraryCell { - period_heading: heading.into(), - // A freshly loaded window has no drag in flight. - lifted: false, - name: c.name.as_str().into(), - thumbnail: slint::Image::default(), - has_thumb: false, - unavailable: false, - // Both are filled straight after by `collections_ui`, which owns - // the selection and queries the badge counts for the whole window - // in one statement rather than one per cell. - selected: false, - collection_count: 0, - // Likewise filled by `sync_ratings` below — one query for the - // window, not one per cell. - rating: 0, - flag: 0, + .map(|(c, heading)| { + let carried = held.get(&c.image_id); + LibraryCell { + period_heading: heading.into(), + // A freshly loaded window has no drag in flight. + lifted: false, + name: c.name.as_str().into(), + thumbnail: carried.map(|h| h.thumbnail.clone()).unwrap_or_default(), + has_thumb: carried.is_some_and(|h| h.has_thumb), + unavailable: carried.is_some_and(|h| h.unavailable), + // Both are filled straight after by `collections_ui`, which owns + // the selection and queries the badge counts for the whole window + // in one statement rather than one per cell. + selected: false, + collection_count: 0, + // Likewise filled by `sync_ratings` below — one query for the + // window, not one per cell. + rating: 0, + flag: 0, + } }) .collect(); @@ -1723,11 +1823,17 @@ fn load_window(window: &AppWindow, ctl: &Rc) { *ctl.sizes.borrow_mut() = cells.iter().map(|c| c.size).collect(); *ctl.image_ids.borrow_mut() = cells.iter().map(|c| c.image_id).collect(); *ctl.needs_metadata.borrow_mut() = cells.iter().map(|c| c.metadata_state < 2).collect(); - ctl.requested.borrow_mut().clear(); + *ctl.thumb_class.borrow_mut() = cells + .iter() + .map(|c| held.get(&c.image_id).and_then(|h| h.class)) + .collect(); + *ctl.requested.borrow_mut() = already_served(&held, cells.iter().map(|c| c.image_id)); + // The model is about to be replaced, so every thumbnail still in flight - // addresses a window that no longer exists. Bumping here — before the swap, - // and beside the `requested` clear that already admits the old fetches no - // longer apply — is what lets `drain_thumbnails` recognise itself as stale. + // addresses a window that no longer exists. Bumping here — before the swap + // — is what lets `drain_thumbnails` recognise itself as stale. The rows + // those fetches would have filled are absent from `requested` above, so the + // batch started below asks for them again. ctl.generation.set(ctl.generation.get().wrapping_add(1)); window.set_library_cells(slint::ModelRc::new(slint::VecModel::from(rows))); @@ -2196,9 +2302,13 @@ fn request_thumbnails(window: &AppWindow, ctl: &Rc) { return; }; + // The drawn cell size decides which class to ask for. Chosen once for the + // batch rather than per row, and carried through to the drain so a cell it + // fills can record what it is now showing. + let cell_pixels = window.get_library_cell_size().max(1.0) as u32; + let class = dr_thumbs::ThumbSize::for_cell(cell_pixels); + let wanted: Vec = { - // The drawn cell size decides which class to ask for. - let cell_pixels = window.get_library_cell_size().max(1.0) as u32; let paths = ctl.paths.borrow(); let file_ids = ctl.file_ids.borrow(); let sizes = ctl.sizes.borrow(); @@ -2210,10 +2320,9 @@ fn request_thumbnails(window: &AppWindow, ctl: &Rc) { .enumerate() .filter_map(|(i, p)| { let image_id = *image_ids.get(i)?; - // Chosen from how large the cell is actually drawn, so a - // zoomed grid asks for detail a 256px thumbnail cannot give + // A zoomed grid asks for detail a 256px thumbnail cannot give, // and a wall of small cells does not pay for it. - let thumb_size = dr_thumbs::ThumbSize::for_cell(cell_pixels); + let thumb_size = class; // Keyed on the photograph, so scrolling back over a cell that // has already been served does not ask for it again. if !requested.insert((image_id, thumb_size)) { @@ -2244,7 +2353,18 @@ fn request_thumbnails(window: &AppWindow, ctl: &Rc) { library::thumbs_dir(&session.server, &session.user_id), library::catalog_path(&session.server, &session.user_id), ); - drain_thumbnails(window.as_weak(), ctl.clone(), rx, requested); + drain_thumbnails(window.as_weak(), ctl.clone(), rx, requested, class); +} + +/// Note which size class a row's pixels came from. +/// +/// Silent about a row past the end: the model and this vector are rebuilt +/// together by [`load_window`], and the generation check above already refuses +/// anything addressed to a window that has since moved. +fn record_class(ctl: &Rc, row: usize, class: dr_thumbs::ThumbSize) { + if let Some(slot) = ctl.thumb_class.borrow_mut().get_mut(row) { + *slot = Some(class); + } } /// Apply thumbnails to the model as they arrive. @@ -2253,6 +2373,7 @@ fn drain_thumbnails( ctl: Rc, rx: Receiver, requested: usize, + class: dr_thumbs::ThumbSize, ) { let timer = slint::Timer::default(); let ctl_cb = ctl.clone(); @@ -2392,6 +2513,9 @@ fn drain_thumbnails( row.thumbnail = to_slint_image(t.width, t.height, &t.rgba); row.has_thumb = true; model.set_row_data(t.row, row); + // What this cell is now showing, so the next reload + // can carry it over and know not to ask again. + record_class(&ctl_cb, t.row, class); } } ThumbnailMessage::Unavailable { row, reason } => { @@ -2400,6 +2524,10 @@ fn drain_thumbnails( if let Some(mut r) = model.row_data(row) { r.unavailable = true; model.set_row_data(row, r); + // A verdict is worth carrying too: "no preview" is + // an answer about the file, and re-asking it on + // every scroll is a fetch that will fail again. + record_class(&ctl_cb, row, class); } } // TRACES: FR-CAT-9 @@ -3252,11 +3380,18 @@ pub fn wire( // A column-count change moves which cells begin a row, and month headings // sit on row-leading cells. + // + // Guarded like the scroll below, and for the same reason: a grid being + // taken down reports its geometry collapsing on the way out, and reloading + // the window against that is work done for a page nobody is looking at. { let weak = window.as_weak(); let ctl = ctl.clone(); window.on_library_columns_changed(move || { if let Some(w) = weak.upgrade() { + if !w.get_show_library() { + return; + } load_window(&w, &ctl); } }); @@ -3270,6 +3405,9 @@ pub fn wire( let ctl = ctl.clone(); window.on_library_capacity(move |capacity| { let Some(w) = weak.upgrade() else { return }; + if !w.get_show_library() { + return; + } let capacity = (capacity.max(0) as usize).max(MIN_WINDOW); if capacity == *ctl.window.borrow() { return; @@ -3290,18 +3428,32 @@ pub fn wire( let Some(w) = weak.upgrade() else { return }; let first_visible = first_visible.max(0) as usize; + // **A report from a grid that is not on screen is not a scroll.** + // + // `show-library` gates an `if`, so opening an image tears the whole + // subtree down — and a Flickable being destroyed passes its viewport + // through zero on the way out, which arrives here indistinguishable + // from the user having flung the grid to the top. Everything below + // then ran on the way *into* develop: the loaded window was reset to + // offset zero, the model was rebuilt against the first rows of the + // library, and a thumbnail batch was issued for photographs nobody + // had asked to see. Those rebuilds landed while the grid was still + // being taken apart, which is what flashed the library over the + // develop view for the first few frames after a click — and on a + // remote library it also spent a burst of requests on the top of the + // catalog every single time an image was opened. + // + // The guard used to cover only `resume_at`, for a narrower version + // of the same reason. It belongs over the whole handler. + if !w.get_show_library() { + return; + } + // Remember where the view is, so leaving for the develop view and // coming back returns here. Latched on every event rather than read // at departure: by the time the grid is hidden its scroll position // is only in the Flickable, which is about to be destroyed. - // - // Only while the grid is the visible page. Building and tearing - // down the subtree moves the viewport through 0, and accepting that - // would erase the position on the way out — the first return would - // work and every one after it would land at the top. - if w.get_show_library() { - ctl.resume_at.set(first_visible); - } + ctl.resume_at.set(first_visible); // Move the timeline marker with the view. Scrolling the grid is a // way of moving through time just as scrubbing is, and a marker @@ -4172,6 +4324,126 @@ mod tests { assert_eq!(img.size().height, 2); } + // --- carrying thumbnails across a reload ------------------------------- + // + // The black flash: `load_window` rebuilds every row, and until these it + // rebuilt them empty — so the three quarters of the grid the user was + // looking at went to `Theme.ground` and back on every scroll. + + /// A model of cells, `has_thumb` set for the ids named. + fn model_of(ids: &[i64], with_pixels: &[i64]) -> slint::ModelRc { + let rows: Vec = ids + .iter() + .map(|id| LibraryCell { + has_thumb: with_pixels.contains(id), + thumbnail: if with_pixels.contains(id) { + to_slint_image(2, 2, &[255u8; 16]) + } else { + slint::Image::default() + }, + ..Default::default() + }) + .collect(); + slint::ModelRc::new(slint::VecModel::from(rows)) + } + + #[test] + fn a_reload_keeps_the_pixels_of_photographs_that_are_still_in_the_window() { + let ids = [10i64, 11, 12]; + let previous = model_of(&ids, &[10, 12]); + let classes = vec![Some(dr_thumbs::ThumbSize::Grid); 3]; + + let held = hold_thumbnails(&previous, &ids, &classes); + + assert!(held.contains_key(&10), "a drawn cell is carried"); + assert!(held.contains_key(&12)); + assert!( + !held.contains_key(&11), + "a cell still waiting has nothing to carry" + ); + } + + #[test] + fn a_no_preview_verdict_is_carried_too() { + // Otherwise every scroll re-asks a question the server has already + // answered, and the cell blinks from "no preview" back to "…". + let ids = [7i64]; + let rows = vec![LibraryCell { + unavailable: true, + ..Default::default() + }]; + let previous = slint::ModelRc::new(slint::VecModel::from(rows)); + + let held = hold_thumbnails(&previous, &ids, &[Some(dr_thumbs::ThumbSize::Grid)]); + assert!(held[&7].unavailable); + assert!(!held[&7].has_thumb); + } + + #[test] + fn a_carried_cell_is_not_fetched_again_but_a_newly_scrolled_in_one_is() { + // The scroll this describes: the window moved down by one image, so + // 11 and 12 are still on screen and 13 has just arrived. + let ids = [10i64, 11, 12]; + let previous = model_of(&ids, &[10, 11, 12]); + let held = hold_thumbnails(&previous, &ids, &[Some(dr_thumbs::ThumbSize::Grid); 3]); + + let served = already_served(&held, [11i64, 12, 13].into_iter()); + + assert!(served.contains(&(11, dr_thumbs::ThumbSize::Grid))); + assert!(served.contains(&(12, dr_thumbs::ThumbSize::Grid))); + assert!( + !served.contains(&(13, dr_thumbs::ThumbSize::Grid)), + "a photograph the window has just reached must still be fetched" + ); + } + + #[test] + fn a_photograph_scrolled_out_of_the_window_is_fetched_again_on_return() { + // The trap in keeping the set rather than rebuilding it: image 10 was + // served once, but the model that held its pixels is long gone, so the + // cell would sit blank for ever if it still counted as served. + let held = hold_thumbnails( + &model_of(&[10], &[10]), + &[10], + &[Some(dr_thumbs::ThumbSize::Grid)], + ); + + let served = already_served(&held, [40i64, 41].into_iter()); + assert!(served.is_empty(), "nothing in this window is already drawn"); + } + + #[test] + fn a_carried_thumbnail_does_not_satisfy_a_zoom_past_its_class() { + // Zooming past 256px reloads the window. The cell keeps showing the + // small thumbnail — no flash — but the large one must still be asked + // for, or the grid would stay soft until something else forced a fetch. + let held = hold_thumbnails( + &model_of(&[5], &[5]), + &[5], + &[Some(dr_thumbs::ThumbSize::Grid)], + ); + let mut served = already_served(&held, [5i64].into_iter()); + + assert!( + served.insert((5, dr_thumbs::ThumbSize::Large)), + "the large class is still unserved" + ); + assert!( + !served.insert((5, dr_thumbs::ThumbSize::Grid)), + "and the small one is not asked for twice" + ); + } + + #[test] + fn a_cell_whose_class_was_never_recorded_is_fetched_again() { + // Pixels with no class are pixels from before this bookkeeping existed + // — or from a row the drain never reached. Showing them is right; + // claiming they were served is not, because nothing knows at what size. + let held = hold_thumbnails(&model_of(&[5], &[5]), &[5], &[None]); + assert!(held.contains_key(&5), "still drawn"); + assert!(already_served(&held, [5i64].into_iter()).is_empty()); + } + /// A thumbnail drain must be able to tell that the window it was started /// for has been replaced. ///