-
Notifications
You must be signed in to change notification settings - Fork 133
feat: add support for DVC pipe proxy in FFI #938
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
Conversation
42142ed
to
d9512e9
Compare
Coverage Report 🤖 ⚙️Past: New: Diff: -0.03% [this comment will be updated automatically] |
d9512e9
to
c5bbc32
Compare
pub fn next_message(&self) -> Result<Option<Box<DvcPipeProxyMessage>>, Box<IronRdpError>> { | ||
Ok(self.0.next_message().map(DvcPipeProxyMessage).map(Box::new)) | ||
} | ||
|
||
pub fn next_message_blocking(&self) -> Result<Box<DvcPipeProxyMessage>, Box<IronRdpError>> { | ||
let message = self.0.next_message_blocking().map(DvcPipeProxyMessage).map(Box::new)?; | ||
|
||
Ok(message) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I suppose next_message will return None when no message is ready to be received? The conventional naming in Rust is to use try_
: try_next_message
for the non-blocking one, and next_message
for the blocking one. May also be a good idea to add documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming/signature was made consistent with other similar type in FFI: https://github.com/Devolutions/IronRDP/blob/feat/ffi-dvc-proxy/ffi/src/clipboard/windows.rs#L37-L46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see 🤔
I would be in favor of changing that, but can be a follow up!
6355c3c
to
fc4540f
Compare
fc4540f
to
e8b1cb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Oh no, I squashed 🥹 |
No description provided.