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

Commit 31ea252f authored by Hans Boehm's avatar Hans Boehm
Browse files

Clean up Calculator state, evaluation and cancellation logic

Bug: 33216049
Bug: 33219408
Bug: 33107696

Add INIT_FOR_RESULT state to represent INIT state, when we know that
computation has previously ended in RESULT state.

Stubbornly refuse to display the history view if we are currently
in EVALUATE or INIT state. Both of these are very temporary.

Break up Calculator.onCreate() to make it a bit more manageable.

Add a number of assertion checks to detect unexpected states, etc.

Add HISTORY_MAIN_INDEX to ensure that we only have one evaluation
listener per expression being evaluated.

Add cancelNonMain to better target cancellation requests.

Remove evaluateInstantIfNecessary() hack in HistoryFragment.
We should no longer be randomly cancelling evaluations we don't
own.

Be a little more aggressive about avoid redundant evaluations.

Change-Id: I5eaf6390b597926f9255c635fb44d50b47cbd1e1
parent 03a566a1
Loading
Loading
Loading
Loading
+108 −67
Original line number Diff line number Diff line
@@ -99,6 +99,7 @@ public class Calculator extends Activity
                        // Not used for instant result evaluation.
        INIT,           // Very temporary state used as alternative to EVALUATE
                        // during reinitialization.  Do not animate on completion.
        INIT_FOR_RESULT,  // Identical to INIT, but evaluation is known to terminate.
        ANIMATE,        // Result computed, animation to enlarge result window in progress.
        RESULT,         // Result displayed, formula invisible.
                        // If we are in RESULT state, the formula was evaluated without
@@ -114,8 +115,6 @@ public class Calculator extends Activity
    // initially evaluate assuming we were given a well-defined problem.  If we
    // were actually asked to compute sqrt(<extremely tiny negative number>) we produce 0
    // unless we are asked for enough precision that we can distinguish the argument from zero.
    // TODO: Consider further heuristics to reduce the chance of observing this?
    //       It already seems to be observable only in contrived cases.
    // ANIMATE, ERROR, and RESULT are translated to an INIT state if the application
    // is restarted in that state.  This leads us to recompute and redisplay the result
    // ASAP.
@@ -298,9 +297,64 @@ public class Calculator extends Activity

    private HistoryFragment mHistoryFragment = new HistoryFragment();

    /**
     * Restore Evaluator state and mCurrentState from savedInstanceState.
     * Return true if the toolbar should be visible.
     */
    private void restoreInstanceState(Bundle savedInstanceState) {
        final CalculatorState savedState = CalculatorState.values()[
                savedInstanceState.getInt(KEY_DISPLAY_STATE,
                        CalculatorState.INPUT.ordinal())];
        setState(savedState);
        CharSequence unprocessed = savedInstanceState.getCharSequence(KEY_UNPROCESSED_CHARS);
        if (unprocessed != null) {
            mUnprocessedChars = unprocessed.toString();
        }
        byte[] state = savedInstanceState.getByteArray(KEY_EVAL_STATE);
        if (state != null) {
            try (ObjectInput in = new ObjectInputStream(new ByteArrayInputStream(state))) {
                mEvaluator.restoreInstanceState(in);
            } catch (Throwable ignored) {
                // When in doubt, revert to clean state
                mCurrentState = CalculatorState.INPUT;
                mEvaluator.clearMain();
            }
        }
        if (savedInstanceState.getBoolean(KEY_SHOW_TOOLBAR, true)) {
            showAndMaybeHideToolbar();
        } else {
            mDisplayView.hideToolbar();
        }
        onInverseToggled(savedInstanceState.getBoolean(KEY_INVERSE_MODE));
        // TODO: We're currently not saving and restoring scroll position.
        //       We probably should.  Details may require care to deal with:
        //         - new display size
        //         - slow recomputation if we've scrolled far.
    }

    private void restoreDisplay() {
        onModeChanged(mEvaluator.getDegreeMode(Evaluator.MAIN_INDEX));
        if (mCurrentState != CalculatorState.RESULT
            && mCurrentState != CalculatorState.INIT_FOR_RESULT) {
            redisplayFormula();
        }
        if (mCurrentState == CalculatorState.INPUT) {
            // This resultText will explicitly call evaluateAndNotify when ready.
            mResultText.setShouldEvaluateResult(CalculatorResult.SHOULD_EVALUATE, this);
        } else {
            // Just reevaluate.
            setState((mCurrentState == CalculatorState.RESULT
                    || mCurrentState == CalculatorState.INIT_FOR_RESULT) ?
                    CalculatorState.INIT_FOR_RESULT : CalculatorState.INIT);
            // Request evaluation when we know display width.
            mResultText.setShouldEvaluateResult(CalculatorResult.SHOULD_REQUIRE, this);
        }
    }

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        setContentView(R.layout.activity_calculator_main);
        setActionBar((Toolbar) findViewById(R.id.toolbar));

