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

Fix Windows pointer #40639

Merged
merged 7 commits into from
Apr 24, 2024
Merged

Fix Windows pointer #40639

merged 7 commits into from
Apr 24, 2024

Conversation

probakowski
Copy link
Contributor

@probakowski probakowski commented Apr 17, 2024

This PR fixes couple of problems with pointer in desktop access:

  • stuck/hidden pointer by handling correctly PointerHidden and PointerDefault messages
  • Windows 2019 not updating cursor at all in most cases by disabling cursor shadows (using DISABLE_CURSOR_SHADOW performance flag)
  • pointer going back to default near the bottom of the screen by resizing big cursor to at most 32x32px which is maximum size fully supported by the browser

Fixes #39986

changelog: Fix Windows cursor getting stuck

Copy link

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.

@probakowski probakowski requested a review from zmb3 April 18, 2024 17:10
lib/srv/desktop/rdp/rdpclient/src/client.rs Outdated Show resolved Hide resolved
@probakowski
Copy link
Contributor Author

@ravicious @ibeckermayer friendly ping

Copy link
Contributor

@ibeckermayer ibeckermayer left a comment

Choose a reason for hiding this comment

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

I'm skeptical this is truly fixing the root of the problem unless this reproduces with FreeRDP.

@@ -1169,6 +1169,7 @@ fn create_config(params: &ConnectParams, pin: String) -> Config {
autologon: true,
pointer_software_rendering: false,
performance_flags: PerformanceFlags::default()
| PerformanceFlags::DISABLE_CURSOR_SHADOW // this is required for pointer to work correctly in Windows 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

Wireshark is showing me a value of 0x00000086 here with FreeRDP, which is PerformanceFlags(DISABLE_FULLWINDOWDRAG | DISABLE_MENUANIMATIONS | ENABLE_FONT_SMOOTHING).

Does this issue reproduce with FreeRDP?

cursor.width = pointer.data.width;
cursor.height = pointer.data.height;
cursor
.getContext('2d', { colorSpace: pointer.data.colorSpace })
.putImageData(pointer.data, 0, 0);
if (pointer.data.width > 32 || pointer.data.height > 32) {
// scale the cursor down to at most 32px - max size fully supported by browsers
Copy link
Contributor

Choose a reason for hiding this comment

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

We currently tell the server we can handle up to 384x384px cursors. Maybe it would be better not to advertise for that?

https://github.com/Devolutions/IronRDP/blob/1f067069bab87e15b758e6e5ecc9a0330bc49639/crates/ironrdp-connector/src/connection_activation.rs#L336-L341

@zmb3
Copy link
Collaborator

zmb3 commented Apr 24, 2024

@ibeckermayer @probakowski I would like to see this merged today so that it goes in the next release. What are we missing in order to make that happen?

@probakowski
Copy link
Contributor Author

As per conversation in Slack we'll go with the disabling shadows for now until we update IronRDP to commit including Devolutions/IronRDP#444

@ibeckermayer
Copy link
Contributor

Update just for reference:

has @probakowski's changes which fix the pointer disappearing issue. Those are merged, however we cannot update to IronRDP's master rev because we're currently relying on the still-open-as-of-this-comment

to fix licensing issues. As a temporary solution, I have merged IronRDP master into Devolutions/IronRDP#436 so it includes Devolutions/IronRDP#444, and we've updated the rev to that.

(I will work on getting Devolutions/IronRDP#436 merged and then updating rev to IronRDP master ASAP)

@probakowski probakowski added this pull request to the merge queue Apr 24, 2024
Merged via the queue into master with commit 59664c8 Apr 24, 2024
41 checks passed
@probakowski probakowski deleted the probakowski/fix-pointer branch April 24, 2024 21:44
@public-teleport-github-review-bot

@probakowski See the table below for backport results.

Branch Result
branch/v15 Failed

probakowski added a commit that referenced this pull request Apr 24, 2024
* Fix Windows pointer

* Use default flags

* comment

* update IronRDP, enable shadow

* Revert "update IronRDP, enable shadow"

This reverts commit 0d7acf3.

* Update IronRDP
github-merge-queue bot pushed a commit that referenced this pull request Apr 25, 2024
* Fix Windows pointer

* Use default flags

* comment

* update IronRDP, enable shadow

* Revert "update IronRDP, enable shadow"

This reverts commit 0d7acf3.

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

Successfully merging this pull request may close these issues.

Desktop access: mouse cursor disappears or gets stuck
3 participants