Support changing the mouse pointer by probakowski · Pull Request #37882 · gravitational/teleport

probakowski · 2024-02-07T23:12:36Z

This change allows server to send mouse pointer updates to properly indicate e.g. resizing with arrows.

changelog: Implemented dynamic mouse pointer updates to reflect context-specific actions, e.g. window resizing.

github-actions · 2024-02-07T23:13:19Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

ibeckermayer

Suggestion for changelog:

Implemented dynamic mouse pointer updates to reflect context-specific actions, e.g. window resizing.

zmb3 · 2024-02-09T06:49:13Z

Are we software rendering the pointer on canvas now? I thought you mentioned that this was just using the CSS cursor property which seems like it would be more efficient.

…Canvas.tsx Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

…Canvas.tsx Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

probakowski · 2024-02-10T00:05:51Z

@zmb3 we do render it to temp canvas once, to convert raw pixels received from RDP to png - only format widely supported by browsers as cursor. We then set it as cursor for the canvas we render the whole desktop on.
We don't have to manually render cursor on each mouse move, only on cursor change. Could be probably improved by hashing cursor data from RDP and using it as caching key for png data we generate.

zmb3 · 2024-02-21T16:24:33Z

    }
  }, [client, clientOnPngFrame]);

+  let previousCursor = 'auto';


Do you need useRef here?

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

zmb3 · 2024-02-21T16:26:08Z

+          'url(' +
+          cursor.toDataURL() +
+          ') ' +
+          pointer.hotspot_x +
+          ' ' +
+          pointer.hotspot_y +
+          ', auto';


I think you can rewrite this with string interpoloation:

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

Yep, you can, changed

gzdunek · 2024-02-22T07:51:33Z

+        } ${pointer.hotspot_y}, auto`;
+      };
+
+      client.on(TdpClientEvent.POINTER, updatePointer);


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

gzdunek · 2024-02-22T08:06:14Z

+        const cursor = document.createElement('canvas');
+        cursor.width = pointer.data.width;
+        cursor.height = pointer.data.height;
+        cursor.getContext('2d').putImageData(pointer.data, 0, 0);


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);

gzdunek · 2024-02-22T08:09:12Z

+    if (client && updatePointer) {
+      const canvas = canvasRef.current;
+      const updatePointer = (pointer: {
+        data: ImageData | boolean;


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.

gzdunek · 2024-02-22T08:13:05Z

  canvasOnMouseWheelScroll?: (cli: TdpClient, e: WheelEvent) => void;
  canvasOnContextMenu?: () => boolean;
  style?: CSSProperties;
+  updatePointer: boolean;


I think we can make this property optional, you won't have to pass false to DesktopPlayer.

Suggested change

updatePointer: boolean;

updatePointer?: boolean;

ibeckermayer · 2024-02-25T17:44:10Z

        cb_context: &JsValue,
        draw_cb: &js_sys::Function,
        respond_cb: &js_sys::Function,
+        update_pointer_cb: &js_sys::Function,


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

teleport/web/packages/teleport/src/ironrdp/src/lib.rs

Lines 174 to 180 in 894920c

/// `tdp_fast_path_frame: Uint8Array`

///

/// `cb_context: tdp.Client`

///

/// `draw_cb: (bitmapFrame: BitmapFrame) => void`

///

/// `respond_cb: (responseFrame: ArrayBuffer) => void`

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

public-teleport-github-review-bot · 2024-02-25T23:59:17Z

@probakowski See the table below for backport results.

Branch	Result
branch/v15	Create PR

Support changing the mouse pointer

ec21312

probakowski added the backport/branch/v15 label Feb 7, 2024

probakowski requested review from ibeckermayer and zmb3 February 7, 2024 23:12

github-actions Bot added desktop-access rdp size/sm ui labels Feb 7, 2024

github-actions Bot requested a review from kimlisa February 7, 2024 23:13

kimlisa removed their request for review February 8, 2024 03:22

ibeckermayer reviewed Feb 9, 2024

View reviewed changes

probakowski and others added 3 commits February 10, 2024 01:00

Update web/packages/teleport/src/components/TdpClientCanvas/TdpClient…

f7591dc

…Canvas.tsx Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

Update web/packages/teleport/src/lib/tdp/client.ts

b95a989

Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

Update web/packages/teleport/src/components/TdpClientCanvas/TdpClient…

1accf8e

…Canvas.tsx Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>

Review comments

2bc371c

probakowski requested a review from ibeckermayer February 20, 2024 00:21

probakowski added 3 commits February 20, 2024 01:40

prettier

78137bd

Merge branch 'master' into probakowski/pointer

a8d67b7

Merge branch 'master' into probakowski/pointer

f5a4102

zmb3 reviewed Feb 21, 2024

View reviewed changes

probakowski added 3 commits February 21, 2024 20:54

use template string

b7f883c

useRef

7b5b354

Merge branch 'master' into probakowski/pointer

38ff429

probakowski requested a review from zmb3 February 21, 2024 19:55

probakowski added 2 commits February 21, 2024 21:48

don't update pointer during replay

783cbf6

don't update pointer during replay

894920c

zmb3 approved these changes Feb 21, 2024

View reviewed changes

gzdunek reviewed Feb 22, 2024

View reviewed changes

ibeckermayer approved these changes Feb 25, 2024

View reviewed changes

Review comments

8e7e62b

probakowski enabled auto-merge February 25, 2024 23:37

Merge branch 'master' into probakowski/pointer

d5b98d4

probakowski added this pull request to the merge queue Feb 25, 2024

Merged via the queue into master with commit 4ded67e Feb 25, 2024

probakowski deleted the probakowski/pointer branch February 25, 2024 23:58

probakowski mentioned this pull request Feb 26, 2024

[v15] Support changing the mouse pointer #38614

Merged

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

Conversation

probakowski commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 7, 2024

Uh oh!

ibeckermayer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmb3 commented Feb 9, 2024

Uh oh!

probakowski commented Feb 10, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibeckermayer Feb 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented Feb 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

probakowski commented Feb 7, 2024 •

edited

Loading

ibeckermayer Feb 25, 2024 •

edited

Loading