Refactor KeyEvent constructors.

KeyEvent has about 10 fields and 10 constructors, and not all
constructors set all thse fields, which can cause issues (for
example, some constructors were setting the display id to
INVALID_DISPLAY, while others weren't - this was probably a mistake
introduced when the new constructors were added).

This CL defines 2 "canonical" constructors (one that only takes
individual parameters, another one that takes a KeyEvent) and
refactor the others to call one of them, which would make them less
error prone.

Notice that there are still some inconsistency on how some fields
are initially set (for example, mDeviceId is set to VIRTUAL_KEYBOARD
and source is set to SOURCE_KEYBOARD by some constructors but they're
left as 0 by others) - it's not the purpose of this CL to fix these
cases, but the new constructors (and unit tests) would make it easier
to do so in the future.

NOTE: this CL is similar to 1aa18e2f2197f96abe5e20ff3062c52f26900923
(which was reverted), but fixing the constructor that took characters.

Test: atest android.view.KeyEventTest CtsInputMethodTestCases:android.view.inputmethod.cts.FocusHandlingTest

Bug: 268103680
Fixes: 270247000

Change-Id: Ic5867212c827065fb14af164b5b90f4815162342

2 files changed, 73 insertions, 100 deletions

diff --git a/core/java/android/view/KeyEvent.java b/core/java/android/view/KeyEvent.java
index 1a5613ebef02..ab8134559189 100644
--- a/core/java/android/view/KeyEvent.java
+++ b/core/java/android/view/KeyEvent.java
@@ -1352,6 +1352,7 @@ public class KeyEvent extends InputEvent implements Parcelable {
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P, trackingBug = 115609023)
     private int mSource;
     private int mDisplayId = INVALID_DISPLAY;
+    // NOTE: mHmac is private and not used in this class, but it's used on native side / parcel.
     private @Nullable byte[] mHmac;
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553)
     private int mMetaState;
