charles/loom

Fork 0

feat(tui): #140 entity face crop dialog (manual rect editor) #149

Merged

charles merged 2 commits from tui/face-crop-dialog into main

2026-04-16 09:31:56 +00:00

claude-desktop commented

2026-04-15 18:58:33 +00:00

Collaborator

Summary

New FaceCrop overlay for manually adjusting an entity's face bounding box on its reference image. Opens via c from the entity Detail pane when a face_rect exists, and automatically right after an AI face-detection result so the user can fine-tune before committing.
Keys: arrows move 1px (Shift ×10), +/- grow/shrink centered, [/] width only, ;/' height only. Preset 1 resets to the AI-detected rect, 2 snaps to a centered square using the image's min dimension. Enter persists the normalised rect via storage, Esc discards.
FaceRect moved from the TUI screen to loom_core::models::entity so it lives on Entity. Adds a face_rect_json column to the entities table with a migration, wires serde round-trip through save_entity / row_to_entity.
New AppAction::SetEntityFaceRect for the manual-save path (keeps the existing EntityFaceResult flow for the AI result, which now also opens the editor).

Closes charles/loom#140

Test plan

cargo build -p loom-tui -p loom-core -p loom-gtk -p loom-cli
cargo test -p loom-tui -p loom-core — 157 + core suite passing, including new unit tests
cargo fmt --all + cargo clippy -p loom-tui -p loom-core --no-deps
Manual smoke: TUI entities screen → select a character with reference image → Ctrl+F detects face → overlay opens → arrows/+/-/[/]/;/' adjust → Enter saves, reload confirms the rect persists.

Added unit tests:

face_rect_clamp_within_unit — clamping produces in-bounds rect
apply_manual_face_rect_persists_roundtrip — ±1px arrow → save → load retains value
set_face_rect_persists — face_rect ends up on the focused entity

🤖 Generated with Claude Code

## Summary - New `FaceCrop` overlay for manually adjusting an entity's face bounding box on its reference image. Opens via `c` from the entity Detail pane when a `face_rect` exists, and automatically right after an AI face-detection result so the user can fine-tune before committing. - Keys: arrows move 1px (Shift ×10), `+`/`-` grow/shrink centered, `[`/`]` width only, `;`/`'` height only. Preset `1` resets to the AI-detected rect, `2` snaps to a centered square using the image's min dimension. `Enter` persists the normalised rect via storage, `Esc` discards. - `FaceRect` moved from the TUI screen to `loom_core::models::entity` so it lives on `Entity`. Adds a `face_rect_json` column to the `entities` table with a migration, wires serde round-trip through `save_entity` / `row_to_entity`. - New `AppAction::SetEntityFaceRect` for the manual-save path (keeps the existing `EntityFaceResult` flow for the AI result, which now also opens the editor). Closes charles/loom#140 ## Test plan - [x] `cargo build -p loom-tui -p loom-core -p loom-gtk -p loom-cli` - [x] `cargo test -p loom-tui -p loom-core` — 157 + core suite passing, including new unit tests - [x] `cargo fmt --all` + `cargo clippy -p loom-tui -p loom-core --no-deps` - [ ] Manual smoke: TUI `entities` screen → select a character with reference image → `Ctrl+F` detects face → overlay opens → arrows/`+`/`-`/`[`/`]`/`;`/`'` adjust → `Enter` saves, reload confirms the rect persists. Added unit tests: - `face_rect_clamp_within_unit` — clamping produces in-bounds rect - `apply_manual_face_rect_persists_roundtrip` — ±1px arrow → save → load retains value - `set_face_rect_persists` — face_rect ends up on the focused entity 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-desktop added 1 commit

2026-04-15 18:58:33 +00:00

feat(tui): #140 entity face crop dialog (manual rect editor)

qa / qa (pull_request) Has been cancelled

Details

38304cbc5d

Adds a FaceCrop overlay for adjusting the face rect on a character entity's
reference image. Arrow keys move by 1px (Shift x10), +/- grow/shrink
centered, [] adjusts width, ;' adjusts height. Preset shapes: 1 resets to
the AI-detected rect, 2 centers a square using the image's min dimension.
Enter persists the normalised rect on the Entity via storage; Esc discards.

The overlay opens via `c` from the entity Detail pane when a face_rect
exists, and automatically after an AI face-detection result so the user
can fine-tune before saving.

Moves FaceRect to loom-core::models::entity so it can be persisted on
Entity, adds a face_rect_json column to the entities table with a
migration, and wires save/load via serde_json.

Closes charles/loom#140

claude-desktop reviewed

2026-04-15 19:35:46 +00:00

claude-desktop left a comment

Nice clean split of FaceRect into loom_core::models::entity with a proper DB migration and serde round-trip. Overall solid, a few notes:

Correctness

FaceCropOverlay::reset_centered_square uses side = iw.min(ih) as i32 (full min dimension), whereas the constructor's no-initial fallback uses min/3. With a non-square image the resulting rect will be non-square after clamp_px (height or width will be trimmed to fit), so the "2 centered square" preset won't actually produce a square. Either compute a true centered square using the smaller dimension and use that for both w+h, or rename the preset.
initial_px is wrapped in Option but is always Some(_). Drop the Option — it makes reset_ai simpler.
The yellow border rectangle drawn over the ratatui-image area will be composited on top of the image buffer after render_stateful_widget rewrites it next frame. In practice ratatui-image's protocol handler may overwrite those cells — worth a manual smoke to confirm the outline is actually visible over the rendered image (not just over the placeholder).

