FIX: [ISXB-1541] Keyboard.allKeys contain null KeyControl for deprecated key Key.IMESelected which results in exception if using sub-script operator for lookup during enumeration of allKeys. #2230
Conversation
…solete key) would result in null KeyControl being returned.
ekcoh
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2230 +/- ##
========================================
Coverage 68.15% 68.16%
========================================
Files 367 367
Lines 53661 53668 +7
========================================
+ Hits 36572 36581 +9
+ Misses 17089 17087 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
ekcoh
changed the title
|
Opening this up for review for now since code generator seem to need to be fixed/changed to cope with nonconsecutive enum. So option (2) I haven't been able to solve yet. Another option would be to optionally just return null from subscript-operator if key is IMESelected. |
| { | ||
| throw new ArgumentOutOfRangeException($"{nameof(key)}: {key}"); | ||
| } | ||
| return m_Keys[index]; |
There was a problem hiding this comment.
I am leaving towards it would be better to return null for index == (int)Key.IMESelected - 1?
| if (index < 0 || index >= m_Keys.Length) | ||
| throw new ArgumentOutOfRangeException(nameof(key)); | ||
| { | ||
| throw new ArgumentOutOfRangeException($"{nameof(key)}: {key}"); |
There was a problem hiding this comment.
It's very odd Codecov is mentioning these lines since there is now a test explicitly testing key/index outside of bounds. Need to investigate if the test is really running in CI (it did locally I believe)
|
Do different platforms have relevance here? Do I need to check Mac/Linux? |
Platform shouldn't matter as far as I am aware. There is no platform specific code at play here for the scope of this PR. |
…in allKeys or if it is present it should not evaluate to a non-null control.
Addressed in b803b1d |
Description
Provides a suggested bug fix for https://issuetracker.unity3d.com/product/unity/issues/guid/ISXB-1541 which is reported as a regression for 1.14.0 due to #2075.
In 1.13.1 subscript operator throws
ArgumentOutOfRangeExceptionforKey.IMESelectedandKeyenumeration type containIMESelecteddefined as 111.Identified the following potential solutions:
IMESelectedwas accidentally added to Key enumeration type whenInput Systemwas first released and later marked obsolete in NEW: Added support for F13 to F24 keys #2075, make sure we still provide a KeyControl forIMESelectedbut advise to not rely on it due to obsoletion status. LetIMESelectedbe part ofKeyboard.allKeys.IMESelectedfromallKeys(since it is not a key). This is cleaner since it is inline with 1.13.1 whereIMESelectedalso wasn't part ofallKeys.This PR contains a fix according to 1). Investigating possibility for 2), hence draft status.
Summary of changes:
KeyControlforIMESelectedbit and returning thisKeyControlfor any sub-script operator look-up using the deprecated keyKey.IMESelected. This should be removed whenIMESelectedcan be removed from API. This fix should be ok given the following API contracts:IMESelectedkey control part ofKeyboard.keyswhich makes sense since it has incorrectly been exposed as a key inKeyenumeration type from version 1.0.Device_Keyboard_SubscriptOperatorShouldBeEquivalentToShiftedAllKeysLookupto capture this requirement.CoreTests_Device_Keyboard.csand added some missing new test cases tied toKeyboardto this class. I removed some obsolete test cases fromCoreTests_Devices.cs. I did not port existing keyboard tests fromCoreTests_Devices.csto the new file as part of this PR since this could be done separately and would make the PR more difficult to review. Hopefully the extended functional tests increase test coverage of keyboard functionality since it turned out many key properties were never accessed in tests. I introduced this file since a single enormous test file just makes it difficult to assess what is being tested. Generally all individual devices need their dedicated tests (which is the case for plugin devices but not for built-in it seems).FastKeyboard.cs(precompiled layout) to match changes toKeyboard.cs.Testing status & QA
Fix should be covered by new (and existing) automated tests, but it is suggested to test keyboard functionality and IME as part of quality assuring this PR. Fix developed and tested on Unity 6000.0.56f1.
Open issue: Check if this really is a regression - investigate behaviour on version before 1.14.0 for using subscript operator with
IMESelectedbefore transitioning this out of draft mode.Overall Product Risks
Medium due to central functionality.
Comments to reviewers
Nothing special to call out apart from that it should be mentioned there is still no property API for IMESelected which is intentional to avoid accessing it.
Checklist
Before review:
Changed,Fixed,Addedsections.Devices_Keyboard_CanGetKeyCodeFromKeyboardKey,Devices_Keyboard_AllKeysEnumeratesAllKeyControls,Devices_Keyboard_AllKeysShouldContainKeyControlsCorrespondingToAllKeys,Devices_Keyboard_SubscriptOperatorCanLookupKeyControlOfCorrespondingKey,Devices_Keyboard_SubscriptOperatorThrowsForInvalidOrOutOfRangeKey.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: