Skip to content

feat(client): add --codecs #783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 9, 2025
Merged

Conversation

elmarco
Copy link
Contributor

@elmarco elmarco commented Apr 30, 2025

Add "--codecs remotefx:on,..." option to the client.

Depends on #782

@CBenoit
Copy link
Member

CBenoit commented May 9, 2025

Hi @elmarco
Thank you for the PR!
This needs to be rebased following the other PR being merged.

Comment on lines +644 to +662
/// This function generates a list of client codec capabilities based on the
/// provided configuration.
///
/// # Arguments
///
/// * `config` - A slice of string slices that specifies which codecs to include
/// in the capabilities. Codecs can be explicitly turned on ("codec:on") or
/// off ("codec:off").
///
/// # List of codecs
///
/// * `remotefx` (on by default)
///
/// # Returns
///
/// A vector of `Codec` structs representing the codec capabilities, or an error
/// suitable for CLI.
pub fn client_codecs_capabilities(config: &[&str]) -> Result<BitmapCodecs, String> {
use std::collections::HashMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I think this kind of CLI-related logic does not belong to ironrdp-pdu. But I have local changes to improve config handling and support the .RDP file format. Maybe we could revisit afterwards.

@@ -1,3 +1,4 @@
#![allow(clippy::print_stdout)]
Copy link
Member

@CBenoit CBenoit May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick:

Suggested change
#![allow(clippy::print_stdout)]
#![allow(clippy::print_stdout)]

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@CBenoit CBenoit merged commit 3d1762c into Devolutions:master May 9, 2025
10 checks passed
@elmarco
Copy link
Contributor Author

elmarco commented May 9, 2025

thanks! yes always happy to take suggestions or improvements on top!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants