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); } } } }