From 262f34dfc11287819daed5bb3edb5ed72c0a1b73 Mon Sep 17 00:00:00 2001 From: Floens Date: Tue, 12 Jan 2016 17:29:33 +0100 Subject: [PATCH] Fix a bug causing an endless layout loop. I've gotten the freeze three times total on my phone, and this time I was near a computer to do some debugging. This is what I found. Clover uses the ViewTreeObserver in some places to wait for the layout phase to be done and then use the calculated views to do some second calculations. This is not always necessary and I've removed/changed code that used it needlessly. I used this mostly out of laziness to create a new view to implement onMeasure, and not having a good understanding of the measure system. This is bad for performance because a second measure pass is not needed most of the time. I've misinterpreted the meaning of the return value of onPreDrawListener. I thought false meant continue with the rendering, true to request a new layout phase. This was the other way around and has been switched on all pieces of code that used it wrong. A perfect example was the use of waitForLayout in the DrawerController. It would wait for a layout pass to get the total width of the containing view and change the width of the drawer. It also used the wrong return value so the layout was not triggered again when needed and triggeded when not needed. This is now fixed and an issue where the drawer width was wrong is now also fixed. Secondly (and this has been an issue before), the listener would not get removed from the correct ViewTreeObserver in some cases, causing the listener to be triggered on every draw call. This is because Android creates a temporary ViewTreeObserver when a view is not attached to the window, and the callbacks are copied to a real ViewTreeObserver when the view gets attached. I'm still unsure why that does not work sometimes. The final trigger of the bug was the measure callback that was placed on the width of the menu width on the toolbar (the things that switches the boards). This callback always returned false (thinking that was do not cancel) and canceled the draw every time. This is not harmful normally but when the listener is not removed because the ViewTreeObserver is temporary it would get stuck in an endless loop. The weird thing is that touch handling still works correctly when Android gets stuck in such an endless loop. This was why you could still press the reply button and the keyboard would show up. It just doesn't get to the draw phase and then appears to look stuck. I've placed some debug logs in the callbacks when the listener doesn't get removed, and it raises an IllegalArgumentException when the ViewTreeObserver is temporarily. --- .../transition/PushControllerTransition.java | 7 +- .../chan/ui/controller/DrawerController.java | 31 --------- .../ui/controller/ImageViewerController.java | 7 +- .../ui/controller/PassSettingsController.java | 10 ++- .../ui/layout/DrawerWidthAdjustingLayout.java | 41 ++++++++++++ .../chan/ui/settings/SettingsController.java | 2 +- .../org/floens/chan/ui/toolbar/Toolbar.java | 13 ++-- .../org/floens/chan/ui/view/FloatingMenu.java | 2 +- .../floens/chan/ui/view/MultiImageView.java | 2 +- .../org/floens/chan/utils/AndroidUtils.java | 65 +++++++++++++++---- .../layout/controller_navigation_drawer.xml | 6 +- 11 files changed, 122 insertions(+), 64 deletions(-) create mode 100644 Clover/app/src/main/java/org/floens/chan/ui/layout/DrawerWidthAdjustingLayout.java diff --git a/Clover/app/src/main/java/org/floens/chan/controller/transition/PushControllerTransition.java b/Clover/app/src/main/java/org/floens/chan/controller/transition/PushControllerTransition.java index acec102f..eb1a75c2 100644 --- a/Clover/app/src/main/java/org/floens/chan/controller/transition/PushControllerTransition.java +++ b/Clover/app/src/main/java/org/floens/chan/controller/transition/PushControllerTransition.java @@ -22,7 +22,6 @@ import android.animation.AnimatorListenerAdapter; import android.animation.AnimatorSet; import android.animation.ObjectAnimator; import android.view.View; -import android.view.animation.AccelerateDecelerateInterpolator; import android.view.animation.DecelerateInterpolator; import org.floens.chan.controller.ControllerTransition; @@ -34,9 +33,9 @@ public class PushControllerTransition extends ControllerTransition { AndroidUtils.waitForMeasure(to.view, new AndroidUtils.OnMeasuredCallback() { @Override public boolean onMeasured(View view) { - Animator fromAlpha = ObjectAnimator.ofFloat(from.view, View.ALPHA, 1f, 0.7f); + /*Animator fromAlpha = ObjectAnimator.ofFloat(from.view, View.ALPHA, 1f, 0.7f); fromAlpha.setDuration(217); - fromAlpha.setInterpolator(new AccelerateDecelerateInterpolator()); // new PathInterpolator(0.4f, 0f, 0.2f, 1f) + fromAlpha.setInterpolator(new AccelerateDecelerateInterpolator()); // new PathInterpolator(0.4f, 0f, 0.2f, 1f)*/ Animator toAlpha = ObjectAnimator.ofFloat(to.view, View.ALPHA, 0f, 1f); toAlpha.setDuration(200); @@ -56,7 +55,7 @@ public class PushControllerTransition extends ControllerTransition { AnimatorSet set = new AnimatorSet(); set.playTogether(/*fromAlpha, */toAlpha, toY); set.start(); - return false; + return true; } }); } diff --git a/Clover/app/src/main/java/org/floens/chan/ui/controller/DrawerController.java b/Clover/app/src/main/java/org/floens/chan/ui/controller/DrawerController.java index d7ce6bfe..b7238c14 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/controller/DrawerController.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/controller/DrawerController.java @@ -19,7 +19,6 @@ package org.floens.chan.ui.controller; import android.content.Context; import android.content.DialogInterface; -import android.content.res.Configuration; import android.support.design.widget.Snackbar; import android.support.v4.widget.DrawerLayout; import android.support.v7.app.AlertDialog; @@ -97,13 +96,6 @@ public class DrawerController extends Controller implements PinAdapter.Callback, itemTouchHelper.attachToRecyclerView(recyclerView); updateBadge(); - - AndroidUtils.waitForMeasure(drawer, new AndroidUtils.OnMeasuredCallback() { - @Override - public boolean onMeasured(View view) { - return setDrawerWidth(); - } - }); } @Override @@ -119,18 +111,6 @@ public class DrawerController extends Controller implements PinAdapter.Callback, childController.onShow(); } - @Override - public void onConfigurationChanged(Configuration newConfig) { - super.onConfigurationChanged(newConfig); - - AndroidUtils.waitForLayout(drawer, new AndroidUtils.OnMeasuredCallback() { - @Override - public boolean onMeasured(View view) { - return setDrawerWidth(); - } - }); - } - @Override public void onClick(View v) { if (v == settings) { @@ -274,17 +254,6 @@ public class DrawerController extends Controller implements PinAdapter.Callback, } } - private boolean setDrawerWidth() { - int width = Math.min(view.getWidth() - dp(56), dp(56) * 6); - if (drawer.getWidth() != width) { - drawer.getLayoutParams().width = width; - drawer.requestLayout(); - return true; - } else { - return false; - } - } - private void openController(Controller controller) { Controller top = getTop(); if (top instanceof NavigationController) { diff --git a/Clover/app/src/main/java/org/floens/chan/ui/controller/ImageViewerController.java b/Clover/app/src/main/java/org/floens/chan/ui/controller/ImageViewerController.java index ede819c3..f4872f72 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/controller/ImageViewerController.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/controller/ImageViewerController.java @@ -130,7 +130,12 @@ public class ImageViewerController extends Controller implements ImageViewerPres pager.addOnPageChangeListener(presenter); loadingBar = (LoadingBar) view.findViewById(R.id.loading_bar); - AndroidUtils.waitForMeasure(view, new AndroidUtils.OnMeasuredCallback() { + // Sanity check + if (parentController.view.getWindowToken() == null) { + throw new IllegalArgumentException("parentController.view not attached"); + } + + AndroidUtils.waitForLayout(parentController.view.getViewTreeObserver(), view, new AndroidUtils.OnMeasuredCallback() { @Override public boolean onMeasured(View view) { presenter.onViewMeasured(); diff --git a/Clover/app/src/main/java/org/floens/chan/ui/controller/PassSettingsController.java b/Clover/app/src/main/java/org/floens/chan/ui/controller/PassSettingsController.java index fbb6ee87..ccae7dce 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/controller/PassSettingsController.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/controller/PassSettingsController.java @@ -80,13 +80,19 @@ public class PassSettingsController extends Controller implements View.OnClickLi inputToken.setText(ChanSettings.passToken.get()); inputPin.setText(ChanSettings.passPin.get()); - AndroidUtils.waitForLayout(view, new AndroidUtils.OnMeasuredCallback() { + // Sanity check + if (parentController.view.getWindowToken() == null) { + throw new IllegalArgumentException("parentController.view not attached"); + } + + // TODO: remove + AndroidUtils.waitForLayout(parentController.view.getViewTreeObserver(), view, new AndroidUtils.OnMeasuredCallback() { @Override public boolean onMeasured(View view) { crossfadeView.getLayoutParams().height = crossfadeView.getHeight(); crossfadeView.requestLayout(); crossfadeView.toggle(!loggedIn, false); - return true; + return false; } }); } diff --git a/Clover/app/src/main/java/org/floens/chan/ui/layout/DrawerWidthAdjustingLayout.java b/Clover/app/src/main/java/org/floens/chan/ui/layout/DrawerWidthAdjustingLayout.java new file mode 100644 index 00000000..76b0aed7 --- /dev/null +++ b/Clover/app/src/main/java/org/floens/chan/ui/layout/DrawerWidthAdjustingLayout.java @@ -0,0 +1,41 @@ +package org.floens.chan.ui.layout; + +import android.content.Context; +import android.support.v4.widget.DrawerLayout; +import android.util.AttributeSet; +import android.view.View; + +import org.floens.chan.R; + +import static org.floens.chan.utils.AndroidUtils.dp; + +public class DrawerWidthAdjustingLayout extends DrawerLayout { + public DrawerWidthAdjustingLayout(Context context) { + super(context); + } + + public DrawerWidthAdjustingLayout(Context context, AttributeSet attrs) { + super(context, attrs); + } + + public DrawerWidthAdjustingLayout(Context context, AttributeSet attrs, int defStyle) { + super(context, attrs, defStyle); + } + + @Override + protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) { +// int widthMode = MeasureSpec.getMode(widthMeasureSpec); +// int heightMode = MeasureSpec.getMode(heightMeasureSpec); + int widthSize = MeasureSpec.getSize(widthMeasureSpec); +// int heightSize = MeasureSpec.getSize(heightMeasureSpec); + + View drawer = findViewById(R.id.drawer); + + int width = Math.min(widthSize - dp(56), dp(56) * 6); + if (drawer.getLayoutParams().width != width) { + drawer.getLayoutParams().width = width; + } + + super.onMeasure(widthMeasureSpec, heightMeasureSpec); + } +} diff --git a/Clover/app/src/main/java/org/floens/chan/ui/settings/SettingsController.java b/Clover/app/src/main/java/org/floens/chan/ui/settings/SettingsController.java index 6f746097..be8ce8d8 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/settings/SettingsController.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/settings/SettingsController.java @@ -61,7 +61,7 @@ public class SettingsController extends Controller implements AndroidUtils.OnMea @Override public boolean onMeasured(View view) { setMargins(); - return true; + return false; } public void onPreferenceChange(SettingView item) { diff --git a/Clover/app/src/main/java/org/floens/chan/ui/toolbar/Toolbar.java b/Clover/app/src/main/java/org/floens/chan/ui/toolbar/Toolbar.java index e50c99e6..63bacac3 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/toolbar/Toolbar.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/toolbar/Toolbar.java @@ -42,6 +42,7 @@ import org.floens.chan.R; import org.floens.chan.ui.drawable.ArrowMenuDrawable; import org.floens.chan.ui.drawable.DropdownArrowDrawable; import org.floens.chan.ui.layout.SearchLayout; +import org.floens.chan.ui.view.FloatingMenu; import org.floens.chan.ui.view.LoadView; import org.floens.chan.utils.AndroidUtils; @@ -453,15 +454,9 @@ public class Toolbar extends LinearLayout implements View.OnClickListener { menu.addView(item.menu, new LayoutParams(LayoutParams.WRAP_CONTENT, LayoutParams.MATCH_PARENT)); } - AndroidUtils.waitForMeasure(titleView, new AndroidUtils.OnMeasuredCallback() { - @Override - public boolean onMeasured(View view) { - if (item.middleMenu != null) { - item.middleMenu.setPopupWidth(Math.max(dp(200), titleView.getWidth())); - } - return false; - } - }); + if (item.middleMenu != null) { + item.middleMenu.setPopupWidth(FloatingMenu.POPUP_WIDTH_ANCHOR); + } return menu; } diff --git a/Clover/app/src/main/java/org/floens/chan/ui/view/FloatingMenu.java b/Clover/app/src/main/java/org/floens/chan/ui/view/FloatingMenu.java index c3105fdd..71bb92e4 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/view/FloatingMenu.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/view/FloatingMenu.java @@ -111,7 +111,7 @@ public class FloatingMenu { popupWindow.setVerticalOffset(-anchor.getHeight() + anchorOffsetY); popupWindow.setHorizontalOffset(anchorOffsetX); if (popupWidth == POPUP_WIDTH_ANCHOR) { - popupWindow.setContentWidth(Math.min(dp(4 * 56), anchor.getWidth())); + popupWindow.setContentWidth(Math.min(dp(8 * 56), Math.max(dp(4 * 56), anchor.getWidth()))); } else if (popupWidth == POPUP_WIDTH_AUTO) { popupWindow.setContentWidth(dp(3 * 56)); } else { diff --git a/Clover/app/src/main/java/org/floens/chan/ui/view/MultiImageView.java b/Clover/app/src/main/java/org/floens/chan/ui/view/MultiImageView.java index 67351ffa..d935ff3f 100644 --- a/Clover/app/src/main/java/org/floens/chan/ui/view/MultiImageView.java +++ b/Clover/app/src/main/java/org/floens/chan/ui/view/MultiImageView.java @@ -130,7 +130,7 @@ public class MultiImageView extends FrameLayout implements View.OnClickListener setVideo(postImage.imageUrl); break; } - return false; + return true; } }); } diff --git a/Clover/app/src/main/java/org/floens/chan/utils/AndroidUtils.java b/Clover/app/src/main/java/org/floens/chan/utils/AndroidUtils.java index f4f11297..e23ea782 100644 --- a/Clover/app/src/main/java/org/floens/chan/utils/AndroidUtils.java +++ b/Clover/app/src/main/java/org/floens/chan/utils/AndroidUtils.java @@ -40,7 +40,6 @@ import android.os.Handler; import android.os.Looper; import android.preference.PreferenceManager; import android.support.design.widget.Snackbar; -import android.util.Log; import android.view.View; import android.view.ViewGroup; import android.view.ViewTreeObserver; @@ -58,6 +57,8 @@ import java.util.List; import java.util.Locale; public class AndroidUtils { + private static final String TAG = "AndroidUtils"; + private static HashMap typefaceCache = new HashMap<>(); public static Typeface ROBOTO_MEDIUM; @@ -254,45 +255,87 @@ public class AndroidUtils { } public interface OnMeasuredCallback { + /** + * Called when the layout is done. + * + * @param view same view as the argument. + * @return true to continue with rendering, false to cancel and redo the layout. + */ boolean onMeasured(View view); } /** * Waits for a measure. Calls callback immediately if the view width and height are more than 0. - * Otherwise it registers an onpredrawlistener and rechedules a layout. - * Warning: the view you give must be attached to the view root!!! + * Otherwise it registers an onpredrawlistener. + * Warning: the view you give must be attached to the view root! */ public static void waitForMeasure(final View view, final OnMeasuredCallback callback) { - waitForMeasure(true, view, callback); + if (view.getWindowToken() == null) { + // If you call getViewTreeObserver on a view when it's not attached to a window will result in the creation of a temporarily viewtreeobserver. + // This is almost always not what you want. + throw new IllegalArgumentException("The view given to waitForMeasure is not attached to the window and does not have a ViewTreeObserver."); + } + + waitForLayoutInternal(true, view.getViewTreeObserver(), view, callback); } + /** + * Always registers an onpredrawlistener. + * Warning: the view you give must be attached to the view root! + */ public static void waitForLayout(final View view, final OnMeasuredCallback callback) { - waitForMeasure(false, view, callback); + if (view.getWindowToken() == null) { + // See comment above + throw new IllegalArgumentException("The view given to waitForLayout is not attached to the window and does not have a ViewTreeObserver."); + } + + waitForLayoutInternal(false, view.getViewTreeObserver(), view, callback); + } + + /** + * Always registers an onpredrawlistener. The given ViewTreeObserver will be used. + */ + public static void waitForLayout(final ViewTreeObserver viewTreeObserver, final View view, final OnMeasuredCallback callback) { + waitForLayoutInternal(false, viewTreeObserver, view, callback); } - private static void waitForMeasure(boolean returnIfNotZero, final View view, final OnMeasuredCallback callback) { + private static void waitForLayoutInternal(boolean returnIfNotZero, final ViewTreeObserver viewTreeObserver, final View view, final OnMeasuredCallback callback) { int width = view.getWidth(); int height = view.getHeight(); if (returnIfNotZero && width > 0 && height > 0) { callback.onMeasured(view); } else { - view.getViewTreeObserver().addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener() { + Logger.d(TAG, "Adding OnPreDrawListener to ViewTreeObserver"); + viewTreeObserver.addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener() { @Override public boolean onPreDraw() { - final ViewTreeObserver observer = view.getViewTreeObserver(); - if (observer.isAlive()) { - observer.removeOnPreDrawListener(this); + Logger.d(TAG, "OnPreDraw callback"); + + ViewTreeObserver usingViewTreeObserver = viewTreeObserver; + if (viewTreeObserver != view.getViewTreeObserver()) { + Logger.e(TAG, "view.getViewTreeObserver() is another viewtreeobserver! replacing with the new one"); + usingViewTreeObserver = view.getViewTreeObserver(); + } + + if (usingViewTreeObserver.isAlive()) { + usingViewTreeObserver.removeOnPreDrawListener(this); + } else { + Logger.w(TAG, "ViewTreeObserver not alive, could not remove onPreDrawListener! This will probably not end well"); } boolean ret; try { ret = callback.onMeasured(view); } catch (Exception e) { - Log.i("AndroidUtils", "Exception in onMeasured", e); + Logger.i(TAG, "Exception in onMeasured", e); throw e; } + if (!ret) { + Logger.w(TAG, "waitForLayout requested a re-layout by returning false"); + } + return ret; } }); diff --git a/Clover/app/src/main/res/layout/controller_navigation_drawer.xml b/Clover/app/src/main/res/layout/controller_navigation_drawer.xml index d8ef7582..e3f5eb57 100644 --- a/Clover/app/src/main/res/layout/controller_navigation_drawer.xml +++ b/Clover/app/src/main/res/layout/controller_navigation_drawer.xml @@ -15,7 +15,7 @@ GNU General Public License for more details. You should have received a copy of the GNU General Public License along with this program. If not, see . --> -. . - +