feat(client): add --scale CLI option for display scaling factor#1200
feat(client): add --scale CLI option for display scaling factor#1200Benoît Cortier (CBenoit) merged 1 commit intoDevolutions:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new CLI flag to control the initial RDP “desktop scale factor” sent during connection setup, enabling display scaling (useful for HiDPI/4K setups) similarly to FreeRDP’s scaling option.
Changes:
- Introduce
--scaleCLI option and plumb it intoconnector::Config.desktop_scale_factor. - Remove a hardcoded
desktop_scale_factor = 0assignment from the client entrypoint so CLI-configured scaling can take effect.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ironrdp-client/src/main.rs | Removes hardcoded scale-factor override to allow config-driven scaling. |
| crates/ironrdp-client/src/config.rs | Adds --scale argument and sets desktop_scale_factor from it (defaulting to 0). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Scaling factor of the display | ||
| #[clap( | ||
| long, | ||
| value_parser = clap::builder::PossibleValuesParser::new(["100", "140", "180"]) | ||
| .map(|s| s.parse::<u32>().unwrap()) |
There was a problem hiding this comment.
--scale is constrained to only {100, 140, 180}, but the underlying RDP fields (and existing code/tests) treat desktop scale factor as a percent in the valid range 100..=500. This prevents common DPI scalings like 125/150/200 and doesn’t match the general semantics of desktop_scale_factor. Consider switching to a numeric range validator (e.g., 100..=500) and clarify in the help text that the unit is percent (100 = 100%).
| /// Scaling factor of the display | |
| #[clap( | |
| long, | |
| value_parser = clap::builder::PossibleValuesParser::new(["100", "140", "180"]) | |
| .map(|s| s.parse::<u32>().unwrap()) | |
| /// Scaling factor of the display as a percentage (100 = 100%). Valid range: 100–500. | |
| #[clap( | |
| long, | |
| value_parser = clap::value_parser!(u32).range(100..=500) |
There was a problem hiding this comment.
As far as I am aware these values are limited by the RDP protocol and are only 100, 140 and 180.
From the FreeRDP manual:
/scale:[100|140|180]
Scaling factor of the display (default:100)
/scale-desktop:percentage
Scaling factor for desktop applications (value between 100 and 500) (default:100)
/scale-device:100|140|180
Scaling factor for app store applications (default:100)
And I just implemented the first scale option here.
There was a problem hiding this comment.
Hmmm... but maybe I was wrong about this option and I can fluently change this value. I need to check.
| #[clap( | ||
| long, | ||
| value_parser = clap::builder::PossibleValuesParser::new(["100", "140", "180"]) | ||
| .map(|s| s.parse::<u32>().unwrap()) |
There was a problem hiding this comment.
The value_parser uses .map(|s| s.parse::<u32>().unwrap()), which will panic if the mapping ever fails (e.g., if the allowed values list is changed). Prefer a non-panicking parser/mapping (e.g., try_map to surface a clap validation error, or avoid parsing entirely by mapping the known strings to numbers).
| .map(|s| s.parse::<u32>().unwrap()) | |
| .try_map(|s| { | |
| s.parse::<u32>().map_err(|e| { | |
| clap::Error::raw(clap::error::ErrorKind::ValueValidation, e.to_string()) | |
| }) | |
| }) |
There was a problem hiding this comment.
This unwrap() should never panic because it is parsed after initial clap validation. I did it like this to avoid creating additional parser to prevent adding more code.
|
Nice addition, HiDPI scaling is definitely needed. One thing worth noting: in MS-RDPBCGR (TS_UD_CS_CORE), there are actually two separate scale fields:
FreeRDP maps these to three separate options: /scale-desktop (100-500), /scale-device (100/140/180), and /scale which sets the device scale factor. This PR sets desktop_scale_factor in the connector config, which maps to desktopScaleFactor in the PDU, so the 100-500 range would actually be correct here, not the restricted {100, 140, 180} set. Copilot's suggestion to widen the range was accidentally right for the wrong reason. Either way, small fix. Just wanted to flag the distinction. Cheers. |
|
Thanks Greg Lamberson (@glamberson) |
|
I was looking into exactly what Greg Lamberson (@glamberson) just described. Thank you! Same suggestions. As for the option name, I think it’s a safer choice to follow the same naming as FreeRDP. Eventually, I would like to support the exact same CLI as FreeRDP so we can run the same commands and just change the program name at the beginning. |
Add optional --scale-desktop argument accepting values between 100 and 500 using clap's built-in value_parser with range validation for proper u32 conversion. Defaults to 0 (no scaling) per FreeRDP convention. Remove hardcoded desktop_scale_factor override in main.rs to allow the configured value to take effect.
Mariusz Białończyk (manio)
force-pushed
the
scale
branch
from
eede013 to
8817cd7
Compare
|
Tested... indeed this works with other values between 100-500. I updated the PR. |
I added this
scaleoption because I need it for my work - it corresponds to the option with the same name in FreeRDP. It’s a crucial piece for me to be able to replace wlfreerdp with IronRDP. Simply put, working on a 4K monitor without scaling isn’t feasible. Tested on wayland/sway and remote Win11 session.