From cc699b4fe396b3f93d45211d0df6f02baa325b2f Mon Sep 17 00:00:00 2001 From: Chet Haase Date: Thu, 13 Dec 2012 09:06:55 -0800 Subject: [PATCH] Fix for requestLayout-during-layout inefficiencies An earlier fix made it possible to call requestLayout() during layout (which is not recommended in most cases outside of a ListView) without ending up with blank content and internal layout flags in a confused state. However, that fix incorrectly detected a problem in some cases (such as ListView practices of adding views during layout) which were actually okay; as long as you make sure to measure and layout your children properly before returning from layout(), then it's not a problem. We were improperly spamming the log with supposed problems, and causing more overhead in correct cases by running a full request/measure/layout pass after the first layout pass, all of which is unnecessary in cases where the containers know what they're doing. This new fix changes the logic to only cause the second layout pass (and third, posted to the next frame, if things are really done incorrectly) if the layout-request flags are still set on the requesting views after the full layout pass is complete. This situation causes the blank screens we've seen in buggy apps, and is exactly what we should avoid. However, correct cases (e.g., ListView) will not have these problems because they run measure/layout correctly after the request calls, which clears these flags. The upshot is that buggy cases will be detected and compensated for (by clearing the flags and then running a second request/measure/layout pass, as in the original fix) and non-buggy cases will be noop'd, going back to their previous, working logic flow. The bug below is one of the buggy apps to demonstrate this problem. I noticed that the original problem (blank screen) is no longer reproducible. I suspect that logic was added to the app to force a refresh after it is attached. You can still detect the problem (and the fix) by seeing that prior to the fix (say, on mr1.1) there is a delay of about a second between the end of the progress bar updates and the showing of content on a screen that used to just remain blank. With the fix (both the previous version and this one), the content is updated immediately, because we now handle the buggy request- during-layout situation in the same frame as it occurs. Issue #6914123 News and Weather app sometimes loads to a blank screen Change-Id: I4c34817cc3dd44ba422ff50de4321624c0824d83 --- core/java/android/view/View.java | 24 +++++- core/java/android/view/ViewRootImpl.java | 96 +++++++++++++++++++----- 2 files changed, 99 insertions(+), 21 deletions(-) diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java index 080e7c0543b09..97287c3b27620 100644 --- a/core/java/android/view/View.java +++ b/core/java/android/view/View.java @@ -15509,17 +15509,27 @@ public class View implements Drawable.Callback, KeyEvent.Callback, * handle possible request-during-layout errors correctly.

