Refactor deprecated winit#101
Conversation
📝 WalkthroughWalkthroughThis PR refactors the native backend's initialization and event loop from a monolithic single-phase design into a two-phase architecture that separates EventLoop creation from window/OpenGL setup, and transitions the event loop from closure-based to struct-based ChangesTwo-Phase Init and Event Loop Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bracket-terminal/src/hal/native/init.rs (1)
235-236: 💤 Low valueRedundant re-assignment of
frame_sleep_timeandresize_scaling.These fields are already set in
init_raw(lines 51-52) from the sameplatform_hints. Sinceinit_runtimereceives the sameInitHintsviaNativeInitSettings, this duplicates the assignment. Consider removing these lines or consolidating the initialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/native/init.rs` around lines 235 - 236, The assignments to be.frame_sleep_time and be.resize_scaling in init_runtime are redundant because init_raw already sets these from the same platform_hints; remove the duplicate lines in init_runtime (references: be.frame_sleep_time, be.resize_scaling inside init_runtime) or consolidate by passing the already-populated InitHints/NativeInitSettings through so init_runtime relies on init_raw's initialization (references: init_raw, init_runtime, NativeInitSettings, InitHints, platform_hints).bracket-terminal/src/hal/native/mainloop.rs (1)
359-370: 💤 Low value
resize_surfacecalled twice per resize event.
queue_resizecallsresize_surfaceimmediately (line 364), then stores the event. Whenredrawprocesses the queued event (line 306), it callsresize_surfaceagain with the same size. Consider removing the call inqueue_resizesince the actual resize happens during redraw anyway.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bracket-terminal/src/hal/native/mainloop.rs` around lines 359 - 370, Remove the eager call to resize_surface from queue_resize: instead of calling resize_surface(physical_size) inside queue_resize, only set self.queued_resize_event = Some(ResizeEvent { physical_size, dpi_scale_factor: scale_factor, send_event }); rely on the existing redraw method (which inspects queued_resize_event and calls resize_surface) to perform the actual surface resize; keep the ResizeEvent struct, queued_resize_event field, and the send_event handling unchanged so redraw still triggers any events after resizing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@bracket-terminal/src/hal/native/init.rs`:
- Around line 235-236: The assignments to be.frame_sleep_time and
be.resize_scaling in init_runtime are redundant because init_raw already sets
these from the same platform_hints; remove the duplicate lines in init_runtime
(references: be.frame_sleep_time, be.resize_scaling inside init_runtime) or
consolidate by passing the already-populated InitHints/NativeInitSettings
through so init_runtime relies on init_raw's initialization (references:
init_raw, init_runtime, NativeInitSettings, InitHints, platform_hints).
In `@bracket-terminal/src/hal/native/mainloop.rs`:
- Around line 359-370: Remove the eager call to resize_surface from
queue_resize: instead of calling resize_surface(physical_size) inside
queue_resize, only set self.queued_resize_event = Some(ResizeEvent {
physical_size, dpi_scale_factor: scale_factor, send_event }); rely on the
existing redraw method (which inspects queued_resize_event and calls
resize_surface) to perform the actual surface resize; keep the ResizeEvent
struct, queued_resize_event field, and the send_event handling unchanged so
redraw still triggers any events after resizing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16db8ffb-3b20-40f0-9ae5-7dd160d38bab
📒 Files selected for processing (3)
bracket-terminal/src/hal/native/init.rsbracket-terminal/src/hal/native/mainloop.rsbracket-terminal/src/hal/native/mod.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format all Rust code usingcargo fmt --all
Fix allcargo clippywarnings — the CI enforces-D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocatedtests.rs
Files:
bracket-terminal/src/hal/native/mod.rsbracket-terminal/src/hal/native/mainloop.rsbracket-terminal/src/hal/native/init.rs
🔇 Additional comments (13)
bracket-terminal/src/hal/native/mod.rs (1)
52-62: LGTM!bracket-terminal/src/hal/native/init.rs (3)
24-28: LGTM!
30-78: LGTM!
103-174: LGTM!Also applies to: 176-246
bracket-terminal/src/hal/native/mainloop.rs (9)
118-130: LGTM!
132-145: LGTM!
147-164: LGTM!
166-194: LGTM!
196-288: LGTM!
290-349: LGTM!
379-417: LGTM!
419-434: LGTM!
584-592: LGTM!Also applies to: 642-642, 734-740
1 participant
What
Refactor the native winit main loop to use
ApplicationHandlerandrun_appinstead of the deprecated closure-basedEventLoop::run.Why
ApplicationHandlerlifecycle model withEventLoop::run_app.the native runtime during
resumedCloses #30
Checklist
Required
cargo check --allpassescargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple-crate-versionspassescargo test --allpassesCloses #123)Functional Validation
Configuration & Docs
README.md,ARCHITECTURE.md, or relevant manual pages, if applicable)If Applicable
cargo auditlocally if needed)Summary by CodeRabbit