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

[UWP] Key events #27114

Merged
merged 3 commits into from Jun 30, 2020
Merged

[UWP] Key events #27114

merged 3 commits into from Jun 30, 2020

Conversation

paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Jun 29, 2020

This is the initial work required for proper keyboard events.
The text controller implementation is very basic, just enough to show the virtual keyboard when it's needed, and have basic key events.

@highfive
Copy link

highfive commented Jun 29, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/Cargo.toml, components/script/Cargo.toml, components/webdriver_server/Cargo.toml
  • @cbrewster: components/constellation/Cargo.toml
  • @jgraham: components/webdriver_server/Cargo.toml
  • @KiChjang: components/script/Cargo.toml, components/script_traits/Cargo.toml

@highfive
Copy link

highfive commented Jun 29, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

catch_any_panic(|| {
debug!("key_down");
let name = unsafe { CStr::from_ptr(name) };
let key = Key::from_str(&name.to_str().expect("Can't read string"));
Copy link
Member

@jdm jdm Jun 29, 2020

Choose a reason for hiding this comment

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

Do we really want to panic here, or could we just warn and return instead?

catch_any_panic(|| {
debug!("key_up");
let name = unsafe { CStr::from_ptr(name) };
let key = Key::from_str(&name.to_str().expect("Can't read string"));
Copy link
Member

@jdm jdm Jun 29, 2020

Choose a reason for hiding this comment

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

Same question.

auto keystr = *hstring2char((e.Text()));
RunOnGLThread([=] { mServo->KeyDown(keystr); });
Copy link
Member

@jdm jdm Jun 29, 2020

Choose a reason for hiding this comment

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

This is scary due to #27115. We should dereference keystr inside of the closure, rather than storing a pointer to the inner string value and using that.

support/hololens/ServoApp/ServoControl/Keys.h Show resolved Hide resolved
auto keyname = *keystr;
RunOnGLThread([=] { mServo->KeyUp(keyname); });
Copy link
Member

@jdm jdm Jun 29, 2020

Choose a reason for hiding this comment

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

Same concern about #27115, even though this is a std::optional value. We should dereference keyname inside the closure to avoid references to memory that isn't owned by the closure.

@jdm jdm assigned jdm and unassigned asajeffrey Jun 29, 2020
@paulrouget
Copy link
Contributor Author

paulrouget commented Jun 30, 2020

I will address *hstring2char issue in another PR.

Addressed your comments.

@jdm
Copy link
Member

jdm commented Jun 30, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

📌 Commit 2dcb78d has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Testing commit 2dcb78d with merge 9130d73...

bors-servo added a commit that referenced this issue Jun 30, 2020
[UWP] Key events

This is the initial work required for proper keyboard events.
The text controller implementation is very basic, just enough to show the virtual keyboard when it's needed, and have basic key events.
@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

💔 Test failed - status-taskcluster

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

Testing commit 2dcb78d with merge db00c1f...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 30, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing db00c1f to master...

@bors-servo bors-servo merged commit db00c1f into servo:master Jun 30, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants