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

Commit cc699b4f authored by Chet Haase's avatar Chet Haase
Browse files

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
parent 412fbe7f
Loading
Loading
Loading
Loading
+20 −4
Original line number Diff line number Diff line
@@ -15509,17 +15509,27 @@ public class View implements Drawable.Callback, KeyEvent.Callback,
     * handle possible request-during-layout errors correctly.</p>
     */
    public void requestLayout() {
        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()) {
            viewRoot.requestLayoutDuringLayout(this);
                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.
+79 −17
Original line number Diff line number Diff line
@@ -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.
     *
     * <p>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).</p>
     *
     * @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<View> 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<View>();
                                }
                                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);
                // Now run layout one more time
                    mInLayout = true;
                    host.layout(0, 0, host.getMeasuredWidth(), host.getMeasuredHeight());
                    mHandlingLayoutInLayoutRequest = false;
                }
                mLayoutRequesters.clear();
            }
        } finally {