From 13922e03037d71a538f24ddf61c0b61bb4eb5af0 Mon Sep 17 00:00:00 2001 From: Romain Guy Date: Tue, 12 May 2009 17:56:14 -0700 Subject: [PATCH 1/2] Fixes #1836075. Adds consistency checks for the View hierarchy. To enable them, you need a debug build and ViewDebug.sConsistencyCheckEnabled set to true in debug.prop. This change also lets you easily enable drawing and layout profiling in ViewRoot by setting ViewRoot.sProfileDrawing, ViewRoot.sProfileLayout and ViewRoot.sShowFps in debug.prop with a debug build. --- core/java/android/view/View.java | 57 ++++++++++++++++- core/java/android/view/ViewDebug.java | 64 +++++++++++++++++++ core/java/android/view/ViewGroup.java | 60 ++++++++++++++++- core/java/android/view/ViewRoot.java | 52 +++++++++------ core/java/android/widget/AbsListView.java | 59 ++++++++++++++++- .../com/android/server/status/IconMerger.java | 8 +-- 6 files changed, 272 insertions(+), 28 deletions(-) diff --git a/core/java/android/view/View.java b/core/java/android/view/View.java index 335b43c7c6e49..af5dca6ee8177 100644 --- a/core/java/android/view/View.java +++ b/core/java/android/view/View.java @@ -49,6 +49,7 @@ import android.util.Poolable; import android.util.Pool; import android.util.Pools; import android.util.PoolableManager; +import android.util.Config; import android.view.ContextMenu.ContextMenuInfo; import android.view.animation.Animation; import android.view.inputmethod.InputConnection; @@ -5641,7 +5642,7 @@ public class View implements Drawable.Callback, KeyEvent.Callback { if (ViewDebug.TRACE_HIERARCHY) { ViewDebug.trace(this, ViewDebug.HierarchyTraceType.BUILD_CACHE); } - if (ViewRoot.PROFILE_DRAWING) { + if (Config.DEBUG && ViewDebug.profileDrawing) { EventLog.writeEvent(60002, hashCode()); } @@ -7165,6 +7166,60 @@ public class View implements Drawable.Callback, KeyEvent.Callback { tags.put(key, tag); } + /** + * @param consistency The type of consistency. See ViewDebug for more information. + * + * @hide + */ + protected boolean dispatchConsistencyCheck(int consistency) { + return onConsistencyCheck(consistency); + } + + /** + * Method that subclasses should implement to check their consistency. The type of + * consistency check is indicated by the bit field passed as a parameter. + * + * @param consistency The type of consistency. See ViewDebug for more information. + * + * @throws IllegalStateException if the view is in an inconsistent state. + * + * @hide + */ + protected boolean onConsistencyCheck(int consistency) { + boolean result = true; + + final boolean checkLayout = (consistency & ViewDebug.CONSISTENCY_LAYOUT) != 0; + final boolean checkDrawing = (consistency & ViewDebug.CONSISTENCY_DRAWING) != 0; + + if (checkLayout) { + if (getParent() == null) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, + "View " + this + " does not have a parent."); + } + + if (mAttachInfo == null) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, + "View " + this + " is not attached to a window."); + } + } + + if (checkDrawing) { + // Do not check the DIRTY/DRAWN flags because views can call invalidate() + // from their draw() method + + if ((mPrivateFlags & DRAWN) != DRAWN && + (mPrivateFlags & DRAWING_CACHE_VALID) == DRAWING_CACHE_VALID) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, + "View " + this + " was invalidated but its drawing cache is valid."); + } + } + + return result; + } + /** * Prints information about this view in the log output, with the tag * {@link #VIEW_LOG_TAG}. diff --git a/core/java/android/view/ViewDebug.java b/core/java/android/view/ViewDebug.java index 367c9a22d953c..4436f4b6e62c5 100644 --- a/core/java/android/view/ViewDebug.java +++ b/core/java/android/view/ViewDebug.java @@ -53,6 +53,27 @@ import java.lang.reflect.AccessibleObject; * Various debugging/tracing tools related to {@link View} and the view hierarchy. */ public class ViewDebug { + /** + * Log tag used to log errors related to the consistency of the view hierarchy. + * + * @hide + */ + public static final String CONSISTENCY_LOG_TAG = "ViewConsistency"; + + /** + * Flag indicating the consistency check should check layout-related properties. + * + * @hide + */ + public static final int CONSISTENCY_LAYOUT = 0x1; + + /** + * Flag indicating the consistency check should check drawing-related properties. + * + * @hide + */ + public static final int CONSISTENCY_DRAWING = 0x2; + /** * Enables or disables view hierarchy tracing. Any invoker of * {@link #trace(View, android.view.ViewDebug.HierarchyTraceType)} should first @@ -79,6 +100,49 @@ public class ViewDebug { */ static final String SYSTEM_PROPERTY_CAPTURE_EVENT = "debug.captureevent"; + /** + * Profiles drawing times in the events log. + * + * @hide + */ + @Debug.DebugProperty + public static boolean profileDrawing = false; + + /** + * Profiles layout times in the events log. + * + * @hide + */ + @Debug.DebugProperty + public static boolean profileLayout = false; + + /** + * Profiles real fps (times between draws) and displays the result. + * + * @hide + */ + @Debug.DebugProperty + public static boolean showFps = false; + + /** + *