@@ -320,6 +374,9 @@ public class Calculator extends Activity
        mFormulaText = (CalculatorFormula) findViewById(R.id.formula);
        mResultText = (CalculatorResult) findViewById(R.id.result);
        mFormulaContainer = (HorizontalScrollView) findViewById(R.id.formula_container);
        mEvaluator = Evaluator.getInstance(this);
        mResultText.setEvaluator(mEvaluator, Evaluator.MAIN_INDEX);
        KeyMaps.setActivity(this);

        mPadViewPager = (ViewPager) findViewById(R.id.pad_pager);
        mDeleteButton = findViewById(R.id.del);
@@ -354,40 +411,12 @@ public class Calculator extends Activity
                findViewById(R.id.op_sqr)
        };

        mEvaluator = Evaluator.getInstance(this);
        mResultText.setEvaluator(mEvaluator, Evaluator.MAIN_INDEX);
        KeyMaps.setActivity(this);

        mDragLayout = (DragLayout) findViewById(R.id.drag_layout);
        mDragLayout.removeDragCallback(mDragCallback);
        mDragLayout.addDragCallback(mDragCallback);

        mHistoryFrame = (FrameLayout) findViewById(R.id.history_frame);

        if (savedInstanceState != null) {
            final CalculatorState savedState = CalculatorState.values()[
                    savedInstanceState.getInt(KEY_DISPLAY_STATE,
                            CalculatorState.INPUT.ordinal())];
            setState(savedState);
            CharSequence unprocessed = savedInstanceState.getCharSequence(KEY_UNPROCESSED_CHARS);
            if (unprocessed != null) {
                mUnprocessedChars = unprocessed.toString();
            }
            byte[] state = savedInstanceState.getByteArray(KEY_EVAL_STATE);
            if (state != null) {
                try (ObjectInput in = new ObjectInputStream(new ByteArrayInputStream(state))) {
                    mEvaluator.restoreInstanceState(in);
                } catch (Throwable ignored) {
                    // When in doubt, revert to clean state
                    mCurrentState = CalculatorState.INPUT;
                    mEvaluator.clearMain();
                }
            }
        } else {
            mCurrentState = CalculatorState.INPUT;
            mEvaluator.clearMain();
        }

        mFormulaText.setOnContextMenuClickListener(mOnFormulaContextMenuClickListener);
        mFormulaText.setOnDisplayMemoryOperationsListener(mOnDisplayMemoryOperationsListener);

@@ -395,31 +424,15 @@ public class Calculator extends Activity
        mFormulaText.addTextChangedListener(mFormulaTextWatcher);
        mDeleteButton.setOnLongClickListener(this);

        onInverseToggled(savedInstanceState != null
                && savedInstanceState.getBoolean(KEY_INVERSE_MODE));

        onModeChanged(mEvaluator.getDegreeMode(Evaluator.MAIN_INDEX));
        if (savedInstanceState != null &&
                savedInstanceState.getBoolean(KEY_SHOW_TOOLBAR, true) == false) {
            mDisplayView.hideToolbar();
        if (savedInstanceState != null) {
            restoreInstanceState(savedInstanceState);
        } else {
            mCurrentState = CalculatorState.INPUT;
            mEvaluator.clearMain();
            showAndMaybeHideToolbar();
            onInverseToggled(false);
        }

        redisplayFormula();
        if (mCurrentState != CalculatorState.INPUT) {
            // Just reevaluate.
            setState(CalculatorState.INIT);
            // Request evaluation when we know display width.
            mResultText.setShouldEvaluateResult(CalculatorResult.SHOULD_REQUIRE, this);
        } else {
            // This resultText will explicitly call evaluateAndNotify when ready.
            mResultText.setShouldEvaluateResult(CalculatorResult.SHOULD_EVALUATE, this);
        }
        // TODO: We're currently not saving and restoring scroll position.
        //       We probably should.  Details may require care to deal with:
        //         - new display size
        //         - slow recomputation if we've scrolled far.
        restoreDisplay();
    }

    @Override
