Make saving a contact less confusing

* Add always visible save menu item (a checkmark icon) on both
  the compact and full editors.  Selecting it saves changes
  and closes the editor (what back does currently).
* On the compact editor, make back perform a discard,
  including warning the user if there are unsaved changes.
* On the full editor, back still saves and goes back to
  the full editor. This is not ideal but it's out of
  scope to not save changes between the two editors
  when editing an existing contact.

Bug 19983789

Change-Id: Ie23b75978db70f2b438a824e39f0ae8fb8aa99ec
diff --git a/res/menu/edit_contact.xml b/res/menu/edit_contact.xml
index fe6d2b9..2ff5f81 100644
--- a/res/menu/edit_contact.xml
+++ b/res/menu/edit_contact.xml
@@ -16,9 +16,10 @@
 
 <menu xmlns:android="http://schemas.android.com/apk/res/android">
     <item
-        android:id="@+id/menu_done"
-        android:alphabeticShortcut="\n"
-        android:showAsAction="always" />
+        android:id="@+id/menu_save"
+        android:showAsAction="always"
+        android:icon="@drawable/ic_done_wht_24dp"
+        android:title="@string/menu_save" />
 
     <item
         android:id="@+id/menu_split"
diff --git a/res/values/strings.xml b/res/values/strings.xml
index 7dbe6c1..9e76bd3 100644
--- a/res/values/strings.xml
+++ b/res/values/strings.xml
@@ -150,6 +150,9 @@
     <!-- Menu item that joins an aggregate with another aggregate -->
     <string name="menu_joinAggregate">Merge</string>
 
+    <!-- Menu item (in the action bar) to indicate that changes should be saved [CHAR LIMIT=20] -->
+    <string name="menu_save">Save</string>
+
     <!-- Heading of the Join Contact screen -->
     <string name="titleJoinContactDataWith">Join contacts</string>
 
diff --git a/src/com/android/contacts/activities/CompactContactEditorActivity.java b/src/com/android/contacts/activities/CompactContactEditorActivity.java
index c45c261..082bb74 100644
--- a/src/com/android/contacts/activities/CompactContactEditorActivity.java
+++ b/src/com/android/contacts/activities/CompactContactEditorActivity.java
@@ -60,7 +60,7 @@
     @Override
     public void onBackPressed() {
         if (mFragment != null) {
-            mFragment.save(ContactEditor.SaveMode.CLOSE, /* backPressed =*/ true);
+            mFragment.revert();
         }
     }
 }
diff --git a/src/com/android/contacts/activities/ContactEditorBaseActivity.java b/src/com/android/contacts/activities/ContactEditorBaseActivity.java
index b9edabc..12f1e96 100644
--- a/src/com/android/contacts/activities/ContactEditorBaseActivity.java
+++ b/src/com/android/contacts/activities/ContactEditorBaseActivity.java
@@ -162,6 +162,12 @@
         boolean save(int saveMode, boolean backPressed);
 
         /**
+         * If there are no unsaved changes, just close the editor, otherwise the user is prompted
+         * before discarding unsaved changes.
+         */
+        boolean revert();
+
+        /**
          * Invoked after the contact is saved.
          */
         void onSaveCompleted(boolean hadChanges, int saveMode, boolean saveSucceeded,
diff --git a/src/com/android/contacts/editor/CompactContactEditorFragment.java b/src/com/android/contacts/editor/CompactContactEditorFragment.java
index 23d5bb3..12b3b62 100644
--- a/src/com/android/contacts/editor/CompactContactEditorFragment.java
+++ b/src/com/android/contacts/editor/CompactContactEditorFragment.java
@@ -36,6 +36,7 @@
 import android.os.Bundle;
 import android.util.Log;
 import android.view.LayoutInflater;
+import android.view.MenuItem;
 import android.view.View;
 import android.view.ViewGroup;
 import android.widget.LinearLayout;
@@ -191,6 +192,14 @@
     }
 
     @Override