Enables or disables views consistency check. Even when this property is enabled, + * view consistency checks happen only if {@link android.util.Config#DEBUG} is set + * to true. The value of this property can be configured externally in one of the + * following files:

+ * + * @hide + */ + @Debug.DebugProperty + public static boolean consistencyCheckEnabled = false; + + static { + Debug.setFieldsOn(ViewDebug.class, true); + } + /** * This annotation can be used to mark fields and methods to be dumped by * the view server. Only non-void methods with no arguments can be annotated diff --git a/core/java/android/view/ViewGroup.java b/core/java/android/view/ViewGroup.java index 31159d722d75d..26fe776ba3506 100644 --- a/core/java/android/view/ViewGroup.java +++ b/core/java/android/view/ViewGroup.java @@ -32,6 +32,7 @@ import android.util.AttributeSet; import android.util.EventLog; import android.util.Log; import android.util.SparseArray; +import android.util.Config; import android.view.animation.Animation; import android.view.animation.AnimationUtils; import android.view.animation.LayoutAnimationController; @@ -1404,7 +1405,7 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager // Clear the flag as early as possible to allow draw() implementations // to call invalidate() successfully when doing animations - child.mPrivateFlags |= DRAWN; + child.mPrivateFlags = (child.mPrivateFlags & ~DIRTY_MASK) | DRAWN; if (!concatMatrix && canvas.quickReject(cl, ct, cr, cb, Canvas.EdgeType.BW) && (child.mPrivateFlags & DRAW_ANIMATION) == 0) { @@ -1494,7 +1495,7 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager cachePaint.setAlpha(255); mGroupFlags &= ~FLAG_ALPHA_LOWER_THAN_ONE; } - if (ViewRoot.PROFILE_DRAWING) { + if (Config.DEBUG && ViewDebug.profileDrawing) { EventLog.writeEvent(60003, hashCode()); } canvas.drawBitmap(cache, 0.0f, 0.0f, cachePaint); @@ -2749,6 +2750,61 @@ public abstract class ViewGroup extends View implements ViewParent, ViewManager return new LayoutParams(LayoutParams.WRAP_CONTENT, LayoutParams.WRAP_CONTENT); } + /** + * @hide + */ + @Override + protected boolean dispatchConsistencyCheck(int consistency) { + boolean result = super.dispatchConsistencyCheck(consistency); + + final int count = mChildrenCount; + final View[] children = mChildren; + for (int i = 0; i < count; i++) { + if (!children[i].dispatchConsistencyCheck(consistency)) result = false; + } + + return result; + } + + /** + * @hide + */ + @Override + protected boolean onConsistencyCheck(int consistency) { + boolean result = super.onConsistencyCheck(consistency); + + final boolean checkLayout = (consistency & ViewDebug.CONSISTENCY_LAYOUT) != 0; + final boolean checkDrawing = (consistency & ViewDebug.CONSISTENCY_DRAWING) != 0; + + if (checkLayout) { + final int count = mChildrenCount; + final View[] children = mChildren; + for (int i = 0; i < count; i++) { + if (children[i].getParent() != this) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, + "View " + children[i] + " has no parent/a parent that is not " + this); + } + } + } + + if (checkDrawing) { + // If this group is dirty, check that the parent is dirty as well + if ((mPrivateFlags & DIRTY_MASK) != 0) { + final ViewParent parent = getParent(); + if (parent != null && !(parent instanceof ViewRoot)) { + if ((((View) parent).mPrivateFlags & DIRTY_MASK) == 0) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, + "ViewGroup " + this + " is dirty but its parent is not: " + this); + } + } + } + } + + return result; + } + /** * {@inheritDoc} */ diff --git a/core/java/android/view/ViewRoot.java b/core/java/android/view/ViewRoot.java index 8494d5e0c2c4d..90453ba5d542c 100644 --- a/core/java/android/view/ViewRoot.java +++ b/core/java/android/view/ViewRoot.java @@ -75,13 +75,6 @@ public final class ViewRoot extends Handler implements ViewParent, private static final boolean DEBUG_IMF = false || LOCAL_LOGV; private static final boolean WATCH_POINTER = false; - static final boolean PROFILE_DRAWING = false; - private static final boolean PROFILE_LAYOUT = false; - // profiles real fps (times between draws) and displays the result - private static final boolean SHOW_FPS = false; - // used by SHOW_FPS - private static int sDrawTime; - /** * Maximum time we allow the user to roll the trackball enough to generate * a key event, before resetting the counters. @@ -97,6 +90,8 @@ public final class ViewRoot extends Handler implements ViewParent, static final ThreadLocal sRunQueues = new ThreadLocal(); + private static int sDrawTime; + long mLastTrackballTime = 0; final TrackballAxis mTrackballAxisX = new TrackballAxis(); final TrackballAxis mTrackballAxisY = new TrackballAxis(); @@ -796,7 +791,7 @@ public final class ViewRoot extends Handler implements ViewParent, final Rect frame = mWinFrame; boolean initialized = false; boolean contentInsetsChanged = false; - boolean visibleInsetsChanged = false; + boolean visibleInsetsChanged; try { boolean hadSurface = mSurface.isValid(); int fl = 0; @@ -937,14 +932,22 @@ public final class ViewRoot extends Handler implements ViewParent, if (DEBUG_ORIENTATION || DEBUG_LAYOUT) Log.v( "ViewRoot", "Laying out " + host + " to (" + host.mMeasuredWidth + ", " + host.mMeasuredHeight + ")"); - long startTime; - if (PROFILE_LAYOUT) { + long startTime = 0L; + if (Config.DEBUG && ViewDebug.profileLayout) { startTime = SystemClock.elapsedRealtime(); } host.layout(0, 0, host.mMeasuredWidth, host.mMeasuredHeight); - if (PROFILE_LAYOUT) { + if (Config.DEBUG && ViewDebug.consistencyCheckEnabled) { + if (!host.dispatchConsistencyCheck(ViewDebug.CONSISTENCY_LAYOUT)) { + throw new IllegalStateException("The view hierarchy is an inconsistent state," + + "please refer to the logs with the tag " + + ViewDebug.CONSISTENCY_LOG_TAG + " for more infomation."); + } + } + + if (Config.DEBUG && ViewDebug.profileLayout) { EventLog.writeEvent(60001, SystemClock.elapsedRealtime() - startTime); } @@ -960,10 +963,11 @@ public final class ViewRoot extends Handler implements ViewParent, mTmpLocation[1] + host.mBottom - host.mTop); host.gatherTransparentRegion(mTransparentRegion); - if (mAppScale != 1.0f) { - mTransparentRegion.scale(mAppScale); - } + // TODO: scale the region, like: + // Region uses native methods. We probabl should have ScalableRegion class. + + // Region does not have equals method ? if (!mTransparentRegion.equals(mPreviousTransparentRegion)) { mPreviousTransparentRegion.set(mTransparentRegion); // reconfigure window manager @@ -1168,6 +1172,9 @@ public final class ViewRoot extends Handler implements ViewParent, canvas.scale(scale, scale); } mView.draw(canvas); + if (Config.DEBUG && ViewDebug.consistencyCheckEnabled) { + mView.dispatchConsistencyCheck(ViewDebug.CONSISTENCY_DRAWING); + } } finally { canvas.restoreToCount(saveCount); } @@ -1175,7 +1182,7 @@ public final class ViewRoot extends Handler implements ViewParent, mEgl.eglSwapBuffers(mEglDisplay, mEglSurface); checkEglErrors(); - if (SHOW_FPS) { + if (Config.DEBUG && ViewDebug.showFps) { int now = (int)SystemClock.elapsedRealtime(); if (sDrawTime != 0) { nativeShowFPS(canvas, now - sDrawTime); @@ -1216,7 +1223,7 @@ public final class ViewRoot extends Handler implements ViewParent, try { if (!dirty.isEmpty() || mIsAnimating) { - long startTime; + long startTime = 0L; if (DEBUG_ORIENTATION || DEBUG_DRAW) { Log.v("ViewRoot", "Surface " + surface + " drawing to bitmap w=" @@ -1224,7 +1231,7 @@ public final class ViewRoot extends Handler implements ViewParent, //canvas.drawARGB(255, 255, 0, 0); } - if (PROFILE_DRAWING) { + if (Config.DEBUG && ViewDebug.profileDrawing) { startTime = SystemClock.elapsedRealtime(); } @@ -1259,11 +1266,15 @@ public final class ViewRoot extends Handler implements ViewParent, canvas.scale(scale, scale); } mView.draw(canvas); + + if (Config.DEBUG && ViewDebug.consistencyCheckEnabled) { + mView.dispatchConsistencyCheck(ViewDebug.CONSISTENCY_DRAWING); + } } finally { canvas.restoreToCount(saveCount); } - if (SHOW_FPS) { + if (Config.DEBUG && ViewDebug.showFps) { int now = (int)SystemClock.elapsedRealtime(); if (sDrawTime != 0) { nativeShowFPS(canvas, now - sDrawTime); @@ -1271,7 +1282,7 @@ public final class ViewRoot extends Handler implements ViewParent, sDrawTime = now; } - if (PROFILE_DRAWING) { + if (Config.DEBUG && ViewDebug.profileDrawing) { EventLog.writeEvent(60000, SystemClock.elapsedRealtime() - startTime); } } @@ -1878,6 +1889,9 @@ public final class ViewRoot extends Handler implements ViewParent, } else { didFinish = false; } + if (event != null) { + event.scale(mAppScaleInverted); + } if (DEBUG_TRACKBALL) Log.v(TAG, "Motion event:" + event); diff --git a/core/java/android/widget/AbsListView.java b/core/java/android/widget/AbsListView.java index 4f503b4884603..1ca59b2430c79 100644 --- a/core/java/android/widget/AbsListView.java +++ b/core/java/android/widget/AbsListView.java @@ -3190,7 +3190,7 @@ public abstract class AbsListView extends AdapterView implements Te // Reclaim views on screen for (int i = 0; i < childCount; i++) { View child = getChildAt(i); - AbsListView.LayoutParams lp = (AbsListView.LayoutParams)child.getLayoutParams(); + AbsListView.LayoutParams lp = (AbsListView.LayoutParams) child.getLayoutParams(); // Don't reclaim header or footer views, or views that should be ignored if (lp != null && mRecycler.shouldRecycleViewType(lp.viewType)) { views.add(child); @@ -3204,6 +3204,63 @@ public abstract class AbsListView extends AdapterView implements Te removeAllViewsInLayout(); } + /** + * @hide + */ + @Override + protected boolean onConsistencyCheck(int consistency) { + boolean result = super.onConsistencyCheck(consistency); + + final boolean checkLayout = (consistency & ViewDebug.CONSISTENCY_LAYOUT) != 0; + + if (checkLayout) { + // The active recycler must be empty + final View[] activeViews = mRecycler.mActiveViews; + int count = activeViews.length; + for (int i = 0; i < count; i++) { + if (activeViews[i] != null) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, + "AbsListView " + this + " has a view in its active recycler: " + + activeViews[i]); + } + } + + // All views in the recycler must NOT be on screen and must NOT have a parent + final ArrayList scrap = mRecycler.mCurrentScrap; + if (!checkScrap(scrap)) result = false; + final ArrayList[] scraps = mRecycler.mScrapViews; + count = scraps.length; + for (int i = 0; i < count; i++) { + if (!checkScrap(scraps[i])) result = false; + } + } + + return result; + } + + private boolean checkScrap(ArrayList scrap) { + if (scrap == null) return true; + boolean result = true; + + final int count = scrap.size(); + for (int i = 0; i < count; i++) { + final View view = scrap.get(i); + if (view.getParent() != null) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, "AbsListView " + this + + " has a view in its scrap heap still attached to a parent: " + view); + } + if (indexOfChild(view) >= 0) { + result = false; + android.util.Log.d(ViewDebug.CONSISTENCY_LOG_TAG, "AbsListView " + this + + " has a view in its scrap heap that is also a direct child: " + view); + } + } + + return result; + } + /** * Sets the recycler listener to be notified whenever a View is set aside in * the recycler for later reuse. This listener can be used to free resources diff --git a/services/java/com/android/server/status/IconMerger.java b/services/java/com/android/server/status/IconMerger.java index 37fdbfbbb329d..5b806383d80b3 100644 --- a/services/java/com/android/server/status/IconMerger.java +++ b/services/java/com/android/server/status/IconMerger.java @@ -8,8 +8,6 @@ import android.widget.LinearLayout; public class IconMerger extends LinearLayout { - private static final boolean SPEW = false; - StatusBarService service; StatusBarIcon moreIcon; @@ -29,7 +27,7 @@ public class IconMerger extends LinearLayout { int fitRight = -1; for (i=N-1; i>=0; i--) { final View child = getChildAt(i); - if (child != null && child.getVisibility() != GONE) { + if (child.getVisibility() != GONE) { fitRight = child.getRight(); break; } @@ -45,7 +43,7 @@ public class IconMerger extends LinearLayout { moreView = child; startIndex = i+1; } - else if (child != null && child.getVisibility() != GONE) { + else if (child.getVisibility() != GONE) { fitLeft = child.getLeft(); break; } @@ -71,7 +69,7 @@ public class IconMerger extends LinearLayout { int number = 0; for (i=startIndex; i Date: Wed, 13 May 2009 15:46:37 -0700 Subject: [PATCH 2/2] Add a new feature to android.os.Debug to add the ability to inject only specific fields when calling setFieldsOn(). --- core/java/android/os/Debug.java | 54 +++++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/core/java/android/os/Debug.java b/core/java/android/os/Debug.java index 9cf7a222c186f..cd52bf3de7eaa 100644 --- a/core/java/android/os/Debug.java +++ b/core/java/android/os/Debug.java @@ -30,6 +30,10 @@ import java.io.PrintWriter; import java.io.Reader; import java.lang.reflect.Field; import java.lang.reflect.Modifier; +import java.lang.annotation.Target; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; import org.apache.harmony.dalvik.ddmc.Chunk; import org.apache.harmony.dalvik.ddmc.ChunkHandler; @@ -856,6 +860,15 @@ href="{@docRoot}guide/developing/tools/traceview.html">Traceview: A Graphical Lo } + /** + * Equivalent to setFieldsOn(cl, false). + * + * @see #setFieldsOn(Class, boolean) + */ + public static void setFieldsOn(Class cl) { + setFieldsOn(cl, false); + } + /** * Reflectively sets static fields of a class based on internal debugging * properties. This method is a no-op if android.util.Config.DEBUG is @@ -868,7 +881,7 @@ href="{@docRoot}guide/developing/tools/traceview.html">Traceview: A Graphical Lo * Class setup: define a class whose only fields are non-final, static * primitive types (except for "char") or Strings. In a static block * after the field definitions/initializations, pass the class to - * this method, Debug.setFieldsOn(). Example: + * this method, Debug.setFieldsOn(). Example: * 
      * package com.example;
      *
@@ -880,12 +893,18 @@ href="{@docRoot}guide/developing/tools/traceview.html">Traceview: A Graphical Lo
      *    public static String ns = null;
      *    public static boolean b = false;
      *    public static int i = 5;
+     *    @Debug.DebugProperty
      *    public static float f = 0.1f;
+     *    @@Debug.DebugProperty
      *    public static double d = 0.5d;
      *
      *    // This MUST appear AFTER all fields are defined and initialized!
      *    static {
+     *        // Sets all the fields
      *        Debug.setFieldsOn(MyDebugVars.class);
+     * 
+     *        // Sets only the fields annotated with @Debug.DebugProperty
+     *        // Debug.setFieldsOn(MyDebugVars.class, true);
      *    }
      * }
      *
