Re-engineer edit screen in enhanced bookmark

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

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.enhancedbookmarks;
+
+import android.os.Bundle;
+import android.support.v7.widget.Toolbar;
+import android.view.Menu;
+import android.view.MenuItem;
+import android.view.View;
+import android.widget.EditText;
+import android.widget.TextView;
+
+import com.google.android.apps.chrome.R;
+
+import org.chromium.chrome.browser.BookmarksBridge.BookmarkItem;
+import org.chromium.chrome.browser.BookmarksBridge.BookmarkModelObserver;
+import org.chromium.chrome.browser.UrlUtilities;
+import org.chromium.chrome.browser.enhanced_bookmarks.EnhancedBookmarksModel;
+import org.chromium.components.bookmarks.BookmarkId;
+
+/**
+ * The activity that enables the user to modify the title, url and parent folder of a bookmark.
+ */
+public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
+    /** The name of the extra in the intent to pass the bookmark id. */

[newt (away), 2015/06/17 23:53:25]

"The intent extra specifying the ID of the bookmark to be edited."

[Ian Wen, 2015/06/18 01:19:34]

Done.

+    public static final String INTENT_BOOKMARK_ID = "EnhancedBookmarkEditActivity.BookmarkId";
+
+    private EnhancedBookmarksModel mEnhancedBookmarksModel;
+    private BookmarkId mBookmarkId;
+    private EditText mTitleEditText;
+    private EditText mUrlEditText;
+    private TextView mFolderTextView;
+
+    private MenuItem mDeleteButton;
+
+    private BookmarkModelObserver mBookmarkModelObserver = new BookmarkModelObserver() {
+        @Override
+        public void bookmarkNodeRemoved(BookmarkItem parent, int oldIndex, BookmarkItem node,
+                boolean isDoingExtensiveChanges) {
+            if (mBookmarkId.equals(node.getId())) {
+                finish();
+            }
+        }
+
+        @Override
+        public void bookmarkNodeMoved(BookmarkItem oldParent, int oldIndex, BookmarkItem newParent,
+                int newIndex) {
+            BookmarkId movedBookmark = mEnhancedBookmarksModel.getChildAt(newParent.getId(),
+                    newIndex);
+            if (movedBookmark.equals(mBookmarkId)) {
+                mFolderTextView.setText(newParent.getTitle());
+            }
+        }
+
+        @Override
+        public void bookmarkNodeChanged(BookmarkItem node) {
+            if (mBookmarkId.equals(node.getId()) || node.getId().equals(
+                    mEnhancedBookmarksModel.getBookmarkById(mBookmarkId).getParentId())) {
+                updateViewContent();
+            }
+        }
+
+        @Override
+        public void bookmarkModelChanged() {
+        }
+    };
+
+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+        EnhancedBookmarkUtils.setTaskDescriptionInDocumentMode(this,
+                getString(R.string.edit_bookmark));
+        mEnhancedBookmarksModel = new EnhancedBookmarksModel();
+        mBookmarkId = BookmarkId.getBookmarkIdFromString(
+                getIntent().getStringExtra(INTENT_BOOKMARK_ID));
+        mEnhancedBookmarksModel.addModelObserver(mBookmarkModelObserver);
+
+        setContentView(R.layout.eb_edit);
+        mTitleEditText = (EditText) findViewById(R.id.title_text);
+        mUrlEditText = (EditText) findViewById(R.id.url_text);
+        mFolderTextView = (TextView) findViewById(R.id.folder_text);
+        mFolderTextView.setOnClickListener(new View.OnClickListener() {
+            @Override
+            public void onClick(View v) {
+                EnhancedBookmarkFolderSelectActivity.startFolderSelectActivity(
+                        EnhancedBookmarkEditActivity.this, mBookmarkId);
+            }
+        });
+        Toolbar toolbar = (Toolbar) findViewById(R.id.toolbar);
+        setSupportActionBar(toolbar);
+        getSupportActionBar().setDisplayHomeAsUpEnabled(true);
+
+        updateViewContent();
+    }
+
+    private void updateViewContent() {
+        BookmarkItem bookmarkItem = mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
+        mTitleEditText.setText(bookmarkItem.getTitle());
+        mUrlEditText.setText(bookmarkItem.getUrl());
+        mFolderTextView.setText(
+                mEnhancedBookmarksModel.getBookmarkTitle(bookmarkItem.getParentId()));
+    }
+
+    @Override
+    public boolean onCreateOptionsMenu(Menu menu) {
+        mDeleteButton = menu.add(0, Menu.NONE, 0,

[newt (away), 2015/06/17 23:53:25]

menu.add(R.string.enhanced_bookmark_action_bar_delete)

[Ian Wen, 2015/06/18 01:19:34]

Done.

+                getResources().getString(R.string.enhanced_bookmark_action_bar_delete)).setIcon(
+                R.drawable.btn_trash);
+        mDeleteButton.setShowAsAction(MenuItem.SHOW_AS_ACTION_ALWAYS);
+        return super.onCreateOptionsMenu(menu);
+    }
+
+    @Override
+    public boolean onOptionsItemSelected(MenuItem item) {
+        if (item == mDeleteButton) {
+            mEnhancedBookmarksModel.deleteBookmarks(mBookmarkId);
+            finish();
+            return true;
+        } else if (item.getItemId() == android.R.id.home) {
+            onBackPressed();
+            return true;
+        }
+        return super.onOptionsItemSelected(item);
+    }
+
+    @Override
+    public void onBackPressed() {
+        String newTitle = mTitleEditText.getText().toString().trim();
+        String newUrl = mUrlEditText.getText().toString().trim();
+        newUrl = UrlUtilities.fixupUrl(newUrl);
+        if (newUrl == null) newUrl = "";
+        mEnhancedBookmarksModel.setBookmarkTitle(mBookmarkId, newTitle);
+        mEnhancedBookmarksModel.setBookmarkUrl(mBookmarkId, newUrl);
+
+        // TODO(ianewn): use EmptyAlertEditText.validate() when EmptyAlertEditText is landed.
+        if (newTitle.isEmpty() || newUrl.isEmpty()) return;
+        super.onBackPressed();
+    }

