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

7 files changed, 71 insertions, 46 deletions

diff --git a/res/menu/edit_contact.xml b/res/menu/edit_contact.xml
index fe6d2b9c4..2ff5f8149 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 7dbe6c10b..9e76bd383 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 c45c261b3..082bb74f3 100644
--- a/src/com/android/contacts/activities/CompactContactEditorActivity.java
+++ b/src/com/android/contacts/activities/CompactContactEditorActivity.java
@@ -60,7 +60,7 @@ public class CompactContactEditorActivity extends ContactEditorBaseActivity {
     @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 b9edabcda..12f1e961b 100644
--- a/src/com/android/contacts/activities/ContactEditorBaseActivity.java
+++ b/src/com/android/contacts/activities/ContactEditorBaseActivity.java
@@ -162,6 +162,12 @@ abstract public class ContactEditorBaseActivity extends ContactsActivity
         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 23d5bb349..12b3b623e 100644
--- a/src/com/android/contacts/editor/CompactContactEditorFragment.java
+++ b/src/com/android/contacts/editor/CompactContactEditorFragment.java
@@ -36,6 +36,7 @@ import android.net.Uri;
 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 @@ public class CompactContactEditorFragment extends ContactEditorBaseFragment impl
     }
 
     @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 @@ public class CompactContactEditorFragment extends ContactEditorBaseFragment impl
         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 e262cee20..434ed56b1 100644
--- a/src/com/android/contacts/editor/ContactEditorBaseFragment.java
+++ b/src/com/android/contacts/editor/ContactEditorBaseFragment.java
@@ -743,7 +743,7 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
         // 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 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
         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 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
     @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 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
         return false;
     }
 
-    private boolean revert() {
+    @Override
+    public boolean revert() {
         if (mState.isEmpty() || !hasPendingChanges()) {
             onCancelEditConfirmed();
         } else {
@@ -870,7 +867,8 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
 
         // 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 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
 
         // 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 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
      * 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 @@ abstract public class ContactEditorBaseFragment extends Fragment implements
         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 7b17d638d..1e06306e4 100644
--- a/src/com/android/contacts/editor/ContactEditorFragment.java
+++ b/src/com/android/contacts/editor/ContactEditorFragment.java
@@ -131,8 +131,7 @@ public class ContactEditorFragment extends ContactEditorBaseFragment implements
 
     @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);