Always show WAIT/PAUSE in dialer overflow menu.

Used to hide menu items when not applicable.  Now we show them always
and only perform the action in those cases where menu items were
previously visible.

Updated code to do zero-position checks when there is no
selection...previously missing check.

Changed code to use chars instead of Strings when dealing with single
digits.

Consolidated duplicate code with updateDialString() function.

bug: 7478840
Change-Id: I2aa5d3badd40079e9aa75abf7e4051f9dba5e667

3 files changed, 133 insertions, 74 deletions

diff --git a/Android.mk b/Android.mk
index 97dfd82d5..ea13c65f8 100644
--- a/Android.mk
+++ b/Android.mk
@@ -34,5 +34,5 @@ LOCAL_PROGUARD_FLAG_FILES := proguard.flags
 
 include $(BUILD_PACKAGE)
 
-# Use the folloing include to make our test apk.
+# Use the following include to make our test apk.
 include $(call all-makefiles-under,$(LOCAL_PATH))
diff --git a/src/com/android/dialer/dialpad/DialpadFragment.java b/src/com/android/dialer/dialpad/DialpadFragment.java
index f70a279e7..123d700b7 100644
--- a/src/com/android/dialer/dialpad/DialpadFragment.java
+++ b/src/com/android/dialer/dialpad/DialpadFragment.java
@@ -85,6 +85,7 @@ import com.android.dialer.util.OrientationUtil;
 import com.android.internal.telephony.ITelephony;
 import com.android.phone.common.CallLogAsync;
 import com.android.phone.common.HapticFeedback;
+import com.google.common.annotations.VisibleForTesting;
 
 import java.util.List;
 
@@ -103,6 +104,8 @@ public class DialpadFragment extends Fragment
     private static final boolean DEBUG = DialtactsActivity.DEBUG;
 
     private static final String EMPTY_NUMBER = "";
+    private static final char PAUSE = ',';
+    private static final char WAIT = ';';
 
     /** The length of DTMF tones in milliseconds */
     private static final int TONE_LENGTH_MS = 150;
@@ -692,8 +695,6 @@ public class DialpadFragment extends Fragment
     private void setupMenuItems(Menu menu) {
         final MenuItem callSettingsMenuItem = menu.findItem(R.id.menu_call_settings_dialpad);
         final MenuItem addToContactMenuItem = menu.findItem(R.id.menu_add_contacts);
-        final MenuItem twoSecPauseMenuItem = menu.findItem(R.id.menu_2s_pause);
-        final MenuItem waitMenuItem = menu.findItem(R.id.menu_add_wait);
 
         // Check if all the menu items are inflated correctly. As a shortcut, we assume all menu
         // items are ready if the first item is non-null.
@@ -710,54 +711,17 @@ public class DialpadFragment extends Fragment
             callSettingsMenuItem.setIntent(DialtactsActivity.getCallSettingsIntent());
         }
 
-        // We show "add to contacts", "2sec pause", and "add wait" menus only when the user is
-        // seeing usual dialpads and has typed at least one digit.
+        // We show "add to contacts" menu only when the user is
+        // seeing usual dialpad and has typed at least one digit.
         // We never show a menu if the "choose dialpad" UI is up.
         if (dialpadChooserVisible() || isDigitsEmpty()) {
             addToContactMenuItem.setVisible(false);
-            twoSecPauseMenuItem.setVisible(false);
-            waitMenuItem.setVisible(false);
         } else {
             final CharSequence digits = mDigits.getText();
 
             // Put the current digits string into an intent
             addToContactMenuItem.setIntent(getAddToContactIntent(digits));
             addToContactMenuItem.setVisible(true);
-
-            // Check out whether to show Pause & Wait option menu items
-            int selectionStart;
-            int selectionEnd;
-            String strDigits = digits.toString();
-
-            selectionStart = mDigits.getSelectionStart();
-            selectionEnd = mDigits.getSelectionEnd();
-
-            if (selectionStart != -1) {
-                if (selectionStart > selectionEnd) {
-                    // swap it as we want start to be less then end
-                    int tmp = selectionStart;
-                    selectionStart = selectionEnd;
-                    selectionEnd = tmp;
-                }
-
-                if (selectionStart != 0) {
-                    // Pause can be visible if cursor is not in the begining
-                    twoSecPauseMenuItem.setVisible(true);
-
-                    // For Wait to be visible set of condition to meet
-                    waitMenuItem.setVisible(showWait(selectionStart, selectionEnd, strDigits));
-                } else {
-                    // cursor in the beginning both pause and wait to be invisible
-                    twoSecPauseMenuItem.setVisible(false);
-                    waitMenuItem.setVisible(false);
-                }
-            } else {
-                twoSecPauseMenuItem.setVisible(true);
-
-                // cursor is not selected so assume new digit is added to the end
-                int strLength = strDigits.length();
-                waitMenuItem.setVisible(showWait(strLength, strLength, strDigits));
-            }
         }
     }
 