[Kibeom Kim (inactive), 2015/06/18 00:25:37]

Q1: How are you planning to structure invalid url warning?

Q2: Not letting to escape when a field is empty seems too invasive for user
action. ianwen@, newt@: any thoughts?

[newt (away), 2015/06/18 00:31:26]

If we had a cancel or "x" button, the user could press it to escape this dialog,
even when the current input URL or title is invalid. Without adding such a
button, I don't have any ideas. Maybe ask the designer?

[Ian Wen, 2015/06/18 01:19:34]

1. https://codereview.chromium.org/1187013002/.

2. We might discuss with Chris (or maybe in the next UI review). My personal
feeling is that simplicity is the top priority for me and having one return
button looks best. For users who insist on pressing back button with an empty
field, he would have noticed the red warning on the edittext.

[Kibeom Kim (inactive), 2015/06/18 02:28:13]

(I thought we had a proper URL validation but It seems it wasn't properly
implemented before :/ )

1. If title or URL is invalid, then we shouldn't save at #134-135. We didn't
save in the previous code by "if (urlOrTitleInvalid) return false;"
2. We should distinguish empty and invalid URL error and show different error
messages. In the old code, we forcefully cleared the URL EditText and in the new
code, we kept showing the invalid URL while the error message is "URL required".
Both are not ideal.

The second one is not a regression so please feel free to file a bug instead.
(but if so, at least keep the old behavior of clearing URL EditText)

[Ian Wen, 2015/06/18 17:27:13]

Done.

+
+    @Override
+    protected void onDestroy() {
+        mEnhancedBookmarksModel.removeModelObserver(mBookmarkModelObserver);
+        mEnhancedBookmarksModel.destroy();
+        mEnhancedBookmarksModel = null;
+        super.onDestroy();
+    }
+}