Skip to content

ui: use theme colors for graphics layer selection - #1905

Open
sanyamsingh001 wants to merge 4 commits into
storytold:mainfrom
sanyamsingh001:codex/graphics-layer-selection-oct14
Open

sanyamsingh001 wants to merge 4 commits into
storytold:mainfrom
sanyamsingh001:codex/graphics-layer-selection-oct14

Conversation

@sanyamsingh001

@sanyamsingh001 sanyamsingh001 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Selected graphics layers used a fixed dark fill that did not follow the active theme. Selected rows now use the theme’s selection fill and accent outline, following the existing Graphics Templates treatment.

Validation on Linux:

  • Formatting, Clippy, workspace tests, layering, assets and WASM gates passed on unchanged source: 3,488 tests passed, 44 ignored; 69 assets attributed and 49 WASM crates checked. Ignored tests and optional external oracles are not claimed as executed.
  • Default-feature desktop build passed.
  • Native Dark and Medium checks produced four captures, all visually reviewed. The original phase then timed out waiting for an inspection response after switching to Light; that failed phase remains recorded.
  • A separate continuation passed both Light selection cases, with both captures visually reviewed, and restored the generated fixture state and original preferences. History comparisons cover public labels, order and count only.

Depends on #1749, including its unchanged #1725 and #1639 dependencies. This follow-up changes only the selected-row painting in crates/ui-egui/src/panels/graphics.rs. Original dependency authorship is preserved.

Historical rendered Graphics evidence

Image index and provenance · immutable raw index

Theme Opening selected Closing selected
Dark Original PNG Original PNG
Medium Original PNG Original PNG
Light Original PNG Original PNG

These are six unchanged 1600×980 historical originals from source-manifest SHA-256 e8ad7f5b8af93109f043130f4d6696e89be7e9973325ea367176298caf76605e and executable SHA-256 1875f7943c0d1e4c6c4f8070d724012babf039e0dd8607c70fa939d06faade56. The four Dark/Medium captures came from phase p, which later aborted; the two Light captures came from a separate continuation q. This is not an uninterrupted six-case pass or current-main/published-tree qualification. Static captures do not establish keyboard, screen-reader, reduced-viewport or accessibility-standard coverage. Existing source, dependency, overlap, conflict and review holds remain open.

haranishi and others added 4 commits October 13, 2026 20:33
Match the packaged MIME declarations in newly generated or rewritten opt-in Linux development desktop entries. Retain the existing packaged-field regression unchanged.

Based on PR storytold#1639; this commit changes only the dev desktop generator.
Refs storytold#1288 for context.

Co-authored-by: Codex <codex@openai.com>
Omit clipped SRT/WebVTT sidecar cues whose endpoints round to the same millisecond. Retain intervals that serialize with distinct endpoints, including positive sub-millisecond residues, without changing media export duration.

Based on PR storytold#1725 and its unchanged PR storytold#1639 dependency. This commit changes only the sidecar precision predicate and its regression tests.
Follow-up to storytold#1547.

Co-authored-by: Codex <codex@openai.com>
Use the theme's selected-row fill and accent outline for selected graphics
layers, following the existing Graphics Templates selection treatment.

Co-authored-by: Codex <codex@openai.com>

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself is right, and I checked it against main. I'm holding it as a comment because of what it is stacked on.

What the commit does (851cf2fe, 2 lines in panels/graphics.rs). The selected layer row in the Graphics properties list used a fixed Color32::from_rgb(0x2e, 0x3d, 0x55) fill, which is a dark blue in every theme. It now uses t.row_selected plus a 1.5 px t.accent outline (StrokeKind::Inside). That is exactly what the Graphics Templates cards do on main (graphics_templates.rs:228 for the fill and :248 for Stroke::new(1.5, t.accent), StrokeKind::Inside), so the PR text's "following the existing Graphics Templates treatment" is accurate. Stroke and StrokeKind are already imported in graphics.rs, and no other site uses the old colour (I searched crates for it), so no fixed-colour selection is left behind. No string changes, so no catalogs are affected.

It is the last of four commits. The branch carries 52668d53 (haranishi's linked-selection change), 51d35c32 (the dev_icon MIME declaration, #1725), 8939b155 (the sidecar cue change, #1749) and this one. The text says it "depends on #1749"; the diff is 12 files and 374 lines, and only graphics.rs is this PR's. I did not review the other three here: the one that touches captions is on #1911 and #1749, and the dev_icon line is what #1958 covers. This PR cannot land until those do, or until it is rebased on main with only its own commit, which would make it a 2-line change that can be reviewed and merged at once. I'd recommend that, since nothing in the change needs the other three.

What I cannot check. The text reports Dark, Medium and Light captures reviewed by eye; I cannot see them. Please attach the three captures here so the maintainers can judge the contrast of the layer name on the new fill in the Light theme.

Hosted CI does not build or test filmcraft, so I'm leaving this as a comment. I'll approve once it is rebased to its own commit and the receipt for that exact head is in.

Validation request (head 851cf2fe52ded83e2eba928d095a66bdcfb6aa35):

  • os: linux
  • commands: cargo fmt --all -- --check; cargo clippy --workspace --all-targets --locked --offline -- -D warnings; cargo test -p filmcraft-ui-egui --test graphics_notice_ui --locked --offline; cargo test -p filmcraft-ui-egui --lib --locked --offline graphics
  • expected: all pass, 0 ignored
  • holds: stacked on #1749, #1725 and a selection commit not in main; clippy nonminimal_bool on main (#2007) is not caused by this PR

@FredApps FredApps left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes on the full stacked acceptance scope: the caption precision dependency is not preserved in the tested RC.

The theme-color tip change is not blamed for this defect. This PR head 851cf2fe52ded83e2eba928d095a66bdcfb6aa35 carries the sidecar dependency and its four precision tests. Reapplying those original test assertions and helper as test-only additions to the exact integrated candidate gives two real assertion failures and two passing positive controls: hosted run and per-test artifacts.

Both caption_srt_sidecar_omits_cues_empty_at_millisecond_precision and the corresponding VTT test fail: a 1.00025-second export writes a zero-duration 1.000 -> 1.000 Clipped residue cue instead of omitting it. Both retains_cues_distinct_at_millisecond_precision controls pass at 1.00075 seconds, retaining 1.000 -> 1.001. Native exports have exact 48012/48036 samples at 48 kHz and leave project captions unchanged. Base and candidate produce identical sidecar bytes, so this is an unmet integrated dependency, not a newly introduced theme-color regression.

Please either rebase this PR to its standalone theme change, as previously requested, or ensure the complete stacked sidecar fix and original tests survive integration and provide the exact-head receipt. Do not waive the zero-duration raw serialization assertion merely because parsing the output drops the invalid cue.

Validation target: Windows RC 0.12.0-rc.20261024.1, candidate ab77f2d8329550fb80a8bd9a20dec206f05b6958, base bfc23840b603e69c782302c6ed04d4a9f6ec26a8. Original synthetic fixtures only. Exact-SHA build receipt.

Review scope: measured RC/integration evidence and current-source checks as explicitly distinguished above. Native QA evidence root: validation/rc-20261024/native-session-20261025/. No Adobe material is attached.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants