From eb2b0af22c7b3fd1cc0297547b9f96c7976a7d59 Mon Sep 17 00:00:00 2001 From: Adam Powell Date: Wed, 20 May 2015 18:26:35 -0700 Subject: [PATCH] Restore app expectations around drawable visibility change timing When we update drawable visibility has changed to be part of View.onVisibilityChanged instead of an overload of setVisibility. While this covers more cases that we were previously missing, it also means that we now set drawable visibility from the View constructor via the call chain view.setFlags to view.onVisibilityChanged to drawable.setVisibility, resulting in us passing a 'this' pointer all over before the object is fully initialized. (i.e. a Bad Thing.) In general we've gotten away with playing fast and loose with this sort of thing as a part of view inflation - calling various non-final setters that may invoke callbacks as needed rather than initializing view fields directly. Unfortunately it also means that we can cause a lot of hard to trace bugs and in the long run we should try to clean up as much of it as we can. In this case, some apps were expecting inflation to have finished completely before any drawable visibility changed. If a view's visibility changes but it's not attached to a window, does it make a callback? The fix: no. We won't dispatch onVisibilityChanged to detached views, but we will dispatch it when a view becomes attached. Also fix a bug where we could end up telling a view its visibility changed to (INVISIBLE | GONE), which just doesn't make any sense. Bug 20103422 Change-Id: Ifba54c36114e85cf64869afcca766c30d601a16c --- core/java/android/view/View.java | 36 +++++++++++++++++---------- core/java/android/view/ViewGroup.java | 5 ++-- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java index 13f626b78b102..4e3c3f158cde7 100644 --- a/core/java/android/view/View.java +++ b/core/java/android/view/View.java @@ -1766,6 +1766,9 @@ public class View implements Drawable.Callback, KeyEvent.Callback, /** * Indicates that we should awaken scroll bars once attached * + * PLEASE NOTE: This flag is now unused as we now send onVisibilityChanged + * during window attachment and it is no longer needed. Feel free to repurpose it. + * * @hide */ private static final int PFLAG_AWAKEN_SCROLL_BARS_ON_ATTACH = 0x08000000; @@ -9524,12 +9527,8 @@ public class View implements Drawable.Callback, KeyEvent.Callback, */ protected void onVisibilityChanged(@NonNull View changedView, @Visibility int visibility) { final boolean visible = visibility == VISIBLE && getVisibility() == VISIBLE; - if (visible) { - if (mAttachInfo != null) { - initialAwakenScrollBars(); - } else { - mPrivateFlags |= PFLAG_AWAKEN_SCROLL_BARS_ON_ATTACH; - } + if (visible && mAttachInfo != null) { + initialAwakenScrollBars(); } final Drawable dr = mBackground; @@ -10585,9 +10584,11 @@ public class View implements Drawable.Callback, KeyEvent.Callback, } else if (mParent != null) { mParent.invalidateChild(this, null); } - dispatchVisibilityChanged(this, newVisibility); - notifySubtreeAccessibilityStateChangedIfNeeded(); + if (mAttachInfo != null) { + dispatchVisibilityChanged(this, newVisibility); + notifySubtreeAccessibilityStateChangedIfNeeded(); + } } if ((changed & WILL_NOT_CACHE_DRAWING) != 0) { @@ -13984,11 +13985,6 @@ public class View implements Drawable.Callback, KeyEvent.Callback, mParent.requestTransparentRegion(this); } - if ((mPrivateFlags & PFLAG_AWAKEN_SCROLL_BARS_ON_ATTACH) != 0) { - initialAwakenScrollBars(); - mPrivateFlags &= ~PFLAG_AWAKEN_SCROLL_BARS_ON_ATTACH; - } - mPrivateFlags3 &= ~PFLAG3_IS_LAID_OUT; jumpDrawablesToCurrentState(); @@ -14432,6 +14428,14 @@ public class View implements Drawable.Callback, KeyEvent.Callback, return mAttachInfo != null ? mAttachInfo.mSession : null; } + /** + * Return the visibility value of the least visible component passed. + */ + int combineVisibility(int vis1, int vis2) { + // This works because VISIBLE < INVISIBLE < GONE. + return Math.max(vis1, vis2); + } + /** * @param info the {@link android.view.View.AttachInfo} to associated with * this view @@ -14473,6 +14477,12 @@ public class View implements Drawable.Callback, KeyEvent.Callback, if (vis != GONE) { onWindowVisibilityChanged(vis); } + + // Send onVisibilityChanged directly instead of dispatchVisibilityChanged. + // As all views in the subtree will already receive dispatchAttachedToWindow + // traversing the subtree again here is not desired. + onVisibilityChanged(this, visibility); + if ((mPrivateFlags&PFLAG_DRAWABLE_STATE_DIRTY) != 0) { // If nobody has evaluated the drawable state yet, then do it now. refreshDrawableState(); diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java index b476e9b3fa61c..a7e739d9c2485 100644 --- a/core/java/android/view/ViewGroup.java +++ b/core/java/android/view/ViewGroup.java @@ -2830,12 +2830,13 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager for (int i = 0; i < count; i++) { final View child = children[i]; child.dispatchAttachedToWindow(info, - visibility | (child.mViewFlags & VISIBILITY_MASK)); + combineVisibility(visibility, child.getVisibility())); } final int transientCount = mTransientIndices == null ? 0 : mTransientIndices.size(); for (int i = 0; i < transientCount; ++i) { View view = mTransientViews.get(i); - view.dispatchAttachedToWindow(info, visibility | (view.mViewFlags & VISIBILITY_MASK)); + view.dispatchAttachedToWindow(info, + combineVisibility(visibility, view.getVisibility())); } }