Offer the film stock in its own group, and let its list scroll itself
Two faults in one control, both reported from the tablet. The stock picker appeared in every group. It is not a parameter, so it is not a row, so the filter that hides every other control when a group is chosen never saw it — "Kodachrome" sat at the top of Light, of Colour and of Detail alike. Three places it does not belong, and the one it does no more prominent than the rest. The descriptor has said `Effect` and only `Effect` since the film moved there; nothing was asking it. So the panel now asks. It cannot ask directly — a generated panel may not know which operation a control belongs to — so the session answers, from what the operation declares it is about, and a stock re-declared as something else would move on its own. The flag is recomputed when the group changes as well as when the film does, which is the half that would have made it stale exactly when it mattered. And the open list was unbounded, so it made the develop column taller and the column scrolled as one: reaching Velvia dragged every slider below it off the screen, an answer given once pushing aside the controls used constantly. It now scrolls within a bounded height of its own. That viewport is counted rather than measured, for the reason the tool rail records a few files away: a viewport that asks a layout how tall it wants to be, while the layout takes its height from the viewport, is a cycle Slint settles by handing back the height it was given — and the content is then clipped in silence rather than scrolling. Every row here is one fixed height, so multiplying is exact. The group rule has a test. The scrolling does not, and cannot: it is a layout, and a layout fault is invisible to the compiler and to every assertion that can be written about it.
This commit is contained in:
@@ -1093,6 +1093,31 @@ impl DevelopSession {
|
||||
.map_or(-1, |i| i as i32)
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// Whether the film stock belongs in the group currently on screen.
|
||||
///
|
||||
/// The stock is not a parameter, so it is not a [`ParamRow`] and the tab
|
||||
/// filter that hides every other control never reached it: the picker was
|
||||
/// drawn above the rows in *every* group, so "Kodachrome" sat at the top of
|
||||
/// Light, of Colour and of Detail alike. Three places it does not belong,
|
||||
/// and the one it does was no more prominent than the rest.
|
||||
///
|
||||
/// Answered here rather than in the panel because it is a question about
|
||||
/// the operation — what is this control *about* — and the panel is not
|
||||
/// allowed to know. It asks the descriptor, so a stock that were ever
|
||||
/// re-declared as something other than an effect would move on its own.
|
||||
pub fn film_in_group(&self) -> bool {
|
||||
let Some(active) = self.active_tab else {
|
||||
// "All" shows everything, the stock included.
|
||||
return true;
|
||||
};
|
||||
self.graph
|
||||
.capabilities()
|
||||
.iter()
|
||||
.find(|c| c.id == dr_pipeline::ops::film_sim::ID)
|
||||
.is_some_and(|c| c.attributes.contains(&active))
|
||||
}
|
||||
|
||||
/// 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)
|
||||
@@ -7757,6 +7782,46 @@ mod tests {
|
||||
/// Asserted over the real chain rather than a fixture, because the failure
|
||||
/// was a property of what is actually declared: a fixture would have to be
|
||||
/// written to reproduce it and would then only prove itself.
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// The film stock is offered in its own group and in "All", nowhere else.
|
||||
///
|
||||
/// It is not a parameter, so it is not a row, so the filter that hides
|
||||
/// every other control when a group is chosen never saw it: the picker sat
|
||||
/// at the top of Light, of Colour and of Detail alike. Three places it does
|
||||
/// not belong, and the one it does no more prominent than the rest.
|
||||
///
|
||||
/// Asserted against whatever the operation actually declares rather than
|
||||
/// against a named group, so a stock re-declared as something else moves
|
||||
/// here on its own and this test still describes the rule.
|
||||
#[test]
|
||||
fn the_film_stock_is_offered_only_where_it_belongs() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let (mut session, _) = grey_session(&ctx);
|
||||
|
||||
let film = session
|
||||
.graph
|
||||
.capabilities()
|
||||
.into_iter()
|
||||
.find(|c| c.id == dr_pipeline::ops::film_sim::ID)
|
||||
.expect("the film stock is in the chain");
|
||||
|
||||
session.set_active_tab(-1);
|
||||
assert!(
|
||||
session.film_in_group(),
|
||||
"\"All\" hides nothing, so the stock is offered there"
|
||||
);
|
||||
|
||||
for (i, (attribute, label)) in session.tabs().into_iter().enumerate() {
|
||||
session.set_active_tab(i as i32);
|
||||
let belongs = film.attributes.contains(&attribute);
|
||||
assert_eq!(
|
||||
session.film_in_group(),
|
||||
belongs,
|
||||
"the stock is offered in {label} but the operation does not claim it"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_lone_parameter_is_named_after_its_operation() {
|
||||
let graph = EditGraph::default_chain();
|
||||
|
||||
@@ -815,6 +815,14 @@ pub(crate) fn sync_film(window: &AppWindow, session: &Rc<RefCell<Option<DevelopS
|
||||
})
|
||||
.unwrap_or(0);
|
||||
|
||||
// Whether the picker belongs in the group on screen. Asked of the session
|
||||
// rather than decided here: it is a fact about what the operation is
|
||||
// about, and the answer moves if the descriptor ever does.
|
||||
let in_group = session
|
||||
.borrow()
|
||||
.as_ref()
|
||||
.is_none_or(|s| s.film_in_group());
|
||||
|
||||
let can_print = choices
|
||||
.get(selected)
|
||||
.and_then(|(id, _)| *id)
|
||||
@@ -830,6 +838,7 @@ pub(crate) fn sync_film(window: &AppWindow, session: &Rc<RefCell<Option<DevelopS
|
||||
.collect::<Vec<_>>(),
|
||||
)));
|
||||
adjustments.set_film_selected(selected as i32);
|
||||
adjustments.set_film_in_group(in_group);
|
||||
adjustments.set_film_can_print(can_print);
|
||||
adjustments.set_film_print(chosen.map(|(_, print)| print).unwrap_or(false));
|
||||
}
|
||||
@@ -2711,6 +2720,11 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
s.set_active_tab(index);
|
||||
}
|
||||
sync_rows(&w, &rows, &session);
|
||||
// The stock picker is filtered by group like everything else, and
|
||||
// it is not a row — so the thing that decides whether it is on
|
||||
// screen has to be recomputed here as well, or it would answer for
|
||||
// whichever group happened to be open when the image was loaded.
|
||||
sync_film(&w, &session);
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -1035,6 +1035,13 @@ export global Adjustments {
|
||||
//
|
||||
// Names are supplied already resolved, and the list is whatever profiles
|
||||
// are installed: this file does not know what a Kodachrome is.
|
||||
/// TRACES: FR-DEV-3f
|
||||
/// Whether the stock picker belongs in the group on screen.
|
||||
///
|
||||
/// Decided in Rust from what the operation says it is about, because a
|
||||
/// stock is not a parameter and so is not filtered by the row builder that
|
||||
/// hides everything else. This file still names no group.
|
||||
in property <bool> film-in-group: true;
|
||||
in property <[string]> film-stocks;
|
||||
/// Index into `film-stocks`. Zero is the first entry, which Rust makes
|
||||
/// "no film" — so a fresh photograph selects it without a sentinel.
|
||||
@@ -1190,6 +1197,10 @@ export component AdjustPanel inherits Rectangle {
|
||||
/// is a lid, and two drawings of this panel may honestly have their lids
|
||||
/// in different positions.
|
||||
private property <bool> film-expanded: false;
|
||||
/// One stock. Shorter than a touch target on purpose — see the row below.
|
||||
private property <length> film-row-height: 32px;
|
||||
/// How much column the open list may take before it scrolls instead.
|
||||
private property <length> film-list-max: 320px;
|
||||
|
||||
background: Theme.surface;
|
||||
|
||||
@@ -1246,7 +1257,7 @@ export component AdjustPanel inherits Rectangle {
|
||||
// Above the rows, because it decides what they mean: the exposure
|
||||
// slider below is a slider on *this stock's* characteristic curve, and
|
||||
// the same number is a different picture on a different film.
|
||||
if Develop.enabled && Adjustments.film-stocks.length > 1: VerticalLayout {
|
||||
if Develop.enabled && Adjustments.film-in-group && Adjustments.film-stocks.length > 1: VerticalLayout {
|
||||
spacing: Theme.gap-sm;
|
||||
|
||||
// **Collapsed to one row, and opened only to change it.**
|
||||
@@ -1300,8 +1311,28 @@ export component AdjustPanel inherits Rectangle {
|
||||
}
|
||||
}
|
||||
|
||||
if root.film-expanded: VerticalLayout {
|
||||
// **Its own scroller, bounded.** Two dozen stocks laid out at full
|
||||
// height made the list the tallest thing in the column, so the
|
||||
// develop panel scrolled as one and reaching Velvia dragged every
|
||||
// slider below it off the screen — the answer to a question that
|
||||
// is asked once pushing aside the controls used constantly.
|
||||
//
|
||||
// The viewport is counted rather than measured, for the reason the
|
||||
// tool rail records: a viewport that asks a layout how tall it
|
||||
// wants to be, while the layout takes its height from the viewport,
|
||||
// is a cycle Slint settles by handing back the height it was given
|
||||
// — and the content is then clipped in silence instead of
|
||||
// scrolling. Every row here is `film-row-height` by construction,
|
||||
// so multiplying is exact.
|
||||
if root.film-expanded: Flickable {
|
||||
height: min(Adjustments.film-stocks.length * root.film-row-height, root.film-list-max);
|
||||
viewport-width: self.width;
|
||||
viewport-height: Adjustments.film-stocks.length * root.film-row-height;
|
||||
|
||||
list := VerticalLayout {
|
||||
height: parent.viewport-height;
|
||||
spacing: 0px;
|
||||
alignment: start;
|
||||
|
||||
for stock[i] in Adjustments.film-stocks: film-row := TouchArea {
|
||||
// Shorter than a touch target, which is a deliberate
|
||||
@@ -1339,6 +1370,7 @@ export component AdjustPanel inherits Rectangle {
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Only for a negative. A reversal stock has no paper — it is the
|
||||
// photograph as it comes — so offering the choice would be
|
||||
|
||||
Reference in New Issue
Block a user