[Android] Make enhanced bookmark editing UI more straightforward.

chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java

Index: chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
index f1244d606d45ed8819a49f0ecd475cf1fb099dc8..01e29d40c1f729b482a2fbf07bfbc8d5a38a4e1e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
@@ -6,6 +6,8 @@ package org.chromium.chrome.browser.enhancedbookmarks;
 
 import android.os.Bundle;
 import android.support.v7.widget.Toolbar;
+import android.text.Editable;
+import android.text.TextWatcher;
 import android.view.Menu;
 import android.view.MenuItem;
 import android.view.View;
@@ -64,6 +66,7 @@ public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
 
         @Override
         public void bookmarkModelChanged() {
+            updateViewContent();
         }
     };
 
@@ -79,8 +82,22 @@ public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
 
         setContentView(R.layout.eb_edit);
         mTitleEditText = (EmptyAlertEditText) findViewById(R.id.title_text);
-        mUrlEditText = (EmptyAlertEditText) findViewById(R.id.url_text);
         mFolderTextView = (TextView) findViewById(R.id.folder_text);
+        mUrlEditText = (EmptyAlertEditText) findViewById(R.id.url_text);
+
+        mTitleEditText.addTextChangedListener(new TextWatcher() {
+            @Override
+            public void onTextChanged(CharSequence s, int start, int before, int count) {
+                mTitleEditText.validate();

[Ian Wen, 2015/07/20 18:41:58]

Isn't this already done in EmptyAlertEdittext?

[Kibeom Kim (inactive), 2015/07/20 20:04:48]

Yeah it removes it, but doesn't add a warning. I was thinking putting this logic
in EmptyAlertEditText (maybe as an option). wdyt?

[Kibeom Kim (inactive), 2015/08/05 10:14:19]

Done.

+            }
+
+            @Override
+            public void beforeTextChanged(CharSequence s, int start, int count, int after) {}
+
+            @Override
+            public void afterTextChanged(Editable s) {}
+        });
+
         mFolderTextView.setOnClickListener(new View.OnClickListener() {
             @Override
             public void onClick(View v) {
@@ -88,6 +105,22 @@ public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
                         EnhancedBookmarkEditActivity.this, mBookmarkId);
             }
         });
+
+        mUrlEditText.addTextChangedListener(new TextWatcher() {
+            @Override
+            public void onTextChanged(CharSequence s, int start, int before, int count) {
+                mUrlEditText.validate();
+                // TODO(kkimlabs): Warn user if UrlUtilities.fixupUrl fails since we can't save
+                //                 unfixable URL anyways.
+            }
+
+            @Override
+            public void beforeTextChanged(CharSequence s, int start, int count, int after) {}
+
+            @Override
+            public void afterTextChanged(Editable s) {}
+        });
+
         Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
         setSupportActionBar(toolbar);
         getSupportActionBar().setDisplayHomeAsUpEnabled(true);
@@ -97,10 +130,16 @@ public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
 
     private void updateViewContent() {
         BookmarkItem bookmarkItem = mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
-        mTitleEditText.setText(bookmarkItem.getTitle());
-        mUrlEditText.setText(bookmarkItem.getUrl());
-        mFolderTextView.setText(
-                mEnhancedBookmarksModel.getBookmarkTitle(bookmarkItem.getParentId()));
+        if (!bookmarkItem.getTitle().equals(mTitleEditText.getTrimmedText())) {

[Ian Wen, 2015/07/20 18:41:58]

updateViewContent() is only called twice: once by onCreate, once by
onBookmarkNodeChange(). If it is not a frequent call, I don't see the value of
adding additional if checks here.

[Kibeom Kim (inactive), 2015/07/20 20:04:48]

Text cursor position is changed as a side-effect of setting text, it was to
avoid that.

+            mTitleEditText.setText(bookmarkItem.getTitle());
+        }
+        String folderTitle = mEnhancedBookmarksModel.getBookmarkTitle(bookmarkItem.getParentId());
+        if (!folderTitle.equals(mFolderTextView.getText().toString())) {
+            mFolderTextView.setText(folderTitle);
+        }
+        if (!bookmarkItem.getUrl().equals(UrlUtilities.fixupUrl(mUrlEditText.getTrimmedText()))) {
+            mUrlEditText.setText(bookmarkItem.getUrl());

[Ian Wen, 2015/07/20 18:41:58]

If textview's text is not correct, while it's fix'd-up version is correct and is
the same as bookmarkItem.getUrl(). Then it will not be updated?

[Kibeom Kim (inactive), 2015/07/20 20:04:48]

Yeah I agree that fixupUrl doesn't fit here. Done.

+        }
     }
 
     @Override
@@ -120,29 +159,23 @@ public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
             finish();
             return true;
         } else if (item.getItemId() == android.R.id.home) {
-            onBackPressed();
+            finish();
             return true;
         }
         return super.onOptionsItemSelected(item);
     }
 
     @Override
-    public void onBackPressed() {
-        String newTitle = mTitleEditText.getTrimmedText();
-        String newUrl = mUrlEditText.getTrimmedText();
-        newUrl = UrlUtilities.fixupUrl(newUrl);
-        if (newUrl == null) newUrl = "";
-        mUrlEditText.setText(newUrl);
-
-        if (!mTitleEditText.validate() || !mUrlEditText.validate()) return;
-
-        mEnhancedBookmarksModel.setBookmarkTitle(mBookmarkId, newTitle);
-        mEnhancedBookmarksModel.setBookmarkUrl(mBookmarkId, newUrl);
-        super.onBackPressed();
-    }
-
-    @Override
     protected void onDestroy() {

[Ian Wen, 2015/07/20 18:41:58]

Don't save data in onDestroy(). I would prefer using onPause() instead.

Per android doc:
"Note: do not count on this method being called as a place for saving data! For
example, if an activity is editing data in a content provider, those edits
should be committed in either onPause() or onSaveInstanceState(Bundle), not
here. This method is usually implemented to free resources like threads that are
associated with an activity, so that a destroyed activity does not leave such
things around while the rest of its application is still running. There are
situations where the system will simply kill the activity's hosting process
without calling this method (or any others) in it, so it should not be used to
do things that are intended to remain around after the process goes away."

[Kibeom Kim (inactive), 2015/07/20 20:04:48]

Done.

+        if (mTitleEditText.isValid()) {
+            mEnhancedBookmarksModel.setBookmarkTitle(mBookmarkId, mTitleEditText.getTrimmedText());
+        }
+
+        if (mUrlEditText.isValid()) {
+            String fixedUrl = UrlUtilities.fixupUrl(mUrlEditText.getTrimmedText());
+            if (fixedUrl != null) mEnhancedBookmarksModel.setBookmarkUrl(mBookmarkId, fixedUrl);
+        }
+
         mEnhancedBookmarksModel.removeModelObserver(mBookmarkModelObserver);
         mEnhancedBookmarksModel.destroy();
         mEnhancedBookmarksModel = null;