+    public boolean onOptionsItemSelected(MenuItem item) {
+        if (item.getItemId() == android.R.id.home) {
+            return revert();
+        }
+        return super.onOptionsItemSelected(item);
+    }
+
+    @Override
     protected void bindEditors() {
         if (!isReadyToBindEditors()) {
             return;
@@ -347,7 +356,7 @@
         if (isInsert) {
             // For inserts, prevent any changes from being saved when the base fragment is destroyed
             mStatus = Status.CLOSING;
-        } else if (hasPendingChanges()) {
+        } else if (hasPendingRawContactChanges()) {
             // Save whatever is in the form
             save(SaveMode.CLOSE, /* backPressed =*/ false);
         }
diff --git a/src/com/android/contacts/editor/ContactEditorBaseFragment.java b/src/com/android/contacts/editor/ContactEditorBaseFragment.java
index e262cee..434ed56 100644
--- a/src/com/android/contacts/editor/ContactEditorBaseFragment.java
+++ b/src/com/android/contacts/editor/ContactEditorBaseFragment.java
@@ -743,7 +743,7 @@
         // This supports the keyboard shortcut to save changes to a contact but shouldn't be visible
         // because the custom action bar contains the "save" button now (not the overflow menu).
         // TODO: Find a better way to handle shortcuts, i.e. onKeyDown()?
-        final MenuItem doneMenu = menu.findItem(R.id.menu_done);
+        final MenuItem saveMenu = menu.findItem(R.id.menu_save);
         final MenuItem splitMenu = menu.findItem(R.id.menu_split);
         final MenuItem joinMenu = menu.findItem(R.id.menu_join);
         final MenuItem helpMenu = menu.findItem(R.id.menu_help);
@@ -753,21 +753,18 @@
         final MenuItem deleteMenu = menu.findItem(R.id.menu_delete);
 
         // Set visibility of menus
-        doneMenu.setVisible(false);
-
         // Discard menu is only available if at least one raw contact is editable
         discardMenu.setVisible(mState != null &&
                 mState.getFirstWritableRawContact(mContext) != null);
 
         // help menu depending on whether this is inserting or editing
-        if (Intent.ACTION_INSERT.equals(mAction) ||
-                ContactEditorBaseActivity.ACTION_INSERT.equals(mAction)) {
+        if (isInsert(mAction)) {
             HelpUtils.prepareHelpMenuItem(mContext, helpMenu, R.string.help_url_people_add);
+            discardMenu.setVisible(false);
             splitMenu.setVisible(false);
             joinMenu.setVisible(false);
             deleteMenu.setVisible(false);
-        } else if (Intent.ACTION_EDIT.equals(mAction) ||
-                ContactEditorBaseActivity.ACTION_EDIT.equals(mAction)) {
+        } else if (isEdit(mAction)) {
             HelpUtils.prepareHelpMenuItem(mContext, helpMenu, R.string.help_url_people_edit);
             // Split only if there is more than one raw (non-user profile) contact and doing so
             // won't result in an empty contact
@@ -795,8 +792,7 @@
     @Override
     public boolean onOptionsItemSelected(MenuItem item) {
         switch (item.getItemId()) {
-            case android.R.id.home:
-            case R.id.menu_done:
+            case R.id.menu_save:
                 return save(SaveMode.CLOSE, /* backPressed =*/ true);
             case R.id.menu_discard:
                 return revert();
@@ -823,7 +819,8 @@
         return false;
     }
 
-    private boolean revert() {
+    @Override
+    public boolean revert() {
         if (mState.isEmpty() || !hasPendingChanges()) {
             onCancelEditConfirmed();
         } else {
@@ -870,7 +867,8 @@
 
         // If we just started creating a new contact and haven't added any data, it's too
         // early to do a join
-        if (mState.size() == 1 && mState.get(0).isContactInsert() && !hasPendingChanges()) {
+        if (mState.size() == 1 && mState.get(0).isContactInsert()
+                && !hasPendingRawContactChanges()) {
             Toast.makeText(mContext, R.string.toast_join_with_empty_contact,
                     Toast.LENGTH_LONG).show();
             return true;
@@ -923,33 +921,7 @@
 
         // Determine if changes were made in the editor that need to be saved
         // See go/editing-read-only-contacts
-        boolean hasPendingChanges;
-        if (mReadOnlyNameEditorView == null || mReadOnlyDisplayName == null) {
-            hasPendingChanges = hasPendingChanges();
-        } else {
-            // We created a new raw contact delta with a default display name. We must test for
-            // pending changes while ignoring the default display name.
-            final String displayName = mReadOnlyNameEditorView.getDisplayName();
-            if (mReadOnlyDisplayName.equals(displayName)) {
-                // The user did not modify the default display name, erase it and
-                // check if the user made any other changes
-                mReadOnlyNameEditorView.setDisplayName(null);
-                if (hasPendingChanges()) {
-                    // Other changes were made to the aggregate contact, restore
-                    // the display name and proceed.
-                    mReadOnlyNameEditorView.setDisplayName(displayName);
-                    hasPendingChanges = true;
-                } else {
-                    // No other changes were made to the aggregate contact. Don't add back
-                    // the displayName so that a "bogus" contact is not created.
-                    hasPendingChanges = false;
-                }
-            } else {
-                hasPendingChanges = true;
-            }
-        }
-
-        if (!hasPendingChanges) {
+        if (!hasPendingChanges()) {
             if (mLookupUri == null && saveMode == SaveMode.RELOAD) {
                 // We don't have anything to save and there isn't even an existing contact yet.
                 // Nothing to do, simply go back to editing mode
@@ -1006,12 +978,42 @@
      * Return true if there are any edits to the current contact which need to
      * be saved.
      */
-    protected boolean hasPendingChanges() {
+    protected boolean hasPendingRawContactChanges() {
         final AccountTypeManager accountTypes = AccountTypeManager.getInstance(mContext);
         return RawContactModifier.hasChanges(mState, accountTypes);
     }
 
     /**
+     * Determines if changes were made in the editor that need to be saved, while taking into
+     * account that name changes are not realfor read-only contacts.
+     * See go/editing-read-only-contacts
+     */
+    protected boolean hasPendingChanges() {
+        if (mReadOnlyNameEditorView == null || mReadOnlyDisplayName == null) {
+            return hasPendingRawContactChanges();
+        }
+        // We created a new raw contact delta with a default display name. We must test for
+        // pending changes while ignoring the default display name.
+        final String displayName = mReadOnlyNameEditorView.getDisplayName();
+        if (mReadOnlyDisplayName.equals(displayName)) {
+            // The user did not modify the default display name, erase it and
+            // check if the user made any other changes
+            mReadOnlyNameEditorView.setDisplayName(null);
+            if (hasPendingRawContactChanges()) {
+                // Other changes were made to the aggregate contact, restore
+                // the display name and proceed.
+                mReadOnlyNameEditorView.setDisplayName(displayName);
+                return true;
+            } else {
+                // No other changes were made to the aggregate contact. Don't add back
+                // the displayName so that a "bogus" contact is not created.
+                return false;
+            }
+        }
+        return true;
+    }
+
+    /**
      * Whether editor inputs and the options menu should be enabled.
      */
     protected boolean isEnabled() {
@@ -1679,4 +1681,9 @@
         return Intent.ACTION_INSERT.equals(action)
                 || ContactEditorBaseActivity.ACTION_INSERT.equals(action);
     }
+
+    protected static boolean isEdit(String action) {
+        return Intent.ACTION_EDIT.equals(action)
+                || ContactEditorBaseActivity.ACTION_EDIT.equals(action);
+    }
 }
diff --git a/src/com/android/contacts/editor/ContactEditorFragment.java b/src/com/android/contacts/editor/ContactEditorFragment.java
index 7b17d63..1e06306 100644
--- a/src/com/android/contacts/editor/ContactEditorFragment.java
+++ b/src/com/android/contacts/editor/ContactEditorFragment.java
@@ -131,8 +131,7 @@
 
     @Override
     public boolean onOptionsItemSelected(MenuItem item) {
-        // Override the home/done options to return to the compact editor
-        if (item.getItemId() == android.R.id.home || item.getItemId() == R.id.menu_done) {
+        if (item.getItemId() == android.R.id.home) {
             return save(SaveMode.COMPACT, /* backPressed =*/ true);
         }
         return super.onOptionsItemSelected(item);