Avoid duplicate requireResult() calls, add checking

Bug: 33011774

Don't needlessly cancel a previous requireResult() call. In addition to
wasting resources, this results in an onCancelled() call that set
the state back to INPUT, with surprising UI results.

If requireResult() is called before the corresponding CalculatorResult
is measured, crash with a stack trace. That seems to be an easy mistake.

Delete code that used to make things kind of work if we ran without
a correct display size.

Delay requireResult call that used to occur during onCreate().
Arrange to get the right listener invoked when the onLayout
requireResult() call completes.

Change-Id: I5da4855bfdda27fca5abc5871c4b312ef6ebede0

3 files changed, 36 insertions, 23 deletions

diff --git a/src/com/android/calculator2/Calculator.java b/src/com/android/calculator2/Calculator.java
index 6cf5399..ee39f9d 100644
--- a/src/com/android/calculator2/Calculator.java
+++ b/src/com/android/calculator2/Calculator.java
@@ -311,8 +311,6 @@ public class Calculator extends Activity
 
         mEvaluator = Evaluator.getInstance(this);
         mResultText.setEvaluator(mEvaluator, Evaluator.MAIN_INDEX);
-        // This resultText should always use evaluateAndNotify, not requireResult().
-        mResultText.setShouldRequireResult(false);
         KeyMaps.setActivity(this);
 
         mDragLayout = (DragLayout) findViewById(R.id.drag_layout);
@@ -365,8 +363,11 @@ public class Calculator extends Activity
             // Just reevaluate.
             redisplayFormula();
             setState(CalculatorState.INIT);
-            mEvaluator.requireResult(Evaluator.MAIN_INDEX, this, mResultText);
+            // Request evaluation when we know display width.
+            mResultText.setShouldRequireResult(true, this);
         } else {
+            // This resultText will explicitly call evaluateAndNotify when ready.
+            mResultText.setShouldRequireResult(false, null);
             redisplayAfterFormulaChange();
         }
         // TODO: We're currently not saving and restoring scroll position.
