From 2330ed25e931869e9fa1d702c51c025848f4174f Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 17 Aug 2026 07:23:01 +0200 Subject: [PATCH] Let a grouped slider be dragged, by flattening the panel that drew it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit White balance, highlights and shadows, and the mixer took a press, jumped once, and went dead under the finger. Exposure and contrast dragged perfectly — which is what made it read as a slider bug rather than a layout one. The panel nested. A group's head row drew the *whole* group, repeating over `row.group-len` and indexing back into `root.rows`; every other row drew nothing. So the inner repeater's model was read off the head row, and depended on that row's identity. Moving any parameter in the group rewrites that row — its own value changed, or `group-modified` flipped for its neighbours — which re-evaluated the repeater, rebuilt its items, and destroyed the `TouchArea` holding the live gesture. A lone parameter had no inner repeater, so the five single-parameter operations were never affected. Now each row draws only itself, so an update touches one control and nothing structural. The facet heading comes off `starts-facet`, which Rust already marks on the first row of a run, and the group heading is drawn by the row that heads it. It also retires the old hazard of a head row building every control in its group — thirty-six live TouchAreas behind the mixer's twelve visible ones. The same identity hazard reached the rows themselves through `ModelRc`, which compares by identity rather than contents: a fresh empty model per row per call made every row differ from itself, so `sync_rows` rewrote all of them on every event. `points` now shares one empty model as `choices` already did. Both are held by tests, because the failure is invisible in a still — every value is right and the panel looks perfect. Curve rows are excluded: their points model carries live coordinates, is rebuilt by design, and `sync_rows` writes values through the existing model rather than swapping it. Co-Authored-By: Claude Opus 5 --- ui/dr-ui/src/develop.rs | 134 +++++++++++++++++++++++++++++++++------ ui/dr-ui/ui/adjust.slint | 122 ++++++++++++++++------------------- 2 files changed, 171 insertions(+), 85 deletions(-) diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 15e62ec..f03f4d3 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -89,24 +89,36 @@ impl DevelopSession { } } +// The empty nested models, each a single shared identity. +// +// **`ModelRc` compares by identity, not by contents**, and `sync_rows` decides +// which controls to invalidate by comparing each freshly built row against the +// one on screen. A brand-new empty model per row per call therefore makes every +// row differ from *itself* on every parameter event, and the panel rewrites all +// of them. +// +// That is not merely wasteful — it breaks dragging. An operation with several +// parameters renders them through a repeater whose model is read off the +// group's head row; rewriting that row re-evaluates the repeater, rebuilding +// its items and destroying the `TouchArea` that holds the gesture. The slider +// takes the press, jumps once, then goes dead under the finger. Only +// multi-parameter operations show it, because a lone parameter has no inner +// repeater to rebuild — which is exactly how it hid: exposure and contrast drag +// perfectly while temperature and tint do not. +// +// Most rows carry neither points nor choices, so the empty case is the common +// one and it costs nothing to make it a constant. + +/// The empty points model, shared by every row that is not a curve. +fn no_points() -> slint::ModelRc { + thread_local! { + static EMPTY: slint::ModelRc = + slint::ModelRc::new(slint::VecModel::from(Vec::::new())); + } + EMPTY.with(Clone::clone) +} + /// The empty choices model, shared by every row that is not an enum. -/// -/// **One identity, deliberately reused.** `ModelRc` compares by *identity*, not -/// by contents, and `sync_rows` decides which controls to invalidate by -/// comparing each fresh row against the one on screen. Handing out a brand-new -/// empty model per row per call therefore makes every row differ from itself on -/// every parameter event, so the panel rewrites all of them. -/// -/// That is not merely wasteful, it breaks dragging. An operation with several -/// parameters renders them through a repeater whose model is read off the -/// group's head row; rewriting that row re-evaluates the repeater, which -/// rebuilds its items and destroys the `TouchArea` holding the gesture. The -/// slider takes the press, jumps once, and then goes dead under the finger — -/// and only for multi-parameter operations, since a lone parameter has no inner -/// repeater to rebuild. -/// -/// `sync_rows` already carries the identical warning about a curve's `points`. -/// This is the same hazard arriving through a second field. fn no_choices() -> slint::ModelRc { thread_local! { static EMPTY: slint::ModelRc = @@ -294,7 +306,7 @@ pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec { precision, unit: unit.into(), // Only curve rows carry points. - points: slint::ModelRc::new(slint::VecModel::from(Vec::::new())), + points: no_points(), // The shared empty model unless this row really has choices — // see `no_choices` for why the identity matters. choices: if choices.is_empty() { @@ -371,7 +383,7 @@ fn curve_row( unit: String::new().into(), points: slint::ModelRc::new(slint::VecModel::from(points)), // A curve is not a choice between named alternatives. - choices: slint::ModelRc::new(slint::VecModel::from(Vec::::new())), + choices: no_choices(), }) } @@ -1134,6 +1146,90 @@ mod tests { assert_eq!(heads, generated); } + #[test] + fn regenerating_the_rows_leaves_unchanged_ones_equal() { + // **This is a dragging test wearing a data disguise.** + // + // `sync_rows` rewrites exactly the rows that compare unequal, and a + // rewritten row re-evaluates the repeater that a multi-parameter + // operation renders its parameters through — which rebuilds the items + // and destroys the `TouchArea` mid-gesture. So a row that differs from + // itself between two identical calls is a slider that takes the press, + // jumps once and then dies under the finger. + // + // It is asserted here rather than left to the eye because the failure + // is invisible in a still: every value is right, the panel looks + // perfect, and only a live drag on a *grouped* parameter shows it. + // `ModelRc` compares by identity, so any new model-valued field + // reintroduces this the moment it is built fresh per call. + let graph = EditGraph::default_chain(); + let caps = graph.capabilities(); + + let first = rows_from(&caps); + let second = rows_from(&caps); + assert_eq!(first.len(), second.len()); + + for (i, (a, b)) in first.iter().zip(second.iter()).enumerate() { + // A curve row is the one legitimate exception: its `points` model + // carries live coordinates, so it genuinely is rebuilt each call + // and `sync_rows` writes the values through the existing model + // instead of swapping it. Every other row must be stable here, at + // the source, rather than relying on a caller to repair it. + if a.kind == "curve" { + continue; + } + assert!( + a == b, + "row {i} ({}) differs from itself across two identical builds, \ + so every parameter event would rewrite it and break dragging", + a.param_label + ); + } + } + + #[test] + fn a_grouped_parameter_survives_a_neighbours_change() { + // The reported bug, at the level it actually occurred. Moving + // temperature flips `group_modified` on *both* of white balance's + // rows — that much is correct and intended. What must not happen is + // the untouched rows of *other* operations also coming back unequal, + // because rewriting a group's head row is what rebuilds the repeater + // holding the live drag. + let mut graph = EditGraph::default_chain(); + let before = rows_from(&graph.capabilities()); + + // Move the first parameter of the first multi-parameter operation, + // named by shape rather than by id so this keeps testing the property + // when the chain changes. + let caps = graph.capabilities(); + let group = caps + .iter() + .find(|c| c.params.len() > 1 && c.presentation.is_none()) + .expect("some operation has several plain parameters"); + let target = &group.params[0]; + graph.set_param(group.id, target.id, target.default + 1.0); + + let after = rows_from(&graph.capabilities()); + assert_eq!(before.len(), after.len()); + + // Curve rows excluded for the reason given in the test above: their + // points model is rebuilt by design and repaired in `sync_rows`. + let changed: Vec<&str> = before + .iter() + .zip(after.iter()) + .filter(|(a, b)| a != b && a.kind != "curve") + .map(|(a, _)| a.param_label.as_str()) + .collect(); + + // Its own group, and nothing beyond it. + assert_eq!( + changed.len(), + group.params.len(), + "moving one parameter should dirty only its own group's rows, \ + but these came back changed: {changed:?}" + ); + } + #[test] fn framing_is_not_generated_as_sliders() { // `GeometryPanel` presents crop, rotation, flips and straightening as diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index c9efe9e..28197ea 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -705,46 +705,50 @@ export component AdjustPanel inherits Rectangle { // narrow to hit confidently is the problem it was added to fix. padding-right: Theme.touch-target; - // One iteration per row, but only a group's *first* row draws - // anything — and it draws the whole group. Every other row - // renders nothing at all. + // **One row, one element, and each renders only itself.** + // + // This used to nest: a group's head row drew the *whole* group + // by repeating over `row.group-len` and indexing back into + // `root.rows` for each member, and every other row drew + // nothing. It produced the right picture and could not be + // dragged. + // + // The reason is worth writing down, because it is invisible in + // a screenshot. The inner repeater's model was `row.group-len` + // — read off the head row — so it depended on the head row's + // *identity*. Moving any parameter in the group rewrites that + // row: its own value changed, or `group-modified` flipped for + // its neighbours. Rewriting it re-evaluated the repeater, which + // rebuilt its items, which destroyed the `TouchArea` holding + // the gesture. The slider took the press, jumped once, and went + // dead under the finger for the rest of the drag. + // + // It only ever affected multi-parameter operations — white + // balance, highlights and shadows, the mixer — because a lone + // parameter had no inner repeater to rebuild. Exposure and + // contrast dragged perfectly the whole time, which is exactly + // what made it look like a slider bug rather than a layout one. + // + // Flat, each element depends only on its own `row`, so an + // update touches one control and touches nothing structural. + // It also drops the old hazard of the head row building every + // control in its group — thirty-six live TouchAreas behind the + // mixer's twelve visible ones. for row[i] in root.rows: VerticalLayout { spacing: 0px; - // **A group of one is not a group.** + // The group's name, drawn by the row that heads it, above + // its own control rather than around the whole run. // - // Five of the pipeline's operations carry a single - // parameter — exposure, contrast, saturation, vibrance, - // brilliance — and wrapping each in a Section produced a - // collapsible heading, a disclosure triangle, a modified - // dot and a hover reset around one slider, with the - // operation's name printed in caps directly above the same - // word as the slider's own label. Five times over, that is - // a column that reads as chrome with controls hidden in it. - // - // So a lone parameter is drawn bare. It loses the group - // reset, which cost nothing: the slider already resets on - // double-click and right-click, and the section's reset was - // a hover-only affordance no finger could reach anyway. - // - // `row` is the entry here — the group's head is its only - // member — so there is nothing to index back into. - if row.group-head == i && row.group-len == 1: ParamControl { - data: row; - curve-samples: root.curve-samples; - drag-changed(on) => { root.slider-dragging = on; } - param-changed(op, param, v) => { - root.param-changed(op, param, v); - } - param-reset(op, param) => { root.param-reset(op, param); } - curve-reset(op) => { root.curve-reset(op); } - } - - // `if` rather than a zero height: a hidden-but-present - // section would still *build* its whole group, so every - // control would exist once per row of its own group — - // thirty-six live TouchAreas behind the colour mixer's - // twelve visible ones. The conditional builds nothing. + // **A group of one is not a group.** Five of the pipeline's + // operations carry a single parameter — exposure, contrast, + // saturation, vibrance, brilliance — and giving each a + // heading printed the operation's name in caps directly + // above the same word as the slider's own label. Five times + // over, that is a column that reads as chrome with controls + // hidden in it. So a lone parameter is drawn bare; it loses + // the group reset, which costs nothing, since the slider + // already resets on double-click and right-click. if row.group-head == i && row.group-len > 1: VerticalLayout { spacing: 0px; padding-top: Theme.gap-sm; @@ -760,39 +764,25 @@ export component AdjustPanel inherits Rectangle { // heading itself has no opinion about it. reset => { root.op-reset(row.op-index); } } + } - // The group's own rows, addressed by offset from its - // head. `root.rows[...]` rather than the loop's `row`: - // this repeats over a count, so `n` is a number and - // the row has to be fetched. - for n in row.group-len: VerticalLayout { - property entry: root.rows[row.group-head + n]; + // A group whose parameters form a grid names each run once. + // Rust has already stacked the rows so a run is contiguous + // and marked its first, which is what lets a flat loop draw + // a heading that belongs to several rows. + if row.starts-facet: FacetHeading { + title: row.facet-label; + } - spacing: 0px; - - // A group whose parameters form a grid names each - // run once. Rust has already stacked the rows so a - // run is contiguous and marked its first row, for - // the same reason `group-head` exists: this model - // is flat and a `for` cannot nest inside a - // boundary discovered at runtime. - if entry.starts-facet: FacetHeading { - title: entry.facet-label; - } - - ParamControl { - data: entry; - curve-samples: root.curve-samples; - drag-changed(on) => { root.slider-dragging = on; } - param-changed(op, param, v) => { - root.param-changed(op, param, v); - } - param-reset(op, param) => { - root.param-reset(op, param); - } - curve-reset(op) => { root.curve-reset(op); } - } + ParamControl { + data: row; + curve-samples: root.curve-samples; + drag-changed(on) => { root.slider-dragging = on; } + param-changed(op, param, v) => { + root.param-changed(op, param, v); } + param-reset(op, param) => { root.param-reset(op, param); } + curve-reset(op) => { root.curve-reset(op); } } } }