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

Keep the view pointing at the same photograph when the columns change

Browse Source 
Two faults behind "the gallery randomly glitches to blank and needs a
scroll to reset it", and behind the scrolling that skips.

Cells are drawn at their absolute place in the library, so the row a
photograph sits on is `index / columns`. When `columns` changes every
cell moves — and the Flickable's `viewport-y` did not move with them. The
view was left pointing at a row that now holds entirely different
photographs, typically thousands of images from the ones the loaded
window covers, so the grid drew nothing at all. It stayed that way until
a scroll reported a first-visible row and dragged the window back under
the view, which is exactly the reset the user found. None of its triggers
are rare: a resize, the collections sidebar opening, a zoom step, or
turning the tablet over.

The last first-visible ordinal names the photograph being looked at, so
it is now sent back through `scroll-to` and the view lands on that same
photograph at whatever row it now occupies.

The second fault is the window-move test. At either end of a scope the
window is pinned — the first screenful cannot be centred further back
than zero, the last cannot start past the last full screenful — so the
margin test was unsatisfiable there and every row crossed in the first or
last quarter of a window re-read the catalog, rebuilt the model and
issued a thumbnail batch to arrive at the offset it already held. On a
library of twenty-odd thousand that is a stutter at the top and the
bottom of every collection, which is where a cull begins and ends.

