Group the panel by what operations say they are about
A strip of groups over the adjust panel — Light, Colour, Detail — derived from the attributes the operations declare. `adjust.slint` names none of them: the strings arrive resolved and the panel only draws them, so a new operation joins the right group by saying what it is and this file does not change (FR-DEV-3a). A group nothing carries is not offered, so a tab never opens onto nothing. Geometry is left out because its one operation prefers an on-canvas widget and is skipped by the row builder — a Geometry tab would be empty while `GeometryPanel` holds the real controls. The strip appears only when there is more than one group to choose between; a single tab is a control with one option. The selected group is underlined rather than filled. The accent means *modified* everywhere else in this interface, and spending it on "which tab" would blunt the one signal the panel has. **The trap, and it nearly bit again.** `op_index` on a row counts over every capability, not over the ones a filter kept — it is how a row routes back to the core. Renumbering it while filtering would make a slider drive a different operation, which looks like a rendering fault rather than a routing one. `rows_filtered` keeps `enumerate` over the full list and only `group_head` is a position within the emitted rows; a test moves a value through a filtered row and checks it lands where it was asked to. Six tests, including that a nonsense index falls back to showing everything rather than to showing nothing.
This commit is contained in:
+195
-15
@@ -86,6 +86,12 @@ pub struct DevelopSession {
|
||||
active_mask: Option<String>,
|
||||
/// Whether to draw the false-coloured region overlay.
|
||||
show_overlay: bool,
|
||||
/// Which attribute the panel is filtered to, or all of them.
|
||||
///
|
||||
/// `None` is "show everything" and is what a frontend that ignores
|
||||
/// attributes leaves it at — the tabs are the interface's idea, not the
|
||||
/// core's, and nothing breaks without them (ARCH §4.3a).
|
||||
active_tab: Option<dr_pipeline::Attribute>,
|
||||
}
|
||||
|
||||
impl DevelopSession {
|
||||
@@ -150,6 +156,7 @@ impl DevelopSession {
|
||||
subject_key: 0,
|
||||
active_mask: None,
|
||||
show_overlay: false,
|
||||
active_tab: None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -158,16 +165,68 @@ impl DevelopSession {
|
||||
/// Built entirely from the capability list. The `kind` string chooses the
|
||||
/// widget; nothing switches on a parameter's identity.
|
||||
pub fn rows(&self) -> Vec<ParamRow> {
|
||||
match self.active_layer() {
|
||||
// A selected layer takes over the panel. The rows are built from
|
||||
// the layer's own capability list, so every control the global
|
||||
// chain offers is offered here too — including ones added later,
|
||||
// which need no work to become local.
|
||||
Some(layer) => rows_from(&layer.capabilities()),
|
||||
None => rows_from(&self.graph.capabilities()),
|
||||
let caps = self.scoped_capabilities();
|
||||
match self.active_tab {
|
||||
Some(attribute) => rows_filtered(&caps, |op| op.attributes.contains(&attribute)),
|
||||
None => rows_from(&caps),
|
||||
}
|
||||
}
|
||||
|
||||
/// The capability list the panel is currently describing.
|
||||
///
|
||||
/// A selected mask layer takes over the panel, so every control the global
|
||||
/// chain offers is offered on a layer too — including operations added
|
||||
/// later, which need no work to become local.
|
||||
fn scoped_capabilities(&self) -> Vec<OpCapability> {
|
||||
match self.active_layer() {
|
||||
Some(layer) => layer.capabilities(),
|
||||
None => self.graph.capabilities(),
|
||||
}
|
||||
}
|
||||
|
||||
/// The attributes worth offering as tabs, in declaration order.
|
||||
///
|
||||
/// **Derived from the chain, never listed here.** The groups are whatever
|
||||
/// the operations say they are about, so a new operation joins the right
|
||||
/// tab by declaring its nature and this file goes on naming none of them
|
||||
/// (FR-DEV-3a). An attribute nothing carries is left out rather than
|
||||
/// offered as a tab that opens onto nothing.
|
||||
///
|
||||
/// Geometry is excluded: its one operation prefers an on-canvas widget and
|
||||
/// is skipped by the row builder, so a Geometry tab would be empty of rows
|
||||
/// while `GeometryPanel` holds the real controls.
|
||||
pub fn tabs(&self) -> Vec<(dr_pipeline::Attribute, String)> {
|
||||
use dr_pipeline::Attribute;
|
||||
let caps = self.scoped_capabilities();
|
||||
Attribute::ALL
|
||||
.into_iter()
|
||||
.filter(|a| *a != Attribute::Geometry)
|
||||
.filter(|a| {
|
||||
caps.iter()
|
||||
.any(|c| c.attributes.contains(a) && !rows_filtered(&caps, |o| o.attributes.contains(a)).is_empty())
|
||||
})
|
||||
.map(|a| (a, crate::labels::resolve(a.label().0)))
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Which tab is selected, as an index into [`Self::tabs`]. `-1` is "all".
|
||||
pub fn active_tab(&self) -> i32 {
|
||||
let Some(active) = self.active_tab else {
|
||||
return -1;
|
||||
};
|
||||
self.tabs()
|
||||
.iter()
|
||||
.position(|(a, _)| *a == active)
|
||||
.map_or(-1, |i| i as i32)
|
||||
}
|
||||
|
||||
/// Select a tab by its index in [`Self::tabs`], or `-1` for all.
|
||||
pub fn set_active_tab(&mut self, index: i32) {
|
||||
self.active_tab = usize::try_from(index)
|
||||
.ok()
|
||||
.and_then(|i| self.tabs().get(i).map(|(a, _)| *a));
|
||||
}
|
||||
|
||||
fn active_layer(&self) -> Option<&MaskLayer> {
|
||||
let id = self.active_mask.as_ref()?;
|
||||
self.graph.masks().get(id)
|
||||
@@ -254,8 +313,28 @@ pub(crate) fn supported(widget: WidgetKind) -> bool {
|
||||
/// heard of appearing in a generated panel — and it cannot be asserted at all
|
||||
/// if generating a row requires a device.
|
||||
pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec<ParamRow> {
|
||||
rows_filtered(caps, |_| true)
|
||||
}
|
||||
|
||||
/// The panel model for the capabilities `keep` accepts.
|
||||
///
|
||||
/// **`op_index` counts over every capability, not over the kept ones.** It is
|
||||
/// how a row routes back to the core, so filtering must not renumber it — a
|
||||
/// row that survived a filter has to still name the operation it came from.
|
||||
/// `group_head` is the opposite: a position within the *emitted* rows, because
|
||||
/// the panel walks back to it through the model it was given.
|
||||
///
|
||||
/// Getting that backwards is how a slider ends up driving a different
|
||||
/// operation, which is the kind of fault that looks like a rendering bug.
|
||||
pub(crate) fn rows_filtered(
|
||||
caps: &[OpCapability],
|
||||
keep: impl Fn(&OpCapability) -> bool,
|
||||
) -> Vec<ParamRow> {
|
||||
let mut rows = Vec::new();
|
||||
for (op_index, op) in caps.iter().enumerate() {
|
||||
if !keep(op) {
|
||||
continue;
|
||||
}
|
||||
// Where this operation's rows begin. The panel groups by walking
|
||||
// back to it, so it has to be taken before any row is pushed.
|
||||
let group_head = rows.len();
|
||||
@@ -533,10 +612,7 @@ impl DevelopSession {
|
||||
/// here is curve-shaped; it walks whatever parameters the operation
|
||||
/// declares.
|
||||
pub fn reset_op(&mut self, op_index: i32) {
|
||||
let caps = match self.active_layer() {
|
||||
Some(layer) => layer.capabilities(),
|
||||
None => self.graph.capabilities(),
|
||||
};
|
||||
let caps = self.scoped_capabilities();
|
||||
let Some(cap) = usize::try_from(op_index).ok().and_then(|i| caps.get(i)) else {
|
||||
return;
|
||||
};
|
||||
@@ -1182,10 +1258,10 @@ impl DevelopSession {
|
||||
// last described — the indices the interface is holding are positions
|
||||
// in *that* list, and reading the global chain while a layer is
|
||||
// selected would map a slider onto a different operation.
|
||||
let caps = match self.active_layer() {
|
||||
Some(layer) => layer.capabilities(),
|
||||
None => self.graph.capabilities(),
|
||||
};
|
||||
// The *unfiltered* scoped list, because `op_index` counts over every
|
||||
// capability — see `rows_filtered`. Indexing a filtered list here is
|
||||
// how a slider would drive the wrong operation once a tab is chosen.
|
||||
let caps = self.scoped_capabilities();
|
||||
let op = caps.get(usize::try_from(op_index).ok()?)?;
|
||||
let param = op.params.get(usize::try_from(param_index).ok()?)?;
|
||||
Some((op.id, param.id))
|
||||
@@ -3055,6 +3131,110 @@ mod tests {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------------
|
||||
// Group tabs (ARCH §4.3a, FR-DEV-3a)
|
||||
// ----------------------------------------------------------------------
|
||||
|
||||
fn tabbed_session(ctx: &GpuContext) -> DevelopSession {
|
||||
let rgba: Vec<u8> = (0..16 * 16).flat_map(|_| [128, 128, 128, 255]).collect();
|
||||
DevelopSession::open_rgb(ctx, &rgba, 16, 16, dr_types::Orientation::NORMAL)
|
||||
.expect("session")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_tabs_come_from_the_chain_not_from_a_list() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let session = tabbed_session(&ctx);
|
||||
|
||||
let names: Vec<String> = session.tabs().into_iter().map(|(_, n)| n).collect();
|
||||
assert!(names.contains(&"Light".to_string()), "got {names:?}");
|
||||
assert!(names.contains(&"Colour".to_string()), "got {names:?}");
|
||||
// Nothing offers a group with no rows behind it.
|
||||
for (attribute, name) in session.tabs() {
|
||||
let mut probe = tabbed_session(&ctx);
|
||||
let index = probe
|
||||
.tabs()
|
||||
.iter()
|
||||
.position(|(a, _)| *a == attribute)
|
||||
.expect("just listed");
|
||||
probe.set_active_tab(index as i32);
|
||||
assert!(!probe.rows().is_empty(), "tab {name} opens onto nothing");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn choosing_a_tab_narrows_the_panel() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let mut session = tabbed_session(&ctx);
|
||||
|
||||
let all = session.rows().len();
|
||||
session.set_active_tab(0);
|
||||
let narrowed = session.rows().len();
|
||||
|
||||
assert!(narrowed > 0, "a tab must show something");
|
||||
assert!(narrowed < all, "and less than everything: {narrowed} of {all}");
|
||||
}
|
||||
|
||||
/// The trap: `op_index` counts over *every* capability, so a row that
|
||||
/// survived a filter must still route to the operation it came from. If it
|
||||
/// renumbered, a slider would drive a different operation once a tab was
|
||||
/// chosen.
|
||||
#[test]
|
||||
fn a_filtered_row_still_drives_its_own_operation() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let mut session = tabbed_session(&ctx);
|
||||
|
||||
// Find a colour row while unfiltered, and remember where it points.
|
||||
let colour = session
|
||||
.tabs()
|
||||
.iter()
|
||||
.position(|(_, n)| n == "Colour")
|
||||
.expect("the chain has colour operations");
|
||||
session.set_active_tab(colour as i32);
|
||||
|
||||
let row = session.rows().into_iter().next().expect("a row");
|
||||
let (op, param) = (row.op_index, row.param_index);
|
||||
|
||||
session.set_param(op, param, 0.5);
|
||||
let after = session
|
||||
.rows()
|
||||
.into_iter()
|
||||
.find(|r| r.op_index == op && r.param_index == param)
|
||||
.expect("the row survived");
|
||||
|
||||
assert!(
|
||||
(after.value - 0.5).abs() < 1e-5,
|
||||
"the value landed on the row that asked for it, not another"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn all_is_reachable_again() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let mut session = tabbed_session(&ctx);
|
||||
let all = session.rows().len();
|
||||
|
||||
session.set_active_tab(0);
|
||||
assert!(session.rows().len() < all);
|
||||
|
||||
session.set_active_tab(-1);
|
||||
assert_eq!(session.rows().len(), all, "-1 means everything");
|
||||
assert_eq!(session.active_tab(), -1);
|
||||
}
|
||||
|
||||
/// An out-of-range index is navigation nonsense, not an edit; it must not
|
||||
/// leave the panel showing nothing.
|
||||
#[test]
|
||||
fn a_nonsense_tab_falls_back_to_everything() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let mut session = tabbed_session(&ctx);
|
||||
let all = session.rows().len();
|
||||
|
||||
session.set_active_tab(99);
|
||||
assert_eq!(session.rows().len(), all);
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
/// `dr_pipeline`'s morphology, as `dr_segment` names it.
|
||||
|
||||
@@ -13,6 +13,15 @@
|
||||
pub fn resolve(key: &str) -> String {
|
||||
match key {
|
||||
// Operations
|
||||
// The attribute names. Short on purpose: these are read as a strip
|
||||
// of tabs, where a long word crowds out the next one.
|
||||
"attr.tone" => "Light".into(),
|
||||
"attr.colour" => "Colour".into(),
|
||||
"attr.detail" => "Detail".into(),
|
||||
"attr.optics" => "Optics".into(),
|
||||
"attr.geometry" => "Geometry".into(),
|
||||
"attr.effect" => "Effects".into(),
|
||||
|
||||
"op.white_balance" => "White Balance".into(),
|
||||
"op.exposure" => "Exposure".into(),
|
||||
"op.highlights_shadows" => "Highlights & Shadows".into(),
|
||||
|
||||
@@ -519,6 +519,20 @@ pub(crate) fn sync_rows(
|
||||
// and flips of an image that no longer has them.
|
||||
sync_framing(window, session);
|
||||
|
||||
// TRACES: FR-DEV-3a
|
||||
// The groups, derived from what the operations say they are about. This
|
||||
// side resolves the labels; nothing here or in `adjust.slint` names an
|
||||
// operation or a group.
|
||||
let (tabs, active_tab) = match session.borrow().as_ref() {
|
||||
Some(s) => (
|
||||
s.tabs().into_iter().map(|(_, name)| name.into()).collect(),
|
||||
s.active_tab(),
|
||||
),
|
||||
None => (Vec::<slint::SharedString>::new(), -1),
|
||||
};
|
||||
window.set_adjust_tabs(slint::ModelRc::new(slint::VecModel::from(tabs)));
|
||||
window.set_adjust_active_tab(active_tab);
|
||||
|
||||
let current = match session.borrow().as_ref() {
|
||||
Some(s) => s.rows(),
|
||||
None => Vec::new(),
|
||||
@@ -1740,6 +1754,21 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
// The local-adjustment panel. Wired as a block rather than inline because
|
||||
// it is a dozen callbacks that all say the same three things, and they
|
||||
// read better beside each other than scattered through this function.
|
||||
{
|
||||
// Choosing a group re-filters the panel and nothing else — no edit,
|
||||
// no render. It is navigation.
|
||||
let weak = window.as_weak();
|
||||
let session = session.clone();
|
||||
let rows = rows.clone();
|
||||
window.on_adjust_tab_picked(move |index| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
if let Some(s) = session.borrow_mut().as_mut() {
|
||||
s.set_active_tab(index);
|
||||
}
|
||||
sync_rows(&w, &rows, &session);
|
||||
});
|
||||
}
|
||||
|
||||
masks_ui::wire(&window, &session, &rows, &redraw, gpu.clone());
|
||||
|
||||
{
|
||||
|
||||
@@ -630,6 +630,14 @@ export component TransferPanel inherits VerticalLayout {
|
||||
|
||||
export component AdjustPanel inherits Rectangle {
|
||||
in property <[ParamRow]> rows;
|
||||
/// The groups worth offering, derived from what the operations say they
|
||||
/// are about. **This file names none of them** — the strings arrive
|
||||
/// already resolved and the panel only has to draw them, which is what
|
||||
/// keeps a new operation a zero-edit change here (FR-DEV-3a).
|
||||
in property <[string]> tabs;
|
||||
/// Index into `tabs`, or -1 for "everything".
|
||||
in property <int> active-tab: -1;
|
||||
callback tab-picked(int);
|
||||
in property <bool> enabled: true;
|
||||
/// The tone curve's sampled shape, evaluated in Rust by the same spline
|
||||
/// the shader runs so the drawn line cannot disagree with the applied one.
|
||||
@@ -677,6 +685,50 @@ export component AdjustPanel inherits Rectangle {
|
||||
|
||||
if !root.enabled: Caption { text: "No image"; }
|
||||
|
||||
// The group strip. Only when there is more than one group to choose
|
||||
// between — a single tab is a control with one option, which is
|
||||
// decoration.
|
||||
if root.enabled && root.tabs.length > 1: HorizontalLayout {
|
||||
spacing: Theme.gap-sm;
|
||||
alignment: start;
|
||||
|
||||
all := TouchArea {
|
||||
width: 34px;
|
||||
height: Theme.touch-target;
|
||||
mouse-cursor: pointer;
|
||||
clicked => { root.tab-picked(-1); }
|
||||
Label {
|
||||
text: "All";
|
||||
emphasised: root.active-tab == -1 || all.has-hover;
|
||||
vertical-alignment: center;
|
||||
}
|
||||
}
|
||||
|
||||
for tab[i] in root.tabs: tab-area := TouchArea {
|
||||
width: label.preferred-width + 8px;
|
||||
height: Theme.touch-target;
|
||||
mouse-cursor: pointer;
|
||||
clicked => { root.tab-picked(i); }
|
||||
|
||||
label := Label {
|
||||
text: tab;
|
||||
emphasised: root.active-tab == i || tab-area.has-hover;
|
||||
vertical-alignment: center;
|
||||
}
|
||||
|
||||
// The selected group is underlined rather than filled: the
|
||||
// accent means *modified* everywhere else in this interface,
|
||||
// and spending it on "which tab" would blunt the one signal
|
||||
// the panel has (ui-refinement.md, principle 2).
|
||||
Rectangle {
|
||||
y: parent.height - 2px;
|
||||
height: 2px;
|
||||
width: parent.width;
|
||||
background: root.active-tab == i ? Theme.ink : transparent;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// No Flickable here any more. The histogram, the capture metadata and
|
||||
// the geometry controls sat *above* this one and could not be scrolled
|
||||
// away, so on a 280px column in portrait they ate the height the
|
||||
|
||||
@@ -673,6 +673,9 @@ export component AppWindow inherits Window {
|
||||
// do. This window knows the *shape* of a control, never which operations
|
||||
// exist (FR-DEV-3a).
|
||||
in property <[ParamRow]> adjust-rows;
|
||||
in property <[string]> adjust-tabs;
|
||||
in property <int> adjust-active-tab: -1;
|
||||
callback adjust-tab-picked(int);
|
||||
in property <bool> adjust-enabled: false;
|
||||
// The tone curve's sampled shape, evaluated by the core so the drawn
|
||||
// line and the applied one cannot disagree.
|
||||
@@ -1960,6 +1963,9 @@ in property <bool> panel-visible: true;
|
||||
|
||||
adjust := AdjustPanel {
|
||||
rows: root.adjust-rows;
|
||||
tabs: root.adjust-tabs;
|
||||
active-tab: root.adjust-active-tab;
|
||||
tab-picked(i) => { root.adjust-tab-picked(i); }
|
||||
enabled: root.adjust-enabled;
|
||||
curve-samples: root.curve-samples;
|
||||
param-changed(op, param, value) => {
|
||||
|
||||
Reference in New Issue
Block a user