Bring master's display and parity work under the new checks
Build and test / Desktop (Linux) (push) Successful in 21m3s
Build and test / Layer separation (push) Successful in 38s
🐳 Android image / Build and push (push) Successful in 2s
Build and test / android-image (push) Successful in 2s
Traceability / Requirement traces (push) Successful in 34s
Build and test / Android (aarch64) (push) Failing after 33m40s
Build and test / Desktop (Linux) (push) Successful in 21m3s
Build and test / Layer separation (push) Successful in 38s
🐳 Android image / Build and push (push) Successful in 2s
Build and test / android-image (push) Successful in 2s
Traceability / Requirement traces (push) Successful in 34s
Build and test / Android (aarch64) (push) Failing after 33m40s
Master moved sixteen commits while these fixes were being written — per-display colour, the frame-budget measurement that decides FR-DSP-2, and a declared node running without being compiled. Merged here rather than on master so the conflicts are resolved where they can be tested. Two files overlapped and neither was interesting. `lib.rs` gained `mod remote` from this branch and `mod display_ui` from master, which git resolved on its own. `docs/traceability.md` is generated, so it was regenerated from the merged tree rather than hand-resolved — hand-editing a generated matrix produces one that agrees with neither side. Coverage reads 59.9% (106/177), up from 55.4%, entirely from master's tagging. The check worth having run is `the_interface_names_no_operation` against master's new `display_ui.rs` and its 195 changed lines of `develop.rs`: a new UI module written without knowledge of this gate passes it. That is the evidence the gate is not merely satisfiable by the code that shipped with it. fmt clean, clippy clean at -D warnings, 2087 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+180
-15
@@ -686,6 +686,22 @@ pub struct DevelopSession {
|
||||
/// open. One value rather than one per operation, for the same reason
|
||||
/// `curve_samples` is one polyline: the panel draws one curve.
|
||||
curve_channel: usize,
|
||||
/// TRACES: FR-DSP-8
|
||||
/// The space the canvas is encoded into, for the display now showing it.
|
||||
///
|
||||
/// **Not part of the edit, and not interface state either.** It is a fact
|
||||
/// about the glass in front of the photographer: the same graph on the
|
||||
/// same file composes differently on a wide-gamut second monitor, and
|
||||
/// neither the sidecar nor the undo stack has any business knowing about
|
||||
/// it. That is also why it lives here rather than on the `EditGraph` —
|
||||
/// `compose_for` deliberately takes the space per call because "the same
|
||||
/// edit goes to the screen in the display's space and to a file in
|
||||
/// whatever the export asks for, and neither is more authoritative".
|
||||
///
|
||||
/// sRGB until the application says otherwise, which is the same answer
|
||||
/// `dr_plat::display`'s fallback gives and means a session constructed in
|
||||
/// a test behaves exactly as it did before this existed.
|
||||
display_space: dr_types::ColourSpace,
|
||||
}
|
||||
|
||||
impl DevelopSession {
|
||||
@@ -756,6 +772,7 @@ impl DevelopSession {
|
||||
show_overlay: false,
|
||||
active_tab: None,
|
||||
curve_channel: 0,
|
||||
display_space: dr_types::ColourSpace::Srgb,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1196,7 +1213,7 @@ fn curve_runs(op: &OpCapability, presentation: &Presentation) -> Option<Vec<Curv
|
||||
}
|
||||
|
||||
let mut runs: Vec<CurveRun> = Vec::new();
|
||||
for id in presentation.params {
|
||||
for id in &presentation.params {
|
||||
// The widget addresses points by offset from the first of its run, so
|
||||
// a run has to be contiguous in the capability list.
|
||||
let at = op.params.iter().position(|p| p.id == *id)?;
|
||||
@@ -2624,6 +2641,30 @@ impl DevelopSession {
|
||||
Some((op.id, param.id))
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// Encode the canvas for a different display from now on.
|
||||
///
|
||||
/// Returns whether anything changed, so a caller polling for window moves
|
||||
/// can redraw only when the answer is genuinely different — the poll runs
|
||||
/// far more often than a monitor is changed, and a redraw per poll would
|
||||
/// undo the point of rendering on demand.
|
||||
///
|
||||
/// Nothing is invalidated here and nothing needs to be. The next
|
||||
/// [`Self::render`] composes against the new space, the pipeline cache
|
||||
/// distinguishes the two shaders by the structure hash the space enters,
|
||||
/// and the mask array — rasterised in source space, sampled through the
|
||||
/// framing — is unaffected because a colour space is not a geometry.
|
||||
pub fn set_display_space(&mut self, space: dr_types::ColourSpace) -> bool {
|
||||
let changed = self.display_space != space;
|
||||
self.display_space = space;
|
||||
changed
|
||||
}
|
||||
|
||||
/// The space the canvas is currently being encoded into.
|
||||
pub fn display_space(&self) -> dr_types::ColourSpace {
|
||||
self.display_space
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-1 | AC-8
|
||||
/// Render at the requested display size and hand back a Slint image.
|
||||
///
|
||||
@@ -2654,11 +2695,24 @@ impl DevelopSession {
|
||||
let (fw, fh) = self.graph.output_size(sw, sh);
|
||||
let (w, h) = fit(fw, fh, width.max(1), height.max(1));
|
||||
|
||||
let shader = self.graph.compose();
|
||||
// TRACES: FR-DSP-8 | FR-DSP-6
|
||||
// **Composed for the display that is showing this canvas**, not for
|
||||
// sRGB. This is the whole of FR-DSP-8's second half arriving at the
|
||||
// pipeline: a display change is a *recomposition* and nothing more,
|
||||
// because the output space was always a parameter of composition and
|
||||
// always entered the structure hash. Moving the window to a P3 panel
|
||||
// therefore costs one shader compile and no pipeline change at all.
|
||||
let space = self.display_space;
|
||||
let shader = self.graph.compose_for(space);
|
||||
|
||||
// Rasterise the masks first: the shader addresses array slices by
|
||||
// index, so the array has to describe *this* stack before it is bound.
|
||||
self.render_with_masks(&shader, w, h, dr_types::ColourSpace::Srgb)?;
|
||||
//
|
||||
// The same `space` to both, necessarily: where a detail stage exists
|
||||
// it is the *last* pass that performs the output transform, and two
|
||||
// halves composed for different spaces would encode the frame twice
|
||||
// or not at all.
|
||||
self.render_with_masks(&shader, w, h, space)?;
|
||||
let texture = self.adjust.output().ok_or("nothing was rendered")?;
|
||||
|
||||
// The import is fallible on format and usage only, and both are fixed
|
||||
@@ -2702,7 +2756,11 @@ impl DevelopSession {
|
||||
/// explicit about. It is in the output colour space, which is what
|
||||
/// FR-DSP-7 asks for — the levels counted are the levels the display will
|
||||
/// show, so a clipped bin means a highlight that is actually gone rather
|
||||
/// than one the transform might still recover. And when the view is zoomed
|
||||
/// than one the transform might still recover. Since FR-DSP-8 that is the
|
||||
/// space of *this display* rather than sRGB, which makes the reading more
|
||||
/// truthful and not less: a highlight that survives on a wide-gamut panel
|
||||
/// and clips on the laptop's screen genuinely is two different facts, and
|
||||
/// the histogram now reports whichever one the photographer is looking at. And when the view is zoomed
|
||||
/// or cropped it describes the visible region, not the whole file: a
|
||||
/// photographer inspecting a highlight at 4× is asking about *that*
|
||||
/// highlight, and a histogram of the parts of the frame off screen would
|
||||
@@ -4480,15 +4538,15 @@ mod tests {
|
||||
presentation: Some(Presentation {
|
||||
// Prefers a gradient handle; this frontend has none, so it
|
||||
// falls back to the next entry, which the canvas does host.
|
||||
widgets: &[WidgetKind::GradientHandle, WidgetKind::CropOverlay],
|
||||
widgets: vec![WidgetKind::GradientHandle, WidgetKind::CropOverlay],
|
||||
demand: WidgetDemand {
|
||||
two_dimensional: true,
|
||||
precise_pointing: false,
|
||||
},
|
||||
params: &[ParamId("a"), ParamId("b")],
|
||||
params: vec![ParamId("a"), ParamId("b")],
|
||||
}),
|
||||
params: vec![param("a"), param("b")],
|
||||
attributes: &[dr_pipeline::Attribute::Tone],
|
||||
attributes: vec![dr_pipeline::Attribute::Tone],
|
||||
};
|
||||
|
||||
assert!(rows_from(&[on_canvas]).is_empty());
|
||||
@@ -4612,7 +4670,7 @@ mod tests {
|
||||
id: ParamId("method"),
|
||||
label: LocalizedKey("param.invented.method"),
|
||||
kind: ParamKind::Enum {
|
||||
variants: &[
|
||||
variants: vec![
|
||||
LocalizedKey("param.invented.method.fast"),
|
||||
LocalizedKey("param.invented.method.exact"),
|
||||
],
|
||||
@@ -4622,7 +4680,7 @@ mod tests {
|
||||
facet: None,
|
||||
},
|
||||
],
|
||||
attributes: &[dr_pipeline::Attribute::Tone],
|
||||
attributes: vec![dr_pipeline::Attribute::Tone],
|
||||
};
|
||||
|
||||
let rows = rows_from(&[invented]);
|
||||
@@ -4660,12 +4718,12 @@ mod tests {
|
||||
label: LocalizedKey("op.grading"),
|
||||
active: false,
|
||||
presentation: Some(Presentation {
|
||||
widgets: &[WidgetKind::ColourWheel],
|
||||
widgets: vec![WidgetKind::ColourWheel],
|
||||
demand: WidgetDemand {
|
||||
two_dimensional: true,
|
||||
precise_pointing: false,
|
||||
},
|
||||
params: &[ParamId("hue"), ParamId("strength")],
|
||||
params: vec![ParamId("hue"), ParamId("strength")],
|
||||
}),
|
||||
params: vec![
|
||||
ParamCapability {
|
||||
@@ -4697,7 +4755,7 @@ mod tests {
|
||||
facet: None,
|
||||
},
|
||||
],
|
||||
attributes: &[dr_pipeline::Attribute::Tone],
|
||||
attributes: vec![dr_pipeline::Attribute::Tone],
|
||||
};
|
||||
|
||||
assert!(!supported(WidgetKind::ColourWheel), "precondition");
|
||||
@@ -5146,15 +5204,15 @@ mod tests {
|
||||
label: LocalizedKey("op.invented_curve"),
|
||||
active: false,
|
||||
presentation: Some(Presentation {
|
||||
widgets: &[WidgetKind::ToneCurve],
|
||||
widgets: vec![WidgetKind::ToneCurve],
|
||||
demand: WidgetDemand {
|
||||
two_dimensional: true,
|
||||
precise_pointing: true,
|
||||
},
|
||||
params: &IDS,
|
||||
params: IDS.to_vec(),
|
||||
}),
|
||||
params: IDS.iter().map(|id| param(*id)).collect(),
|
||||
attributes: &[dr_pipeline::Attribute::Tone],
|
||||
attributes: vec![dr_pipeline::Attribute::Tone],
|
||||
};
|
||||
|
||||
let presentation = plain.presentation.as_ref().expect("declares a widget");
|
||||
@@ -5407,4 +5465,111 @@ mod tests {
|
||||
session.set_active_tab(99);
|
||||
assert_eq!(session.rows().len(), all);
|
||||
}
|
||||
|
||||
// ----------------------------------------------------------------------
|
||||
// Per-display colour (FR-DSP-8)
|
||||
// ----------------------------------------------------------------------
|
||||
|
||||
/// TRACES: FR-DSP-8 | FR-DSP-6
|
||||
/// The canvas is encoded for the display, not always for sRGB.
|
||||
///
|
||||
/// This is the assertion the requirement is actually about. Before it,
|
||||
/// `render` composed `ColourSpace::Srgb` unconditionally, and a second
|
||||
/// monitor with a different profile got sRGB pixels *labelled* as its own
|
||||
/// space by the compositor — the silent wrongness FR-DSP-8 calls a
|
||||
/// correctness defect. If someone re-hardcodes the space, these pixels
|
||||
/// stop differing and this fails.
|
||||
///
|
||||
/// A saturated red is the probe deliberately: it sits near the edge of
|
||||
/// sRGB's gamut, so re-encoding it into a wider one moves it a long way,
|
||||
/// where a mid grey would move by almost nothing in any of the four and
|
||||
/// the test would pass on a broken build.
|
||||
#[test]
|
||||
fn the_canvas_is_encoded_for_the_display_showing_it() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let rgba: Vec<u8> = (0..64 * 64).flat_map(|_| [230u8, 20, 20, 255]).collect();
|
||||
let mut session =
|
||||
DevelopSession::open_rgb(&ctx, &rgba, 64, 64, dr_types::Orientation::NORMAL)
|
||||
.expect("session");
|
||||
|
||||
assert_eq!(
|
||||
session.display_space(),
|
||||
dr_types::ColourSpace::Srgb,
|
||||
"a session starts on the fallback, so nothing changes for a \
|
||||
desktop whose display server will not say otherwise"
|
||||
);
|
||||
let on_srgb = read_back(&ctx, &session.render(64, 64).expect("render"));
|
||||
|
||||
assert!(session.set_display_space(dr_types::ColourSpace::AdobeRgb));
|
||||
let on_wide = read_back(&ctx, &session.render(64, 64).expect("render"));
|
||||
|
||||
assert_ne!(
|
||||
on_srgb, on_wide,
|
||||
"the same edit rendered for two displays produced the same pixels"
|
||||
);
|
||||
|
||||
// And back again, because a photographer dragging a window between
|
||||
// two monitors expects the first one to look as it did rather than to
|
||||
// accumulate a transform.
|
||||
assert!(session.set_display_space(dr_types::ColourSpace::Srgb));
|
||||
let returned = read_back(&ctx, &session.render(64, 64).expect("render"));
|
||||
assert_eq!(on_srgb, returned);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// A move that changes nothing reports nothing, so nothing is redrawn.
|
||||
///
|
||||
/// The window's position is polled twice a second and two displays often
|
||||
/// share a profile. A setter that reported a change every time it was
|
||||
/// called would turn that poll into a redraw loop.
|
||||
#[test]
|
||||
fn setting_the_same_display_space_twice_is_not_a_change() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let rgba: Vec<u8> = (0..8 * 8).flat_map(|_| [128u8, 128, 128, 255]).collect();
|
||||
let mut session =
|
||||
DevelopSession::open_rgb(&ctx, &rgba, 8, 8, dr_types::Orientation::NORMAL)
|
||||
.expect("session");
|
||||
|
||||
assert!(session.set_display_space(dr_types::ColourSpace::DisplayP3));
|
||||
assert!(!session.set_display_space(dr_types::ColourSpace::DisplayP3));
|
||||
assert_eq!(session.display_space(), dr_types::ColourSpace::DisplayP3);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8 | FR-EXP-2
|
||||
/// The display's space is the *canvas's*, and reaches nothing else.
|
||||
///
|
||||
/// A thumbnail goes into a shard that syncs between devices and an export
|
||||
/// claims the space the export dialogue asked for. Letting the monitor in
|
||||
/// front of the photographer decide either would write a file whose
|
||||
/// profile describes the desk it was made at.
|
||||
#[test]
|
||||
fn a_wide_gamut_monitor_does_not_reach_the_thumbnail_or_the_export() {
|
||||
let Some(ctx) = headless() else { return };
|
||||
let rgba: Vec<u8> = (0..32 * 32).flat_map(|_| [230u8, 20, 20, 255]).collect();
|
||||
let mut session =
|
||||
DevelopSession::open_rgb(&ctx, &rgba, 32, 32, dr_types::Orientation::NORMAL)
|
||||
.expect("session");
|
||||
|
||||
let thumb_before = session.render_thumbnail(16).expect("thumbnail");
|
||||
let export_before = session
|
||||
.render_for_export(dr_types::ColourSpace::Srgb)
|
||||
.expect("export")
|
||||
.rgba;
|
||||
|
||||
session.set_display_space(dr_types::ColourSpace::ProPhoto);
|
||||
|
||||
assert_eq!(
|
||||
session.render_thumbnail(16).expect("thumbnail"),
|
||||
thumb_before,
|
||||
"the grid's thumbnail followed the monitor"
|
||||
);
|
||||
assert_eq!(
|
||||
session
|
||||
.render_for_export(dr_types::ColourSpace::Srgb)
|
||||
.expect("export")
|
||||
.rgba,
|
||||
export_before,
|
||||
"an sRGB export followed the monitor"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,411 @@
|
||||
//! TRACES: FR-DSP-8
|
||||
//! Following the canvas from one display to the next.
|
||||
//!
|
||||
//! `dr_plat::display` establishes what each display *is*. This decides which
|
||||
//! of them is showing the canvas right now, keeps the develop session's output
|
||||
//! space pointed at it, and renders at that display's physical pixel size
|
||||
//! rather than its logical one.
|
||||
//!
|
||||
//! # Why a poll and not a callback
|
||||
//!
|
||||
//! Slint reports a window's size and its scale factor and does not report that
|
||||
//! it has moved — there is no `on_moved`, on any backend. What there is, is
|
||||
//! `Window::position()`, so this samples it. A sample is three integers
|
||||
//! compared against three integers and happens twice a second; a monitor is
|
||||
//! changed at human speed, and nothing here runs on the frame path.
|
||||
//!
|
||||
//! The re-survey is what costs something — a Wayland roundtrip pair, or an X11
|
||||
//! property fetch — so it is deliberately *not* done every tick. It runs when
|
||||
//! the geometry actually changed, which is also the moment a monitor could
|
||||
//! have been plugged in or unplugged: both of those move or rescale the window
|
||||
//! on every desktop that does anything sensible.
|
||||
|
||||
use std::cell::{Cell, RefCell};
|
||||
use std::rc::Rc;
|
||||
|
||||
use dr_plat::DisplaySurvey;
|
||||
use slint::ComponentHandle;
|
||||
|
||||
use crate::AppWindow;
|
||||
|
||||
/// How often the window's geometry is sampled.
|
||||
const POLL: std::time::Duration = std::time::Duration::from_millis(500);
|
||||
|
||||
/// The render-target size for a canvas that occupies `logical` logical pixels
|
||||
/// on a display scaled by `scale`.
|
||||
///
|
||||
/// # FR-DSP-8's scaling clause, in one multiplication
|
||||
///
|
||||
/// "Fractional and mixed DPI scaling are handled without resampling artefacts
|
||||
/// in the canvas." The canvas is a `wgpu::Texture` handed straight to the
|
||||
/// compositor (ARCH §6.1), which draws it into a box measured in *logical*
|
||||
/// pixels. Render 1600 logical pixels wide onto a display at 1.25 and the
|
||||
/// compositor has 1600 samples to fill 2000 device pixels, so every frame is
|
||||
/// resampled up by a quarter — a softness that reads like a bad demosaic
|
||||
/// rather than like a scaling bug, which is exactly why the requirement calls
|
||||
/// it out and why it is worth an explicit test.
|
||||
///
|
||||
/// Rendering the physical count instead makes the presentation 1:1. It costs
|
||||
/// what the extra pixels cost — 56% more work at 1.25 — and that cost is the
|
||||
/// requirement: the alternative is not cheaper, it is blurrier.
|
||||
///
|
||||
/// A `scale` of zero or worse is clamped rather than trusted. It arrives from
|
||||
/// the windowing system, and a zero would collapse the render target to one
|
||||
/// pixel and blank the canvas.
|
||||
pub(crate) fn physical(logical: (u32, u32), scale: f32) -> (u32, u32) {
|
||||
let scale = if scale.is_finite() && scale > 0.01 {
|
||||
scale
|
||||
} else {
|
||||
1.0
|
||||
};
|
||||
let convert = |v: u32| ((v as f32) * scale).round().max(1.0) as u32;
|
||||
(convert(logical.0), convert(logical.1))
|
||||
}
|
||||
|
||||
/// The geometry the window had when the display was last resolved.
|
||||
///
|
||||
/// Position *and* scale factor, because either changing can mean a different
|
||||
/// display: dragging across a seam moves the window, and a compositor that
|
||||
/// rescales in place — a fractional-scaling change, or a monitor unplugged
|
||||
/// from under a window — changes only the second.
|
||||
#[derive(Clone, Copy, PartialEq)]
|
||||
struct Geometry {
|
||||
x: i32,
|
||||
y: i32,
|
||||
scale_milli: u32,
|
||||
}
|
||||
|
||||
impl Geometry {
|
||||
fn of(window: &AppWindow) -> Self {
|
||||
let position = window.window().position();
|
||||
Self {
|
||||
x: position.x,
|
||||
y: position.y,
|
||||
// Compared as thousandths rather than as a float: this is an
|
||||
// equality test running twice a second, and a scale factor that
|
||||
// differs in its last bit must not read as a display change and
|
||||
// provoke a re-survey on every tick.
|
||||
scale_milli: (window.window().scale_factor().max(0.0) * 1000.0) as u32,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Everything the window needs to keep the canvas on the right display.
|
||||
pub(crate) struct DisplayWatch {
|
||||
survey: RefCell<DisplaySurvey>,
|
||||
/// Which entry of the survey is showing the canvas.
|
||||
current: Cell<usize>,
|
||||
seen: Cell<Option<Geometry>>,
|
||||
/// The canvas's size in *logical* pixels, as Slint last reported it.
|
||||
///
|
||||
/// Kept so that a scale-factor change can re-derive the physical size
|
||||
/// without waiting for a resize that may never come: moving a window
|
||||
/// between a 1× and a 2× display changes what to render and not how large
|
||||
/// the box is.
|
||||
logical_canvas: Cell<(u32, u32)>,
|
||||
}
|
||||
|
||||
impl DisplayWatch {
|
||||
/// Ask the platform once, at startup.
|
||||
pub(crate) fn probe() -> Rc<Self> {
|
||||
let survey = DisplaySurvey::probe();
|
||||
log::info!(
|
||||
"display server: {}, {} display(s)",
|
||||
survey.server.label(),
|
||||
survey.displays.len()
|
||||
);
|
||||
for display in &survey.displays {
|
||||
log::info!(" {}: {}", display.name, display.profile.describe());
|
||||
}
|
||||
Rc::new(Self {
|
||||
survey: RefCell::new(survey),
|
||||
current: Cell::new(0),
|
||||
seen: Cell::new(None),
|
||||
logical_canvas: Cell::new((1024, 768)),
|
||||
})
|
||||
}
|
||||
|
||||
/// The output space the canvas should be encoded into right now.
|
||||
pub(crate) fn space(&self) -> dr_types::ColourSpace {
|
||||
self.survey.borrow().profile(self.current.get()).space
|
||||
}
|
||||
|
||||
/// Record the canvas's logical size and return the size to render at.
|
||||
pub(crate) fn canvas_resized(&self, window: &AppWindow, logical: (u32, u32)) -> (u32, u32) {
|
||||
self.logical_canvas.set(logical);
|
||||
physical(logical, window.window().scale_factor())
|
||||
}
|
||||
|
||||
/// The size the canvas should be rendered at, from the last known logical
|
||||
/// size and the current scale factor.
|
||||
fn canvas_physical(&self, window: &AppWindow) -> (u32, u32) {
|
||||
physical(self.logical_canvas.get(), window.window().scale_factor())
|
||||
}
|
||||
|
||||
/// Push the About-page readouts.
|
||||
fn publish(&self, window: &AppWindow) {
|
||||
let Some(readouts) = readouts(&self.survey.borrow(), self.current.get()) else {
|
||||
return;
|
||||
};
|
||||
window.set_display_name(readouts.name.into());
|
||||
window.set_display_colour(readouts.colour.into());
|
||||
window.set_display_others(readouts.others.into());
|
||||
}
|
||||
|
||||
/// Re-ask the platform and re-resolve which display is showing the canvas.
|
||||
///
|
||||
/// Returns whether the output space changed, which is what decides a
|
||||
/// redraw. Nothing is pushed into the develop session from here: the
|
||||
/// session is asked for its space on the way into every render (see
|
||||
/// `render_now` in the crate root), so "the canvas is composed for the
|
||||
/// display showing it" is structural rather than something a callback has
|
||||
/// to remember to do. A session opened while the window sits on the second
|
||||
/// monitor is then right on its first frame, without this having to know
|
||||
/// that a photograph was opened at all.
|
||||
fn resolve(&self, window: &AppWindow) -> bool {
|
||||
let before = self.space();
|
||||
*self.survey.borrow_mut() = DisplaySurvey::probe();
|
||||
|
||||
// The window's *centre*, in the display server's own screen
|
||||
// coordinates. A window straddling a seam is showing more of itself on
|
||||
// one side, and its centre says which — where its top-left corner
|
||||
// would flip the transform as soon as one pixel crossed.
|
||||
let position = window.window().position();
|
||||
let size = window.window().size();
|
||||
let centre_x = position.x.saturating_add_unsigned(size.width / 2);
|
||||
let centre_y = position.y.saturating_add_unsigned(size.height / 2);
|
||||
|
||||
let index = self.survey.borrow().containing(centre_x, centre_y);
|
||||
self.current.set(index);
|
||||
self.publish(window);
|
||||
self.space() != before
|
||||
}
|
||||
}
|
||||
|
||||
/// The three strings the About page shows about the session's colour path.
|
||||
pub(crate) struct Readouts {
|
||||
pub name: String,
|
||||
pub colour: String,
|
||||
pub others: String,
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// Turn a survey into what the About page says.
|
||||
///
|
||||
/// A free function over the survey rather than a method that writes into the
|
||||
/// window, because the requirement's visibility clause is the part worth
|
||||
/// asserting: FR-DSP-8 asks for a fallback that is *defined*, and a reason
|
||||
/// that reaches the code but never the screen satisfies half of that. This is
|
||||
/// where a test can hold the strings.
|
||||
///
|
||||
/// `None` only for a survey with no displays at all, which `probe` does not
|
||||
/// produce.
|
||||
pub(crate) fn readouts(survey: &DisplaySurvey, index: usize) -> Option<Readouts> {
|
||||
// Clamped rather than indexed: a monitor unplugged between the survey and
|
||||
// this call leaves an index pointing past the end, and the page must
|
||||
// describe *a* display rather than nothing.
|
||||
let index = if index < survey.displays.len() {
|
||||
index
|
||||
} else {
|
||||
0
|
||||
};
|
||||
let here = survey.displays.get(index)?;
|
||||
|
||||
// The other monitors, listed rather than hidden. FR-DSP-8 is a
|
||||
// multi-monitor requirement and the failure it names — the second display
|
||||
// showing wrong colours — is invisible from the first, so a page that
|
||||
// described only the display it was being read on would report nothing
|
||||
// about the case that matters.
|
||||
let others: Vec<String> = survey
|
||||
.displays
|
||||
.iter()
|
||||
.enumerate()
|
||||
.filter(|(i, _)| *i != index)
|
||||
.map(|(_, d)| format!("{}: {}", d.name, d.profile.describe()))
|
||||
.collect();
|
||||
|
||||
Some(Readouts {
|
||||
name: format!("{} ({})", here.name, survey.server.label()),
|
||||
colour: here.profile.describe(),
|
||||
others: others.join(" · "),
|
||||
})
|
||||
}
|
||||
|
||||
/// Start following the window's display, and answer once immediately.
|
||||
///
|
||||
/// `redraw` is the window's ordinary re-render, so a display change costs
|
||||
/// exactly one recomposition and one dispatch — the property the fused design
|
||||
/// was always going to give us here (`compose_with_framing`'s output space is
|
||||
/// a parameter, and enters the structure hash).
|
||||
pub(crate) fn attach(
|
||||
window: &AppWindow,
|
||||
watch: &Rc<DisplayWatch>,
|
||||
viewport: &Rc<RefCell<(u32, u32)>>,
|
||||
redraw: Rc<dyn Fn(&AppWindow)>,
|
||||
) {
|
||||
// The first answer, before any frame is drawn. Without it the canvas's
|
||||
// first render is sRGB on every desktop and corrects itself half a second
|
||||
// later, which on a wide-gamut panel is a visible flash of the wrong
|
||||
// colour on every image opened.
|
||||
watch.seen.set(Some(Geometry::of(window)));
|
||||
watch.resolve(window);
|
||||
|
||||
let timer = slint::Timer::default();
|
||||
let weak = window.as_weak();
|
||||
let watch = watch.clone();
|
||||
let viewport = viewport.clone();
|
||||
timer.start(slint::TimerMode::Repeated, POLL, move || {
|
||||
let Some(window) = weak.upgrade() else { return };
|
||||
let now = Geometry::of(&window);
|
||||
if watch.seen.get() == Some(now) {
|
||||
return;
|
||||
}
|
||||
watch.seen.set(Some(now));
|
||||
|
||||
// The physical size follows the scale factor, so a move onto a
|
||||
// differently-scaled display changes what to render as well as what
|
||||
// to render it into (FR-DSP-8's two halves arriving together).
|
||||
let size = watch.canvas_physical(&window);
|
||||
let resized = *viewport.borrow() != size;
|
||||
if resized {
|
||||
*viewport.borrow_mut() = size;
|
||||
}
|
||||
|
||||
if watch.resolve(&window) || resized {
|
||||
redraw(&window);
|
||||
}
|
||||
});
|
||||
// The timer stops when it is dropped, and it must outlive this function.
|
||||
// Leaked deliberately rather than threaded through the window's state:
|
||||
// it lives exactly as long as the process, and there is nothing that
|
||||
// could sensibly stop it.
|
||||
std::mem::forget(timer);
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use dr_plat::{Bounds, DisplayInfo, DisplayProfile, DisplayServer, FallbackReason};
|
||||
|
||||
fn two_monitors() -> DisplaySurvey {
|
||||
DisplaySurvey {
|
||||
server: DisplayServer::X11,
|
||||
displays: vec![
|
||||
DisplayInfo {
|
||||
name: "eDP-1".to_string(),
|
||||
bounds: Some(Bounds {
|
||||
x: 0,
|
||||
y: 0,
|
||||
width: 1920,
|
||||
height: 1200,
|
||||
}),
|
||||
profile: DisplayProfile::fallback(FallbackReason::NoWaylandProtocol),
|
||||
},
|
||||
DisplayInfo {
|
||||
name: "DP-2".to_string(),
|
||||
bounds: Some(Bounds {
|
||||
x: 1920,
|
||||
y: 0,
|
||||
width: 2560,
|
||||
height: 1440,
|
||||
}),
|
||||
profile: DisplayProfile {
|
||||
space: dr_types::ColourSpace::DisplayP3,
|
||||
source: dr_plat::ProfileSource::X11RootProperty(
|
||||
"_ICC_PROFILE_1".to_string(),
|
||||
),
|
||||
described_as: Some("EIZO CG279X".to_string()),
|
||||
approximated: true,
|
||||
},
|
||||
},
|
||||
],
|
||||
}
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// The fallback is defined *and visible*, which is what the requirement
|
||||
/// asks for and the half that is easy to leave out.
|
||||
#[test]
|
||||
fn the_about_page_says_which_acquisition_path_the_session_is_on() {
|
||||
let survey = two_monitors();
|
||||
let shown = readouts(&survey, 0).expect("a display");
|
||||
|
||||
assert!(shown.name.contains("eDP-1"), "{}", shown.name);
|
||||
assert!(shown.name.contains("X11"), "{}", shown.name);
|
||||
// A photographer being shown sRGB because the compositor would not say
|
||||
// otherwise must be able to find that out rather than wonder.
|
||||
assert!(shown.colour.contains("sRGB"), "{}", shown.colour);
|
||||
assert!(shown.colour.contains("assumed"), "{}", shown.colour);
|
||||
assert!(
|
||||
shown.colour.contains("colour management"),
|
||||
"the reason for the fallback did not reach the page: {}",
|
||||
shown.colour
|
||||
);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// The second monitor is described on the page read on the first.
|
||||
///
|
||||
/// The requirement's whole subject is the display the reader is *not*
|
||||
/// looking at: "showing wrong colours on the second display is a
|
||||
/// correctness defect". A page that listed only the current one would say
|
||||
/// nothing about the case it exists for.
|
||||
#[test]
|
||||
fn the_other_monitor_is_named_on_the_page_read_from_this_one() {
|
||||
let survey = two_monitors();
|
||||
let shown = readouts(&survey, 0).expect("a display");
|
||||
assert!(shown.others.contains("DP-2"), "{}", shown.others);
|
||||
assert!(shown.others.contains("Display P3"), "{}", shown.others);
|
||||
// Approximated, and saying so. An honest approximation the user can
|
||||
// see is the whole trade made in `dr_plat::display`.
|
||||
assert!(shown.others.contains("nearest to"), "{}", shown.others);
|
||||
assert!(shown.others.contains("EIZO CG279X"), "{}", shown.others);
|
||||
|
||||
// And from the second monitor's point of view, the first is the other.
|
||||
let shown = readouts(&survey, 1).expect("a display");
|
||||
assert!(shown.colour.contains("Display P3"), "{}", shown.colour);
|
||||
assert!(shown.others.contains("eDP-1"), "{}", shown.others);
|
||||
}
|
||||
|
||||
/// A single-monitor desktop leaves the row empty rather than repeating
|
||||
/// itself, which is what makes the row conditional in `settings.slint`.
|
||||
#[test]
|
||||
fn one_display_has_no_others_to_list() {
|
||||
let survey = DisplaySurvey::assumed_srgb(FallbackReason::NoWaylandProtocol);
|
||||
let shown = readouts(&survey, 0).expect("a display");
|
||||
assert!(shown.others.is_empty(), "{}", shown.others);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// Fractional scaling renders more pixels, not the same pixels stretched.
|
||||
#[test]
|
||||
fn a_fractionally_scaled_canvas_is_rendered_at_its_physical_size() {
|
||||
// GNOME's fractional steps, against a canvas of a plausible size. The
|
||||
// failure being guarded is silent: at 1.25 the compositor is handed
|
||||
// 1600 samples for 2000 device pixels and the photograph goes soft.
|
||||
assert_eq!(physical((1600, 1000), 1.25), (2000, 1250));
|
||||
assert_eq!(physical((1600, 1000), 1.5), (2400, 1500));
|
||||
assert_eq!(physical((1600, 1000), 1.75), (2800, 1750));
|
||||
assert_eq!(physical((1600, 1000), 2.0), (3200, 2000));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unscaled_display_renders_exactly_what_it_is_asked_for() {
|
||||
// The other half of the same requirement: 1:1 must stay 1:1 rather
|
||||
// than acquiring a rounding error that resamples every frame by a
|
||||
// fraction of a pixel.
|
||||
assert_eq!(physical((1923, 1081), 1.0), (1923, 1081));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_nonsense_scale_factor_does_not_blank_the_canvas() {
|
||||
// It comes from the windowing system, and a zero would collapse the
|
||||
// render target to a single pixel with no error anywhere.
|
||||
assert_eq!(physical((800, 600), 0.0), (800, 600));
|
||||
assert_eq!(physical((800, 600), f32::NAN), (800, 600));
|
||||
assert_eq!(physical((800, 600), -2.0), (800, 600));
|
||||
// And a canvas that has been collapsed to nothing by a dragged
|
||||
// splitter still has to produce a renderable target.
|
||||
assert_eq!(physical((0, 0), 2.0), (1, 1));
|
||||
}
|
||||
}
|
||||
+43
-1
@@ -23,6 +23,7 @@ mod activity;
|
||||
mod collections_ui;
|
||||
mod derived_sync;
|
||||
mod develop;
|
||||
mod display_ui;
|
||||
mod export;
|
||||
pub mod faces;
|
||||
mod gradient;
|
||||
@@ -1378,8 +1379,20 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
let rows: Rc<slint::VecModel<ParamRow>> = Rc::new(slint::VecModel::default());
|
||||
window.set_adjust_rows(rows.clone().into());
|
||||
// Viewport size, tracked so a re-render after a slider move matches it.
|
||||
//
|
||||
// In *physical* pixels, which is what the render target wants and what
|
||||
// FR-DSP-8 means by handling fractional scaling without resampling. The
|
||||
// one caller that thinks in logical pixels is Slint's resize callback,
|
||||
// and it converts on the way in.
|
||||
let viewport = Rc::new(RefCell::new((1024u32, 768u32)));
|
||||
|
||||
// TRACES: FR-DSP-8
|
||||
// What the displays are, and which one the canvas is on. Probed once here
|
||||
// so that the About page can describe the session's colour path before any
|
||||
// photograph is opened — the acquisition path is a property of the desktop
|
||||
// and not of the image.
|
||||
let display = display_ui::DisplayWatch::probe();
|
||||
|
||||
// Re-render the current session into the canvas.
|
||||
//
|
||||
// Called on every slider change, so it must do no more than run the
|
||||
@@ -1394,10 +1407,26 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
let session = session.clone();
|
||||
let viewport = viewport.clone();
|
||||
let drawn_history = drawn_history.clone();
|
||||
let display = display.clone();
|
||||
Rc::new(move |window: &AppWindow, draft: bool| {
|
||||
let mut slot = session.borrow_mut();
|
||||
let Some(s) = slot.as_mut() else { return };
|
||||
|
||||
// TRACES: FR-DSP-8 | FR-DSP-6
|
||||
// **Every canvas render is encoded for the display showing it.**
|
||||
//
|
||||
// Set here rather than pushed from the display watch, and rather
|
||||
// than set once when a photograph is opened, because this is the
|
||||
// one path every frame takes. A session opened while the window
|
||||
// sits on the second monitor is then correct on its *first* frame
|
||||
// — where a push would leave it sRGB until the next poll, which is
|
||||
// a visible flash of the wrong colour on every image opened.
|
||||
//
|
||||
// Free when nothing has changed: the field is compared before it
|
||||
// is written, and an unchanged output space composes to the same
|
||||
// structure hash and the same cached pipeline.
|
||||
s.set_display_space(display.space());
|
||||
|
||||
// TRACES: FR-DEV-5
|
||||
// Whether undo has anywhere to go, pushed from here because every
|
||||
// edit ends in a redraw and nothing else is on all of their paths:
|
||||
@@ -2571,13 +2600,22 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
|
||||
// Track the canvas size so the adjust pass renders at viewport
|
||||
// resolution rather than sensor resolution (FR-DSP-1).
|
||||
//
|
||||
// TRACES: FR-DSP-8
|
||||
// Slint reports the canvas in *logical* pixels, which is the box the
|
||||
// compositor will draw into and not the number of device pixels it will
|
||||
// fill. `display_ui::physical` converts, so that a fractionally scaled
|
||||
// desktop is presented 1:1 rather than resampled — see that function for
|
||||
// why a soft canvas is the failure being avoided here.
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let viewport = viewport.clone();
|
||||
let redraw = redraw.clone();
|
||||
let display = display.clone();
|
||||
window.on_canvas_resized(move |w_px, h_px| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
let size = (w_px.max(1) as u32, h_px.max(1) as u32);
|
||||
let logical = (w_px.max(1) as u32, h_px.max(1) as u32);
|
||||
let size = display.canvas_resized(&w, logical);
|
||||
if *viewport.borrow() == size {
|
||||
return;
|
||||
}
|
||||
@@ -2586,6 +2624,10 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
});
|
||||
}
|
||||
|
||||
// TRACES: FR-DSP-8 | FR-DSP-6
|
||||
// And which display that canvas is on, from now until the window closes.
|
||||
display_ui::attach(&window, &display, &viewport, redraw.clone());
|
||||
|
||||
// FR-UI-1: layout class from window width. Computed here rather than in
|
||||
// Slint because a property that both derives from and feeds the layout is
|
||||
// a binding loop.
|
||||
|
||||
+18
-5
@@ -275,6 +275,13 @@ export component AppWindow inherits Window {
|
||||
in property <string> adapter: "detecting…";
|
||||
in property <string> backend: "—";
|
||||
in property <int> fps: 0;
|
||||
/// TRACES: FR-DSP-8
|
||||
/// The display showing the canvas and the colour transform it is getting.
|
||||
/// Shown under ABOUT in Settings; see `settings.slint` for why the second
|
||||
/// one is a sentence rather than a name.
|
||||
in property <string> display-name: "detecting…";
|
||||
in property <string> display-colour: "detecting…";
|
||||
in property <string> display-others: "";
|
||||
/// TRACES: FR-UI-2
|
||||
/// Set from `CARGO_PKG_VERSION`, so the About line cannot disagree with
|
||||
/// the binary it is part of.
|
||||
@@ -1267,6 +1274,9 @@ in property <bool> panel-visible: true;
|
||||
fps: root.fps;
|
||||
layout-class: root.layout-class;
|
||||
app-version: root.app-version;
|
||||
display-name: root.display-name;
|
||||
display-colour: root.display-colour;
|
||||
display-others: root.display-others;
|
||||
|
||||
original-budget: root.settings-original-budget;
|
||||
original-unlimited: root.settings-original-unlimited;
|
||||
@@ -1700,11 +1710,14 @@ in property <bool> panel-visible: true;
|
||||
// that far. Below 1:1 it stays smooth, where filtering is
|
||||
// what keeps the image from aliasing.
|
||||
//
|
||||
// This matters even at moderate zoom on a HiDPI display:
|
||||
// the canvas is rendered at *logical* size and Slint scales
|
||||
// it up by the device pixel ratio, so the buffer is
|
||||
// resampled on its way to the screen whatever the pipeline
|
||||
// did.
|
||||
// TRACES: FR-DSP-8
|
||||
// The canvas is rendered at the *physical* pixel size of
|
||||
// the box this image occupies, so `contain` presents it
|
||||
// 1:1 and neither filter is reached at all until the
|
||||
// photographer zooms. That is the point: on a fractionally
|
||||
// scaled desktop the buffer used to be a logical-sized one
|
||||
// that the compositor stretched, and no choice of filter
|
||||
// recovers detail that was never rendered.
|
||||
image-rendering: root.magnified
|
||||
? ImageRendering.pixelated
|
||||
: ImageRendering.smooth;
|
||||
|
||||
@@ -117,6 +117,25 @@ export component SettingsPage inherits Rectangle {
|
||||
in property <string> layout-class;
|
||||
in property <string> app-version;
|
||||
|
||||
/// TRACES: FR-DSP-8
|
||||
/// The display showing the canvas, and the colour it is being given.
|
||||
///
|
||||
/// These are the same kind of thing as the two above — a fact about the
|
||||
/// session, not a setting — with one difference that earns them their own
|
||||
/// row rather than a line in a log. FR-DSP-8's fallback is *defined* to be
|
||||
/// sRGB where the display server will not say otherwise, and a photographer
|
||||
/// being shown sRGB because their compositor has no colour-management
|
||||
/// protocol has no other way to find that out. `display-colour` says which
|
||||
/// of the acquisition paths this session is on, in words.
|
||||
in property <string> display-name;
|
||||
in property <string> display-colour;
|
||||
/// The *other* monitors, where there are any. Empty on a single-display
|
||||
/// desktop, which is why the row below is conditional: FR-DSP-8 is about
|
||||
/// the second display, and the failure it names is invisible from the
|
||||
/// first, so this page has to be readable about a monitor the reader is
|
||||
/// not currently looking at.
|
||||
in property <string> display-others;
|
||||
|
||||
callback original-budget-changed(string);
|
||||
callback original-unlimited-toggled(bool);
|
||||
callback thumbnail-budget-changed(string);
|
||||
@@ -708,10 +727,46 @@ export component SettingsPage inherits Rectangle {
|
||||
Value { text: root.layout-class; horizontal-stretch: 1; }
|
||||
}
|
||||
|
||||
// TRACES: FR-DSP-8
|
||||
// Which display, and what colour it is being sent.
|
||||
HorizontalLayout {
|
||||
spacing: Theme.gap;
|
||||
Label { text: "Display"; }
|
||||
Value {
|
||||
text: root.display-name;
|
||||
horizontal-stretch: 1;
|
||||
overflow: elide;
|
||||
}
|
||||
}
|
||||
|
||||
HorizontalLayout {
|
||||
spacing: Theme.gap;
|
||||
Label { text: "Display colour"; }
|
||||
Value {
|
||||
text: root.display-colour;
|
||||
horizontal-stretch: 1;
|
||||
// Wraps rather than elides: this is the one
|
||||
// value on the page that is a sentence, and
|
||||
// eliding it would cut off the half that says
|
||||
// *why* — which is the half FR-DSP-8 asks for.
|
||||
wrap: word-wrap;
|
||||
}
|
||||
}
|
||||
|
||||
if root.display-others != "": HorizontalLayout {
|
||||
spacing: Theme.gap;
|
||||
Label { text: "Other displays"; }
|
||||
Value {
|
||||
text: root.display-others;
|
||||
horizontal-stretch: 1;
|
||||
wrap: word-wrap;
|
||||
}
|
||||
}
|
||||
|
||||
Caption {
|
||||
text: "Graphics and frame rate describe this session, "
|
||||
+ "not a setting — they are here so a bug report "
|
||||
+ "can quote them.";
|
||||
text: "Graphics, frame rate and display colour "
|
||||
+ "describe this session, not a setting — they "
|
||||
+ "are here so a bug report can quote them.";
|
||||
wrap: word-wrap;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user