@@ -1377,7 +1378,7 @@ public class KeyEvent extends InputEvent implements Parcelable {
      */
     private long mEventTime;
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553)
-    private String mCharacters;
+    private @Nullable String mCharacters;
 
     public interface Callback {
         /**
@@ -1441,7 +1442,11 @@ public class KeyEvent extends InputEvent implements Parcelable {
     private static native int nativeKeyCodeFromString(String keyCode);
     private static native int nativeNextId();
 
-    private KeyEvent() {}
+    private KeyEvent() {
+        this(/* downTime= */ 0, /* eventTime= */ 0, /* action= */ 0, /* code= */0, /* repeat= */ 0,
+                /* metaState= */ 0, /* deviceId= */ 0, /* scancode= */ 0, /* flags= */ 0,
+                /* source= */ 0, /* characters= */ null);
+    }
 
     /**
      * Create a new key event.
@@ -1451,11 +1456,9 @@ public class KeyEvent extends InputEvent implements Parcelable {
      * @param code The key code.
      */
     public KeyEvent(int action, int code) {
-        mId = nativeNextId();
-        mAction = action;
-        mKeyCode = code;
-        mRepeatCount = 0;
-        mDeviceId = KeyCharacterMap.VIRTUAL_KEYBOARD;
+        this(/* downTime= */ 0, /* eventTime= */ 0, action, code, /* repeat= */ 0,
+                /* metaState= */ 0, /* deviceId= */ KeyCharacterMap.VIRTUAL_KEYBOARD,
+                /* scancode= */ 0, /* flags= */ 0, /* source= */ 0, /* characters= */ null);
     }
 
     /**
@@ -1473,13 +1476,9 @@ public class KeyEvent extends InputEvent implements Parcelable {
      */
     public KeyEvent(long downTime, long eventTime, int action,
                     int code, int repeat) {
-        mId = nativeNextId();
-        mDownTime = TimeUnit.NANOSECONDS.convert(downTime, TimeUnit.MILLISECONDS);
-        mEventTime = TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS);
-        mAction = action;
-        mKeyCode = code;
-        mRepeatCount = repeat;
-        mDeviceId = KeyCharacterMap.VIRTUAL_KEYBOARD;
+        this(downTime, eventTime, action, code, repeat, /* metaState= */ 0,
+                KeyCharacterMap.VIRTUAL_KEYBOARD, /* scancode= */ 0, /* flags= */ 0,
+                /* source= */ 0, /* characters= */ null);
     }
 
     /**
@@ -1498,14 +1497,8 @@ public class KeyEvent extends InputEvent implements Parcelable {
      */
     public KeyEvent(long downTime, long eventTime, int action,
                     int code, int repeat, int metaState) {
-        mId = nativeNextId();
-        mDownTime = TimeUnit.NANOSECONDS.convert(downTime, TimeUnit.MILLISECONDS);
-        mEventTime = TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS);
-        mAction = action;
-        mKeyCode = code;
-        mRepeatCount = repeat;
-        mMetaState = metaState;
-        mDeviceId = KeyCharacterMap.VIRTUAL_KEYBOARD;
+        this(downTime, eventTime, action, code, repeat, metaState, KeyCharacterMap.VIRTUAL_KEYBOARD,
+                /* scancode= */ 0, /* flags= */ 0, /* source= */ 0, /* characters= */ null);
     }
 
     /**
@@ -1527,15 +1520,8 @@ public class KeyEvent extends InputEvent implements Parcelable {
     public KeyEvent(long downTime, long eventTime, int action,
                     int code, int repeat, int metaState,
                     int deviceId, int scancode) {
-        mId = nativeNextId();
-        mDownTime = TimeUnit.NANOSECONDS.convert(downTime, TimeUnit.MILLISECONDS);
-        mEventTime = TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS);
-        mAction = action;
-        mKeyCode = code;
-        mRepeatCount = repeat;
-        mMetaState = metaState;
-        mDeviceId = deviceId;
-        mScanCode = scancode;
+        this(downTime, eventTime, action, code, repeat, metaState, deviceId, scancode,
+                /* flags= */ 0, /* source= */ 0, /* characters= */ null);
     }
 
     /**
@@ -1558,16 +1544,8 @@ public class KeyEvent extends InputEvent implements Parcelable {
     public KeyEvent(long downTime, long eventTime, int action,
                     int code, int repeat, int metaState,
                     int deviceId, int scancode, int flags) {
-        mId = nativeNextId();
-        mDownTime = TimeUnit.NANOSECONDS.convert(downTime, TimeUnit.MILLISECONDS);
-        mEventTime = TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS);
-        mAction = action;
-        mKeyCode = code;
-        mRepeatCount = repeat;
-        mMetaState = metaState;
-        mDeviceId = deviceId;
-        mScanCode = scancode;
-        mFlags = flags;
+        this(downTime, eventTime, action, code, repeat, metaState, deviceId, scancode, flags,
+                /* source= */ 0, /* characters= */ null);
     }
 
     /**
@@ -1591,6 +1569,14 @@ public class KeyEvent extends InputEvent implements Parcelable {
     public KeyEvent(long downTime, long eventTime, int action,
                     int code, int repeat, int metaState,
                     int deviceId, int scancode, int flags, int source) {
+        this(downTime, eventTime, action, code, repeat, metaState, deviceId, scancode, flags,
+                source, /* characters= */ null);
+    }
+
+    private KeyEvent(long downTime, long eventTime, int action, int code, int repeat, int metaState,
+            int deviceId, int scancode, int flags, int source,  @Nullable String characters) {
+        // NOTE: this is the canonical constructor, other constructors that takes KeyEvent
+        // attributes should call it
         mId = nativeNextId();
         mDownTime = TimeUnit.NANOSECONDS.convert(downTime, TimeUnit.MILLISECONDS);
         mEventTime = TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS);
