Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2b77439d authored by Mady Mellor's avatar Mady Mellor
Browse files

Fix an issue with bubble being dismissed and added back at the same time 

When expanded and the user drags the last bubble to remove it, the
scrim animates out and THEN the views related to that last bubble
get cleaned up.

Within this time, a new update for this bubble could be posted. If the
timing is right, this can result in a crash as the view being added to
stackView is already added (i.e. the view hasn't finished animating
out yet).

This CL makes the following changes:
1) when adding a bubble - check for an existing parent & removing
   the bubble and adding it back (need to re-add it to get the add
   animation)
2) before removing a bubble - check if it's still in the stack before
   removing it / cleaning up expanded view
3) the expanded view state also gets messed up in this scenario, need
   to remove it from the hierachy while it's dragged out so that it
   gets the correct transitions (TO_BACK) so that it can get a TO_FRONT
   when it gets added back, previously we'd fade it out but from WM
   perspective that task is still visible.

Also added some tests for removeBubble behavior - general coverage
and coverage for this specific bug.

Bug: 420487074
Flag: com.android.wm.shell.fix_bubbles_add_same_bubble_being_removed
Test: atest BubbleStackViewTest
Test: manual - send two bubbles every second and continuously expand
               and dismiss them -- nothing should crash and bubbles
               that are removed & added back should be visible
             - while expanded, start dragging a bubble but release it
               before entering the dismiss target -- expanded view
               should hide and then appear when the up event happens
Change-Id: I7d30f486b3b726ea09d6e7686fcd12e145d1731f
parent 0a482a98

libs/WindowManager/Shell/multivalentTests/src/com/android/wm/shell/bubbles/BubbleStackViewTest.kt

+269 −12
Original line number Diff line number Diff line
@@ -16,6 +16,7 @@ 


package com.android.wm.shell.bubbles



import android.animation.AnimatorTestRule

import android.content.Context

import android.content.Intent

import android.content.pm.ShortcutInfo
@@ -68,6 +69,7 @@ import java.util.function.Consumer 
class BubbleStackViewTest {



    @get:Rule val setFlagsRule = SetFlagsRule()

    @get:Rule val animatorTestRule: AnimatorTestRule = AnimatorTestRule(this)



    private val context = ApplicationProvider.getApplicationContext<Context>()

    private lateinit var positioner: BubblePositioner
@@ -640,6 +642,7 @@ class BubbleStackViewTest { 
    @DisableFlags(Flags.FLAG_ENABLE_OPTIONAL_BUBBLE_OVERFLOW)

    @Test

    fun testCreateStackView_noOverflowContents_hasOverflow() {

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView =

                    BubbleStackView(

                            context,
@@ -651,7 +654,7 @@ class BubbleStackViewTest { 
                            { sysuiProxy },

                            shellExecutor

                    )



        }

        assertThat(bubbleData.overflowBubbles).isEmpty()

        val bubbleOverflow = bubbleData.overflow

        assertThat(bubbleStackView.getBubbleIndex(bubbleOverflow)).isGreaterThan(-1)
@@ -837,6 +840,260 @@ class BubbleStackViewTest { 
        assertThat(expandedViewContainer.visibility).isEqualTo(View.VISIBLE)

    }



    @Test

    fun removeBubble_notExpanded() {

        val bubble1 = createAndInflateChatBubble("key1")

        val bubble2 = createAndInflateChatBubble("key2")

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble1)

            bubbleStackView.addBubble(bubble2)

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(2)

        assertThat(bubble1.expandedView).isNotNull()

        assertThat(bubble1.iconView).isNotNull()

        assertThat(bubble2.expandedView).isNotNull()

        assertThat(bubble2.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // dismiss it in data so it's in the overflow

            bubbleData.dismissBubbleWithKey(bubble1.key, Bubbles.DISMISS_USER_GESTURE)

            // remove it from the stack

            bubbleStackView.removeBubble(bubble1)

            shellExecutor.flushAll()

        }



        // Check that proper changes to removed bubble happened

        assertThat(bubble1.expandedView).isNull()

        // still have bubbles + this was overflowed so should have icon view

        assertThat(bubble1.iconView).isNotNull()



        // And the bubble that is still in the stack is not affected

        assertThat(bubble2.expandedView).isNotNull()

        assertThat(bubble2.iconView).isNotNull()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

    }



    @Test

    fun removeBubble_notExpanded_notOverflowed() {

        val bubble1 = createAndInflateChatBubble("key1")

        val bubble2 = createAndInflateChatBubble("key2")

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble1)

            bubbleStackView.addBubble(bubble2)

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(2)

        assertThat(bubble1.expandedView).isNotNull()

        assertThat(bubble1.iconView).isNotNull()

        assertThat(bubble2.expandedView).isNotNull()

        assertThat(bubble2.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // remove it from stack + data, don't overflow it

            bubbleData.dismissBubbleWithKey(bubble1.key, Bubbles.DISMISS_NO_LONGER_BUBBLE)

            // remove it from the stack

            bubbleStackView.removeBubble(bubble1)

            shellExecutor.flushAll()

        }



        // Check that proper changes to removed bubble happened

        assertThat(bubble1.expandedView).isNull()

        assertThat(bubble1.iconView).isNull() // not in overflow so no icon



        // And the bubble that is still in the stack is not affected

        assertThat(bubble2.expandedView).isNotNull()

        assertThat(bubble2.iconView).isNotNull()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

    }