Nits

ensure_image swallows every failure silently (path missing, picker lock poisoned, image::open failure). At minimum log at warn so a user with a broken reference image gets a clue.
apply_manual_face_rect and apply_face_result now duplicate 8 lines of persistence logic verbatim. Extract a persist_entity helper.
Shift-multiplier of 10 on +/- is handled by scale(step * 2, step * 2) — i.e. Shift++ grows by 20 pixels. Document that the "×10" in the hint line is only true for arrow translation, not scaling.

Nice clean split of `FaceRect` into `loom_core::models::entity` with a proper DB migration and serde round-trip. Overall solid, a few notes: **Correctness** - `FaceCropOverlay::reset_centered_square` uses `side = iw.min(ih) as i32` (full min dimension), whereas the constructor's no-initial fallback uses `min/3`. With a non-square image the resulting rect will be non-square after `clamp_px` (height or width will be trimmed to fit), so the "2 centered square" preset won't actually produce a square. Either compute a true centered square using the smaller dimension and use that for both w+h, or rename the preset. - `initial_px` is wrapped in `Option` but is always `Some(_)`. Drop the `Option` — it makes `reset_ai` simpler. - The yellow border rectangle drawn over the ratatui-image area will be composited on top of the image buffer after `render_stateful_widget` rewrites it next frame. In practice ratatui-image's protocol handler may overwrite those cells — worth a manual smoke to confirm the outline is actually visible over the rendered image (not just over the placeholder). **Nits** - `ensure_image` swallows every failure silently (path missing, picker lock poisoned, image::open failure). At minimum log at `warn` so a user with a broken reference image gets a clue. - `apply_manual_face_rect` and `apply_face_result` now duplicate 8 lines of persistence logic verbatim. Extract a `persist_entity` helper. - Shift-multiplier of 10 on `+`/`-` is handled by `scale(step * 2, step * 2)` — i.e. Shift+`+` grows by 20 pixels. Document that the "×10" in the hint line is only true for arrow translation, not scaling.

charles added 1 commit

2026-04-15 19:46:43 +00:00

refactor(tui): #140 address review feedback — true centered square, drop Option wrap, DRY persist, log ensure_image failures

qa / qa (pull_request) Has been cancelled

Details

214676a5de

- FaceCropOverlay::reset_centered_square now computes a true centered
  square from the image's min dimension and does not rely on clamp_px
  (which treats w/h independently) to preserve aspect.
- Drop the Option<_> wrap around initial_px — it's always set at
  construction, simplifies reset_ai.
- Extract persist_entity_face_rect helper out of apply_face_result and
  apply_manual_face_rect on EntitiesScreen (DRY the 8 duplicated lines
  of clamp + assign + updated_at + storage.save_entity).
- ensure_image now logs a tracing::warn! with the reason on every
  failure path (missing path, no picker, poisoned lock, image::open
  error) instead of swallowing silently.
- Hint text and overlay doc comment clarified: arrows use Shift ×10,
  scaling uses Shift ×2.

claude-desktop commented

2026-04-15 19:47:07 +00:00

Author

Collaborator

Review feedback addressed in 214676a:

Real bug fix: reset_centered_square now computes a true centered square from min(iw, ih) for both width and height and centers it directly — no longer relies on clamp_px (which treats w/h independently) to preserve aspect.
Polish:
- Dropped Option<_> around initial_px (always Some(_)); reset_ai is now a two-liner.
- Extracted persist_entity_face_rect helper from apply_manual_face_rect + apply_face_result on EntitiesScreen — DRY'd the clamp/assign/updated_at/save_entity block.
- ensure_image now emits tracing::warn! on every failure path (missing path, no picker, poisoned lock, image::open error) with the path for context.
- Hint text + doc comment updated to distinguish arrow Shift ×10 from scaling Shift ×2.

cargo fmt, cargo build, cargo test -p loom-tui, cargo clippy -p loom-tui -- -D warnings all pass.

Review feedback addressed in 214676a: - **Real bug fix**: `reset_centered_square` now computes a true centered square from `min(iw, ih)` for both width and height and centers it directly — no longer relies on `clamp_px` (which treats w/h independently) to preserve aspect. - **Polish**: - Dropped `Option<_>` around `initial_px` (always `Some(_)`); `reset_ai` is now a two-liner. - Extracted `persist_entity_face_rect` helper from `apply_manual_face_rect` + `apply_face_result` on `EntitiesScreen` — DRY'd the clamp/assign/`updated_at`/`save_entity` block. - `ensure_image` now emits `tracing::warn!` on every failure path (missing path, no picker, poisoned lock, `image::open` error) with the path for context. - Hint text + doc comment updated to distinguish arrow `Shift ×10` from scaling `Shift ×2`. `cargo fmt`, `cargo build`, `cargo test -p loom-tui`, `cargo clippy -p loom-tui -- -D warnings` all pass.

charles force-pushed tui/face-crop-dialog from 214676a5de

qa / qa (pull_request) Has been cancelled

Details

to 5edd4f741b

qa / qa (pull_request) Successful in 17m13s

Details

2026-04-16 08:58:37 +00:00

Compare

charles merged commit 970dfa4fec into main

2026-04-16 09:31:56 +00:00

charles deleted branch tui/face-crop-dialog

2026-04-16 09:31:56 +00:00