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

Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. #37962

Merged
merged 10 commits into from
Feb 21, 2024

Conversation

ibeckermayer
Copy link
Contributor

@ibeckermayer ibeckermayer commented Feb 8, 2024

Both of these changes are necessary in order to get the windows key to work again.

Corresponds with Devolutions/IronRDP#364

changelog: Allow users to send the Windows key for desktop sessions.

Copy link

github-actions bot commented Feb 8, 2024

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.

Comment on lines +467 to +470
let extended = key.code & 0xE000 == 0xE000;
if extended {
flags |= KeyboardFlags::EXTENDED;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where we actually set the EXTENDED flag; the rest of the changes are to interface with the updated IronRDP apis.

Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Let's get the disconnect reason plumbed through to the UI soon though. That was a really nice enhancement when we added it for rdp-rs

This required a change to the state management for DesktopSession. State
is still quite complex to get right here, but this is a step in the right
direction.
@ibeckermayer
Copy link
Contributor Author

@zmb3 @probakowski Please take another look.

I ended up needing to include some pretty substantive changes in order to pipe messages through to the client itself. I also realize we forgot to swap in the desktopplayback endpoint in #37962 new authenticated websocket paradigm, so those changes are included now as well.

@probakowski
Copy link
Contributor

Are changes around playback ws the same here as in #37981? If so would it make sense to based this PR on top of that one?

lib/srv/desktop/rdp/rdpclient/client.go Show resolved Hide resolved
lib/srv/desktop/rdp/rdpclient/client.go Show resolved Hide resolved
@ibeckermayer
Copy link
Contributor Author

Are changes around playback ws the same here as in #37981? If so would it make sense to based this PR on top of that one?

That may have been the case when you asked the question, but everything should be up to date now.

@ibeckermayer ibeckermayer added this pull request to the merge queue Feb 21, 2024
Merged via the queue into master with commit 5c309ad Feb 21, 2024
43 checks passed
@ibeckermayer ibeckermayer deleted the isaiah/update-ironrdp-latest branch February 21, 2024 17:59
@public-teleport-github-review-bot

@ibeckermayer See the table below for backport results.

Branch Result
branch/v15 Failed

ibeckermayer added a commit that referenced this pull request Feb 27, 2024
… IronRDP master commit hash. (#37962)

* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash.

* cleaning up disconnect result

* Pipes disconnect messages through to the client.

This required a change to the state management for DesktopSession. State
is still quite complex to get right here, but this is a step in the right
direction.

* Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520

* prettier formatting

* adding clarifying comments

* Further refactoring to make DesktopSession's state machine more comprehensible

* Remove unnecessary items from dependency lists
github-merge-queue bot pushed a commit that referenced this pull request Feb 28, 2024
* Bump the rust group with 2 updates (#37739)

Bumps the rust group with 2 updates: [tokio](https://github.com/tokio-rs/tokio) and [time](https://github.com/time-rs/time).


Updates `tokio` from 1.35.1 to 1.36.0
- [Release notes](https://github.com/tokio-rs/tokio/releases)
- [Commits](tokio-rs/tokio@tokio-1.35.1...tokio-1.36.0)

Updates `time` from 0.3.31 to 0.3.34
- [Release notes](https://github.com/time-rs/time/releases)
- [Changelog](https://github.com/time-rs/time/blob/main/CHANGELOG.md)
- [Commits](time-rs/time@v0.3.31...v0.3.34)

---
updated-dependencies:
- dependency-name: tokio
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: rust
- dependency-name: time
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: rust
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash. (#37962)

* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash.

* cleaning up disconnect result

* Pipes disconnect messages through to the client.

This required a change to the state management for DesktopSession. State
is still quite complex to get right here, but this is a step in the right
direction.

* Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520

* prettier formatting

* adding clarifying comments

* Further refactoring to make DesktopSession's state machine more comprehensible

* Remove unnecessary items from dependency lists

* Fix sticky windows key (#38198)

* Adds the extended keycode where appropriate and updates to the latest IronRDP master commit hash.

* cleaning up disconnect result

* Pipes disconnect messages through to the client.

This required a change to the state management for DesktopSession. State
is still quite complex to get right here, but this is a step in the right
direction.

* Updates desktopPlaybackHandle to the new authenticated websocket paradigm as it was forgotten in #37520

* prettier formatting

* adding clarifying comments

* Further refactoring to make DesktopSession's state machine more comprehensible

* Remove unnecessary items from dependency lists

* Adds the KeyboardHandler class
that manages the keyboard events, including new functionality to handle
withholding the Windows/Alt keys to prevent such keys from "sticking"
when the user switches between windows.

This technique works to stop the sticking, though there is still an edge
case where the user cmd/alt-tabs super quickly, in a manner that the browser
sees both the down and up events for the Windows key, despite the user's
intention being to change windows. In the case of the Windows key, this
causes the start menu to pop open, and the user must press the Windows key
again to close it. This is not a showstopper, but it is a bit annoying.
A similar problem would occur with the Alt key, where such behavior would
cause the current window (on the Windows machine) to focus on the menu bar.

The next commit will be to add a timeout mechanism for the relevant keyup events
in order to mitigate this edge case.

* Adds a delayUp cache for delaying the up event
of the sticky windows key. This is to prevent the edge case
where the user presses cmd/alt-tab really quickly in a manner
that causes the down + up events to be registered in spite of
the user's intention to be to change out of the window.

* Refactoring KeyboardHandler

* Convert caches into Maps

* Moves the rest of keyboard functionality into KeyboardHandler, rearranges methods, adds an onUnmount for clearing all the intervals on unmount

* Moves KeyboardHandler to its own file

* simplifies withholding logic by including a Withholder class with a single array that manages all the withheld keystrokes

* Makes useEffect symmetrical

* Rename onUnmount to dispose

* sp

* Adds unit tests for Withholder

* Splitting Withholder out into its own file

* nits

* CR

* make fix-license

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

3 participants