dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Carry a photograph's thumbnail across a reload

Browse Source 
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.
This commit is contained in:
dtourolle
2026-08-21 20:31:48 +02:00
parent 4d8196174c
commit deabac0923
1 changed files with 307 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/library_ui.rs
+307 -35
View File
@@ -61,6 +61,17 @@ pub struct LibraryController {
/// Catalog row ids and whether each still needs its EXIF read.
image_ids: RefCell<Vec<i64>>,
needs_metadata: RefCell<Vec<bool>>,
/// 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<Vec<Option<dr_thumbs::ThumbSize>>>,
/// Where in the catalog the current window starts. Scrubbing moves this.
offset: RefCell<usize>,
/// 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<dr_thumbs::ThumbSize>,
}
/// **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<LibraryCell>,
ids: &[i64],
classes: &[Option<dr_thumbs::ThumbSize>],
) -> std::collections::HashMap<i64, Held> {
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<i64, Held>,
ids: impl Iterator<Item = i64>,
) -> 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<LibraryController>) {
})
.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<LibraryCell> = 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<LibraryController>) {
*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<LibraryController>) {
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<library::ThumbnailRequest> = {
// 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<LibraryController>) {
.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<LibraryController>) {
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<LibraryController>, 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<LibraryController>,
rx: Receiver<ThumbnailMessage>,
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<F>(
// 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<F>(
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<F>(
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<LibraryCell> {
let rows: Vec<LibraryCell> = 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.
///