@@ -509,9 +522,12 @@ public class Calculator extends Activity
    }

    public boolean isResultLayout() {
        return mCurrentState == CalculatorState.INIT
                || mCurrentState == CalculatorState.RESULT
                || mCurrentState == CalculatorState.ANIMATE;
        if (mCurrentState == CalculatorState.ANIMATE) {
            throw new AssertionError("impossible state");
        }
        // Note that ERROR has INPUT, not RESULT layout.
        return mCurrentState == CalculatorState.INIT_FOR_RESULT
                || mCurrentState == CalculatorState.RESULT;
    }

    @Override
@@ -591,9 +607,7 @@ public class Calculator extends Activity
        // Always cancel unrequested in-progress evaluation of the main expression, so that
        // we don't have to worry about subsequent asynchronous completion.
        // Requested in-progress evaluations are handled below.
        if (mCurrentState != CalculatorState.EVALUATE) {
            mEvaluator.cancel(Evaluator.MAIN_INDEX, true);
        }
        cancelUnrequested();

        switch (keyCode) {
            case KeyEvent.KEYCODE_NUMPAD_ENTER:
@@ -776,10 +790,7 @@ public class Calculator extends Activity
        stopActionModeOrContextMenu();

        // See onKey above for the rationale behind some of the behavior below:
        if (mCurrentState != CalculatorState.EVALUATE) {
            // Cancel main expression evaluations that were not specifically requested.
            mEvaluator.cancel(Evaluator.MAIN_INDEX, true);
        }
        cancelUnrequested();

        final int id = view.getId();
        switch (id) {
@@ -870,8 +881,9 @@ public class Calculator extends Activity
        invalidateOptionsMenu();

        mResultText.onEvaluate(index, initDisplayPrec, msd, leastDigPos, truncatedWholeNumber);
        if (mCurrentState != CalculatorState.INPUT) { // in EVALUATE or INIT state
            onResult(mCurrentState != CalculatorState.INIT);
        if (mCurrentState != CalculatorState.INPUT) {
            // In EVALUATE, INIT, or INIT_FOR_RESULT state.
            onResult(mCurrentState == CalculatorState.EVALUATE);
        }
    }

@@ -923,14 +935,20 @@ public class Calculator extends Activity
     */
    private boolean cancelIfEvaluating(boolean quiet) {
        if (mCurrentState == CalculatorState.EVALUATE) {
            // TODO: Maybe just cancel main expression evaluation?
            mEvaluator.cancelAll(quiet);
            mEvaluator.cancel(Evaluator.MAIN_INDEX, quiet);
            return true;
        } else {
            return false;
        }
    }


    private void cancelUnrequested() {
        if (mCurrentState == CalculatorState.INPUT) {
            mEvaluator.cancel(Evaluator.MAIN_INDEX, true);
        }
    }

    private boolean haveUnprocessed() {
        return mUnprocessedChars != null && !mUnprocessedChars.isEmpty();
    }
@@ -1067,7 +1085,8 @@ public class Calculator extends Activity
                           mResultText.onError(index, errorResourceId);
                        }
                    });
        } else if (mCurrentState == CalculatorState.INIT) {
        } else if (mCurrentState == CalculatorState.INIT
                || mCurrentState == CalculatorState.INIT_FOR_RESULT /* very unlikely */) {
            setState(CalculatorState.ERROR);
            mResultText.onError(index, errorResourceId);
        } else {
@@ -1218,11 +1237,33 @@ public class Calculator extends Activity
        }
    }

    /**
     * Change evaluation state to one that's friendly to the history fragment.
     * Return false if that was not easily possible.
     */
    private boolean prepareForHistory() {
        if (mCurrentState == CalculatorState.ANIMATE) {
            throw new AssertionError("onUserInteraction should have ended animation");
        } else if (mCurrentState == CalculatorState.EVALUATE
                || mCurrentState == CalculatorState.INIT) {
            // Easiest to just refuse.  Otherwise we can see a state change
            // while in history mode, which causes all sorts of problems.
            // TODO: Consider other alternatives. If we're just doing the decimal conversion
            // at the end of an evaluation, we could treat this as RESULT state.
            return false;
        }
        // We should be in INPUT, INIT_FOR_RESULT, RESULT, or ERROR state.
        return true;
    }

    private void showHistoryFragment(int transit) {
        final FragmentManager manager = getFragmentManager();
        if (manager == null || manager.isDestroyed()) {
            return;
        }
        if (!prepareForHistory()) {
            return;
        }
        if (!mDragLayout.isOpen()) {
            manager.beginTransaction()
                    .replace(R.id.history_frame, mHistoryFragment, HistoryFragment.TAG)
+52 −10
Original line number Diff line number Diff line
@@ -193,10 +193,14 @@ public class Evaluator implements CalculatorExpr.ExprResolver {

    public static final long MAIN_INDEX = 0;  // Index of main expression.
    // Once final evaluation of an expression is complete, or when we need to save
    // a partial result, we copy the expression to a non-zero index.
    // a partial result, we copy the main expression to a non-zero index.
    // At that point, the expression no longer changes, and is preserved
    // until the entire history is cleared. Only expressions at nonzero indices
    // may be embedded in other expressions.
    // Each expression index can only have one outstanding evaluation request at a time.
    // To avoid conflicts between the history and main View, we copy the main expression
    // to allow independent evaluation by both.
    public static final long HISTORY_MAIN_INDEX = -1;  // Read-only copy of main expression.
    // To update e.g. "memory" contents, we copy the corresponding expression to a permanent
    // index, and then remember that index.
    private long mSavedIndex;  // Index of "saved" expression mirroring clipboard. 0 if unused.
@@ -265,7 +269,8 @@ public class Evaluator implements CalculatorExpr.ExprResolver {

    /**
     * An individual CalculatorExpr, together with its evaluation state.
     * Only the main expression may be changed in-place.
     * Only the main expression may be changed in-place. The HISTORY_MAIN_INDEX expression is
     * periodically reset to be a fresh immutable copy of the main expression.
     * All other expressions are only added and never removed. The expressions themselves are
     * never modified.
     * All fields other than mExpr and mVal are touched only by the UI thread.
@@ -285,6 +290,9 @@ public class Evaluator implements CalculatorExpr.ExprResolver {

        // Currently running expression evaluator, if any.  This is either an AsyncEvaluator
        // (if mResultString == null or it's obsolete), or an AsyncReevaluator.
        // We arrange that only one evaluator is active at a time, in part by maintaining
        // two separate ExprInfo structure for the main and history view, so that they can
        // arrange for independent evaluators.
        public AsyncTask mEvaluator;

        // The remaining fields are valid only if an evaluation completed successfully.
@@ -1130,14 +1138,14 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
            // 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) {
        ExprInfo ei = ensureExprIsCached(index);
        if (ei.mResultString != null && !(index == MAIN_INDEX && mChangedValue)) {
            // Already done. Just notify.
            notifyImmediately(MAIN_INDEX, mMainExpr, listener, cmi);
            return;
            }
        } else {
            ensureExprIsCached(index);
        } else if (ei.mEvaluator != null) {
            // We only allow a single listener per expression, so this request must be redundant.
            return;
        }
        evaluateResult(index, listener, cmi, false);
    }
@@ -1241,6 +1249,20 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
        }
    }

    /**
     * Quietly cancel all evaluations associated with expressions other than the main one.
     * These are currently the evaluations associated with the history fragment.
     */
    public void cancelNonMain() {
        // TODO: May want to keep active evaluators in a HashSet to avoid traversing
        // all expressions we've looked at.
        for (ExprInfo expr: mExprs.values()) {
            if (expr != mMainExpr) {
                cancel(expr, true);
            }
        }
    }

    /**
     * Restore the evaluator state, including the current expression.
     */
@@ -1400,6 +1422,9 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
        }
        // Add newly assigned date to the cache.
        ei.mTimeStamp = rd.mTimeStamp;
        if (resultIndex == MAIN_INDEX) {
            throw new AssertionError("Should not store main expression");
        }
        mExprs.put(resultIndex, ei);
        return resultIndex;
    }
@@ -1430,9 +1455,23 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
            return;
        }
        ExprInfo ei = copy(MAIN_INDEX, true);
        if (resultIndex == MAIN_INDEX) {
            throw new AssertionError("Should not store main expression");
        }
        mExprs.put(resultIndex, ei);
    }

    /**
     * Discard previous expression in HISTORY_MAIN_INDEX and replace it by a fresh copy
     * of the main expression. Note that the HISTORY_MAIN_INDEX expresssion is not preserved
     * in the database or anywhere else; it is always reconstructed when needed.
     */
    public void copyMainToHistory() {
        cancel(HISTORY_MAIN_INDEX, true /* quiet */);
        ExprInfo ei = copy(MAIN_INDEX, true);
        mExprs.put(HISTORY_MAIN_INDEX, ei);
    }

    /**
     * @return the {@link CalculatorExpr} representation of the result of the given
     * expression.
@@ -1679,6 +1718,9 @@ public class Evaluator implements CalculatorExpr.ExprResolver {
        if (ei != null) {
            return ei;
        }
        if (index == MAIN_INDEX) {
            throw new AssertionError("Main expression should be cached");
        }
        ExpressionDB.RowData row = mExprDB.getRow(index);
        DataInputStream serializedExpr =
                new DataInputStream(new ByteArrayInputStream(row.mExpression));
+2 −2
Original line number Diff line number Diff line
@@ -73,7 +73,7 @@ public class HistoryAdapter extends RecyclerView.Adapter<HistoryAdapter.ViewHold
        holder.mFormula.setText(item.getFormula());
        // Note: HistoryItems that are not the current expression will always have interesting ops.
        holder.mResult.setEvaluator(mEvaluator, item.getEvaluatorIndex());
        if (item.getEvaluatorIndex() == Evaluator.MAIN_INDEX) {
        if (item.getEvaluatorIndex() == Evaluator.HISTORY_MAIN_INDEX) {
            holder.mDate.setText(mCurrentExpressionDescription);
            holder.mDate.setContentDescription(mCurrentExpressionDescription);
        } else {
+5 −8
Original line number Diff line number Diff line
@@ -49,8 +49,7 @@ public class HistoryFragment extends Fragment {

                @Override
                public void onClosed() {
                    // TODO: only cancel historical evaluations
                    mEvaluator.cancelAll(true);
                    mEvaluator.cancelNonMain();
                }

                @Override
@@ -152,8 +151,9 @@ public class HistoryFragment extends Fragment {
                // recyclerview).
                // If we are in the result state, the result will animate to the last history
                // element in the list and there will be no "Current Expression."
                newDataSet.add(new HistoryItem(Evaluator.MAIN_INDEX, System.currentTimeMillis(),
                        mEvaluator.getExprAsSpannable(0)));
                mEvaluator.copyMainToHistory();
                newDataSet.add(new HistoryItem(Evaluator.HISTORY_MAIN_INDEX,
                        System.currentTimeMillis(), mEvaluator.getExprAsSpannable(0)));
            }
            for (long i = 0; i < maxIndex; ++i) {
                newDataSet.add(null);
@@ -200,11 +200,8 @@ public class HistoryFragment extends Fragment {
            dragLayout.removeDragCallback(mDragCallback);
        }

        mEvaluator.cancelAll(true);
        mEvaluator.cancelNonMain();
        super.onDestroy();
        // FIXME: There are probably better ways to do this. But we can end up cancelling
        // an in-progress evaluation for the main expression that we have to restart.
        ((Calculator)(getActivity())).evaluateInstantIfNecessary();
    }

    private void initializeController(boolean isResult) {