Rework the key insertion logic

Bug: 21495243

This changes the behavior to be much more compatible with L.

We generally do not allow consecutive binary operators to be inserted.
In the case of a minus operator however, the logic is more complicated.
We do allow a minus after multiplication, division and power.
When the minus is explicitly entered, so the user sees our corrections,
we do not allow a minus after additive operators.  In pasted text,
we do.

We no longer reject additions that would result in implicit
multiplications.  We do immediately reject binary operators in leading
contexts, e.g. after a left parenthesis, in which they must result in
a syntax error.

Change-Id: I1d35d74335371f6f113808d68a4f293b699d9bd0

3 files changed, 110 insertions, 29 deletions

diff --git a/src/com/android/calculator2/Calculator.java b/src/com/android/calculator2/Calculator.java
index 87e011a..e0c8a93 100644
--- a/src/com/android/calculator2/Calculator.java
+++ b/src/com/android/calculator2/Calculator.java
@@ -14,9 +14,6 @@
  * limitations under the License.
  */
 
-// FIXME: Menu handling, particularly for cut/paste, is very ugly
-//        and not the way it was intended.
-//        Other menus are not handled brilliantly either.
 // TODO: Better indication of when the result is known to be exact.
 // TODO: Check and possibly fix accessability issues.
 // TODO: Copy & more general paste in formula?  Note that this requires
@@ -170,7 +167,7 @@ public class Calculator extends Activity
                         mCurrentButton = mEqualButton;
                         onEquals();
                     } else {
-                        addChars(String.valueOf(c));
+                        addChars(String.valueOf(c), true);
                         redisplayAfterFormulaChange();
                     }
             }
@@ -441,6 +438,7 @@ public class Calculator extends Activity
     // Add the given button id to input expression.
     // If appropriate, clear the expression before doing so.
     private void addKeyToExpr(int id) {
+        // FIXME: Other states?
         if (mCurrentState == CalculatorState.ERROR) {
             setState(CalculatorState.INPUT);
         } else if (mCurrentState == CalculatorState.RESULT) {
@@ -456,6 +454,18 @@ public class Calculator extends Activity
         }
     }
 
+    /**
+     * Add the given button id to input expression, assuming it was explicitly
+     * typed/touched.
+     * We perform slightly more aggressive correction than in pasted expressions.
+     */
+    private void addExplicitKeyToExpr(int id) {
+        if (mCurrentState == CalculatorState.INPUT && id == R.id.op_sub) {
+            mEvaluator.getExpr().removeTrailingAdditiveOperators();
+        }
+        addKeyToExpr(id);
+    }
+
     private void redisplayAfterFormulaChange() {
         // TODO: Could do this more incrementally.
         redisplayFormula();
@@ -514,7 +524,7 @@ public class Calculator extends Activity
                 }
                 break;
             default:
-                addKeyToExpr(id);
+                addExplicitKeyToExpr(id);
                 redisplayAfterFormulaChange();
                 break;
         }
@@ -858,11 +868,15 @@ public class Calculator extends Activity
         displayMessage(msg);
     }
 