*/ public void requestLayout() { - ViewRootImpl viewRoot = getViewRootImpl(); - if (viewRoot != null && viewRoot.isInLayout()) { - viewRoot.requestLayoutDuringLayout(this); - return; + if (mAttachInfo != null && mAttachInfo.mViewRequestingLayout == null) { + // Only trigger request-during-layout logic if this is the view requesting it, + // not the views in its parent hierarchy + ViewRootImpl viewRoot = getViewRootImpl(); + if (viewRoot != null && viewRoot.isInLayout()) { + if (!viewRoot.requestLayoutDuringLayout(this)) { + return; + } + } + mAttachInfo.mViewRequestingLayout = this; } + mPrivateFlags |= PFLAG_FORCE_LAYOUT; mPrivateFlags |= PFLAG_INVALIDATED; if (mParent != null && !mParent.isLayoutRequested()) { mParent.requestLayout(); } + if (mAttachInfo != null && mAttachInfo.mViewRequestingLayout == this) { + mAttachInfo.mViewRequestingLayout = null; + } } /** @@ -17999,6 +18009,12 @@ public class View implements Drawable.Callback, KeyEvent.Callback, */ final Point mPoint = new Point(); + /** + * Used to track which View originated a requestLayout() call, used when + * requestLayout() is called during layout. + */ + View mViewRequestingLayout; + /** * Creates a new set of attachment information with the specified * events handler and thread. diff --git a/core/java/android/view/ViewRootImpl.java b/core/java/android/view/ViewRootImpl.java index 85fab2859a0fa..9377cd4a6bf14 100644 --- a/core/java/android/view/ViewRootImpl.java +++ b/core/java/android/view/ViewRootImpl.java @@ -1894,22 +1894,37 @@ public final class ViewRootImpl implements ViewParent, } /** - * Called by {@link android.view.View#requestLayout()} if the view hiearchy is currently - * undergoing a layout pass. requestLayout() should not be called during layout, but if it - * is called anyway, we handle the situation here rather than leave the hierarchy in an - * indeterminate state. The solution is to queue up all requests during layout, apply those - * requests as soon as layout is complete, and then run layout once more immediately. If + * Called by {@link android.view.View#requestLayout()} if the view hierarchy is currently + * undergoing a layout pass. requestLayout() should not generally be called during layout, + * unless the container hierarchy knows what it is doing (i.e., it is fine as long as + * all children in that container hierarchy are measured and laid out at the end of the layout + * pass for that container). If requestLayout() is called anyway, we handle it correctly + * by registering all requesters during a frame as it proceeds. At the end of the frame, + * we check all of those views to see if any still have pending layout requests, which + * indicates that they were not correctly handled by their container hierarchy. If that is + * the case, we clear all such flags in the tree, to remove the buggy flag state that leads + * to blank containers, and force a second request/measure/layout pass in this frame. If * more requestLayout() calls are received during that second layout pass, we post those - * requests to the next frame, to avoid possible infinite loops. + * requests to the next frame to avoid possible infinite loops. + * + *

The return value from this method indicates whether the request should proceed + * (if it is a request during the first layout pass) or should be skipped and posted to the + * next frame (if it is a request during the second layout pass).

* * @param view the view that requested the layout. + * + * @return true if request should proceed, false otherwise. */ - void requestLayoutDuringLayout(final View view) { + boolean requestLayoutDuringLayout(final View view) { + if (view.mParent == null || view.mAttachInfo == null) { + // Would not normally trigger another layout, so just let it pass through as usual + return true; + } if (!mHandlingLayoutInLayoutRequest) { if (!mLayoutRequesters.contains(view)) { - Log.w("View", "requestLayout() called by " + view + " during layout pass"); mLayoutRequesters.add(view); } + return true; } else { Log.w("View", "requestLayout() called by " + view + " during second layout pass: " + "posting to next frame"); @@ -1919,6 +1934,7 @@ public final class ViewRootImpl implements ViewParent, view.requestLayout(); } }); + return false; } } @@ -1937,20 +1953,66 @@ public final class ViewRootImpl implements ViewParent, Trace.traceBegin(Trace.TRACE_TAG_VIEW, "layout"); try { host.layout(0, 0, host.getMeasuredWidth(), host.getMeasuredHeight()); + mInLayout = false; int numViewsRequestingLayout = mLayoutRequesters.size(); if (numViewsRequestingLayout > 0) { - // requestLayout() was called during layout: unusual, but try to handle correctly - mHandlingLayoutInLayoutRequest = true; + // requestLayout() was called during layout. + // If no layout-request flags are set on the requesting views, there is no problem. + // If some requests are still pending, then we need to clear those flags and do + // a full request/measure/layout pass to handle this situation. + + // Check state of layout flags for all requesters + ArrayList mValidLayoutRequesters = null; for (int i = 0; i < numViewsRequestingLayout; ++i) { - mLayoutRequesters.get(i).requestLayout(); + View view = mLayoutRequesters.get(i); + if ((view.mPrivateFlags & View.PFLAG_FORCE_LAYOUT) == View.PFLAG_FORCE_LAYOUT) { + while (view != null && view.mAttachInfo != null && view.mParent != null && + (view.mPrivateFlags & View.PFLAG_FORCE_LAYOUT) != 0) { + if ((view.mViewFlags & View.VISIBILITY_MASK) != View.GONE) { + // Only trigger new requests for non-GONE views + Log.w(TAG, "requestLayout() improperly called during " + + "layout: running second layout pass for " + view); + if (mValidLayoutRequesters == null) { + mValidLayoutRequesters = new ArrayList(); + } + mValidLayoutRequesters.add(view); + break; + } + if (view.mParent instanceof View) { + view = (View) view.mParent; + } else { + view = null; + } + } + } + } + if (mValidLayoutRequesters != null) { + // Clear flags throughout hierarchy, walking up from each flagged requester + for (int i = 0; i < numViewsRequestingLayout; ++i) { + View view = mLayoutRequesters.get(i); + while (view != null && + (view.mPrivateFlags & View.PFLAG_FORCE_LAYOUT) != 0) { + view.mPrivateFlags &= ~View.PFLAG_FORCE_LAYOUT; + if (view.mParent instanceof View) { + view = (View) view.mParent; + } else { + view = null; + } + } + } + // Process fresh layout requests, then measure and layout + mHandlingLayoutInLayoutRequest = true; + int numValidRequests = mValidLayoutRequesters.size(); + for (int i = 0; i < numValidRequests; ++i) { + mValidLayoutRequesters.get(i).requestLayout(); + } + measureHierarchy(host, lp, mView.getContext().getResources(), + desiredWindowWidth, desiredWindowHeight); + mInLayout = true; + host.layout(0, 0, host.getMeasuredWidth(), host.getMeasuredHeight()); + mHandlingLayoutInLayoutRequest = false; } - measureHierarchy(host, lp, mView.getContext().getResources(), - desiredWindowWidth, desiredWindowHeight); - // Now run layout one more time - mInLayout = true; - host.layout(0, 0, host.getMeasuredWidth(), host.getMeasuredHeight()); - mHandlingLayoutInLayoutRequest = false; mLayoutRequesters.clear(); } } finally {