-
Notifications
You must be signed in to change notification settings - Fork 104
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
refactor(web-client): refactor iron-remote-gui
into iron-remote-desktop
#722
base: master
Are you sure you want to change the base?
Conversation
web-client/iron-remote-desktop-rdp/src/services/logging.service.ts
Outdated
Show resolved
Hide resolved
web-client/iron-remote-desktop-rdp/src/services/wasm-bridge.service.ts
Outdated
Show resolved
Hide resolved
web-client/iron-remote-desktop-rdp/src/iron-remote-desktop.svelte
Outdated
Show resolved
Hide resolved
crates/ironrdp-web/src/error.rs
Outdated
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.
This file and sessions.rs contain a good amount of duplicates with IronVNC logic.
Does it make sense to add a new crate(like iron-remote-desktop-rs
) that would describe the interface of the Rust part of the project? It would include all the sharable structures and logic between IronVNC and IronRDP parts.
The components chain would look like this way:
- VNC:
UI (TS) ->iron-remote-desktop-vnc
(TS) ->iron-remote-desktop
(TS) ->iron-remote-desktop-rs
(Rust) ->ironvnc-web
(Rust) - RDP:
UI (TS) ->iron-remote-desktop-rdp
(TS) ->iron-remote-desktop
(TS) ->iron-remote-desktop-rs
(Rust) ->ironrdp-web
(Rust)
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 agree that we could provide a companion crate to avoid further code duplication! 👍
That would be a good follow up!
Let’s call this crate iron-remote-desktop
, and make it live in the IronRDP repository.
Is it possible to export WASM functions from a dependency? My expectation is that it should be possible?
We could also provide a trait + macro helpers to define proper interfaces. Let’s discuss that on Slack!
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.
Decided to do it in a follow-up PR
web-client/iron-svelte-client/src/services/server-bridge.service.ts
Outdated
Show resolved
Hide resolved
I think you are aware of this yourself as far as I can see in your own comments, but this is not really what I imagined when I talked about dependency injection. To clarify: the new I’m not very good at JavaScript/TypeScript, so let me illustrate what I mean using Rust: // -- iron-remote-desktop --
// Implements all the backend-agnostic logic, based on a given "remote desktop interface".
trait RemoteDesktopApi {
fn remote_desktop_init(&self);
}
fn run(api: Box<dyn RemoteDesktopApi>) {
// The logic is calling the remote desktop API, and does not know whether this is RDP or VNC.
api.remote_desktop_init();
}
// -- ironrdp-web --
// This is what actually implements the interface.
// But TypeScript is kind of working in a duck-typed way,
// so unlike Rust you don’t need to import the interface definition in order to implement it!
pub struct Api;
impl RemoteDesktopApi for Api {
fn remote_desktop_init(&self) {
// do stuff
}
}
// -- iron-remote-desktop-rdp --
// The purpose is to inline the WASM module in base64,
// but essentially, it exposes the exact same things as ironrdp-web.
pub use ironrdp_web::Api;
// -- iron-svelte-client --
// Injects the dependency (IronRDP) into the web component (iron-remote-desktop), and uses it.
// It knows that its IronRDP behind the scenes, and may inject additional extensions (future improvement).
iron_remote_desktop::run(Box::new(iron_remote_desktop_rdp::Api)); Because of the way JavaScript/TypeScript works, we don’t need Keep in mind that I’m only trying to illustrate what I meant for dependency injection. I do not ask you to modify drastically any Rust code (ideally My expectation is that export * as default from './path/to/wasm/module' = Re-export the whole interface of the WASM module. You can then import this module in import * as rdp from '@devolutions/iron-remote-desktop-rdp'
// I think we have something called "userInteraction" somewhere?
// Use it to inject the module ("dependency").
userInteraction.setBackend(rdp);
// Or maybe it’s `userInteractionService` which wraps the `userInteraction`?
// Try to look for the `onMount` function.
// We retrieve the `userInteraction` (`event.detail.irgUserInteraction`) and
// store it in a `writeable` that is accessible globally. (Also refer to this section.) If you hit a problem, let me know in Slack so we can think through this together. I never implemented such a thing myself, so I don’t know all the problems you may encounter beforehand in details, but I have a solid idea of what it should looks like. |
Let's clarify: iron-remote-desktop-rdp has to re-export WASM and provide extension types. The next step is to modify the UserInteraction to accept the "backend" which will be either RDP or VNC and implements an interface (I have really missed the fact that JS/TS has a duck types). The next step should be the refactoring of the |
Exactly! |
import type { ClipboardTransaction as IClipboardTransaction } from './interfaces/ClipboardTransaction'; | ||
import { ClipboardContent, ClipboardTransaction } from './../../iron-remote-desktop-rdp/src/main'; | ||
import * as remote_desktop from './../../iron-remote-desktop-rdp/src/main'; | ||
// import { ClipboardContent, ClipboardTransaction } from './../../../../ironvnc/web-client/iron-remote-desktop-vnc/src/main'; |
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.
Here as you can see, we can import VNC instead of an RDP and this will inject selected module into a WasmBridgeService
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.
Wonderful!
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.
Wait, I was expecting this logic to be in iron-svelte-client, not iron-remote-desktop.
Is it possible to choose between injecting IronRDP or injecting IronVNC from iron-svelte-client?
…e interface declarations
…e-desktop` package
…ct` to be able to create interfaces and call them via interfaces; move logic into `iron-remote-desktop` crate as Benoit have asked; implemented dependency injection
3a4a47a
to
f7a4b86
Compare
Coverage Report 🤖 ⚙️Past: New: Diff: +0.00% [this comment will be updated automatically] |
…`iron-remote-desktop` package
f7a4b86
to
b5d6fdf
Compare
#### [`web-client/iron-remote-gui`](./web-client/iron-remote-gui) | ||
#### [`web-client/iron-remote-desktop`](./web-client/iron-remote-desktop) | ||
|
||
TypeScript interfaces exposed by WebAssembly bindings from `ironrdp-web` and used by `iron-remote-desktop-rdp`. | ||
|
||
This crate is an **API Boundary**. | ||
|
||
#### [`web-client/iron-remote-desktop-rdp`](./web-client/iron-remote-desktop-rdp) | ||
|
||
Core frontend UI used by `iron-svelte-client` as a Web Component. | ||
|
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.
note: This needs to be updated
pub fn new() -> Self { | ||
pub fn construct() -> Self { |
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.
question: Is there a reason for moving away from new
here?
self.0.borrow_mut().use_display_control = use_display_control | ||
} | ||
}, | ||
Err(err) => error!("Provided JsValue is not a valid extension value: {err:?}"), |
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.
Err(err) => error!("Provided JsValue is not a valid extension value: {err:?}"), | |
Err(error) => error!(%error, "Provided JsValue is not a valid extension value"), |
@@ -297,11 +301,11 @@ impl SessionBuilder { | |||
loop { | |||
match ws.state() { | |||
websocket::State::Closing | websocket::State::Closed => { | |||
return Err(IronRdpError::from(anyhow::anyhow!( | |||
return Err(RemoteDesktopError::from(anyhow::anyhow!( | |||
"Failed to connect to {proxy_address} (WebSocket is `{:?}`)", |
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.
Not something you added, but this should be:
"Failed to connect to {proxy_address} (WebSocket is `{:?}`)", | |
"failed to connect to {proxy_address} (WebSocket is `{:?}`)", |
match serde_wasm_bindgen::from_value::<SessionExtensionCall>(value) | ||
.map_err(|err| anyhow::anyhow!("provided JsValue is not a valid extension call: {err:?}"))? |
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.
style: Can be simplified:
match serde_wasm_bindgen::from_value::<SessionExtensionCall>(value) | |
.map_err(|err| anyhow::anyhow!("provided JsValue is not a valid extension call: {err:?}"))? | |
match serde_wasm_bindgen::from_value::<SessionExtensionCall>(value) | |
.context("provided JsValue is not a valid extension call")? |
RemoteDesktopError::from(anyhow::anyhow!("received an RDCleanPath error: {error}")) | ||
.with_kind(RemoteDesktopErrorKind::RDCleanPath), |
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.
note: Again, not your code, but it’s better to do this:
RemoteDesktopError::from(anyhow::anyhow!("received an RDCleanPath error: {error}")) | |
.with_kind(RemoteDesktopErrorKind::RDCleanPath), | |
RemoteDesktopError::from(anyhow::Error::new(error).context("received an RDCleanPath error")) | |
.with_kind(RemoteDesktopErrorKind::RDCleanPath), |
pub fn ironrdp_init(log_level: &str) { | ||
pub fn remote_desktop_init(log_level: &str) { |
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.
naming: iron_init
@@ -2,7 +2,7 @@ | |||
|
|||
IronRDP also supports the web browser as a first class target. | |||
|
|||
See the [iron-remote-gui](./iron-remote-gui) for the reusable Web Component, and [iron-svelte-client](./iron-svelte-client) for a demonstration. | |||
See the [iron-remote-desktop-rdp](./iron-remote-desktop-rdp) for the reusable Web Component, and [iron-svelte-client](./iron-svelte-client) for a demonstration. |
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.
issue: The reusable web component is iron-remote-desktop
.
See the [iron-remote-desktop-rdp](./iron-remote-desktop-rdp) for the reusable Web Component, and [iron-svelte-client](./iron-svelte-client) for a demonstration. | |
See the [iron-remote-desktop](./iron-remote-desktop) for the reusable Web Component, and [iron-svelte-client](./iron-svelte-client) for a demonstration. |
iron-remote-desktop-rdp
would be the RDP backend based on IronRDP that we inject (or not) into iron-remote-desktop
.
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.
This should be the README of iron-remote-desktop
, not iron-remote-desktop-rdp
I think
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.
issue: I think pre-build.js should be in iron-remote-desktop
, not iron-remote-desktop-rdp
suggestion: Maybe double check you didn’t needlessly copied too many files into iron-remote-desktop-rdp
?
new_key_released(scancode: number): DeviceEvent; | ||
new_unicode_pressed(unicode: string): DeviceEvent; | ||
new_unicode_released(unicode: string): DeviceEvent; | ||
free(): void; |
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.
question: Do we need functions such as free
in these interfaces?
This is a simple wrapper around the `iron-remote-gui` Web Component demonstrating how to use the API. | ||
This is a simple wrapper around the `iron-remote-desktop-rdp` Web Component demonstrating how to use the API. |
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.
issue: Needs update. The web component is from iron-remote-desktop
. You can also mention iron-remote-desktop-rdp
as the backend. My expectation is that iron-svelte-client
has a dependency on both iron-remote-desktop
and iron-remote-desktop-rdp
.
const IRON_REMOTE_DESKTOP_PATH: &str = "./web-client/iron-remote-desktop"; | ||
const IRON_REMOTE_DESKTOP_PATH_RDP: &str = "./web-client/iron-remote-desktop-rdp"; |
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.
const IRON_REMOTE_DESKTOP_PATH: &str = "./web-client/iron-remote-desktop"; | |
const IRON_REMOTE_DESKTOP_PATH_RDP: &str = "./web-client/iron-remote-desktop-rdp"; | |
const IRON_REMOTE_DESKTOP_PATH: &str = "./web-client/iron-remote-desktop"; | |
const IRON_REMOTE_DESKTOP_RDP_PATH: &str = "./web-client/iron-remote-desktop-rdp"; |
Good work! I like the direction! Let me clarify my expectations further:
|
this.session?.synchronize_lock_keys( | ||
syncScrollLockActive, | ||
syncNumsLockActive, | ||
syncCapsLockActive, | ||
syncKanaModeActive, | ||
); | ||
this.session?.extension_call({ | ||
SynchronizeLockKeys: { | ||
scroll_lock: syncScrollLockActive, | ||
num_lock: syncNumsLockActive, | ||
caps_lock: syncCapsLockActive, | ||
kana_lock: syncKanaModeActive, | ||
}, | ||
}); |
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.
thought: Maybe it’s simpler to keep synchronize_lock_keys
in the common API, the same way as previously. It’s fine if it’s a no-op in ironvnc-web
.
This is the first part of the "Modularize the remote desktop WebComponent" ticket.
PR changes the
web-client
of the IronRDP:iron-remote-gui
packageiron-remote-desktop
package with the TypeScript interfaces and exposed more interfaces from the WASM bindingsiron-remote-desktop-rdp
package which combines WASM bindings and the TypeScript interfaces from theiron-remote-desktop
to create aniron-remote-desktop
HTML tag which is responsible for the RDP connection.xtask
Although GitHub says that there are 5K changes, most of the things are just file moves/renaming from
iron-remote-gui
toiron-remote-desktop
andiron-remote-desktop-rdp
. It doesn't seem to be able to diff it properly.