1 files changed, 43 insertions, 23 deletions

diff --git a/java/src/com/android/inputmethod/research/ResearchLogger.java b/java/src/com/android/inputmethod/research/ResearchLogger.java
index ec54616b7..aa4a866b8 100644
--- a/java/src/com/android/inputmethod/research/ResearchLogger.java
+++ b/java/src/com/android/inputmethod/research/ResearchLogger.java
@@ -83,6 +83,8 @@ import java.util.List;
 import java.util.Random;
 import java.util.regex.Pattern;
 
+// TODO: Add a unit test for every "logging" method (i.e. that is called from the IME and calls
+// enqueueEvent to record a LogStatement).
 /**
  * Logs the use of the LatinIME keyboard.
  *
@@ -852,23 +854,22 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang
         // The user has deleted this word and returned to the previous.  Check that the word in the
         // logUnit matches the expected word.  If so, restore the last log unit committed to be the
         // current logUnit.  I.e., pull out the last LogUnit from all the LogBuffers, and make
-        // restore it to mCurrentLogUnit so the new edits are captured with the word.  Optionally
-        // dump the contents of mCurrentLogUnit (useful if they contain deletions of the next word
-        // that should not be reported to protect user privacy)
+        // it the mCurrentLogUnit so the new edits are captured with the word.  Optionally dump the
+        // contents of mCurrentLogUnit (useful if they contain deletions of the next word that
+        // should not be reported to protect user privacy)
         //
         // Note that we don't use mLastLogUnit here, because it only goes one word back and is only
         // needed for reverts, which only happen one back.
         final LogUnit oldLogUnit = mMainLogBuffer.peekLastLogUnit();
 
-        // Check that expected word matches.
+        // Check that expected word matches.  It's ok if both strings are null, because this is the
+        // case where the LogUnit is storing a non-word, e.g. a separator.
         if (oldLogUnit != null) {
-            final String oldLogUnitWords = oldLogUnit.getWordsAsString();
             // Because the word is stored in the LogUnit with digits scrubbed, the comparison must
             // be made on a scrubbed version of the expectedWord as well.
-            if (oldLogUnitWords != null && !oldLogUnitWords.equals(
-                    scrubDigitsFromString(expectedWord))) {
-                return;
-            }
+            final String scrubbedExpectedWord = scrubDigitsFromString(expectedWord);
+            final String oldLogUnitWords = oldLogUnit.getWordsAsString();
+            if (!TextUtils.equals(scrubbedExpectedWord, oldLogUnitWords)) return;
         }
 
         // Uncommit, merging if necessary.
@@ -984,7 +985,8 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang
         return Character.isDigit(codePoint) ? DIGIT_REPLACEMENT_CODEPOINT : codePoint;
     }
 
-    /* package for test */ static String scrubDigitsFromString(String s) {
+    /* package for test */ static String scrubDigitsFromString(final String s) {
+        if (s == null) return null;
         StringBuilder sb = null;
         final int length = s.length();
         for (int i = 0; i < length; i = s.offsetByCodePoints(i, 1)) {
@@ -1463,21 +1465,39 @@ public class ResearchLogger implements SharedPreferences.OnSharedPreferenceChang
     public static void latinIME_revertCommit(final String committedWord,
             final String originallyTypedWord, final boolean isBatchMode,
             final String separatorString) {
+        // TODO: Prioritize adding a unit test for this method (as it is especially complex)
+        // TODO: Update the UserRecording LogBuffer as well as the MainLogBuffer
         final ResearchLogger researchLogger = getInstance();
-        // TODO: Verify that mCurrentLogUnit has been restored and contains the reverted word.
-        final LogUnit logUnit;
-        logUnit = researchLogger.mMainLogBuffer.peekLastLogUnit();
-        if (originallyTypedWord.length() > 0 && hasLetters(originallyTypedWord)) {
-            if (logUnit != null) {
-                logUnit.setWords(originallyTypedWord);
-            }
-        }
-        researchLogger.enqueueEvent(logUnit != null ? logUnit : researchLogger.mCurrentLogUnit,
-                LOGSTATEMENT_LATINIME_REVERTCOMMIT, committedWord, originallyTypedWord,
-                separatorString);
-        if (logUnit != null) {
-            logUnit.setContainsUserDeletions();
+        //
+        // 1. Remove separator LogUnit
+        final LogUnit lastLogUnit = researchLogger.mMainLogBuffer.peekLastLogUnit();
+        // Check that we're not at the beginning of input
+        if (lastLogUnit == null) return;
+        // Check that we're after a separator
+        if (lastLogUnit.getWordsAsString() != null) return;
+        // Remove separator
+        final LogUnit separatorLogUnit = researchLogger.mMainLogBuffer.unshiftIn();
+
+        // 2. Add revert LogStatement
+        final LogUnit revertedLogUnit = researchLogger.mMainLogBuffer.peekLastLogUnit();
+        if (revertedLogUnit == null) return;
+        if (!revertedLogUnit.getWordsAsString().equals(scrubDigitsFromString(committedWord))) {
+            // Any word associated with the reverted LogUnit has already had its digits scrubbed, so
+            // any digits in the committedWord argument must also be scrubbed for an accurate
+            // comparison.
+            return;
         }
+        researchLogger.enqueueEvent(revertedLogUnit, LOGSTATEMENT_LATINIME_REVERTCOMMIT,
+                committedWord, originallyTypedWord, separatorString);
+
+        // 3. Update the word associated with the LogUnit
+        revertedLogUnit.setWords(originallyTypedWord);
+        revertedLogUnit.setContainsUserDeletions();
+
+        // 4. Re-add the separator LogUnit
+        researchLogger.mMainLogBuffer.shiftIn(separatorLogUnit);
+
+        // 5. Record stats
         researchLogger.mStatistics.recordRevertCommit(SystemClock.uptimeMillis());
     }