@@ -1602,6 +1588,7 @@ public class KeyEvent extends InputEvent implements Parcelable {
         mScanCode = scancode;
         mFlags = flags;
         mSource = source;
+        mCharacters = characters;
     }
 
     /**
@@ -1617,36 +1604,18 @@ public class KeyEvent extends InputEvent implements Parcelable {
      * @param flags The flags for this key event
      */
     public KeyEvent(long time, String characters, int deviceId, int flags) {
-        mId = nativeNextId();
-        mDownTime = TimeUnit.NANOSECONDS.convert(time, TimeUnit.MILLISECONDS);
-        mEventTime = TimeUnit.NANOSECONDS.convert(time, TimeUnit.MILLISECONDS);
-        mCharacters = characters;
-        mAction = ACTION_MULTIPLE;
-        mKeyCode = KEYCODE_UNKNOWN;
-        mRepeatCount = 0;
-        mDeviceId = deviceId;
-        mFlags = flags;
-        mSource = InputDevice.SOURCE_KEYBOARD;
+        this(/* downTime= */ time, /* eventTime= */ time, ACTION_MULTIPLE, KEYCODE_UNKNOWN,
+                /* repeat= */ 0, /* metaState= */ 0, deviceId, /* scancode= */ 0, flags,
+                /* source= */ InputDevice.SOURCE_KEYBOARD, characters);
     }
 
     /**
      * Make an exact copy of an existing key event.
      */
     public KeyEvent(KeyEvent origEvent) {
-        mId = origEvent.mId;
-        mDownTime = origEvent.mDownTime;
-        mEventTime = origEvent.mEventTime;
-        mAction = origEvent.mAction;
-        mKeyCode = origEvent.mKeyCode;
-        mRepeatCount = origEvent.mRepeatCount;
-        mMetaState = origEvent.mMetaState;
-        mDeviceId = origEvent.mDeviceId;
-        mSource = origEvent.mSource;
-        mDisplayId = origEvent.mDisplayId;
-        mHmac = origEvent.mHmac == null ? null : origEvent.mHmac.clone();
-        mScanCode = origEvent.mScanCode;
-        mFlags = origEvent.mFlags;
-        mCharacters = origEvent.mCharacters;
+        this(origEvent, origEvent.mId, origEvent.mEventTime, origEvent.mAction,
+                origEvent.mRepeatCount, origEvent.mHmac == null ? null : origEvent.mHmac.clone(),
+                origEvent.mCharacters);
     }
 
     /**
@@ -1662,20 +1631,30 @@ public class KeyEvent extends InputEvent implements Parcelable {
      */
     @Deprecated
     public KeyEvent(KeyEvent origEvent, long eventTime, int newRepeat) {
-        mId = nativeNextId();  // Not an exact copy so assign a new ID.
+        // Not an exact copy so assign a new ID.
+        // Don't copy HMAC, it will be invalid because eventTime is changing
+        this(origEvent, nativeNextId(),
+                TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS), origEvent.mAction,
+                newRepeat, /* hmac= */ null, origEvent.mCharacters);
+    }
+
+    // This is the canonical constructor that should be called for constructors that take a KeyEvent
+    private KeyEvent(KeyEvent origEvent, int id, long eventTime, int action, int newRepeat,
+            @Nullable byte[] hmac, @Nullable String characters) {
+        mId = id;
         mDownTime = origEvent.mDownTime;
-        mEventTime = TimeUnit.NANOSECONDS.convert(eventTime, TimeUnit.MILLISECONDS);
-        mAction = origEvent.mAction;
+        mEventTime = eventTime;
+        mAction = action;
         mKeyCode = origEvent.mKeyCode;
         mRepeatCount = newRepeat;
         mMetaState = origEvent.mMetaState;
         mDeviceId = origEvent.mDeviceId;
         mSource = origEvent.mSource;
         mDisplayId = origEvent.mDisplayId;
-        mHmac = null; // Don't copy HMAC, it will be invalid because eventTime is changing
+        mHmac = hmac;
         mScanCode = origEvent.mScanCode;
         mFlags = origEvent.mFlags;
-        mCharacters = origEvent.mCharacters;
+        mCharacters = characters;
     }
 
     private static KeyEvent obtain() {
@@ -1857,21 +1836,11 @@ public class KeyEvent extends InputEvent implements Parcelable {
      * @param action The new action code of the event.
      */
     private KeyEvent(KeyEvent origEvent, int action) {
-        mId = nativeNextId();  // Not an exact copy so assign a new ID.
-        mDownTime = origEvent.mDownTime;
-        mEventTime = origEvent.mEventTime;
-        mAction = action;
-        mKeyCode = origEvent.mKeyCode;
-        mRepeatCount = origEvent.mRepeatCount;
-        mMetaState = origEvent.mMetaState;
-        mDeviceId = origEvent.mDeviceId;
-        mSource = origEvent.mSource;
-        mDisplayId = origEvent.mDisplayId;
-        mHmac = null; // Don't copy the hmac, it will be invalid since action is changing
-        mScanCode = origEvent.mScanCode;
-        mFlags = origEvent.mFlags;
-        // Don't copy mCharacters, since one way or the other we'll lose it
-        // when changing the action.
+        // Not an exact copy so assign a new ID
+        // Don't copy the hmac, it will be invalid since action is changing
+        // Don't copy mCharacters, since one way or the other we'll lose it when changing action.
+        this(origEvent, nativeNextId(), origEvent.mEventTime, action, origEvent.mRepeatCount,
+                /* hmac= */ null, /* characters= */ null);
     }
 
     /**
@@ -3219,6 +3188,8 @@ public class KeyEvent extends InputEvent implements Parcelable {
     }
 
     private KeyEvent(Parcel in) {
+        // NOTE: ideally this constructor should call the canonical one, but that would require
+        // changing the order the fields are written to the parcel, which could break native code
         mId = in.readInt();
         mDeviceId = in.readInt();
         mSource = in.readInt();
diff --git a/core/tests/coretests/src/android/view/KeyEventTest.java b/core/tests/coretests/src/android/view/KeyEventTest.java
index 32078ca99b48..f723f15d2b26 100644
--- a/core/tests/coretests/src/android/view/KeyEventTest.java
+++ b/core/tests/coretests/src/android/view/KeyEventTest.java
@@ -47,14 +47,14 @@ public final class KeyEventTest {
     private static final int ACTION = KeyEvent.ACTION_DOWN;
     private static final int ANOTHER_ACTION = KeyEvent.ACTION_UP;
     private static final int KEYCODE = KeyEvent.KEYCODE_0;
-    private static final int REPEAT = 0;
-    private static final int ANOTHER_REPEAT = 42;
-    private static final int METASTATE = 0;
-    private static final int DEVICE_ID = 0;
-    private static final int SCAN_CODE = 0;
-    private static final int FLAGS = 0;
+    private static final int REPEAT = 4;
+    private static final int ANOTHER_REPEAT = 8;
+    private static final int METASTATE = 15;
+    private static final int DEVICE_ID = 16;
+    private static final int SCAN_CODE = 23;
+    private static final int FLAGS = 42;
     private static final int SOURCE = InputDevice.SOURCE_KEYBOARD;
-    private static final String CHARACTERS = null;
+    private static final String CHARACTERS = "CHARACTERS, Y U NO @NONNULL?";
 
     private static final int ID_SOURCE_MASK = 0x3 << 30;
 
@@ -178,7 +178,7 @@ public final class KeyEventTest {
                 DEVICE_ID, SCAN_CODE, FLAGS, SOURCE);
 
         assertKeyEventFields(key, DOWN_TIME, EVENT_TIME, ACTION, KEYCODE, REPEAT, METASTATE,
-                DEVICE_ID, SCAN_CODE, FLAGS, SOURCE, INVALID_DISPLAY, CHARACTERS);
+                DEVICE_ID, SCAN_CODE, FLAGS, SOURCE, INVALID_DISPLAY, /* characters= */ null);
     }
 
     @Test