@@ -898,25 +917,31 @@ href="{@docRoot}guide/developing/tools/traceview.html">Traceview: A Graphical Lo * {@hide} * * @param cl The class to (possibly) modify + * @param partial If false, sets all static fields, otherwise, only set + * fields with the {@link android.os.Debug.DebugProperty} + * annotation * @throws IllegalArgumentException if any fields are final or non-static, * or if the type of the field does not match the type of * the internal debugging property value. */ - public static void setFieldsOn(Class cl) { + public static void setFieldsOn(Class cl, boolean partial) { if (Config.DEBUG) { if (debugProperties != null) { /* Only look for fields declared directly by the class, * so we don't mysteriously change static fields in superclasses. */ for (Field field : cl.getDeclaredFields()) { - final String propertyName = cl.getName() + "." + field.getName(); - boolean isStatic = Modifier.isStatic(field.getModifiers()); - boolean isFinal = Modifier.isFinal(field.getModifiers()); - if (!isStatic || isFinal) { - throw new IllegalArgumentException(propertyName + - " must be static and non-final"); + if (!partial || field.getAnnotation(DebugProperty.class) != null) { + final String propertyName = cl.getName() + "." + field.getName(); + boolean isStatic = Modifier.isStatic(field.getModifiers()); + boolean isFinal = Modifier.isFinal(field.getModifiers()); + + if (!isStatic || isFinal) { + throw new IllegalArgumentException(propertyName + + " must be static and non-final"); + } + modifyFieldIfSet(field, debugProperties, propertyName); } - modifyFieldIfSet(field, debugProperties, propertyName); } } } else { @@ -925,4 +950,15 @@ href="{@docRoot}guide/developing/tools/traceview.html">Traceview: A Graphical Lo ") called in non-DEBUG build"); } } + + /** + * Annotation to put on fields you want to set with + * {@link Debug#setFieldsOn(Class, boolean)}. + * + * @hide + */ + @Target({ ElementType.FIELD }) + @Retention(RetentionPolicy.RUNTIME) + public @interface DebugProperty { + } }