From ffab2343fe7f161d828169b24cb8a583133b43d3 Mon Sep 17 00:00:00 2001 From: Arthur Hung Date: Wed, 23 Feb 2022 09:55:51 +0000 Subject: [PATCH] Let rule could decide dispatching excess when detecting combination key The KeyCombinationManager would excess the delay before it dispatching to the client side when detecting a potential combination key, it may cause some key affects UI performance dramatically such as dpad center or back. This CL provide a way for combination key rule could control the timeout. Bug: 220669913 Test: atest KeyCombinationTests Change-Id: Iab68cce050fbc1376307463cefc4d4d8d26d7c43 --- .../server/policy/KeyCombinationManager.java | 24 +++++-- .../server/policy/PhoneWindowManager.java | 18 ++++- .../server/policy/KeyCombinationTests.java | 69 ++++++++++++------- 3 files changed, 80 insertions(+), 31 deletions(-) diff --git a/services/core/java/com/android/server/policy/KeyCombinationManager.java b/services/core/java/com/android/server/policy/KeyCombinationManager.java index 68e078c519ba4..9213c87f12eca 100644 --- a/services/core/java/com/android/server/policy/KeyCombinationManager.java +++ b/services/core/java/com/android/server/policy/KeyCombinationManager.java @@ -48,7 +48,7 @@ public class KeyCombinationManager { // The rule has been triggered by current keys. @GuardedBy("mLock") private TwoKeysCombinationRule mTriggeredRule; - private final Handler mHandler = new Handler(); + private final Handler mHandler; // Keys in a key combination must be pressed within this interval of each other. private static final long COMBINE_KEY_DELAY_MILLIS = 150; @@ -93,6 +93,11 @@ public class KeyCombinationManager { return false; } + // The excessive delay before it dispatching to client. + long getKeyInterceptDelayMs() { + return COMBINE_KEY_DELAY_MILLIS; + } + abstract void execute(); abstract void cancel(); @@ -103,7 +108,8 @@ public class KeyCombinationManager { } } - public KeyCombinationManager() { + public KeyCombinationManager(Handler handler) { + mHandler = handler; } void addRule(TwoKeysCombinationRule rule) { @@ -195,10 +201,18 @@ public class KeyCombinationManager { */ long getKeyInterceptTimeout(int keyCode) { synchronized (mLock) { - if (forAllActiveRules((rule) -> rule.shouldInterceptKey(keyCode))) { - return mDownTimes.get(keyCode) + COMBINE_KEY_DELAY_MILLIS; + if (mDownTimes.get(keyCode) == 0) { + return 0; } - return 0; + long delayMs = 0; + for (final TwoKeysCombinationRule rule : mActiveRules) { + if (rule.shouldInterceptKey(keyCode)) { + delayMs = Math.max(delayMs, rule.getKeyInterceptDelayMs()); + } + } + // Make sure the delay is less than COMBINE_KEY_DELAY_MILLIS. + delayMs = Math.min(delayMs, COMBINE_KEY_DELAY_MILLIS); + return mDownTimes.get(keyCode) + delayMs; } } diff --git a/services/core/java/com/android/server/policy/PhoneWindowManager.java b/services/core/java/com/android/server/policy/PhoneWindowManager.java index cc93d4c1c3ffc..e79148d52715d 100644 --- a/services/core/java/com/android/server/policy/PhoneWindowManager.java +++ b/services/core/java/com/android/server/policy/PhoneWindowManager.java @@ -2124,7 +2124,7 @@ public class PhoneWindowManager implements WindowManagerPolicy { } private void initKeyCombinationRules() { - mKeyCombinationManager = new KeyCombinationManager(); + mKeyCombinationManager = new KeyCombinationManager(mHandler); final boolean screenshotChordEnabled = mContext.getResources().getBoolean( com.android.internal.R.bool.config_enableScreenshotChord); @@ -2215,11 +2215,20 @@ public class PhoneWindowManager implements WindowManagerPolicy { mBackKeyHandled = true; interceptAccessibilityGestureTv(); } - @Override void cancel() { cancelAccessibilityGestureTv(); } + @Override + long getKeyInterceptDelayMs() { + // Use a timeout of 0 to prevent additional latency in processing of + // this key. This will potentially cause some unwanted UI actions if the + // user does end up triggering the key combination later, but in most + // cases, the user will simply hit a single key, and this will allow us + // to process it without first waiting to see if the combination is + // going to be triggered. + return 0; + } }); mKeyCombinationManager.addRule( @@ -2229,11 +2238,14 @@ public class PhoneWindowManager implements WindowManagerPolicy { mBackKeyHandled = true; interceptBugreportGestureTv(); } - @Override void cancel() { cancelBugreportGestureTv(); } + @Override + long getKeyInterceptDelayMs() { + return 0; + } }); } } diff --git a/services/tests/wmtests/src/com/android/server/policy/KeyCombinationTests.java b/services/tests/wmtests/src/com/android/server/policy/KeyCombinationTests.java index 75479de26b1d8..cf571815b4cb1 100644 --- a/services/tests/wmtests/src/com/android/server/policy/KeyCombinationTests.java +++ b/services/tests/wmtests/src/com/android/server/policy/KeyCombinationTests.java @@ -36,6 +36,9 @@ import androidx.test.filters.SmallTest; import org.junit.Before; import org.junit.Test; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + /** * Test class for {@link KeyCombinationManager}. * @@ -47,16 +50,18 @@ import org.junit.Test; public class KeyCombinationTests { private KeyCombinationManager mKeyCombinationManager; - private boolean mAction1Triggered = false; - private boolean mAction2Triggered = false; - private boolean mAction3Triggered = false; + private final CountDownLatch mAction1Triggered = new CountDownLatch(1); + private final CountDownLatch mAction2Triggered = new CountDownLatch(1); + private final CountDownLatch mAction3Triggered = new CountDownLatch(1); private boolean mPreCondition = true; private static final long SCHEDULE_TIME = 300; + private Handler mHandler; @Before public void setUp() { - mKeyCombinationManager = new KeyCombinationManager(); + mHandler = new Handler(Looper.getMainLooper()); + mKeyCombinationManager = new KeyCombinationManager(mHandler); initKeyCombinationRules(); } @@ -67,7 +72,7 @@ public class KeyCombinationTests { KEYCODE_POWER) { @Override void execute() { - mAction1Triggered = true; + mAction1Triggered.countDown(); } @Override @@ -86,12 +91,17 @@ public class KeyCombinationTests { @Override void execute() { - mAction2Triggered = true; + mAction2Triggered.countDown(); } @Override void cancel() { } + + @Override + long getKeyInterceptDelayMs() { + return 0; + } }); // Rule 3 : power + volume_up schedule and trigger action after timeout. @@ -100,10 +110,9 @@ public class KeyCombinationTests { final Runnable mAction = new Runnable() { @Override public void run() { - mAction3Triggered = true; + mAction3Triggered.countDown(); } }; - final Handler mHandler = new Handler(Looper.getMainLooper()); @Override void execute() { @@ -149,60 +158,74 @@ public class KeyCombinationTests { } @Test - public void testTriggerRule() { + public void testTriggerRule() throws InterruptedException { final long eventTime = SystemClock.uptimeMillis(); pressKeys(eventTime, KEYCODE_POWER, eventTime, KEYCODE_VOLUME_DOWN); - assertTrue(mAction1Triggered); + assertTrue(mAction1Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); pressKeys(eventTime, KEYCODE_VOLUME_UP, eventTime, KEYCODE_VOLUME_DOWN); - assertTrue(mAction2Triggered); + assertTrue(mAction2Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); pressKeys(eventTime, KEYCODE_POWER, eventTime, KEYCODE_VOLUME_UP, SCHEDULE_TIME + 50); - assertTrue(mAction3Triggered); + assertTrue(mAction3Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); } /** * Nothing should happen if there is no definition. */ @Test - public void testNotTrigger_NoRule() { + public void testNotTrigger_NoRule() throws InterruptedException { final long eventTime = SystemClock.uptimeMillis(); pressKeys(eventTime, KEYCODE_BACK, eventTime, KEYCODE_VOLUME_DOWN); - assertFalse(mAction1Triggered); - assertFalse(mAction2Triggered); - assertFalse(mAction3Triggered); + assertFalse(mAction1Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); + assertFalse(mAction2Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); + assertFalse(mAction3Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); } /** * Nothing should happen if the interval of press time is too long. */ @Test - public void testNotTrigger_Interval() { + public void testNotTrigger_Interval() throws InterruptedException { final long eventTime = SystemClock.uptimeMillis(); final long earlyEventTime = eventTime - 200; // COMBINE_KEY_DELAY_MILLIS = 150; pressKeys(earlyEventTime, KEYCODE_POWER, eventTime, KEYCODE_VOLUME_DOWN); - assertFalse(mAction1Triggered); + assertFalse(mAction1Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); } /** * Nothing should happen if the condition is false. */ @Test - public void testNotTrigger_Condition() { + public void testNotTrigger_Condition() throws InterruptedException { final long eventTime = SystemClock.uptimeMillis(); // we won't trigger action 2 because the condition is false. mPreCondition = false; pressKeys(eventTime, KEYCODE_VOLUME_UP, eventTime, KEYCODE_VOLUME_DOWN); - assertFalse(mAction2Triggered); + assertFalse(mAction2Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); } /** * Nothing should happen if the keys released too early. */ @Test - public void testNotTrigger_EarlyRelease() { + public void testNotTrigger_EarlyRelease() throws InterruptedException { final long eventTime = SystemClock.uptimeMillis(); pressKeys(eventTime, KEYCODE_POWER, eventTime, KEYCODE_VOLUME_UP); - assertFalse(mAction3Triggered); + assertFalse(mAction3Triggered.await(SCHEDULE_TIME, TimeUnit.MILLISECONDS)); } -} + + /** + * The KeyInterceptTimeout should return the max timeout value. + */ + @Test + public void testKeyInterceptTimeout() { + final long eventTime = SystemClock.uptimeMillis(); + final KeyEvent firstKeyDown = new KeyEvent(eventTime, eventTime, ACTION_DOWN, + KEYCODE_VOLUME_UP, 0 /* repeat */, 0 /* metaState */); + // Press KEYCODE_VOLUME_UP would activate rule2 and rule3, + // and rule2's intercept delay is 0. + mKeyCombinationManager.interceptKey(firstKeyDown, true); + assertTrue(mKeyCombinationManager.getKeyInterceptTimeout(KEYCODE_VOLUME_UP) > eventTime); + } +} \ No newline at end of file