-    // Add input characters to the end of the expression by mapping them to
-    // the appropriate button pushes when possible.  Leftover characters
-    // are added to mUnprocessedChars, which is presumed to immediately
-    // precede the newly added characters.
-    private void addChars(String moreChars) {
+    /**
+     * Add input characters to the end of the expression.
+     * Map them to the appropriate button pushes when possible.  Leftover characters
+     * are added to mUnprocessedChars, which is presumed to immediately precede the newly
+     * added characters.
+     * @param moreChars Characters to be added.
+     * @param explicit These characters were explicitly typed by the user.
+     */
+    private void addChars(String moreChars, boolean explicit) {
         if (mUnprocessedChars != null) {
             moreChars = mUnprocessedChars + moreChars;
         }
@@ -873,7 +887,11 @@ public class Calculator extends Activity
             int k = KeyMaps.keyForChar(c);
             if (k != View.NO_ID) {
                 mCurrentButton = findViewById(k);
-                addKeyToExpr(k);
+                if (explicit) {
+                    addExplicitKeyToExpr(k);
+                } else {
+                    addKeyToExpr(k);
+                }
                 if (Character.isSurrogate(c)) {
                     current += 2;
                 } else {
@@ -884,7 +902,11 @@ public class Calculator extends Activity
             int f = KeyMaps.funForString(moreChars, current);
             if (f != View.NO_ID) {
                 mCurrentButton = findViewById(f);
-                addKeyToExpr(f);
+                if (explicit) {
+                    addExplicitKeyToExpr(f);
+                } else {
+                    addKeyToExpr(f);
+                }
                 if (f == R.id.op_sqrt) {
                     // Square root entered as function; don't lose the parenthesis.
                     addKeyToExpr(R.id.lparen);
@@ -920,7 +942,7 @@ public class Calculator extends Activity
             mEvaluator.addSaved();
             redisplayAfterFormulaChange();
         } else {
-            addChars(item.coerceToText(this).toString());
+            addChars(item.coerceToText(this).toString(), false);
         }
         return true;
     }
diff --git a/src/com/android/calculator2/CalculatorExpr.java b/src/com/android/calculator2/CalculatorExpr.java
index 8b3cb40..73e09bf 100644
--- a/src/com/android/calculator2/CalculatorExpr.java
+++ b/src/com/android/calculator2/CalculatorExpr.java
@@ -359,35 +359,44 @@ class CalculatorExpr {
         return (KeyMaps.isBinary(o.mId));
     }
 
-    // Append press of button with given id to expression.
-    // Returns false and does nothing if this would clearly
-    // result in a syntax error.
+    /**
+     * Append press of button with given id to expression.
+     * If the insertion would clearly result in a syntax error, either just return false
+     * and do nothing, or make an adjustment to avoid the problem.  We do the latter only
+     * for unambiguous consecutive binary operators, in which case we delete the first
+     * operator.
+     */
     boolean add(int id) {
         int s = mExpr.size();
         int d = KeyMaps.digVal(id);
         boolean binary = KeyMaps.isBinary(id);
-        if (s == 0 && binary && id != R.id.op_sub) return false;
-        if (binary && hasTrailingBinary()
-            && (id != R.id.op_sub || isOperatorUnchecked(s-1, R.id.op_sub))) {
-            return false;
+        Token lastTok = s == 0 ? null : mExpr.get(s-1);
+        int lastOp = lastTok instanceof Operator ? ((Operator) lastTok).mId : 0;
+        // Quietly replace a trailing binary operator with another one, unless the second
+        // operator is minus, in which case we just allow it as a unary minus.
+        if (binary && !KeyMaps.isPrefix(id)) {
+            if (s == 0 || lastOp == R.id.lparen || KeyMaps.isFunc(lastOp)
+                    || KeyMaps.isPrefix(lastOp) && lastOp != R.id.op_sub) {
+                return false;
+            }
+            while (hasTrailingBinary()) {
+                delete();
+            }
+            // s invalid and not used below.
         }
         boolean isConstPiece = (d != KeyMaps.NOT_DIGIT || id == R.id.dec_point);
         if (isConstPiece) {
+            // Since we treat juxtaposition as multiplication, a constant can appear anywhere.
             if (s == 0) {
                 mExpr.add(new Constant());
                 s++;
             } else {
                 Token last = mExpr.get(s-1);
                 if(!(last instanceof Constant)) {
-                    if (!(last instanceof Operator)) {
-                        return false;
-                    }
-                    int lastOp = ((Operator)last).mId;
-                    if (lastOp == R.id.const_e || lastOp == R.id.const_pi
-                        || lastOp == R.id.op_fact
-                        || lastOp == R.id.rparen) {
-                        // Constant cannot possibly follow; reject immediately
-                        return false;
+                    if (last instanceof PreEval) {
+                        // Add explicit multiplication to avoid confusing display.
+                        mExpr.add(new Operator(R.id.op_mul));
+                        s++;
                     }
                     mExpr.add(new Constant());
                     s++;
@@ -400,6 +409,21 @@ class CalculatorExpr {
         }
     }
 
+    /**
+     * Remove trailing op_add and op_sub operators.
+     */
+    void removeTrailingAdditiveOperators() {
+        while (true) {
+            int s = mExpr.size();
+            if (s == 0) break;
+            Token lastTok = mExpr.get(s-1);
+            if (!(lastTok instanceof Operator)) break;
+            int lastOp = ((Operator) lastTok).mId;
+            if (lastOp != R.id.op_add && lastOp != R.id.op_sub) break;
+            delete();
+        }
+    }
+
     // Append the contents of the argument expression.
     // It is assumed that the argument expression will not change,
     // and thus its pieces can be reused directly.
diff --git a/src/com/android/calculator2/KeyMaps.java b/src/com/android/calculator2/KeyMaps.java
index 5385424..c28e80d 100644
--- a/src/com/android/calculator2/KeyMaps.java
+++ b/src/com/android/calculator2/KeyMaps.java
@@ -132,6 +132,41 @@ public class KeyMaps {
     }
 
     /**
+     * Does a button id correspond to a function that introduces an implicit lparen?
+     * Pure function.
+     */
+    public static boolean isFunc(int id) {
+        switch(id) {
+            case R.id.fun_sin:
+            case R.id.fun_cos:
+            case R.id.fun_tan:
+            case R.id.fun_arcsin:
+            case R.id.fun_arccos:
+            case R.id.fun_arctan:
+            case R.id.fun_ln:
+            case R.id.fun_log:
+            case R.id.fun_exp:
+                return true;
+            default:
+                return false;
+        }
+    }
+
+    /**
+     * Does a button id correspond to a prefix operator?
+     * Pure function.
+     */
+    public static boolean isPrefix(int id) {
+        switch(id) {
+            case R.id.op_sqrt:
+            case R.id.op_sub:
+                return true;
+            default:
+                return false;
+        }
+    }
+
+    /**
      * Does a button id correspond to a suffix operator?
      */
     public static boolean isSuffix(int id) {