[MD Bookmarks] Add Select for Bookmarks.

chrome/browser/resources/md_bookmarks/store.js

Index: chrome/browser/resources/md_bookmarks/store.js
diff --git a/chrome/browser/resources/md_bookmarks/store.js b/chrome/browser/resources/md_bookmarks/store.js
index baddea20b2e1979feb20095a57cae3ba320e3edd..c2447e2c9dfcd189375e35fb5e564be3df8adfef 100644
--- a/chrome/browser/resources/md_bookmarks/store.js
+++ b/chrome/browser/resources/md_bookmarks/store.js
@@ -37,6 +37,10 @@ var BookmarksStore = Polymer({
     },
 
     idToNodeMap_: Object,
+
+    anchorIndex_: Number,
+
+    idToSearchResultMap_: Object,

[tsergeant, 2017/01/25 06:02:13]

I recently discovered how to closure annotate something like this:

/** @type{Object<string, !BookmarkTreeNode>} */

[jiaxi, 2017/01/30 03:28:58]

Done.

   },
 
   /** @private {Object} */
@@ -45,9 +49,10 @@ var BookmarksStore = Polymer({
   /** @override */
   attached: function() {
     this.documentListeners_ = {
-      'selected-folder-changed': this.onSelectedFolderChanged_.bind(this),
       'folder-open-changed': this.onFolderOpenChanged_.bind(this),
       'search-term-changed': this.onSearchTermChanged_.bind(this),
+      'select-item': this.onItemSelected_.bind(this),
+      'selected-folder-changed': this.onSelectedFolderChanged_.bind(this),
     };
     for (var event in this.documentListeners_)
       document.addEventListener(event, this.documentListeners_[event]);
@@ -91,7 +96,8 @@ var BookmarksStore = Polymer({
   /** @private */
   deselectFolders_: function() {
     this.unlinkPaths('displayedList');
-    this.set(this.idToNodeMap_[this.selectedId].path + '.isSelected', false);
+    this.set(
+        this.idToNodeMap_[this.selectedId].path + '.isSelectedFolder', false);
     this.selectedId = null;
   },
 
@@ -114,10 +120,14 @@ var BookmarksStore = Polymer({
       this.fire('selected-folder-changed', this.rootNode.children[0].id);
     } else {
       chrome.bookmarks.search(this.searchTerm, function(results) {
+        this.clearSelectedItems_();
+        this.anchorIndex_ = undefined;
+        this.idToSearchResultMap_ = {};
+
         if (this.selectedId)
           this.deselectFolders_();
 
-        this._setDisplayedList(results);
+        this.setupSearchResults_(results);
       }.bind(this));
     }
   },
@@ -128,6 +138,9 @@ var BookmarksStore = Polymer({
     if (!this.selectedId)
       return;
 
+    this.clearSelectedItems_();
+    this.anchorIndex_ = undefined;
+
     var selectedNode = this.idToNodeMap_[this.selectedId];
     this.linkPaths('displayedList', selectedNode.path + '.children');
     this._setDisplayedList(selectedNode.children);
@@ -150,6 +163,59 @@ var BookmarksStore = Polymer({
     delete this.idToNodeMap_[id];
   },
 
+  /**
+   * Remove all selected items in the list.
+   * @private
+   */
+  clearSelectedItems_: function() {
+    if (!this.displayedList)
+      return;
+
+    for (var i = 0; i < this.displayedList.length; i++) {
+      if (!this.displayedList[i].isSelectedItem)
+        continue;
+
+      this.set('displayedList.#' + i + '.isSelectedItem', false);
+    }
+  },
+
+  /**
+   * Return the index in the search result of an item.
+   * @param {BookmarkTreeNode} item
+   * @return {number}
+   * @private
+   */
+  getIndexInList_: function(item) {
+    return this.searchTerm ? item.searchResultIndex : item.index;
+  },
+
+  /**
+   * @param {BookmarkTreeNode} item
+   * @return {boolean}
+   * @private
+   */
+  isInDisplayedList_: function(id) {
+    return this.searchTerm ? this.idToSearchResultMap_[id] :

[tsergeant, 2017/01/25 06:02:13]

This doesn't always return a boolean? Sometimes it returns an Element?

If you want to convert that to a boolean, you can do !!
eg,

!!this.idToSearchResultMap_[id]

[jiaxi, 2017/01/30 03:28:58]

Discussed offline, changing this map into a set.

+                             this.idToNodeMap_[id].parentId == this.selectedId;
+  },
+
+  /**
+   * Initializes the search results returned by the API as follows:
+   * - Populates |idToSearchResultMap_| with a mapping of all result ids to
+   *   their corresponding result.
+   * - Sets up the |searchResultIndex|.
+   * @param {array<BookmarkTreeNode>} item

[tsergeant, 2017/01/25 06:02:13]

The Array type has an uppercase 'A'

[jiaxi, 2017/01/30 03:28:59]

Done.

+   * @private
+   */
+  setupSearchResults_: function(results) {
+    for (var i = 0; i < results.length; i++) {
+      results[i].searchResultIndex = i;
+      this.idToSearchResultMap_[results[i].id] = results[i];
+    }

[tsergeant, 2017/01/25 06:02:13]

Can you set `isSelectedItem = false` in here? It's always set for regular
results, so it seems strange that it might not happen for search results.

[jiaxi, 2017/01/30 03:28:58]

Done.

+
+    this._setDisplayedList(results);
+  },
+
 ////////////////////////////////////////////////////////////////////////////////
 // bookmarks-store, bookmarks API event listeners:
 
@@ -163,17 +229,45 @@ var BookmarksStore = Polymer({
    *           node: BookmarkTreeNode}} removeInfo
    */
   onBookmarkRemoved_: function(id, removeInfo) {
-    if (this.isAncestorOfSelected_(this.idToNodeMap_[id]))
-      this.fire('selected-folder-changed', removeInfo.parentId);
-
-    var parentNode = this.idToNodeMap_[removeInfo.parentId];
-    this.splice(parentNode.path + '.children', removeInfo.index, 1);
-    this.removeDescendantsFromMap_(id);
-    BookmarksStore.generatePaths(parentNode, removeInfo.index);
-
-    // Regenerate the search list if its displayed.
-    if (this.searchTerm)
-      this.updateSearchDisplay_();
+    chrome.bookmarks.getSubTree(removeInfo.parentId, function(parentNodes) {
+      var parentNode = parentNodes[0];
+      var isAncestor = this.isAncestorOfSelected_(this.idToNodeMap_[id]);
+      var isInDisplayedList = this.isInDisplayedList_(id);
+
+      // Update the tree.

[tsergeant, 2017/01/25 06:02:13]

Updates the tree...for what reason?

The fact that you've gone to all this trouble to do it this way makes me
suspicious that something subtle but important is happening. It would be good to
make sure that comments explain anything unusual that's going on.

[jiaxi, 2017/01/30 03:28:58]

Done.

+      this.removeDescendantsFromMap_(id);
+      parentNode.path = this.idToNodeMap_[parentNode.id].path;
+      BookmarksStore.generatePaths(parentNode, 0);
+      BookmarksStore.initNodes(parentNode, this.idToNodeMap_);
+      this.set(parentNode.path, parentNode)
+
+      // Update selectedId if the removed node is an ancestor of the current
+      // selected node.
+      if (isAncestor)
+        this.fire('selected-folder-changed', removeInfo.parentId);
+
+      // Only update the displayedList if the removed node is in the
+      // displayedList.
+      if (!isInDisplayedList)
+        return;
+
+      if (this.anchorIndex_ == this.displayedList.length - 1)
+        this.anchorIndex_--;
+
+      if (this.searchTerm) {
+        chrome.bookmarks.search(this.searchTerm, function(results) {

[tsergeant, 2017/01/25 06:02:13]

This spooks me a little because there are now two places where a search can
happen. It seems like it would be easy to add some logic to
updateSearchDisplay_() that doesn't get called when this version of search
happens.

Is it possible to just call updateSearchDisplay here? Is there a good reason not
to?

[jiaxi, 2017/01/30 03:28:58]

I can't think of a good way to using promise to reuse updateSearchDisplay. The
newest patch uses tricky way that updateSearchDisplay doesn't have a parameter
when been called not as an observer to reuse the code.

+          delete this.idToSearchResultMap_[id];
+          this.setupSearchResults_(results);
+          this.set(
+              'displayedList.#' + this.anchorIndex_ + '.isSelectedItem', true);
+        }.bind(this));
+      } else {
+        this._setDisplayedList(parentNode.children);
+
+        this.set(
+            'displayedList.#' + this.anchorIndex_ + '.isSelectedItem', true);
+      }
+    }.bind(this));
   },
 
   /**
@@ -213,12 +307,13 @@ var BookmarksStore = Polymer({
       this.searchTerm = '';
 
     // Deselect the old folder if defined.
-    if (this.selectedId)
-      this.set(this.idToNodeMap_[this.selectedId].path + '.isSelected', false);
+    if (this.selectedId && this.idToNodeMap_[this.selectedId])
+      this.set(
+          this.idToNodeMap_[this.selectedId].path + '.isSelectedFolder', false);
 
     var selectedId = /** @type {string} */ (e.detail);
     var newFolder = this.idToNodeMap_[selectedId];
-    this.set(newFolder.path + '.isSelected', true);
+    this.set(newFolder.path + '.isSelectedFolder', true);
     this.selectedId = selectedId;
   },
 
@@ -233,6 +328,68 @@ var BookmarksStore = Polymer({
     if (!folder.isOpen && this.isAncestorOfSelected_(folder))
       this.fire('selected-folder-changed', folder.id);
   },
+
+  /**
+   * Select a single item in the list.
+   * @param {BookmarkTreeNode} item
+   * @private
+   */
+  onSingleItemSelected_: function(item) {
+    this.clearSelectedItems_();
+    this.anchorIndex_ = this.getIndexInList_(item);
+    this.set(
+        'displayedList.#' + this.anchorIndex_ + '.isSelectedItem',
+        true);
+  },
+
+  /**
+   * Select multiple items based on |anchorIndex_| and the selected
+   * item. If |anchorIndex_| is not set, single select the item.
+   * @param {BookmarkTreeNode} item
+   * @private
+   */
+  onMultipleItemsShiftSelected_: function(item) {
+    this.clearSelectedItems_();
+    var startIndex, endIndex;
+    if (this.anchorIndex_ == undefined) {
+      this.anchorIndex_ = this.getIndexInList_(item);
+      startIndex = this.anchorIndex_;
+      endIndex = this.anchorIndex_;
+    } else {
+      var selectedIndex = this.getIndexInList_(item);
+      startIndex = Math.min(this.anchorIndex_, selectedIndex);
+      endIndex = Math.max(this.anchorIndex_, selectedIndex);
+    }
+    for (var i = startIndex; i <= endIndex; i++)
+      this.set('displayedList.#' + i + '.isSelectedItem', true);
+  },
+
+  /**
+   * Select multiple items with the index of the last elected item as
+   * |anchorIndex_|.
+   * @param {BookmarkTreeNode} item
+   * @private
+   */
+  onMultipleItemsCtrlSelected_: function(item) {
+    this.anchorIndex_ = this.getIndexInList_(item);
+    this.set(
+        'displayedList.#' + this.anchorIndex_ + '.isSelectedItem',
+        true);
+  },
+
+  /**
+   * Select item according to keyboard behaviours.
+   * @param {CustomEvent} e
+   * @private
+   */
+  onItemSelected_: function(e) {

[tsergeant, 2017/01/25 06:02:13]

Since store is supposed to be isolated from UI, I would be happier to name these
functions and variables according to what they do to the data rather than what
keys they correspond to.

"ctrl" means "add to the selection" and "shift" means "select a range".

So, the Event structure could become

{add: Boolean,
 range: Boolean}

Reframing it in this way means that we're just thinking about the data
modifications that need to be made, which means we can simplify the code.

If `e.add` is *false*, then we need to clear the selection before doing anything
else.

If `e.range` is true, then we need to do a range selection. Otherwise, we select
just a single item.

So the whole onItemSelected_ function becomes something like:

if (!e.add)
  this.clearSelectedItems_()
if (e.range)
  this.selectRange_(index)
else
  this.selectItem_(index)

Hooray! Now there's one fewer function needed, all of the functions have names
which describe how they modify the data, and it's possible to use Ctrl and Shift
and the same time.

[jiaxi, 2017/01/30 03:28:58]

The old bmm doesn't support Ctrl and Shift operation at the same time. The Shift
will overwrite everything. I've made the change so it matches the bahaviours of
the old bmm.

Also, should we do tests for the rangeSelect and addSelect in the store-test
instead of testing single select, shift select and ctrl select?

+    if (e.detail.shiftKey)
+      this.onMultipleItemsShiftSelected_(e.detail.item);
+    else if (e.detail.ctrlKey)
+      this.onMultipleItemsCtrlSelected_(e.detail.item);
+    else
+      this.onSingleItemSelected_(e.detail.item);
+  },
 });
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -262,13 +419,14 @@ BookmarksStore.generatePaths = function(bookmarkNode, startIndex) {
  * @param {Object=} idToNodeMap
  */
 BookmarksStore.initNodes = function(bookmarkNode, idToNodeMap) {
+  bookmarkNode.isSelectedItem = false;
   if (idToNodeMap)
     idToNodeMap[bookmarkNode.id] = bookmarkNode;
 
   if (bookmarkNode.url)
     return;
 
-  bookmarkNode.isSelected = false;
+  bookmarkNode.isSelectedFolder = false;
   bookmarkNode.isOpen = true;
   for (var i = 0; i < bookmarkNode.children.length; i++)
     BookmarksStore.initNodes(bookmarkNode.children[i], idToNodeMap);