Let a grouped slider be dragged, by flattening the panel that drew it
Build and test / Desktop (Linux) (push) Failing after 1h4m48s
Build and test / Layer separation (push) Successful in 34s
🐳 Android image / Build and push (push) Successful in 4s
Build and test / android-image (push) Successful in 4s
Traceability / Requirement traces (push) Failing after 1m4s
Build and test / Android (aarch64) (push) Failing after 9m43s
Build and test / Desktop (Linux) (push) Failing after 1h4m48s
Build and test / Layer separation (push) Successful in 34s
🐳 Android image / Build and push (push) Successful in 4s
Build and test / android-image (push) Successful in 4s
Traceability / Requirement traces (push) Failing after 1m4s
Build and test / Android (aarch64) (push) Failing after 9m43s
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 <noreply@anthropic.com>
This commit is contained in:
+115
-19
@@ -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<f32> {
|
||||
thread_local! {
|
||||
static EMPTY: slint::ModelRc<f32> =
|
||||
slint::ModelRc::new(slint::VecModel::from(Vec::<f32>::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<slint::SharedString> {
|
||||
thread_local! {
|
||||
static EMPTY: slint::ModelRc<slint::SharedString> =
|
||||
@@ -294,7 +306,7 @@ pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec<ParamRow> {
|
||||
precision,
|
||||
unit: unit.into(),
|
||||
// Only curve rows carry points.
|
||||
points: slint::ModelRc::new(slint::VecModel::from(Vec::<f32>::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::<slint::SharedString>::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
|
||||
|
||||
Reference in New Issue
Block a user