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

Support changing the mouse pointer #37882

Merged
merged 15 commits into from
Feb 25, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/srv/desktop/rdp/rdpclient/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1101,7 +1101,7 @@ fn create_config(width: u16, height: u16, pin: String) -> Config {
// https://github.com/FreeRDP/FreeRDP/blob/4e24b966c86fdf494a782f0dfcfc43a057a2ea60/libfreerdp/core/settings.c#LL49C34-L49C70
client_dir: "C:\\Windows\\System32\\mstscax.dll".to_string(),
platform: MajorPlatformType::UNSPECIFIED,
no_server_pointer: true,
no_server_pointer: false,
autologon: true,
pointer_software_rendering: false,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,47 @@ function TdpClientCanvas(props: Props) {
}
}, [client, clientOnPngFrame]);

let previousCursor = 'auto';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you need useRef here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't hurt for sure, I've added it


useEffect(() => {
if (client) {
const canvas = canvasRef.current;
const updatePointer = (pointer: {
data: ImageData | boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a short JSDoc comment explaining that if data is boolean then true means that we want to show a cursor and false means we want to hide it.

hotspot_x?: number;
hotspot_y?: number;
}) => {
if (typeof pointer.data === 'boolean') {
if (pointer.data) {
canvas.style.cursor = previousCursor;
} else {
previousCursor = canvas.style.cursor;
canvas.style.cursor = 'none';
}
return;
}
const cursor = document.createElement('canvas');
cursor.width = pointer.data.width;
cursor.height = pointer.data.height;
cursor.getContext('2d').putImageData(pointer.data, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably doesn't change much in this case, but pointer.data returns a color space that we can pass to the canvas:

        cursor
          .getContext('2d', { colorSpace: pointer.data.colorSpace })
          .putImageData(pointer.data, 0, 0);

canvas.style.cursor =
'url(' +
cursor.toDataURL() +
') ' +
pointer.hotspot_x +
' ' +
pointer.hotspot_y +
', auto';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can rewrite this with string interpoloation:

canvas.style.cursor = `url('${cursor.toDataURL()') ${pointer.hotspot_x} ${pointer.hotspot_y} auto`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you can, changed

};

client.on(TdpClientEvent.POINTER, updatePointer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for consistency, I'd use client.addListenter (or client.off in line 133).


return () => {
client.removeListener(TdpClientEvent.POINTER, updatePointer);
};
}
}, [client]);

useEffect(() => {
if (client && clientOnBmpFrame) {
const canvas = canvasRef.current;
Expand Down
43 changes: 37 additions & 6 deletions web/packages/teleport/src/ironrdp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl FastPathProcessor {
user_channel_id,
// These should be set to the same values as they're set to in the
// `Config` object in lib/srv/desktop/rdp/rdpclient/src/client.rs.
no_server_pointer: true,
no_server_pointer: false,
pointer_software_rendering: false,
zmb3 marked this conversation as resolved.
Show resolved Hide resolved
}
.build(),
Expand All @@ -184,6 +184,7 @@ impl FastPathProcessor {
cb_context: &JsValue,
draw_cb: &js_sys::Function,
respond_cb: &js_sys::Function,
update_pointer_cb: &js_sys::Function,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's document the signature of this function like we do for the others

/// `tdp_fast_path_frame: Uint8Array`
///
/// `cb_context: tdp.Client`
///
/// `draw_cb: (bitmapFrame: BitmapFrame) => void`
///
/// `respond_cb: (responseFrame: ArrayBuffer) => void`

Copy link
Contributor

Choose a reason for hiding this comment

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

It might also be worth documenting the meanings of the different options for the data param.

) -> Result<(), JsValue> {
self.check_remote_fx(tdp_fast_path_frame)?;

Expand Down Expand Up @@ -211,13 +212,19 @@ impl FastPathProcessor {
UpdateKind::Region(region) => {
outputs.push(ActiveStageOutput::GraphicsUpdate(region));
}
UpdateKind::PointerDefault
| UpdateKind::PointerHidden
| UpdateKind::PointerPosition { .. }
| UpdateKind::PointerBitmap(_) => {
warn!("Pointer updates are not supported");
UpdateKind::PointerDefault => {
outputs.push(ActiveStageOutput::PointerDefault);
}
UpdateKind::PointerHidden => {
outputs.push(ActiveStageOutput::PointerHidden);
}
UpdateKind::PointerPosition { .. } => {
warn!("Pointer position updates are not supported");
continue;
}
UpdateKind::PointerBitmap(pointer) => {
outputs.push(ActiveStageOutput::PointerBitmap(pointer))
}
}
}

Expand All @@ -240,6 +247,30 @@ impl FastPathProcessor {
ActiveStageOutput::Terminate => {
return Err(JsValue::from_str("Terminate should never be returned"));
}
ActiveStageOutput::PointerBitmap(pointer) => {
let data = &pointer.bitmap_data;
let image_data = create_image_data_from_image_and_region(
data,
InclusiveRectangle {
left: 0,
top: 0,
right: pointer.width - 1,
bottom: pointer.height - 1,
},
)?;
update_pointer_cb.call3(
cb_context,
&JsValue::from(image_data),
&JsValue::from(pointer.hotspot_x),
&JsValue::from(pointer.hotspot_y),
)?;
}
ActiveStageOutput::PointerDefault => {
update_pointer_cb.call1(cb_context, &JsValue::from(true))?;
}
ActiveStageOutput::PointerHidden => {
update_pointer_cb.call1(cb_context, &JsValue::from(false))?;
}
_ => {
debug!("Unhandled ActiveStageOutput: {:?}", output);
}
Expand Down
4 changes: 4 additions & 0 deletions web/packages/teleport/src/lib/tdp/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export enum TdpClientEvent {
WS_OPEN = 'ws open',
WS_CLOSE = 'ws close',
RESET = 'reset',
POINTER = 'pointer',
}

export enum LogType {
Expand Down Expand Up @@ -348,6 +349,9 @@ export default class Client extends EventEmitterWebAuthnSender {
},
(responseFrame: ArrayBuffer) => {
this.sendRDPResponsePDU(responseFrame);
},
(data: ImageData | boolean, hotspot_x?: number, hotspot_y?: number) => {
this.emit(TdpClientEvent.POINTER, { data, hotspot_x, hotspot_y });
}
);
} catch (e) {
Expand Down