Repository navigation
Conversation
… canvas paint stay out of the notes pane (storytold#1922)
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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
left a comment
There was a problem hiding this comment.
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.
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
Tip For best results, initiate chat on the files or code changes. @Naseq Thanks for the update! You are interacting with an AI system. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
|
Thanks @coderabbitai[bot] for the review and feedback! Looking into this and addressing it. |
|
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. |
Fixes #1922.
Root cause
Inside the CentralPanel the notes pane is a bottom
Panelshown inside the sameUi, so the canvas is given a smalleravailable_rect, but theUi's clip rect still covers the whole central area (notes pane included). The slide texture and video frames go throughui.painter_at(avail)and were fine; the media control bar does not:media_barputs it 6 pt under the clip andcontrol_barpaints withui.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 notesScrollAreabars, painted in their own pass, stayed on top: the inconsistent layering in the report.Change
crates/ui-egui/src/canvas.rs:canvas::showclips itsUito the rect it was given (ui.set_clip_rect(avail.intersect(ui.clip_rect()))). Every painter derived from thatUiafterwards — the media bar, selection handles, GIF buttons, scroll bars, the drag overlays — is bounded by the canvas, andui.interactuses 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
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 onmain(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