From e6dfd1c22959cda18973b2423b1fdb785e210cbe Mon Sep 17 00:00:00 2001 From: Robin Lee Date: Fri, 31 Mar 2023 00:09:56 +0000 Subject: Cancel current animation instead of candidate Otherwise everything along the way confidently ignores this and Keyguard doesn't even see the timeout and force-abort coming when it hits. This leaves an animation timer running for 950ms which sets occlude status on finish callback and causes unpleasant race conditions. Test: atest android.server.wm.KeyguardTests Test: specific case to be added in b/274003805 Bug: 275650364 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:8ffedf11e398f9a6aa8b30cff5f06b102fd54536) Merged-In: I7b3dcd06e7483fde745a1d56dfee7c4efc262ed7 Change-Id: I7b3dcd06e7483fde745a1d56dfee7c4efc262ed7 --- .../com/android/systemui/keyguard/KeyguardService.java | 18 ++++++++---------- .../systemui/keyguard/KeyguardViewMediator.java | 13 +++++++++---- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardService.java b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardService.java index 4d40db0a0cfd..f1a8c95469f6 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardService.java +++ b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardService.java @@ -260,22 +260,20 @@ public class KeyguardService extends Service { ); } - public void mergeAnimation(IBinder transition, TransitionInfo info, - SurfaceControl.Transaction t, IBinder mergeTarget, - IRemoteTransitionFinishedCallback finishCallback) { + public void mergeAnimation(IBinder candidateTransition, TransitionInfo candidateInfo, + SurfaceControl.Transaction candidateT, IBinder currentTransition, + IRemoteTransitionFinishedCallback candidateFinishCallback) { try { - final IRemoteTransitionFinishedCallback origFinishCB; + final IRemoteTransitionFinishedCallback currentFinishCB; synchronized (mFinishCallbacks) { - origFinishCB = mFinishCallbacks.remove(transition); + currentFinishCB = mFinishCallbacks.remove(currentTransition); } - info.releaseAllSurfaces(); - t.close(); - if (origFinishCB == null) { - // already finished (or not started yet), so do nothing. + if (currentFinishCB == null) { + Slog.e(TAG, "Called mergeAnimation, but finish callback is missing"); return; } runner.onAnimationCancelled(false /* isKeyguardOccluded */); - origFinishCB.onTransitionFinished(null /* wct */, null /* t */); + currentFinishCB.onTransitionFinished(null /* wct */, null /* t */); } catch (RemoteException e) { // nothing, we'll just let it finish on its own I guess. } diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java index 82189763def6..d608abc3b008 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java +++ b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java @@ -958,10 +958,15 @@ public class KeyguardViewMediator implements CoreStartable, Dumpable, @Override public void onAnimationCancelled(boolean isKeyguardOccluded) { - if (mOccludeByDreamAnimator != null) { - mOccludeByDreamAnimator.cancel(); - } - setOccluded(isKeyguardOccluded /* isOccluded */, false /* animate */); + mContext.getMainExecutor().execute(() -> { + if (mOccludeByDreamAnimator != null) { + mOccludeByDreamAnimator.cancel(); + } + }); + // The value of isKeyguardOccluded here may come from mergeAnimation, which + // isn't reliable. In all cases, after running or cancelling this animation, + // keyguard should be occluded. + setOccluded(true /* isOccluded */, false /* animate */); if (DEBUG) { Log.d(TAG, "Occlude by Dream animation cancelled. Occluded state is now: " + mOccluded); -- cgit v1.2.3 From a7e27bc03f489edb1f84eb5ea4c0b110d1fb0762 Mon Sep 17 00:00:00 2001 From: Robin Lee Date: Mon, 12 Jun 2023 21:00:20 +0200 Subject: Keyguard: use transition state for syncing occlude [RESTRICT AUTOMERGE] Previously keyguard occlude state was passed in onAnimationCancelled which was not reliable for two reasons: (1) onAnimationCancelled is not called if the animation immediately finishes itself due to unhappiness at the contents of the transition it's been asked to play (eg. empty app list) (2) Clients have inconsistent handling of that paramter - some ignore it, some respect it, one may get passed wrong information by a local caller in mergeAnimation. Instead note the Keyguard flags at the start of every app transition and queue up a call to setKeyguardOccluded if the transition may cause a change. This should be sent after the animation is already finished. This is ideally similar to 140a05907d905e38aa5690320d26c90ca29bf772 (Run keyguard occlusion update after transitions) which was done for WmShell transitions. Test: atest KeyguardTests Test: atest ActivityLifecycleKeyguardTests Test: atest MultiDisplayKeyguardTests Test: atest MultiDisplayLockedKeyguardTests Bug: 286099078 (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:831787c50d4d74913d5b12d344e25743760aa437) Merged-In: Ifcdac241bfe33f44f5f03f1a6db682c57f0cd388 Change-Id: Ifcdac241bfe33f44f5f03f1a6db682c57f0cd388 --- .../systemui/keyguard/KeyguardViewMediator.java | 19 +++---------------- .../com/android/server/policy/PhoneWindowManager.java | 18 ++++++++++++++++-- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java index d608abc3b008..58ed3aae233e 100644 --- a/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java +++ b/packages/SystemUI/src/com/android/systemui/keyguard/KeyguardViewMediator.java @@ -963,14 +963,7 @@ public class KeyguardViewMediator implements CoreStartable, Dumpable, mOccludeByDreamAnimator.cancel(); } }); - // The value of isKeyguardOccluded here may come from mergeAnimation, which - // isn't reliable. In all cases, after running or cancelling this animation, - // keyguard should be occluded. - setOccluded(true /* isOccluded */, false /* animate */); - if (DEBUG) { - Log.d(TAG, "Occlude by Dream animation cancelled. Occluded state is now: " - + mOccluded); - } + Log.d(TAG, "Occlude by Dream animation cancelled."); } @Override @@ -1076,10 +1069,7 @@ public class KeyguardViewMediator implements CoreStartable, Dumpable, } }); - setOccluded(isKeyguardOccluded /* isOccluded */, false /* animate */); - Log.d(TAG, "Unocclude animation cancelled. Occluded state is now: " - + mOccluded); - + Log.d(TAG, "Unocclude animation cancelled."); mInteractionJankMonitor.cancel(CUJ_LOCKSCREEN_OCCLUSION); } @@ -3446,10 +3436,7 @@ public class KeyguardViewMediator implements CoreStartable, Dumpable, public void onAnimationCancelled(boolean isKeyguardOccluded) throws RemoteException { super.onAnimationCancelled(isKeyguardOccluded); - Log.d(TAG, "Occlude animation cancelled by WM. " - + "Setting occluded state to: " + isKeyguardOccluded); - setOccluded(isKeyguardOccluded /* occluded */, false /* animate */); - + Log.d(TAG, "Occlude animation cancelled by WM."); mInteractionJankMonitor.cancel(CUJ_LOCKSCREEN_OCCLUSION); } } diff --git a/services/core/java/com/android/server/policy/PhoneWindowManager.java b/services/core/java/com/android/server/policy/PhoneWindowManager.java index f064f83393c0..6e5421782261 100644 --- a/services/core/java/com/android/server/policy/PhoneWindowManager.java +++ b/services/core/java/com/android/server/policy/PhoneWindowManager.java @@ -2134,10 +2134,14 @@ public class PhoneWindowManager implements WindowManagerPolicy { } mWindowManagerInternal.registerAppTransitionListener(new AppTransitionListener() { + private boolean mOccludeChangingInTransition = false; + @Override public int onAppTransitionStartingLocked(boolean keyguardGoingAway, boolean keyguardOccluding, long duration, long statusBarAnimationStartTime, long statusBarAnimationDuration) { + mOccludeChangingInTransition = keyguardGoingAway || keyguardOccluding; + // When remote animation is enabled for KEYGUARD_GOING_AWAY transition, SysUI // receives IRemoteAnimationRunner#onAnimationStart to start animation, so we don't // need to call IKeyguardService#keyguardGoingAway here. @@ -2153,6 +2157,11 @@ public class PhoneWindowManager implements WindowManagerPolicy { 0 /* duration */); synchronized (mLock) { + if (mOccludeChangingInTransition) { + mKeyguardOccludedChanged = true; + mOccludeChangingInTransition = false; + } + applyKeyguardOcclusionChange(false); mLockAfterAppTransitionFinished = false; } } @@ -2160,12 +2169,16 @@ public class PhoneWindowManager implements WindowManagerPolicy { @Override public void onAppTransitionFinishedLocked(IBinder token) { synchronized (mLock) { + if (mOccludeChangingInTransition) { + mKeyguardOccludedChanged = true; + mOccludeChangingInTransition = false; + } + applyKeyguardOcclusionChange(false /* transitionStarted */); if (!mLockAfterAppTransitionFinished) { return; } mLockAfterAppTransitionFinished = false; } - lockNow(null); } }); @@ -3355,7 +3368,7 @@ public class PhoneWindowManager implements WindowManagerPolicy { if (mKeyguardOccludedChanged) { if (DEBUG_KEYGUARD) Slog.d(TAG, "transition/occluded changed occluded=" + mPendingKeyguardOccluded); - if (setKeyguardOccludedLw(mPendingKeyguardOccluded, false /* force */, + if (setKeyguardOccludedLw(mPendingKeyguardOccluded, true /* force */, transitionStarted)) { return FINISH_LAYOUT_REDO_LAYOUT | FINISH_LAYOUT_REDO_WALLPAPER; } @@ -3616,6 +3629,7 @@ public class PhoneWindowManager implements WindowManagerPolicy { private boolean setKeyguardOccludedLw(boolean isOccluded, boolean force, boolean transitionStarted) { if (DEBUG_KEYGUARD) Slog.d(TAG, "setKeyguardOccluded occluded=" + isOccluded); + mPendingKeyguardOccluded = isOccluded; mKeyguardOccludedChanged = false; if (isKeyguardOccluded() == isOccluded && !force) { return false; -- cgit v1.2.3