@@ -187,7 +187,8 @@ public final class KeyEventTest {
                 DEVICE_ID, SCAN_CODE, FLAGS);
 
         assertKeyEventFields(key, DOWN_TIME, EVENT_TIME, ACTION, KEYCODE, REPEAT, METASTATE,
-                DEVICE_ID, SCAN_CODE, FLAGS, /* source= */ 0, INVALID_DISPLAY, CHARACTERS);
+                DEVICE_ID, SCAN_CODE, FLAGS, /* source= */ 0, INVALID_DISPLAY,
+                /* characters= */ null);
     }
 
     @Test
@@ -197,7 +198,7 @@ public final class KeyEventTest {
 
         assertKeyEventFields(key, DOWN_TIME, EVENT_TIME, ACTION, KEYCODE, REPEAT, METASTATE,
                 DEVICE_ID, SCAN_CODE, /* flags= */ 0, /* source= */ 0, INVALID_DISPLAY,
-                CHARACTERS);
+                /* characters= */ null);
     }
 
     @Test
@@ -206,7 +207,7 @@ public final class KeyEventTest {
 
         assertKeyEventFields(key, DOWN_TIME, EVENT_TIME, ACTION, KEYCODE, REPEAT, METASTATE,
                 /* deviceId= */ KeyCharacterMap.VIRTUAL_KEYBOARD, /* scanCode= */ 0, /* flags= */ 0,
-                /* source= */ 0, INVALID_DISPLAY, CHARACTERS);
+                /* source= */ 0, INVALID_DISPLAY, /* characters= */ null);
     }
 
     @Test
