dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Document trash, collections, thumbnails, and the UI direction

Browse Source 
Specs for the work that follows: soft delete via a MOVE that preserves the
remote id, the collection tree and smart collections, the thumbnail store,
and the derived-state folder.

Adds two design documents. ui-refinement.md names the structural gaps
between the v0.1 UI and something that feels like a photo editor.
view-composition.md proposes a view controller for the display layer,
against the 500-line run() that has become one by accretion.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-09 20:36:23 +02:00
co-authored by Claude Opus 5
parent 5365123d92
commit 02b66ddfcf
6 changed files with 1335 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/view-composition.md
+296
View File
@@ -0,0 +1,296 @@
# View composition: a controller for the display layer
TRACES: FR-UI-1 | FR-UI-6 | FR-UI-8 | FR-DEV-3a | NFR-P9
**Status:** Draft · 2026-08-09
**Companion to:** [architecture.md](architecture.md) §4.3a, [ui-refinement.md](ui-refinement.md)
## Why
`dr-ui` has four views — launch, library, develop, collections — and no view
layer. `run()` in `ui/dr-ui/src/lib.rs` is 500 lines that construct every
controller, wire every cross-view callback, own the develop session, and hold
the only complete picture of what is on screen. It has become the controller
by accretion rather than by design, and it shows in three specific ways.
**The back-patched knot.** `run()` declares
```rust
let open_from_library: Rc<RefCell<Option<Rc<dyn Fn(String)>>>> = ...
```
wired empty at line 328 and filled at line 604, because the library grid needs
to open an image in develop and the develop closure needs the GPU context that
is built after the library is wired. The comment in the source is candid about
it: *"This cell is the knot between them: wired empty here, filled once `show`
exists."* A nullable function slot resolved at runtime is what a dependency
cycle looks like when the language will not let you write one directly.
**Two copies of load-and-display.** `show` (lib.rs:524) and the remote-fetch
timer body (lib.rs:657) run the same sequence after their inputs diverge: set
`load_error` empty, set camera / exposure / dimensions from metadata, branch on
whether sensor data was recovered, then either `set_adjust_enabled(true)` +
`sync_rows` + `redraw`, or clear the session, empty the rows, disable adjust
and show the fallback image — and on failure, clear the session and report the
error. One takes a path and one takes bytes; everything downstream is written
twice.
The shared preamble has *already* been factored out into `reset_view_state`,
which both branches call, so the direction is established — the post-load half
is simply the part that has not been done yet. The two halves have not yet
drifted in the fields they set; the argument for consolidating is to keep it
that way, since every future display field must currently be added in two
places.
**Diffuse ownership of window state.** Distinct `window.set_*` calls by module:
| Module | Distinct window properties written |
|---|---|
| `library_ui` | 24 |
| `lib.rs` (`run`) | 23 |
| `launch_ui` | 16 |
| `collections_ui` | 9 |
No one owns "what is on screen". The clearest symptom is view switching
itself: `show-launch` and `show-library` are two booleans encoding one piece
of state, written from seven call sites across three modules
(`lib.rs:353`, `launch_ui.rs:171`, `library_ui.rs:246/254/1486/1645/1655`).
Nothing prevents both being true, and `app.slint` compensates with
`if !root.show-launch && root.show-library` chains at lines 349, 386 and 491.
`develop.rs` is the exception that proves the point: it writes no window
properties at all, taking capabilities in and returning rows and images out.
It is the one module already shaped the way this document argues for.
None of this is broken. It works, and several of the surrounding patterns are
load-bearing and correct — the mpsc-plus-timer worker shape exists because
Slint's event loop must never block (NFR-P9), and `sync_rows` mutates rows in
place because replacing the model breaks slider dragging. This document
changes ownership, not threading and not Slint model handling.
## Non-goals
- No change to the threading model. Workers stay on threads; results still
return through channels drained by Slint timers (NFR-P9).
- No change to the pipeline, catalog, sync layer, or render path.
- No change to how operations reach the adjust panel. `develop.rs` already
composes from capabilities and names no operation (FR-DEV-3a); that stays.
- **No dynamic view instantiation.** See the constraint below.
## The constraint that shapes all of this
Slint has no runtime component instantiation. Views are selected by statically
compiled conditionals over properties — `if root.show-launch: LaunchScreen`
and friends — so every view must exist in the `.slint` source at build time.
Descriptors therefore drive **composition and chrome**: which views exist,
their labels, their order, whether each is currently available, and which one
is active. They cannot conjure a view body. This is a smaller claim than
"self-describing modules" might suggest, and stating it here is deliberate:
a design that assumed otherwise would hit the wall at codegen.
`build.rs` already generates `theme.slint` from `style.yaml`, so generating a
tab bar from the descriptor set is *possible* later. It is not proposed now —
generated `.slint` is markedly harder to debug than generated tokens, and the
win does not yet justify it.
---
## Stage 1 — `ViewController`
Independently landable, and worth landing whether or not stages 2 and 3
follow.
A `ViewController` owns the window handle and the state that currently floats
in `run()`'s closure captures: `session`, `entries`, `index`, `viewport`,
`rows`, and `redraw`. Sub-controllers are held by it rather than wired to each
other through `run()`.
```rust
pub struct ViewController {
window: slint::Weak<AppWindow>,
gpu: Option<GpuContext>,
session: RefCell<Option<DevelopSession>>,
entries: RefCell<Vec<PathBuf>>,
index: Cell<usize>,
viewport: Cell<(u32, u32)>,
rows: Rc<slint::VecModel<ParamRow>>,
library: Rc<LibraryController>,
collections: Rc<CollectionsController>,
launch: Rc<LaunchController>,
}
```
Three changes follow, in order:
**1a. One `present()`.** Both load routes converge on a single method:
```rust
impl ViewController {
fn present(&self, loaded: Result<Loaded, String>, name: &str) { ... }
}
```
The local path calls `self.present(load(gpu, path), &name)`; the remote timer
calls `self.present(load_bytes(gpu, &bytes), &name)`. The duplicated post-load
sequence exists once, so a new display field can only be added in one place.
**1b. `Rc<ViewController>` replaces the nullable callback cell.** The grid's
click handler captures the controller and calls
`controller.open_remote(path)`. Both sides now depend on the controller rather
than on each other, so the cycle disappears and the `Option` slot with it.
**1c. Collapse the four adjust callbacks.** `on_param_changed` (lib.rs:702),
`on_param_reset` (716), `on_reset_all` (730) and `on_curve_reset` (747) are
four blocks differing only in which `DevelopSession` method they call; each
then runs the identical `sync_rows` + `redraw` pair. A single
`self.mutate_session(|s| ...)` helper that performs the mutation and then
re-syncs makes the shared tail impossible to forget.
Note that not every handler wants that tail — the view/pan handlers below them
deliberately `redraw` without `sync_rows`, because panning changes no
parameter. The helper must therefore be the *opinionated* path for parameter
mutation, not a mandatory funnel for everything that touches the session.
**Verification.** These are refactors with no behavioural change: existing
tests in `lib.rs` (`describe_camera`, `describe_exposure`, `collect`) must pass
untouched, and manual checks cover launch → library → develop, next/previous,
slider drag, reset, and canvas resize.
---
## Stage 2 — The `View` trait
The same discipline `descriptor.rs` already applies to operations, applied one
level up. That precedent matters: the core publishes `OpDescriptor` /
`ParamKind` / `Presentation`, and `develop.rs` builds controls from it without
naming a single operation. Views are the same shape of problem.
```rust
pub struct ViewDescriptor {
pub id: ViewId,
pub label: LocalizedKey,
pub kind: ViewKind,
}
/// Closed, not a string — a controller must be able to match exhaustively
/// and know it has covered everything. Same reasoning as `WidgetKind`.
pub enum ViewKind {
/// Occupies the window alone; no chrome, not tabbable. Launch.
Modal,
/// Participates in the tab set. Library, develop.
Primary,
/// Renders beside a primary view. Collections.
Adjunct,
}
pub enum Availability {
Available,
/// Greyed out, with a reason the UI can show. Not hidden — a missing
/// tab is indistinguishable from a bug.
Unavailable(LocalizedKey),
}
pub trait View {
fn descriptor(&self) -> ViewDescriptor;
fn availability(&self, ctx: &AppContext) -> Availability;
fn activate(&self, ctx: &AppContext) {}
fn deactivate(&self, ctx: &AppContext) {}
}
```
Two properties are carried up from `descriptor.rs` deliberately, because they
are why that design works:
- **`ViewKind` is a closed enum.** Its `WidgetKind` counterpart says so
explicitly: *"a UI must be able to match exhaustively and know it has
covered everything the core can ask for."*
- **Descriptors are hints with a working fallback.** A curve degrades to
sliders. A view whose `kind` a shell does not implement still renders
standalone; nothing about tabbing is required for a view to function.
Labels are `LocalizedKey`, reusing the type `dr-pipeline` already exports —
`dr-ui` depends on `dr-pipeline`, so this adds no coupling, and it keeps view
labels on the same footing as operation labels. `labels::resolve` already takes
a `&str` and derives a readable fallback for uncatalogued keys, so a new view
appears with a sensible label before anyone writes a translation, exactly as a
new operation does today.
`availability` is the part that earns the trait rather than merely tidying.
Develop-without-a-GPU and collections-without-a-catalog are handled
inconsistently today — `run()` logs a warning and sets `backend` to "NO GPU",
`library_ui` guards each catalog access separately — and the differences are
not intentional. One method, asked before a view is offered, makes those cases
uniform and gives the shell something honest to display.
`activate`/`deactivate` exist so switching away can stop timers and release
GPU resources instead of leaking them, which nothing does today.
**Scope check.** Four views, all known, all in-tree, no third-party authors.
This abstraction has to justify itself against an `if` chain, and on tab
composition alone it would be close. The `availability` consolidation is what
tips it, because that is a correctness fix rather than a tidiness one.
**Verification.** Stage 2 retrofits the trait to the four existing views and
changes no Slint. The descriptors must reproduce current behaviour exactly
before stage 3 consumes them.
---
## Stage 3 — Data-driven view switching
With descriptors in place, the boolean pair collapses:
```
- in property <bool> show-launch;
- in property <bool> show-library;
+ in property <string> active-view;
+ in-out property <[TabEntry]> view-tabs;
```
`app.slint`'s conditionals key off `active-view == "library"` rather than a
two-boolean conjunction, and the illegal both-true state stops being
representable. `view-tabs` is a model the controller publishes from the
descriptor set, carrying label, id, and availability — so the tab bar is built
from data even though the view bodies are static.
Only `ViewController` writes `active-view`. The seven scattered `set_show_*`
call sites become `controller.activate(ViewId::Library)`, which is also the
hook where `deactivate` on the outgoing view runs.
**Verification.** Behavioural parity on every transition currently reachable:
launch → library on sign-in, library → develop on cell click, develop →
library on back, and the startup paths in `launch::Startup` (all three arms).
---
## Sequencing
| Stage | Depends on | Independently valuable |
|---|---|---|
| 1 — `ViewController` + `present()` | — | Yes: removes the cycle and the duplication |
| 2 — `View` trait | 1 | Yes: uniform availability handling |
| 3 — `active-view` | 2 | Yes: illegal states unrepresentable |
Stage 1 first regardless. The descriptor layer needs a controller to live in,
and the `present()` duplication is a live bug source independent of how views
are composed.
## Requirements
Stages 1 and 2 are traced by existing IDs — FR-UI-6 (shared components),
FR-DEV-3a (self-describing), NFR-P9 (no UI-thread blocking). Stage 3's claim,
that view composition is data-driven and view identity single-valued, has no
requirement covering it. Proposed for `requirements.md` §3.5, after FR-UI-7:
> **FR-UI-8 — Self-describing views.** Each view publishes a descriptor —
> identity, label key, kind, and current availability — and a single
> controller composes the interface from the descriptor set. View identity is
> single-valued: exactly one primary view is active at a time. A view that
> cannot currently function reports why, and the shell presents it as
> unavailable rather than omitting it. Adding a view requires no change to the
> shell beyond registering it and declaring its body.
Not added to the register by this document; adding it is a separate edit,
since `requirements.md` is the register of record and renumbering there ripples
into `traceability.md`.