The decision is now a rule with tests rather than four lines inside the
scroll handler: both of its ways of being wrong are invisible in the code
and obvious on a tablet.
This commit is contained in:
dtourolle
2026-08-21 20:34:01 +02:00
parent deabac0923
commit 80f1a210cc
1 changed files with 158 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/library_ui.rs
+158 -13
View File
@@ -1663,6 +1663,61 @@ fn already_served(
.collect()
}
/// Where the loaded window should move to for a view at `first_visible`, or
/// `None` to leave it where it is.
///
/// # Why this is a rule and not four lines in the scroll handler
///
/// It decides how often the grid re-reads the catalog while a finger is on it,
/// and both of its ways of being wrong are invisible in the code and obvious
/// on a tablet: too eager and every scroll stutters, too lazy and the view
/// runs off the end of the loaded rows into blank ones.
///
/// # The rule
///
/// The window is centred on the view — a quarter of it behind, so scrolling
/// back has loaded rows to move into — and clamped to the last position where
/// it is still full. It moves only once the view comes within a quarter-window
/// of an edge of what is loaded, so a drag reloads a few times per screenful
/// rather than on every row.
///
/// And it never moves to where it already is. That is the case the margin test
/// alone gets wrong: at either end of a scope the window is *pinned* — the
/// first screenful cannot be centred further back than zero, and the last
/// cannot start past `max_offset` — so the margin is unsatisfiable there and
/// every row crossed in the first or last quarter of a window re-read the
/// catalog, rebuilt the model and issued a thumbnail batch to arrive at the
/// offset it already had. On a library of twenty-odd thousand that is a
/// stutter at the top and the bottom of every collection, which is exactly
/// where a cull begins and ends.
fn window_move(
first_visible: usize,
current: usize,
window_size: usize,
total: usize,
) -> Option<usize> {
// The same clamp `load_window` applies, repeated here so this compares
// against where the window would come to rest rather than where it was
// asked to go.
let max_offset = total.saturating_sub(window_size.min(total));
let desired = first_visible
.saturating_sub(window_size / 4)
.min(max_offset);
if desired == current {
return None;
}
let margin = window_size / 4;
let inside =
first_visible >= current + margin && first_visible + margin < current + window_size;
if inside {
return None;
}
Some(desired)
}
/// Fill the model from the catalog and start fetching thumbnails.
///
/// Reads the window starting at the controller's current offset, which the
@@ -3392,7 +3447,34 @@ pub fn wire<F>(
if !w.get_show_library() {
return;
}
// **The viewport has to be re-anchored, not just the window.**
//
// Cells are drawn at their absolute place in the library, so
// the row a photograph sits on is `index / columns` — and this
// callback is the news that `columns` just changed. Every cell
// therefore moved. The Flickable's `viewport-y` did not: the
// view was left pointing at a row that now holds entirely
// different photographs, thousands of images from the ones the
// loaded window covers. The grid goes blank, and stays blank
// until a scroll reports a first-visible row and drags the
// window back under the view.
//
// That is the "the gallery randomly goes blank and a scroll
// fixes it" report. Nothing about its triggers is rare: a
// resize, the collections sidebar opening, a zoom step, or
// turning the tablet over.
//
// `resume_at` is the last first-visible ordinal reported, so it
// names the photograph the user was looking at. Sending it back
// through `scroll-to` puts that same photograph at the top of
// the view, at whatever row it now occupies.
let anchor = ctl.resume_at.get();
let window_size = *ctl.window.borrow();
*ctl.offset.borrow_mut() = anchor.saturating_sub(window_size / 4);
load_window(&w, &ctl);
w.set_library_scroll_to(anchor as i32);
w.set_library_scroll_token(w.get_library_scroll_token() + 1);
}
});
}
@@ -3495,21 +3577,16 @@ pub fn wire<F>(
// Centre the window on the view, so scrolling either way has
// loaded rows ahead of it rather than only below.
let window_size = *ctl.window.borrow();
let desired = first_visible.saturating_sub(window_size / 4);
let current = *ctl.offset.borrow();
// Reload only once the view nears an edge of what is loaded.
// Reacting to every scroll event would re-query and re-fetch
// continuously during a drag; this fires a few times per screenful.
let margin = window_size / 4;
let inside =
first_visible >= current + margin && first_visible + margin < current + window_size;
if inside {
let Some(offset) = window_move(
first_visible,
*ctl.offset.borrow(),
*ctl.window.borrow(),
w.get_library_total().max(0) as usize,
) else {
return;
}
};
*ctl.offset.borrow_mut() = desired;
*ctl.offset.borrow_mut() = offset;
load_window(&w, &ctl);
});
}
@@ -4324,6 +4401,74 @@ mod tests {
assert_eq!(img.size().height, 2);
}
// --- moving the loaded window ------------------------------------------
//
// 360 images loaded out of 24,000, centred on the view a quarter back.
const W: usize = 360;
const TOTAL: usize = 24_000;
#[test]
fn a_view_well_inside_the_loaded_window_does_not_move_it() {
// The whole point of loading a screenful either side: scrolling within
// it must not touch the catalog.
assert_eq!(window_move(5_000, 4_910, W, TOTAL), None);
assert_eq!(window_move(5_100, 4_910, W, TOTAL), None);
}
#[test]
fn a_view_reaching_the_edge_of_the_loaded_window_moves_it() {
// Close enough to the bottom of what is loaded that scrolling on would
// run into rows nobody has read.
let moved = window_move(5_200, 4_910, W, TOTAL).expect("the window follows the view");
assert_eq!(moved, 5_200 - W / 4, "centred a quarter behind the view");
}
#[test]
fn the_top_of_the_library_is_not_reloaded_on_every_row() {
// `first_visible` cannot be centred further back than zero, so the
// margin test can never be satisfied here. Before this rule every one
// of these re-read the catalog to arrive at the offset it already had,
// which is the stutter at the top of every scope.
for first_visible in [0, 6, 30, 89] {
assert_eq!(
window_move(first_visible, 0, W, TOTAL),
None,
"row {first_visible} asked for a move to offset 0, which is where it is"
);
}
}
#[test]
fn the_end_of_the_library_is_not_reloaded_on_every_row() {
// The mirror of the above, and the worse of the two: the window is
// clamped to `max_offset` while the view keeps travelling past it.
let pinned = TOTAL - W;
for first_visible in [TOTAL - W / 2, TOTAL - 30, TOTAL - 1] {
assert_eq!(
window_move(first_visible, pinned, W, TOTAL),
None,
"row {first_visible} asked for a move to the offset it already had"
);
}
}
#[test]
fn the_window_never_starts_past_the_last_full_screenful() {
// Otherwise a scrub to the very end loads a handful of cells and the
// rest of the window addresses images that do not exist.
let moved = window_move(TOTAL - 1, 0, W, TOTAL).expect("a scrub to the end moves");
assert_eq!(moved, TOTAL - W);
}
#[test]
fn a_library_smaller_than_the_window_stays_at_the_beginning() {
// `max_offset` is zero, so there is one valid position and the view
// must never ask for another.
assert_eq!(window_move(0, 0, W, 40), None);
assert_eq!(window_move(39, 0, W, 40), None);
}
// --- carrying thumbnails across a reload -------------------------------
//
// The black flash: `load_window` rebuilds every row, and until these it