Skip to content
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

Add new redirection-properties RedirectCameras and RedirectLocation, and a event to allow RdpClient customization before connection #7

Merged
merged 9 commits into from
Sep 2, 2024

Conversation

haefele
Copy link
Contributor

@haefele haefele commented Aug 8, 2024

Hey, it's me again 😄
I added two more redirection-properties (RedirectCameras and RedirectLocation) in this PR.
And also added a event that allows users (like me) to customize the RdpClient, before connection is established.

Would highly appreciate if this PR got merged, and a new version of the Nuget-package could be released.

@haefele haefele closed this Aug 23, 2024
@StefanKoell
Copy link
Member

Thanks for the PR. I don't know why but I wasn't notified by github until you closed the PR. I'm happy to look at it on Monday and merge it/publish a new package with your extensions. Sorry for the delay!

@haefele
Copy link
Contributor Author

haefele commented Aug 26, 2024

It would be nice to get everything merged, but unfortunately I noticed some issues in my PR.

For RedirectCameras to work I'm accessing IMsRdpClientNonScriptable7 - which is available in MsRdpClient11NotSafeForScripting. That's version 12 of the RDP client.
Right now the code only supports version 10 max. So a whole new implementation of IRDPClient would be needed.

Also, I would like to make some more changes, that I already did in my local fork:

  • Add NetworkConnectionType.Automatic
  • Add IRdpClient.AudioQualityMode and RedirectionConfiguration.AudioQualityMode
  • Add IRdpClient.VideoPlaybackMode and RedirectionConfiguration.RedirectVideoRendering
  • Add IRdpClient.KeepAliveInterval and ConnectionConfiguration.ConnectionKeepAliveInterval (that's AdvancedSettings2.keepAliveInterval)

Let me know if you would be okay with all these additional configuration options.
Then I will gladly make the necessary changes to make everything work in this PR (implementing the version 12 of the RDPClient).

@StefanKoell
Copy link
Member

Wohoo! Got notified by github about this. Great!

Sure, all new props and features are welcome. Implementing a new client can be challenging to test on older OS versions but if you implement newer versions, I'm not complaining 😅

Looking forward to the updated PR and thanks for helping out!

@haefele
Copy link
Contributor Author

haefele commented Aug 26, 2024

Thanks GitHub for sending notifications 🥳

Great! Really appreciate how open you are to changes and new features!

About the new client - what would be the best way about implementing it?
Duplicate the current version, change the base-class, and add it to the factory?
Not sure if I can manually test every prop and feature.

Will update the PR either today or tomorrow then!

@haefele haefele reopened this Aug 26, 2024
@StefanKoell
Copy link
Member

About the new client - what would be the best way about implementing it?
Duplicate the current version, change the base-class, and add it to the factory?
Not sure if I can manually test every prop and feature.

Yes, I think that's the best way to do it. If you add the new props and methods to the interface, you need to gracefully (log and ignore) the setting in the older versions which don't support those props/methods. When the factory is able to instantiate the new client, most of the time, it will work then. I had a situation once which was a bit more tricky but I think this was more of an edge case hopefully.

As for testing, there are a couple of scenarios to consider. Sometimes you don't want to "touch" the ActiveX property if the corresponding property has not been set (mostly strings). For bool and int values it should be sufficient to simply set the ActiveX's default in the configuration. Then you just try to set them to a non-default value and see if the client can handle it. That should do it for most cases.

@StefanKoell StefanKoell merged commit 2c14ad7 into royalapplications:main Sep 2, 2024
1 check failed
@StefanKoell
Copy link
Member

LGTM

Will do some more tests and publish nuget soon.

Thanks again for the PR!

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

Successfully merging this pull request may close these issues.

2 participants