[MD Bookmarks] Add Select for Bookmarks.

chrome/browser/resources/md_bookmarks/store.js

 // Copyright 2016 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.
 
 var BookmarksStore = Polymer({
   is: 'bookmarks-store',
 
   properties: {
     /** @type {BookmarkTreeNode} */
     rootNode: {
@@ skipping 19 matching lines @@
      * selected folder.
      * @type {Array<BookmarkTreeNode>}
      */
     displayedList: {
       type: Array,
       notify: true,
       readOnly: true,
     },
 
     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} */
   documentListeners_: null,
 
   /** @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]);
   },
 
   /** @override */
   detached: function() {
     for (var event in this.documentListeners_)
       document.removeEventListener(event, this.documentListeners_[event]);
   },
@@ skipping 23 matching lines @@
     this.idToNodeMap_ = {};
     this.rootNode.path = 'rootNode';
     BookmarksStore.generatePaths(rootNode, 0);
     BookmarksStore.initNodes(this.rootNode, this.idToNodeMap_);
     this.fire('selected-folder-changed', this.rootNode.children[0].id);
   },
 
   /** @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;
   },
 
   /**
    * @param {BookmarkTreeNode} folder
    * @private
    * @return {boolean}
    */
   isAncestorOfSelected_: function(folder) {
     if (!this.selectedId)
       return false;
 
     var selectedNode = this.idToNodeMap_[this.selectedId];
     return selectedNode.path.startsWith(folder.path);
   },
 
   /** @private */
   updateSearchDisplay_: function() {
     if (this.searchTerm == '') {
       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));
     }
   },
 
   /** @private */
   updateSelectedDisplay_: function() {
     // Don't change to the selected display if ID was cleared.
     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);
   },
 
   /**
    * Remove all descendants of a given node from the map.
    * @param {string} id
    * @private
    */
   removeDescendantsFromMap_: function(id) {
     var node = this.idToNodeMap_[id];
     if (!node)
       return;
 
     if (node.children) {
       for (var i = 0; i < node.children.length; i++)
         this.removeDescendantsFromMap_(node.children[i].id);
     }
     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:
 
   /**
    * Callback for when a bookmark node is removed.
    * If a folder is selected or is an ancestor of a selected folder, the parent
    * of the removed folder will be selected.
    * @param {string} id The id of the removed bookmark node.
    * @param {!{index: number,
    *           parentId: string,
    *           node: BookmarkTreeNode}} removeInfo
    */
   onBookmarkRemoved_: function(id, removeInfo) {
-    if (this.isAncestorOfSelected_(this.idToNodeMap_[id]))
-      this.fire('selected-folder-changed', removeInfo.parentId);
+    chrome.bookmarks.getSubTree(removeInfo.parentId, function(parentNodes) {
+      var parentNode = parentNodes[0];
+      var isAncestor = this.isAncestorOfSelected_(this.idToNodeMap_[id]);
+      var isInDisplayedList = this.isInDisplayedList_(id);
 
-    var parentNode = this.idToNodeMap_[removeInfo.parentId];
-    this.splice(parentNode.path + '.children', removeInfo.index, 1);
-    this.removeDescendantsFromMap_(id);
-    BookmarksStore.generatePaths(parentNode, removeInfo.index);
+      // 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)
 
-    // Regenerate the search list if its displayed.
-    if (this.searchTerm)
-      this.updateSearchDisplay_();
+      // 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));
   },
 
   /**
    * Called when the title of a bookmark changes.
    * @param {string} id The id of changed bookmark node.
    * @param {!Object} changeInfo
    */
   onBookmarkChanged_: function(id, changeInfo) {
     if (changeInfo.title)
       this.set(this.idToNodeMap_[id].path + '.title', changeInfo.title);
@@ skipping 19 matching lines @@
    * Selects the folder specified by the event and deselects the previously
    * selected folder.
    * @param {CustomEvent} e
    * @private
    */
   onSelectedFolderChanged_: function(e) {
     if (this.searchTerm)
       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;
   },
 
   /**
    * Handles events that open and close folders.
    * @param {CustomEvent} e
    * @private
    */
   onFolderOpenChanged_: function(e) {
     var folder = this.idToNodeMap_[e.detail.id];
     this.set(folder.path + '.isOpen', e.detail.open);
     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);
+  },
 });
 
 ////////////////////////////////////////////////////////////////////////////////
 // bookmarks-store, static methods:
 
 /**
  * Stores the path from the store to a node inside the node.
  * @param {BookmarkTreeNode} bookmarkNode
  * @param {number} startIndex
  */
 BookmarksStore.generatePaths = function(bookmarkNode, startIndex) {
   if (!bookmarkNode.children)
     return;
 
   for (var i = startIndex; i < bookmarkNode.children.length; i++) {
     bookmarkNode.children[i].path = bookmarkNode.path + '.children.#' + i;
     BookmarksStore.generatePaths(bookmarkNode.children[i], 0);
   }
 };
 
 /**
  * Initializes the nodes in the bookmarks tree as follows:
  * - Populates |idToNodeMap_| with a mapping of all node ids to their
  *   corresponding BookmarkTreeNode.
  * - Sets all the nodes to not selected and open by default.
  * @param {BookmarkTreeNode} bookmarkNode
  * @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);
 };