Add option for custom resolution with width param #175
Conversation
WalkthroughAdds a new public enum variant Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Resolution
Note over Resolution: New variant: Custom { width }
Caller->>Resolution: value(aspect_ratio)
alt Custom
Resolution->>Resolution: height = floor(width / aspect_ratio)
Resolution-->>Caller: [width, height]
else Fixed variants
Resolution-->>Caller: [predefined_width, predefined_height]
else Captured
Resolution-->>Caller: panic
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/capturer/mod.rs (1)
23-27: Plan a major semver release and document the newCustomvariantI’ve confirmed that all internal
matchexpressions overResolutioneither explicitly handleCustom(incapturer/mod.rs) or use a catch-all arm (in the engine backends), so there are no missing pattern arms in this repo. However, sinceResolutionis a public enum without anon_exhaustiveattribute, addingCustomis a breaking API change for downstream users who match exhaustively.• Mark this release as breaking (major semver bump) and call out the new
Resolution::Customvariant in your changelog or upgrade guide.
• Optionally, annotate the enum with#[non_exhaustive]if you want to avoid future breaking changes when adding variants.
• Validate that callers never construct a zero-widthCustom { width: 0 }. You can either:
– Add a runtime check at each construction site (e.g.assert!(width > 0)), or
– Switch to usingNonZeroU32for type-level safety:
```rust
use std::num::NonZeroU32;pub enum Resolution { // … Custom { width: NonZeroU32 }, #[default] Captured, } ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/capturer/mod.rs(2 hunks)
src/capturer/mod.rs
Outdated
| Resolution::Custom { width } => { | ||
| [width, width / aspect_ratio.floor() as u32] | ||
| } |
There was a problem hiding this comment.
Fix incorrect height calculation (integer division, zero-division risk).
This uses integer division by aspect_ratio.floor() as u32, which:
- Produces wrong heights (e.g., 16:9 → floor=1 → height==width).
- Can panic for 0 < aspect_ratio < 1 (floor→0, divide-by-zero).
Align with other variants: compute using float division and floor the result.
- Resolution::Custom { width } => {
- [width, width / aspect_ratio.floor() as u32]
- }
+ Resolution::Custom { width } => {
+ let height = (width as f32 / aspect_ratio).floor() as u32;
+ [width, height]
+ }Optional (keep encoder-friendly even dimensions):
- Resolution::Custom { width } => {
- let height = (width as f32 / aspect_ratio).floor() as u32;
- [width, height]
- }
+ Resolution::Custom { width } => {
+ let mut height = (width as f32 / aspect_ratio).floor() as u32;
+ // make even
+ height &= !1;
+ let width = width & !1;
+ [width, height]
+ }Add unit tests (example):
- width=1280, ar=16/9 → [1280, 720]
- width=1000, ar=21/9 → [1000, 428]
- width=800, ar=9/16 (portrait) → [800, 1422]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Resolution::Custom { width } => { | |
| [width, width / aspect_ratio.floor() as u32] | |
| } | |
| Resolution::Custom { width } => { | |
| let height = (width as f32 / aspect_ratio).floor() as u32; | |
| [width, height] | |
| } |
🤖 Prompt for AI Agents
In src/capturer/mod.rs around lines 38-40, the Custom branch computes height
using integer division by aspect_ratio.floor() as u32 which yields wrong results
and can divide-by-zero; replace that with a float-based computation: cast width
to f64, divide by aspect_ratio (as f64), floor the result and cast back to u32
to get the height; add a guard for aspect_ratio == 0.0 (return a sensible
default or error) and optionally make the height even by incrementing if odd;
add unit tests for the provided cases (1280 @ 16/9 -> 720, 1000 @ 21/9 -> 428,
800 @ 9/16 -> 1422).
borisowww
force-pushed
the
add-custom-resolution-option
branch
from
f3add49 to
80d7832
Compare
This PR adds the option to set a custom output resolution based on width input, for edge cases where a lower/higher resolution is needed.
Summary by CodeRabbit
New Features
Notes