-
Notifications
You must be signed in to change notification settings - Fork 326
NEW: Correctly expose Media keys through keyboard #2212
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2212 +/- ##
===========================================
+ Coverage 68.09% 68.11% +0.01%
===========================================
Files 367 367
Lines 53602 53629 +27
===========================================
+ Hits 36500 36529 +29
+ Misses 17102 17100 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I compared with latest #2075 PR that also added more keys and only found one thing missing which I commented.
Packages/com.unity.inputsystem/InputSystem/Devices/Precompiled/FastKeyboard.cs
Show resolved
Hide resolved
…utSystem into expose-media-keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but CI errors need to be addressed
Ok it's probably something to be done once we release 1.14.2, we need to bump to 1.15 since we're adding new APIs. |
Alright, I'll wait for 1.15 bump. Thx ! |
I submitted https://issuetracker.unity3d.com/issues/android-tv-some-of-the-remote-controls-not-detected-with-inputsystem-dot-devices which I think this PR is designed to address, so I thought I'd try this branch out. Do you have any guidance on how to use it? I added bindings for: and added logging of any action being performed, and of these 4 only ContextMenu was triggered when using a Fire TV remote. |
@dri-richard this PR only covers the frontend, the backend changes (where Unity forwards Android input data to input system package) are in internal PR, not accessible publicly. When the internal PR for backend will land, the status in https://issuetracker.unity3d.com/issues/android-tv-some-of-the-remote-controls-not-detected-with-inputsystem-dot-devices will change. Internal PR has to separately land in affected Unity versions - 6000.3, 6000.2, etc |
@jfreire-unity @Pauliusd01 iOS tests are failing on develop branch too - https://unity-ci.cds.internal.unity3d.com/project/130/branch/develop/jobDefinition/.yamato%2Fupm-ci.yml%23run_ios_trunk_functional/recent-jobs, I as |
@todi1856 yeah we are aware (also, trunk tests are actually not required for PRs to land in our repo). |
Sorry was busy with release, taking a look today |
Since I do need the editor change to actually use these, do we want any kind of documentation or comments to let users know which editors support it? Since IntelliSense will suggest them, users might try it and it will silently fail |
Other than my comment above, everything seems fine to me. Checked functionality for the added keys and that F24 key still works as well as there was a related change to it. I can approve this, just let me know |
I don't have this information yet, as per https://jira.unity3d.com/browse/UUM-113004 it will land in 2022.3, 6000.0, 6000.1, 6000.2, 6000.3, but I don't know the exact versions yet. |
That's good, matches the package release so no need for anything extra then |
@Pauliusd01 what now, will somebody from Input team needs to click Merge button ? P.S I don't have authorization to click Merge. |
Description
See https://github.cds.internal.unity3d.com/unity/unity/pull/74515 for details
Testing status & QA
Added coverage in unity/unity repo, also manually tested media keys on FireTV using Media.zip from https://jira.unity3d.com/browse/UUM-113004
Overall Product Risks
Comments to reviewers
Please describe any additional information such as what to focus on, or historical info for the reviewers.
Checklist
Before review:
Changed
,Fixed
,Added
sections.Area_CanDoX
,Area_CanDoX_EvenIfYIsTheCase
,Area_WhenIDoX_AndYHappens_ThisIsTheResult
.