@@ -1528,10 +1492,10 @@ public class DialpadFragment extends Fragment
     public boolean onOptionsItemSelected(MenuItem item) {
         switch (item.getItemId()) {
             case R.id.menu_2s_pause:
-                updateDialString(",");
+                updateDialString(PAUSE);
                 return true;
             case R.id.menu_add_wait:
-                updateDialString(";");
+                updateDialString(WAIT);
                 return true;
             default:
                 return false;
@@ -1547,7 +1511,12 @@ public class DialpadFragment extends Fragment
      * Updates the dial string (mDigits) after inserting a Pause character (,)
      * or Wait character (;).
      */
-    private void updateDialString(String newDigits) {
+    private void updateDialString(char newDigit) {
+        if(newDigit != WAIT && newDigit != PAUSE) {
+            Log.wtf(TAG, "Not expected for anything other than PAUSE & WAIT");
+            return;
+        }
+
         int selectionStart;
         int selectionEnd;
 
@@ -1558,20 +1527,19 @@ public class DialpadFragment extends Fragment
         selectionStart = Math.min(anchor, point);
         selectionEnd = Math.max(anchor, point);
 
+        if (selectionStart == -1) {
+            selectionStart = selectionEnd = mDigits.length();
+        }
+
         Editable digits = mDigits.getText();
-        if (selectionStart != -1) {
-            if (selectionStart == selectionEnd) {
-                // then there is no selection. So insert the pause at this
-                // position and update the mDigits.
-                digits.replace(selectionStart, selectionStart, newDigits);
-            } else {
-                digits.replace(selectionStart, selectionEnd, newDigits);
-                // Unselect: back to a regular cursor, just pass the character inserted.
-                mDigits.setSelection(selectionStart + 1);
+
+        if (canAddDigit(digits, selectionStart, selectionEnd, newDigit)) {
+            digits.replace(selectionStart, selectionEnd, Character.toString(newDigit));
+
+            if (selectionStart != selectionEnd) {
+              // Unselect: back to a regular cursor, just pass the character inserted.
+              mDigits.setSelection(selectionStart + 1);
             }
-        } else {
-            int len = mDigits.length();
-            digits.replace(len, len, newDigits);
         }
     }
 
@@ -1617,28 +1585,38 @@ public class DialpadFragment extends Fragment
     }
 
     /**
-     * This function return true if Wait menu item can be shown
-     * otherwise returns false. Assumes the passed string is non-empty
-     * and the 0th index check is not required.
+     * Returns true of the newDigit parameter can be added at the current selection
+     * point, otherwise returns false.
+     * Only prevents input of WAIT and PAUSE digits at an unsupported position.
+     * Fails early if start == -1 or start is larger than end.
      */
-    private static boolean showWait(int start, int end, String digits) {
-        if (start == end) {
-            // visible false in this case
-            if (start > digits.length()) return false;
+    @VisibleForTesting
+    /* package */ static boolean canAddDigit(CharSequence digits, int start, int end,
+                                             char newDigit) {
+        if(newDigit != WAIT && newDigit != PAUSE) {
+            Log.wtf(TAG, "Should not be called for anything other than PAUSE & WAIT");
+            return false;
+        }
 
-            // preceding char is ';', so visible should be false
-            if (digits.charAt(start - 1) == ';') return false;
+        // False if no selection, or selection is reversed (end < start)
+        if (start == -1 || end < start) {
+            return false;
+        }
 
-            // next char is ';', so visible should be false
-            if ((digits.length() > start) && (digits.charAt(start) == ';')) return false;
-        } else {
-            // visible false in this case
-            if (start > digits.length() || end > digits.length()) return false;
+        // unsupported selection-out-of-bounds state
+        if (start > digits.length() || end > digits.length()) return false;
 
-            // In this case we need to just check for ';' preceding to start
-            // or next to end
-            if (digits.charAt(start - 1) == ';') return false;
+        // Special digit cannot be the first digit
+        if (start == 0) return false;
+
+        if (newDigit == WAIT) {
+            // preceding char is ';' (WAIT)
+            if (digits.charAt(start - 1) == WAIT) return false;
+
+            // next char is ';' (WAIT)
+            if ((digits.length() > end) && (digits.charAt(end) == WAIT)) return false;
         }
+
         return true;
     }
 
diff --git a/tests/src/com/android/dialer/dialpad/DialpadFragmentTest.java b/tests/src/com/android/dialer/dialpad/DialpadFragmentTest.java
new file mode 100644
index 000000000..a123e745e
--- /dev/null
+++ b/tests/src/com/android/dialer/dialpad/DialpadFragmentTest.java
@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.dialer.dialpad;
+
+import android.test.suitebuilder.annotation.SmallTest;
+
+import junit.framework.TestCase;
+
+/** Unit tests for {@link DialpadFragment}. */
+@SmallTest
+public class DialpadFragmentTest extends TestCase {
+
+    public void testCanAddDigit_Valid() {
+        // end, middle, selection to end, middle selection
+        assertTrue(DialpadFragment.canAddDigit("123", 3, 3, ';'));
+        assertTrue(DialpadFragment.canAddDigit("123", 1, 1, ','));
+        assertTrue(DialpadFragment.canAddDigit("123", 1, 3, ';'));
+        assertTrue(DialpadFragment.canAddDigit("123", 1, 2, ','));
+    }
+
+    public void testCanAddDigit_InvalidCharacter() {
+        // only handles wait/pause
+        assertFalse(DialpadFragment.canAddDigit("123", 1, 1, '5'));
+    }
+
+    public void testCanAddDigit_BadOrNoSelection() {
+        // no selection
+        assertFalse(DialpadFragment.canAddDigit("123", -1, -1, ';'));
+        assertFalse(DialpadFragment.canAddDigit("123", -1, 1, ','));
+
+        // start > end
+        assertFalse(DialpadFragment.canAddDigit("123", 2, 1, ','));
+    }
+
+    public void testCanAddDigit_OutOfBounds() {
+        // start or end is > digits.length()
+        assertFalse(DialpadFragment.canAddDigit("123", 1, 4, ';'));
+        assertFalse(DialpadFragment.canAddDigit("123", 4, 4, ','));
+    }
+
+    public void testCanAddDigit_AsFirstCharacter() {
+        assertFalse(DialpadFragment.canAddDigit("", 0, 0, ','));
+        assertFalse(DialpadFragment.canAddDigit("123", 0, 0, ';'));
+        assertFalse(DialpadFragment.canAddDigit("123", 0, 2, ','));
+        assertFalse(DialpadFragment.canAddDigit("123", 0, 3, ','));
+    }
+
+    public void testCanAddDigit_AdjacentCharacters_Before() {
+        // before
+        assertFalse(DialpadFragment.canAddDigit("55;55", 2, 2, ';')); // WAIT
+        assertFalse(DialpadFragment.canAddDigit("55;55", 1, 2, ';'));
+        assertTrue(DialpadFragment.canAddDigit("55,55", 2, 2, ',')); // PAUSE
+        assertTrue(DialpadFragment.canAddDigit("55,55", 1, 2, ','));
+        assertTrue(DialpadFragment.canAddDigit("55;55", 2, 2, ',')); // WAIT & PAUSE
+        assertTrue(DialpadFragment.canAddDigit("55,55", 1, 2, ';'));
+    }
+
+    public void testCanAddDigit_AdjacentCharacters_After() {
+        // after
+        assertFalse(DialpadFragment.canAddDigit("55;55", 3, 3, ';')); // WAIT
+        assertFalse(DialpadFragment.canAddDigit("55;55", 3, 4, ';'));
+        assertTrue(DialpadFragment.canAddDigit("55,55", 3, 3, ',')); // PAUSE
+        assertTrue(DialpadFragment.canAddDigit("55,55", 3, 4, ','));
+        assertTrue(DialpadFragment.canAddDigit("55;55", 3, 3, ',')); // WAIT & PAUSE
+        assertTrue(DialpadFragment.canAddDigit("55,55", 3, 4, ';'));
+    }
+}