Skip to content

ADD primitive KeyEvent handling enough to handle BACK key#1839

Merged
msiglreith merged 9 commits into
rust-windowing:masterfrom
mmacedoeu:KeyEvent
Feb 23, 2021
Merged

ADD primitive KeyEvent handling enough to handle BACK key#1839
msiglreith merged 9 commits into
rust-windowing:masterfrom
mmacedoeu:KeyEvent

Conversation

@mmacedoeu

@mmacedoeu mmacedoeu commented Jan 25, 2021

Copy link
Copy Markdown
Contributor
  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • cargo doc builds successfully
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@ArturKovacs ArturKovacs added the DS - android Affects the Android backend label Feb 20, 2021
Comment thread src/platform_impl/android/mod.rs Outdated
virtual_keycode: None,
modifiers: event::ModifiersState::default(),
},
is_synthetic: true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@chrisduerr

Copy link
Copy Markdown
Contributor

@msiglreith / @dvc94ch / @francesca64 might be able to help you out with a review.

@dvc94ch

dvc94ch commented Feb 21, 2021

Copy link
Copy Markdown
Member

If it works, it looks good to me

@mmacedoeu

Copy link
Copy Markdown
Contributor Author

If it works, it looks good to me

it does the job, key codes comes as is and you can later handle the back button/key aka, exit app

@chrisduerr

Copy link
Copy Markdown
Contributor

If it works, it looks good to me

Do you not have the ability to test this code? Generally the maintainer of a platform should be able to test it, otherwise that kinda defeats the purpose...

@dvc94ch

dvc94ch commented Feb 21, 2021

Copy link
Copy Markdown
Member

Not really no. Am I on some sort of list? At the company I work for we don't use rust for ui's (unlikely that we will, since we really just provide typescript bindings for customers) and my focus in my spare time is on research projects that we'll eventually use either internally or in production. Currently I'm grooming 3 such projects and I have a back log of future projects. It would be nice if someone who has developed more recently for android could try it.

@chrisduerr

Copy link
Copy Markdown
Contributor

You are: https://github.com/rust-windowing/winit/wiki/Testers-and-Contributors

Please remove yourself if you're not able to test/review on Android.

@mmacedoeu

Copy link
Copy Markdown
Contributor Author

If it works, it looks good to me

Do you not have the ability to test this code? Generally the maintainer of a platform should be able to test it, otherwise that kinda defeats the purpose...

About testing it, there is not too much to test here, it's just a glue code, the key comes from android NDK and you don't wanna test the NDK, later it copies the key code into winit struct and dispatch it using the already tested macro, so you ended up testing if the key got copied properly and for this you are testing the rust language itself. I don't think this is worth testing such small glue. Android plataform is currently useless because you can't handle app exit when user press the BACK button/key, I hope this get merged soon as possible.

@msiglreith msiglreith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks and sorry as this PR slipped off my backlog - looks good in general!
Please add a short CHANGELOG entry.

Tested with some minimal wgpu example, pressing back and volume buttons to check if it's working

02-22 19:42:19.555 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 4, state: Released, virtual_keycode: None, modifiers: (empty) })
02-22 19:42:20.116 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 4, state: Released, virtual_keycode: None, modifiers: (empty) })
02-22 19:42:20.148 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 4, state: Released, virtual_keycode: None, modifiers: (empty) })
02-22 19:42:21.432 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 24, state: Released, virtual_keycode: None, modifiers: (empty) })
02-22 19:42:21.457 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 24, state: Released, virtual_keycode: None, modifiers: (empty) })
02-22 19:42:22.018 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 25, state: Released, virtual_keycode: None, modifiers: (empty) })
02-22 19:42:22.019 15413 15699 I RustStdoutStderr: (DeviceId(DeviceId), KeyboardInput { scancode: 25, state: Released, virtual_keycode: None, modifiers: (empty) })

Comment thread src/platform_impl/android/mod.rs Outdated
Comment thread src/platform_impl/android/mod.rs Outdated
@msiglreith msiglreith added the C - waiting on author Waiting for a response or another PR label Feb 22, 2021
@mmacedoeu mmacedoeu requested a review from msiglreith February 22, 2021 23:46

@msiglreith msiglreith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, please add the changelog entry and should be ready to merge then

@mmacedoeu

Copy link
Copy Markdown
Contributor Author

Thanks, please add the changelog entry and should be ready to merge then

Done @msiglreith

@msiglreith msiglreith merged commit 952edcb into rust-windowing:master Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C - waiting on author Waiting for a response or another PR DS - android Affects the Android backend

Development

Successfully merging this pull request may close these issues.

5 participants