    @Test

    fun removeLastBubble_notExpanded() {

        val bubble = createAndInflateBubble()

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble)

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

        assertThat(bubble.expandedView).isNotNull()

        assertThat(bubble.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // remove it from the stack + data

            bubbleData.dismissBubbleWithKey(bubble.key, Bubbles.DISMISS_USER_GESTURE)

            bubbleStackView.removeBubble(bubble)

            shellExecutor.flushAll()

        }



        // Last bubble removed, so everything is null

        assertThat(bubble.expandedView).isNull()

        assertThat(bubble.iconView).isNull()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(0)

    }



    @Test

    fun removeBubble_whileExpanded() {

        val bubble1 = createAndInflateChatBubble("key1")

        val bubble2 = createAndInflateChatBubble("key2")

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble1)

            bubbleStackView.addBubble(bubble2)

            bubbleStackView.setExpanded(true)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.isExpanded).isTrue()

        assertThat(bubbleStackView.bubbleCount).isEqualTo(2)

        assertThat(bubble1.expandedView).isNotNull()

        assertThat(bubble1.iconView).isNotNull()

        assertThat(bubble2.expandedView).isNotNull()

        assertThat(bubble2.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // remove it from the stack + data

            bubbleData.dismissBubbleWithKey(bubble2.key, Bubbles.DISMISS_USER_GESTURE)

            bubbleStackView.removeBubble(bubble2)

            // stack would also be told to select the next bubble

            bubbleStackView.setSelectedBubble(bubble1)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        // Check that proper changes to removed bubble happened

        assertThat(bubble2.expandedView).isNull()

        // still have bubbles + this was overflowed so should have icon view

        assertThat(bubble2.iconView).isNotNull()



        assertThat(bubble1.expandedView).isNotNull()

        assertThat(bubble1.iconView).isNotNull()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

        assertThat(bubbleStackView.isExpanded).isTrue()

    }



    @Test

    fun removeBubble_whileExpanded_notOverflowed() {

        val bubble1 = createAndInflateChatBubble("key1")

        val bubble2 = createAndInflateChatBubble("key2")

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble1)

            bubbleStackView.addBubble(bubble2)

            bubbleStackView.setExpanded(true)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.isExpanded).isTrue()

        assertThat(bubbleStackView.bubbleCount).isEqualTo(2)

        assertThat(bubble1.expandedView).isNotNull()

        assertThat(bubble1.iconView).isNotNull()

        assertThat(bubble2.expandedView).isNotNull()

        assertThat(bubble2.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // remove it from the stack + data; don't overflow it

            bubbleData.dismissBubbleWithKey(bubble2.key, Bubbles.DISMISS_NO_LONGER_BUBBLE)

            bubbleStackView.removeBubble(bubble2)

            // stack would also be told to select the next bubble

            bubbleStackView.setSelectedBubble(bubble1)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        // Check that proper changes to removed bubble happened

        assertThat(bubble2.expandedView).isNull()

        assertThat(bubble2.iconView).isNull() // not in overflow so null



        assertThat(bubble1.expandedView).isNotNull()

        assertThat(bubble1.iconView).isNotNull()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

        assertThat(bubbleStackView.isExpanded).isTrue()

    }



    @Test

    fun removeLastBubble_whileExpanded() {

        val bubble = createAndInflateBubble()

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble)

            bubbleStackView.setExpanded(true)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.isExpanded).isTrue()

        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

        assertThat(bubble.expandedView).isNotNull()

        assertThat(bubble.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // remove it from the stack + data

            bubbleData.dismissBubbleWithKey(bubble.key, Bubbles.DISMISS_USER_GESTURE)

            bubbleStackView.removeBubble(bubble)

            // stack would also be told to collapse when last bubble removed

            bubbleStackView.setExpanded(false)

            // Run the scrim animation

            animatorTestRule.advanceTimeBy(300)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        // Last bubble removed, so everything is null

        assertThat(bubble.expandedView).isNull()

        assertThat(bubble.iconView).isNull()



        assertThat(bubbleStackView.bubbleCount).isEqualTo(0)

    }



    @EnableFlags(Flags.FLAG_FIX_BUBBLES_ADD_SAME_BUBBLE_BEING_REMOVED)

    @Test

    fun removeLastBubble_whileExpanded_addBack() {

        val bubble = createAndInflateBubble()

        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            bubbleStackView.addBubble(bubble)

            bubbleStackView.setExpanded(true)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        assertThat(bubbleStackView.isExpanded).isTrue()

        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

        assertThat(bubble.expandedView).isNotNull()

        assertThat(bubble.iconView).isNotNull()



        InstrumentationRegistry.getInstrumentation().runOnMainSync {

            // remove it from the data + stack

            bubbleData.dismissBubbleWithKey(bubble.key, Bubbles.DISMISS_USER_GESTURE)

            bubbleStackView.removeBubble(bubble)

            // typically stack would also be told to collapse when last bubble removed

            bubbleStackView.setExpanded(false)

            // Start the scrim animation

            animatorTestRule.advanceTimeBy(100)

            // Add the same bubble back

            bubbleData.notificationEntryUpdated(bubble, false, true)

            bubbleStackView.addBubble(bubble)

            shellExecutor.flushAll()

        }

        InstrumentationRegistry.getInstrumentation().waitForIdleSync()



        // Check that proper changes to removed bubble happened

        assertThat(bubble.expandedView).isNotNull()

        assertThat(bubble.expandedView!!.contentAlpha).isEqualTo(1)

        assertThat(bubble.expandedView!!.alpha).isEqualTo(1)

        assertThat(bubble.iconView).isNotNull()

        assertThat(bubble.iconView!!.alpha).isEqualTo(1)

        assertThat(bubble.iconView!!.scaleX).isEqualTo(1)

        assertThat(bubble.iconView!!.scaleY).isEqualTo(1)

        assertThat(bubbleStackView.bubbleCount).isEqualTo(1)

    }



    private fun createAndInflateChatBubble(key: String): Bubble {

        val icon = Icon.createWithResource(context.resources, R.drawable.bubble_ic_overflow_button)

        val shortcutInfo = ShortcutInfo.Builder(context, "fakeId").setIcon(icon).build()

libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/BubbleStackView.java

+90 −19
Original line number Diff line number Diff line
@@ -2230,6 +2230,17 @@ public class BubbleStackView extends FrameLayout 
        // expand / collapse on.

        bubble.getIconView().setTranslationX(mStackAnimationController.getStackPosition().x);



        // If it is currently being animated out (and newly added again here), remove the view

        // and add it back so that it animates in.

        if (Flags.fixBubblesAddSameBubbleBeingRemoved()

                && bubble.getIconView().getParent() != null) {

            mBubbleContainer.removeViewNoAnimation(bubble.getIconView());

            // If the view was being animated out, need to reset its state

            bubble.getIconView().setAlpha(1f);

            bubble.getIconView().setVisibility(VISIBLE);

            bubble.getIconView().setScaleX(1f);

            bubble.getIconView().setScaleY(1f);

        }

        mBubbleContainer.addView(bubble.getIconView(), 0,

                new FrameLayout.LayoutParams(mPositioner.getBubbleSize(),

                        mPositioner.getBubbleSize()));
@@ -2247,18 +2258,63 @@ public class BubbleStackView extends FrameLayout 


    // via BubbleData.Listener

    void removeBubble(Bubble bubble) {

        if (isExpanded() && getBubbleCount() == 1) {

        boolean isLastBubble = getBubbleCount() == 1;

        if (isExpanded() && isLastBubble) {

            mRemovingLastBubbleWhileExpanded = true;

            // We're expanded while the last bubble is being removed. Let the scrim animate away

            // and then remove our views (removing the icon view triggers the removal of the

            // bubble window so do that at the end of the animation so we see the scrim animate).

            BadgedImageView iconView = bubble.getIconView();

            BubbleExpandedView bev = bubble.getExpandedView();

            final BubbleViewProvider expandedBubbleBeforeScrim = mExpandedBubble;

            // Notify the stack anim controller before running the scrim animation. In case

            // another bubble gets added during it.

            mStackAnimationController.onLastBubbleRemoved();



            showScrim(false, () -> {

                mRemovingLastBubbleWhileExpanded = false;

                if (Flags.fixBubblesAddSameBubbleBeingRemoved()) {

                    boolean removedBubbleBackInStack = mBubbleData.hasBubbleInStackWithKey(

                            bubble.getKey());

                    // In the time it takes for the scrim animation, that bubble may have gotten

                    // a new update and be added back -- only remove the view if it's not in the

                    // stack.

                    if (!removedBubbleBackInStack) {

                        if (iconView != null) {

                            mBubbleContainer.removeView(iconView);

                        }

                        if (bev != null) {

                            mExpandedViewContainer.removeView(bev);

                        }

                    } else if (bev != null) {

                        // If we were dragging out, reset the expanded view in case the bubble gets

                        // re-added

                        bev.setContentAlpha(1f);

                        bev.setBackgroundAlpha(1f);

                    }



                    mExpandedViewContainer.setAnimationMatrix(null);

                    mExpandedViewTemporarilyHidden = false;



                    if (!removedBubbleBackInStack) {

                        // Not in the stack -- clean up all the views

                        bubble.cleanupViews();

                    }



                    // Bubble keys may not have changed if we receive an update to the same bubble.

                    // Compare bubble object instances to see if the expanded bubble has changed.

                    if (expandedBubbleBeforeScrim == mExpandedBubble && !removedBubbleBackInStack) {

                        // Only clear expanded bubble if it has not changed since the scrim

                        // animation started.

                        // Scrim animation can take some time run and it is possible for a new

                        // bubble to be added while the animation is running.

                        // This causes the expanded bubble to change. Make sure we only clear the

                        // expanded bubble if it did not change between when the scrim animation

                        // started and completed.

                        mExpandedBubble = null;

                        updateExpandedView();

                    }

                } else {

                    bubble.cleanupExpandedView();

                    if (iconView != null) {

                        mBubbleContainer.removeView(iconView);
@@ -2268,14 +2324,16 @@ public class BubbleStackView extends FrameLayout 
                    // Bubble keys may not have changed if we receive an update to the same bubble.

                    // Compare bubble object instances to see if the expanded bubble has changed.

                    if (expandedBubbleBeforeScrim == mExpandedBubble) {

                    // Only clear expanded bubble if it has not changed since the scrim animation

                    // started.

                    // Scrim animation can take some time run and it is possible for a new bubble

                    // to be added while the animation is running. This causes the expanded

                    // bubble to change. Make sure we only clear the expanded bubble if it did

                    // not change between when the scrim animation started and completed.

                        // Only clear expanded bubble if it has not changed since the scrim

                        // animation started.

                        // Scrim animation can take some time run and it is possible for a new

                        // bubble to be added while the animation is running.

                        // This causes the expanded bubble to change. Make sure we only clear the

                        // expanded bubble if it did not change between when the scrim animation

                        // started and completed.

                        mExpandedBubble = null;

                    }

                }

            });

            logBubbleEvent(bubble, FrameworkStatsLog.BUBBLE_UICHANGED__ACTION__DISMISSED);

            return;
@@ -2289,13 +2347,14 @@ public class BubbleStackView extends FrameLayout 
            if (v instanceof BadgedImageView

                    && ((BadgedImageView) v).getKey().equals(bubble.getKey())) {

                mBubbleContainer.removeViewAt(i);

                if (mBubbleData.hasOverflowBubbleWithKey(bubble.getKey())) {

                if (!isLastBubble && mBubbleData.hasOverflowBubbleWithKey(bubble.getKey())) {

                    // Overflow bubbles show the icon view so just clean up the expanded view.

                    bubble.cleanupExpandedView();

                } else {

                    bubble.cleanupViews();

                }

                updateExpandedView();

                if (getBubbleCount() == 0 && !isExpanded()) {

                if (isLastBubble && !isExpanded()) {

                    // This is the last bubble and the stack is collapsed

                    updateStackPosition();

                }
@@ -2734,6 +2793,12 @@ public class BubbleStackView extends FrameLayout 
                        mScaleOutSpringConfig)

                .addUpdateListener((target, values) ->

                        mExpandedViewContainer.setAnimationMatrix(mExpandedViewContainerMatrix))

                .withEndActions(() -> {

                    if (Flags.fixBubblesAddSameBubbleBeingRemoved()

                            && mExpandedViewTemporarilyHidden) {

                        mExpandedViewContainer.removeView(mExpandedBubble.getExpandedView());

                    }

                })

                .start();



        // Animate alpha from 1f to 0f.
@@ -2749,6 +2814,12 @@ public class BubbleStackView extends FrameLayout 
        }



        mExpandedViewTemporarilyHidden = false;

        if (Flags.fixBubblesAddSameBubbleBeingRemoved()

                && mExpandedBubble != null

                && mExpandedBubble.getExpandedView() != null

                && mExpandedBubble.getExpandedView().getParent() == null) {

            mExpandedViewContainer.addView(mExpandedBubble.getExpandedView());

        }



        PhysicsAnimator.getInstance(mExpandedViewContainerMatrix)

                .spring(AnimatableScaleMatrix.SCALE_X,

libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/animation/PhysicsAnimationLayout.java

+5 −0
Original line number Diff line number Diff line
@@ -35,6 +35,7 @@ import androidx.dynamicanimation.animation.DynamicAnimation; 
import androidx.dynamicanimation.animation.SpringAnimation;

import androidx.dynamicanimation.animation.SpringForce;



import com.android.wm.shell.Flags;

import com.android.wm.shell.R;



import java.util.ArrayList;
@@ -339,6 +340,10 @@ public class PhysicsAnimationLayout extends FrameLayout { 
    /** Removes the child view immediately. */

    public void removeViewNoAnimation(View view) {

        super.removeView(view);

        if (Flags.fixBubblesAddSameBubbleBeingRemoved()) {

            removeTransientView(view);

            cancelAnimationsOnView(view);

        }

        view.setTag(R.id.physics_animator_tag, null);

    }

libs/WindowManager/Shell/src/com/android/wm/shell/bubbles/animation/StackAnimationController.java

+4 −0
Original line number Diff line number Diff line
@@ -831,6 +831,10 @@ public class StackAnimationController extends 


    // TODO: do we need this & BubbleStackView#updateBadgesAndZOrder?

    private void updateBadgesAndZOrder(View v, int index) {

        if (v == null) {

            // View was removed

            return;

        }

        v.setZ(index < NUM_VISIBLE_WHEN_RESTING ? (mMaxBubbles * mElevation) - index : 0f);

        BadgedImageView bv = (BadgedImageView) v;

        if (index == 0) {