@@ -215,7 +216,8 @@ public final class KeyEventTest {
 
         assertKeyEventFields(key, DOWN_TIME, EVENT_TIME, ACTION, KEYCODE, REPEAT,
                 /* metaState= */ 0, /* deviceId= */ KeyCharacterMap.VIRTUAL_KEYBOARD,
-                /* scanCode= */ 0, /* flags= */ 0, /* source= */ 0, INVALID_DISPLAY, CHARACTERS);
+                /* scanCode= */ 0, /* flags= */ 0, /* source= */ 0, INVALID_DISPLAY,
+                /* characters= */ null);
     }
 
     @Test
@@ -233,8 +235,8 @@ public final class KeyEventTest {
 
         assertKeyEventFields(key, /* downTime= */ 0, /* eventTime= */ 0, ACTION, KEYCODE,
                 /* repeat= */ 0, /* metaState= */ 0,
-                /* deviceId= */ KeyCharacterMap.VIRTUAL_KEYBOARD, /* scanCode= */ 0, FLAGS,
-                /* source= */ 0, INVALID_DISPLAY, CHARACTERS);
+                /* deviceId= */ KeyCharacterMap.VIRTUAL_KEYBOARD, /* scanCode= */ 0, /* flags= */ 0,
+                /* source= */ 0, INVALID_DISPLAY, /* characters= */ null);
     }
 
     @Test
@@ -244,7 +246,7 @@ public final class KeyEventTest {
 
         assertKeyEventFields(key2, DOWN_TIME, EVENT_TIME, ANOTHER_ACTION, KEYCODE, REPEAT,
                 METASTATE, DEVICE_ID, SCAN_CODE, FLAGS, InputDevice.SOURCE_KEYBOARD,
-                INVALID_DISPLAY, CHARACTERS);
+                INVALID_DISPLAY, /* characters= */ null);
         expect.withMessage("id (key1=%s, key2=%s", key1, key2).that(key2.getId())
                 .isNotEqualTo(key1.getId());
     }