feat: support grayscale mode to save bandwidth (partial)#568
Conversation
Signed-off-by: Zhenyu FU <ysfcore@outlook.com>
📝 WalkthroughWalkthroughThis PR adds an ChangesColor-mode configuration and chroma enum
Estimated code review effort: 1 (Trivial) | ~5 minutes Related Issues: None specified. Related PRs: None specified. Suggested labels: enhancement, config Suggested reviewers: rustdesk maintainers familiar with config.rs and message.proto 🐰 A whisper of chroma, a new mode in sight, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config.rs (1)
2882-2883: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisplaced comment.
// i444 is for backward compatibilitysits directly above theOPTION_COLOR_MODEconstant, but the constant itself has nothing to do with backward compatibility — the compatibility logic lives inUserDefaultConfig::get()(Lines 2385-2390). This is confusing when read in isolation.✏️ Suggested fix
pub const OPTION_I444: &str = "i444"; - // i444 is for backward compatibility + // color-mode superseded OPTION_I444; kept for backward compatibility with old configs pub const OPTION_COLOR_MODE: &str = "color-mode";🤖 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 `@src/config.rs` around lines 2882 - 2883, The backward-compatibility note is attached to the wrong symbol, since OPTION_COLOR_MODE is just a constant and the compatibility behavior is implemented in UserDefaultConfig::get(). Move or rewrite the comment so it sits with the actual compatibility handling in UserDefaultConfig::get(), and keep the OPTION_COLOR_MODE constant comment focused only on the constant itself.
🤖 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 `@src/config.rs`:
- Around line 2882-2883: The backward-compatibility note is attached to the
wrong symbol, since OPTION_COLOR_MODE is just a constant and the compatibility
behavior is implemented in UserDefaultConfig::get(). Move or rewrite the comment
so it sits with the actual compatibility handling in UserDefaultConfig::get(),
and keep the OPTION_COLOR_MODE constant comment focused only on the constant
itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6d2d021-3ad9-45f3-b41f-e78132cabfcc
📒 Files selected for processing (2)
protos/message.protosrc/config.rs
There was a problem hiding this comment.
Pull request overview
This PR introduces a new configurable “color mode” setting (including a grayscale option) intended to reduce bandwidth usage, and extends the protocol chroma enum to represent grayscale.
Changes:
- Added a
color-modeuser default option with logic intended to map legacyi444settings to the new mode. - Added
I400(grayscale) to theChromaenum in the protobuf schema.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/config.rs | Adds OPTION_COLOR_MODE and getter logic to migrate/interpret legacy i444 settings. |
| protos/message.proto | Extends Chroma enum with I400 for grayscale support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| keys::OPTION_COLOR_MODE => { | ||
| let v = self.get_string(key, "i420", vec!["i444", "i400"]); | ||
| if v == "i420" { | ||
| if self.options.get(keys::OPTION_I444) == Some(&"Y".to_string()) { | ||
| "i444".to_string() | ||
| } else { | ||
| "i420".to_string() | ||
| } | ||
| } else { | ||
| v | ||
| } | ||
| } |
| pub const OPTION_I444: &str = "i444"; | ||
| // i444 is for backward compatibility | ||
| pub const OPTION_COLOR_MODE: &str = "color-mode"; |
Associated with:
feat: support grayscale mode to save bandwidth (partial) rustdesk#15527
Summary by CodeRabbit