Skip to content

Canvas: clip the canvas Ui to its own rect so nothing it paints or hit-tests reaches the notes pane (#1922) - #1970

Open
Naseq wants to merge 1 commit into
storytold:mainfrom
Naseq:fix-1922-canvas-clip-notes
Open

Naseq wants to merge 1 commit into
storytold:mainfrom
Naseq:fix-1922-canvas-clip-notes

Conversation

@Naseq

@Naseq Naseq commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1922.

Root cause

Inside the CentralPanel the notes pane is a bottom Panel shown inside the same Ui, so the canvas is given a smaller available_rect, but the Ui's clip rect still covers the whole central area (notes pane included). The slide texture and video frames go through ui.painter_at(avail) and were fine; the media control bar does not: media_bar puts it 6 pt under the clip and control_bar paints with ui.painter_at(bar.expand(2)), which only intersects with the Ui's clip — the whole panel. A video or audio clip dragged to the bottom edge of the slide therefore had its bar drawn over the notes text (and taking clicks there), while the notes ScrollArea bars, painted in their own pass, stayed on top: the inconsistent layering in the report.

Change

crates/ui-egui/src/canvas.rs: canvas::show clips its Ui to the rect it was given (ui.set_clip_rect(avail.intersect(ui.clip_rect()))). Every painter derived from that Ui afterwards — the media bar, selection handles, GIF buttons, scroll bars, the drag overlays — is bounded by the canvas, and ui.interact uses the same clip, so the bar can no longer take a click through the notes pane. The notes pane, status bar and side panes are unaffected (they paint before the canvas with their own Uis). This is the "notes pane is editor chrome above the canvas" behaviour the issue asks for, done once at the boundary instead of per painter.

Verification

  • New media_bar_tests::the_media_bar_never_paints_or_clicks_into_the_notes_pane: an audio clip across the slide's bottom edge with the notes pane open, so the bar's row falls inside the pane. Asserts the bar's background has no visible area below the canvas edge, and that a click where the bar would be does not start playback but focuses the notes field. Fails on main (the bar is painted into the notes pane: [[589.7 749.0] - [819.7 775.0]]), passes with the change.
  • cargo test -p deckcraft-ui-egui --lib: 496 passed (every canvas, drag, ruler, guides and media test still passes with the clip in place).

Summary by CodeRabbit

  • Bug Fixes
    • Canvas content is now clipped to its visible area when it extends beyond the slide boundary.
    • Audio controls no longer appear or respond in the speaker-notes area. Clicking where a control would otherwise appear focuses the notes field instead of starting playback.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The canvas is now clipped to the intersection of its available rectangle and the existing UI clip rectangle. A regression test checks media-bar painting and clicks near the speaker-notes pane.

Changes: Canvas and speaker-notes clipping

Layer / File(s) Summary
Clip canvas rendering and interactions
crates/ui-egui/src/canvas.rs, crates/ui-egui/src/media_bar_tests.rs
show restricts canvas rendering and interaction to the intersection of the available rectangle and existing UI clip rectangle. The regression test checks that a media bar does not paint into the notes pane or start playback when clicked there.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: kinower, aiongg, dylanmtaylor

Merge Risk: Minimal · up to 0a07e

The canvas clipping change addresses painting and clicks at the notes boundary. No actionable merge-blocking risk is established; normal checks remain appropriate.

Pre-merge checks: Passed 5

Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title states the main change: the canvas Ui is clipped to its own rectangle, so nothing it paints or hit-tests reaches the notes pane. The raw summary confirms this change in canvas.rs, where show…
Linked Issues check Passed Issue #1922 is the only directly linked, open issue, so its coding requirements apply. Its expected behavior is that the canvas clips video at the canvas/notes boundary and that notes-pane content, in…
Out of Scope Changes check Passed The whole-PR diff against the merge base changes two files. crates/ui-egui/src/canvas.rs adds one clip-rect statement with an explanatory comment that cites #1922. `crates/ui-egui/src/media_bar_test…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Title check

Explanation

The title states the main change: the canvas Ui is clipped to its own rectangle, so nothing it paints or hit-tests reaches the notes pane. The raw summary confirms this change in canvas.rs, where show clips the UI to the intersection of its available rect and existing clip rect. The objectives describe the same change, linked to issue #1922. The title's PR number (#1922) differs from the PR number in the objectives (#1970), and it matches the linked issue number instead. This mismatch does not affect whether the title describes the change.

Full details: Linked Issues check

Explanation

Issue #1922 is the only directly linked, open issue, so its coding requirements apply. Its expected behavior is that the canvas clips video at the canvas/notes boundary and that notes-pane content, including its scrollbar, stays above the video. At the reviewed head, canvas::show calls ui.set_clip_rect(avail.intersect(ui.clip_rect())) before any painting or ui.interact call. Every painter derived from that Ui afterwards is bounded by the canvas rect. This covers the media bar, GIF buttons, scroll bars, and drag overlays that use ui.painter() or ui.interact. Video frames in canvas/media.rs (media_frames) paint through ui.painter_at(avail), so they are bounded to the canvas rect. The notes pane is a bottom Panel laid out before canvas::show, so the canvas no longer paints into the notes area. The new test the_media_bar_never_paints_or_clicks_into_the_notes_pane covers the audio media bar case and asserts that the bar has no visible area below the canvas edge and that a click there focuses the notes field. Limits of this evidence: the test covers an audio bar, not a video frame or a video drag, and the PR's own root-cause text says video frames were already bounded before this change. The reported video overlap therefore relies on the drag-overlay path, which I could not read in full because canvas.rs output was truncated. I did not run the test suite.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff against the merge base changes two files. crates/ui-egui/src/canvas.rs adds one clip-rect statement with an explanatory comment that cites #1922. crates/ui-egui/src/media_bar_tests.rs adds one regression test for the same layering bug. Both changes implement the linked issue's layering requirement and its automated test. No unrelated behavior change appears in the reviewed diff.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

Clippy (1.99.1)

Clippy execution timed out

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 48e2203 and 0a07e9e.

Files selected for processing: 2

  • crates/ui-egui/src/canvas.rs
  • crates/ui-egui/src/media_bar_tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@aiongg aiongg added the bugfix Fixes something that is broken: wrong result, crash, data loss label Nov 8, 2026
@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for looking into the feedback!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for looking into the feedback!

You are interacting with an AI system.

@aiongg aiongg 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.

AI-generated review (Opus). Not a maintainer decision.

No blocking problems found.

Clipping the canvas Ui to the rect it was given bounds everything the canvas paints or hit-tests after that point, including the media control bar that hangs below a clip. That bar could previously spill into the speaker-notes pane.

  • crates/ui-egui/src/canvas.rs:159 intersects with the parent clip instead of replacing it, so the canvas never widens its clip.
  • The media bar is placed 6 pt under the clip (crates/ui-egui/src/canvas/media.rs:64), and control_bar paints with painter_at(rect.expand(2.0)) (crates/ui-egui/src/media.rs:473). The new Ui clip now bounds both the bar's painting and its hit area.
  • canvas::show is the last call in the CentralPanel closure (crates/ui-egui/src/app.rs:341-345), so the notes pane, thumbnails and side panes are not affected by the new clip.
  • The GIF buttons (crates/ui-egui/src/canvas/media.rs:21), the ruler markers and the scroll-bar hit areas also use this Ui, so they get the same bound.

Claims checked:

  • verified: canvas::show clips its Ui to the rect it was given (crates/ui-egui/src/canvas.rs:159 calls ui.set_clip_rect(avail.intersect(ui.clip_rect())) right after avail is taken at line 153, before any painting or interaction.)
  • verified: The slide texture and video frames were already clipped; the media bar was not, because it sits 6 pt below the clip and its painter only intersects with the Ui clip (crates/ui-egui/src/canvas.rs:252 builds painter_at(avail), which is used for the texture (line 286) and media_frames (line 289). crat…
  • verified: The bar can no longer take a click through the notes pane (crates/ui-egui/src/media.rs:472 control_bar registers its hit areas through the Ui passed from crates/ui-egui/src/canvas/media.rs:66. egui's Ui::interact intersects each hit rect with the Ui clip rect, which is now the canvas rect.)
  • verified: Selection handles, GIF buttons, scroll bars and drag overlays are bounded by the canvas (crates/ui-egui/src/canvas.rs:344-347 and 356-361 paint through painter (derived from ui at line 252) or through ui itself, both after the clip at line 159. crates/ui-egui/src/canvas/media.rs:21 uses ui.painter(…
  • verified: The notes pane, status bar and side panes are unaffected (crates/ui-egui/src/app.rs:341-345: notes_pane runs before canvas::show inside the CentralPanel closure, and canvas::show is its last call. The side panes are shown outside that closure at lines 333-340.)
  • verified: Fixes #1922 (canvas content painted over the speaker-notes pane) (crates/ui-egui/src/canvas.rs:159 bounds everything the canvas paints after it. The bar case the issue describes, painted over the notes text while the notes ScrollArea bars stay above it, comes from the bar at crates/ui-egui/src/canv…

Reviewed at 0a07e9e.

Recommendation: merge after CI and checks pass.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for looking into the feedback. Let me know if you have any questions.

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update. Let me know if you have questions about the feedback.

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update. Let me know if you have questions about the feedback.

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for looking into the feedback!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for the update!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for looking into the feedback!

You are interacting with an AI system.

@Naseq

Naseq commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Naseq Thanks for looking into the feedback!

You are interacting with an AI system.

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

bugfix Fixes something that is broken: wrong result, crash, data loss

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Video frame paints over speaker-notes text but under the notes scrollbar while dragging

2 participants