Re-engineer edit screen in enhanced bookmark

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

Index: chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
diff --git a/chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java b/chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
new file mode 100644
index 0000000000000000000000000000000000000000..ba16c4923091eede0c54fa40cae6719ad39a2859
--- /dev/null
+++ b/chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
@@ -0,0 +1,133 @@
+// 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.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.enhanced_bookmarks.EnhancedBookmarksModel;
+import org.chromium.components.bookmarks.BookmarkId;
+
+/**
+ * The activity that enables the user to modify the title, url, parent folder of a bookmark.

[newt (away), 2015/06/17 04:42:22]

"and parent folder"

[Ian Wen, 2015/06/17 21:07:42]

Done.

+ */
+public class EnhancedBookmarkEditActivity extends EnhancedBookmarkActivityBase {
+    public static final String INTENT_BOOKMARK_ID = "EnhancedBookmarkEditActivity.BookmarkId";

[newt (away), 2015/06/17 04:42:23]

document public constants

[Ian Wen, 2015/06/17 21:07:42]

Done.

+
+    private EnhancedBookmarksModel mEnhancedBookmarksModel;
+    private BookmarkId mBookmarkId;
+    private EditText mTitleEditText;
+    private EditText mUrlEditText;
+    private TextView mFolderTextView;
+
+    private MenuItem mDeleteBtn;

[newt (away), 2015/06/17 04:42:23]

s/Btn/Button

Avoid abbrvtns

[Ian Wen, 2015/06/17 21:07:42]

Done.

+
+    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())) updateViewContent();

[Kibeom Kim (inactive), 2015/06/17 06:10:30]

Q: what happens if parent node's name is changed?

[Ian Wen, 2015/06/17 21:07:42]

Then it's an old bug in detail page. I can fix it now.

+        }
+
+        @Override
+        public void bookmarkModelChanged() {
+        }
+    };
+
+    @Override
+    protected void onCreate(Bundle savedInstanceState) {
+        super.onCreate(savedInstanceState);
+        getSupportActionBar().setDisplayHomeAsUpEnabled(true);
+        EnhancedBookmarkUtils.setTaskDescriptionInDocumentMode(this,
+                getString(R.string.accessibility_enhanced_bookmark_edit_title));
+        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);
+        updateViewContent();
+    }
+
+    private void updateViewContent() {
+        BookmarkItem bookmarkItem = mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);

[newt (away), 2015/06/17 04:42:23]

might this ever return null?

[Kibeom Kim (inactive), 2015/06/17 06:10:30]

I don't know if it's possible for a bookmark to be deleted between
EnhancedBookmarkEditActivity launching and here. Overly defensive null checking
seems considered harmful
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/KJdw-QiKdxY
. How about landing as it is and monitoring crash reports?

Or, I'm not sure, but maybe this can happen when 1. user opens this activity 2.
killed in background due to memory pressure. 3. user clears app data 4. Come
back to this activity.

[Ian Wen, 2015/06/17 21:07:42]

If the bookmark is deleted, the activity will be notified and it will call
finish(). I have seen several crashes where getbookmarkbyid() will cause
assertion failure because the bookmark does not exist anymore. However I don't
see any reason why the method would give me null ptr...

[Ian Wen, 2015/06/17 21:07:42]

The framework will kill the app entirely if the user clicks "clear app data".

[Kibeom Kim (inactive), 2015/06/17 21:18:25]

Since updateViewCount() is called in onCreate(), this can be null if bookmark is
deleted between activity launching and onCreate() here. Though, I don't know if
that can happen, or if there is any guarantee.

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

Discussed offline.

Ian pointed out that EnhancedBookmarksModel#getBookmarkById doesn't return null,
but it crashes on passing an invalid ID. I don't have confidence that assuming
mBookmarkId valid is safe for the flow mentioned above, but anyways we have the
same problem across our codebase overall, and we're indeed getting crashes.
https://code.google.com/p/chromium/issues/detail?id=433431 I think we should
address this issue separately so let's leave as it is. I'm considering making
EnhancedBookmarksModel.getBookmarkById() actually return null so that we know
what places are causing problems.

+        mTitleEditText.setText(bookmarkItem.getTitle());
+        mUrlEditText.setText(bookmarkItem.getUrl());
+        mFolderTextView.setText(
+                mEnhancedBookmarksModel.getBookmarkTitle(bookmarkItem.getParentId()));
+        mFolderTextView.setOnClickListener(new View.OnClickListener() {

[Kibeom Kim (inactive), 2015/06/17 06:10:30]

I guess we don't need to set listener on every update? Maybe in onCreate?

[Ian Wen, 2015/06/17 21:07:42]

Done.

+                @Override

[newt (away), 2015/06/17 04:42:23]

fix indentation

[Ian Wen, 2015/06/17 21:07:42]

Done.

+            public void onClick(View v) {
+                EnhancedBookmarkFolderSelectActivity.startFolderSelectActivity(
+                        EnhancedBookmarkEditActivity.this, mBookmarkId);
+            }
+        });
+    }
+
+    @Override
+    public boolean onCreateOptionsMenu(Menu menu) {
+        mDeleteBtn = menu.add(0, 0, 0, "delete").setIcon(R.drawable.btn_trash);

[newt (away), 2015/06/17 04:42:22]

For the sake of accessibility users, use a localized string:

    menu.add(R.string.delete)...

[Kibeom Kim (inactive), 2015/06/17 06:10:30]

Seems like Menu.None is recommended instead of 0.
http://developer.android.com/reference/android/view/Menu.html#add(int, int, int,
java.lang.CharSequence)

Optional: This is all static, so I think it's a good place to use menu xml. But
since this is only one I'll leave up to you.

[newt (away), 2015/06/17 17:31:17]

My personal preference is to avoid menu xml with a single menu item. IMO, the
xml reduces clarity due to the extra indirection. Either way is fine though.

[Ian Wen, 2015/06/17 21:07:42]

Oops this is a mistake.

+        mDeleteBtn.setShowAsAction(MenuItem.SHOW_AS_ACTION_ALWAYS);
+        return super.onCreateOptionsMenu(menu);
+    }
+
+    @Override
+    public boolean onOptionsItemSelected(MenuItem item) {
+        if (item.equals(mDeleteBtn)) {

[newt (away), 2015/06/17 04:42:22]

use == instead of equals(), since we only care about identity, not equality.

[Ian Wen, 2015/06/17 21:07:42]

Done.

+            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();
+        mEnhancedBookmarksModel.setBookmarkTitle(mBookmarkId, newTitle);

[newt (away), 2015/06/17 04:42:23]

why don't you validate newTitle and newUrl?

[Ian Wen, 2015/06/17 21:07:42]

:P Done. However the error message will be handled by EmptyALertEditText.

+        mEnhancedBookmarksModel.setBookmarkUrl(mBookmarkId, newUrl);
+        super.onBackPressed();
+    }
+
+    @Override
+    protected void onDestroy() {
+        mEnhancedBookmarksModel.removeModelObserver(mBookmarkModelObserver);
+        mEnhancedBookmarksModel.destroy();
+        mEnhancedBookmarksModel = null;
+        super.onDestroy();
+    }
+}