@@ -412,6 +413,8 @@ public class Calculator extends Activity
     private void setState(CalculatorState state) {
         if (mCurrentState != state) {
             if (state == CalculatorState.INPUT) {
+                // We'll explicitly request evaluation from now on.
+                mResultText.setShouldRequireResult(false, null);
                 restoreDisplayPositions();
             }
             mCurrentState = state;
diff --git a/src/com/android/calculator2/CalculatorResult.java b/src/com/android/calculator2/CalculatorResult.java
index bfabfce..5c97bfa 100644
--- a/src/com/android/calculator2/CalculatorResult.java
+++ b/src/com/android/calculator2/CalculatorResult.java
@@ -98,8 +98,9 @@ public class CalculatorResult extends AlignedTextView implements MenuItem.OnMenu
                             // Protects the next five fields.  These fields are only
                             // Updated by the UI thread, and read accesses by the UI thread
                             // sometimes do not acquire the lock.
-    private int mWidthConstraint = -1;
+    private int mWidthConstraint = 0;
                             // Our total width in pixels minus space for ellipsis.
+                            // 0 ==> uninitialized.
     private float mCharWidth = 1;
                             // Maximum character width. For now we pretend that all characters
                             // have this width.
@@ -114,8 +115,8 @@ public class CalculatorResult extends AlignedTextView implements MenuItem.OnMenu
                             // Fraction of digit width saved by both replacing ellipsis with digit
                             // and avoiding scientific notation.
     private boolean mShouldRequireResult = true;
-    private static final int MAX_WIDTH = 100;
-                            // Maximum number of digits displayed.
+    private Evaluator.EvaluationListener mEvaluationListener = this;
+                            // Listener to use if/when evaluation is requested.
     public static final int MAX_LEADING_ZEROES = 6;
                             // Maximum number of leading zeroes after decimal point before we
                             // switch to scientific notation with negative exponent.
@@ -311,12 +312,13 @@ public class CalculatorResult extends AlignedTextView implements MenuItem.OnMenu
         if (mEvaluator != null && mShouldRequireResult) {
             final CalculatorExpr expr = mEvaluator.getExpr(mIndex);
             if (expr != null && expr.hasInterestingOps()) {
-                mEvaluator.requireResult(mIndex, this, this);
+                mEvaluator.requireResult(mIndex, mEvaluationListener, this);
             }
         }
     }
 
-    public void setShouldRequireResult(boolean should) {
+    public void setShouldRequireResult(boolean should, Evaluator.EvaluationListener listener) {
+        mEvaluationListener = listener;
         mShouldRequireResult = should;
     }
 
@@ -780,20 +782,13 @@ public class CalculatorResult extends AlignedTextView implements MenuItem.OnMenu
     /**
      * Return the maximum number of characters that will fit in the result display.
      * May be called asynchronously from non-UI thread. From Evaluator.CharMetricsInfo.
+     * Returns zero if measurement hasn't completed.
      */
     @Override
     public int getMaxChars() {
         int result;
         synchronized(mWidthLock) {
-            result = (int) Math.floor(mWidthConstraint / mCharWidth);
-            // We can apparently finish evaluating before onMeasure in CalculatorFormula has been
-            // called, in which case we get 0 or -1 as the width constraint.
-        }
-        if (result <= 0) {
-            // Return something conservatively big, to force sufficient evaluation.
-            return MAX_WIDTH;
-        } else {
-            return result;
+            return (int) Math.floor(mWidthConstraint / mCharWidth);
         }
     }
 
diff --git a/src/com/android/calculator2/Evaluator.java b/src/com/android/calculator2/Evaluator.java
index a37a44a..8ea518f 100644
--- a/src/com/android/calculator2/Evaluator.java
+++ b/src/com/android/calculator2/Evaluator.java
@@ -151,6 +151,7 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
          * the supplied string prefix.
          * The prefix consists of the first len characters of string s, which is presumed to
          * represent a whole number. Callable from non-UI thread.
+         * Returns zero if metrics information is not yet available.
          */
         public float separatorChars(String s, int len);
         /**
@@ -282,8 +283,8 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
             mVal = new AtomicReference<UnifiedReal>();
         }
 
-        // Currently running expression evaluator, if any.  This is an AsyncEvaluator if
-        // mVal.get() == null, an AsyncReevaluator otherwise.
+        // Currently running expression evaluator, if any.  This is either an AsyncEvaluator
+        // (if mResultString == null or it's obsolete), or an AsyncReevaluator.
         public AsyncTask mEvaluator;
 
         // The remaining fields are valid only if an evaluation completed successfully.
@@ -449,7 +450,7 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
      */
     class AsyncEvaluator extends AsyncTask<Void, Void, InitialResult> {
         private boolean mDm;  // degrees
-        private boolean mRequired; // Result was requested by user.
+        public boolean mRequired; // Result was requested by user.
         private boolean mQuiet;  // Suppress cancellation message.
         private Runnable mTimeoutRunnable = null;
         private EvaluationListener mListener;  // Completion callback.
@@ -1123,8 +1124,13 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
      * Start optional evaluation of expression and display when ready.
      * @param index of expression to be evaluated.
      * Can quietly time out without a listener callback.
+     * No-op if cmi.getMaxChars() == 0.
      */
     public void evaluateAndNotify(long index, EvaluationListener listener, CharMetricsInfo cmi) {
+        if (cmi.getMaxChars() == 0) {
+            // Probably shouldn't happen. If it does, we didn't promise to do anything anyway.
+            return;
+        }
         if (index == MAIN_INDEX) {
             if (mMainExpr.mResultString != null && !mChangedValue) {
                 // Already done. Just notify.
@@ -1141,13 +1147,22 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
      * Start required evaluation of expression at given index and call back listener when ready.
      * If index is MAIN_INDEX, we may also directly display a timeout message.
      * Uses longer timeouts than optional evaluation.
+     * Requires cmi.getMaxChars() != 0.
      */
     public void requireResult(long index, EvaluationListener listener, CharMetricsInfo cmi) {
+        if (cmi.getMaxChars() == 0) {
+            throw new AssertionError("requireResult called too early");
+        }
         ExprInfo ei = ensureExprIsCached(index);
         if (ei.mResultString == null || (index == MAIN_INDEX && mChangedValue)) {
-            // Restart evaluator in requested mode, i.e. with longer timeout.
-            cancel(ei, true);
-            evaluateResult(index, listener, cmi, true);
+            if ((ei.mEvaluator instanceof AsyncEvaluator)
+                    && ((AsyncEvaluator)(ei.mEvaluator)).mRequired) {
+                // Duplicate request; ignore.
+            } else {
+                // Restart evaluator in requested mode, i.e. with longer timeout.
+                cancel(ei, true);
+                evaluateResult(index, listener, cmi, true);
+            }
         } else {
             notifyImmediately(index, ei, listener, cmi);
         }