[MD Bookmarks] Refine mouse selection

chrome/browser/resources/md_bookmarks/actions.js

 // Copyright 2017 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.
 
 /**
  * @fileoverview Module for functions which produce action objects. These are
  * listed in one place to document available actions and their parameters.
  */
 
 cr.define('bookmarks.actions', function() {
@@ skipping 123 matching lines @@
 
   /** @return {!Action} */
   function deselectItems() {
     return {
       name: 'deselect-items',
     };
   }
 
   /**
    * @param {string} id
-   * @param {boolean} add
-   * @param {boolean} range
    * @param {BookmarksPageState} state
+   * @param {{add: boolean,

[tsergeant, 2017/05/18 23:13:37]

`add` is a confusing name now, since if `add` is true, it allows you to remove
things. How about flipping the boolean and naming it 'clear': If `clear` is
true, then the existing selection will be cleared before any new operations are
performed.

Relatedly, it would be nice to document these config options: Is there any nice
way to do that here?

[calamity, 2017/05/19 06:46:56]

#+1
Done.

+              range: boolean,
+              toggle: boolean,
+              updateAnchor: boolean}} config

[tsergeant, 2017/05/18 23:13:37]

Every call site at the moment has `updateAnchor == !range`. Is there any
particular reason why you need to pass it in as a separate config option rather
than just figuring it out automatically?

[calamity, 2017/05/19 06:46:56]

I thought I'd need special config for mouse vs keyboard, but it turns out they
align. Removed!

    * @return {!Action}
    */
-  function selectItem(id, add, range, state) {
+  function selectItem(id, state, config) {
+    assert(!(config.range && config.toggle));

[tsergeant, 2017/05/18 23:13:37]

toggle is a no-op unless add is true, so do you also want to assert

toggle IMPLIES add

or, uh, <furious propositional logic>

!toggle OR add

here?

[calamity, 2017/05/19 06:46:56]

Done.

+
     var anchor = state.selection.anchor;
     var toSelect = [];
 
-    // TODO(tsergeant): Make it possible to deselect items by ctrl-clicking them
-    // again.
-    if (range && anchor) {
+    if (config.range && anchor) {
       var displayedList = bookmarks.util.getDisplayedList(state);
       var selectedIndex = displayedList.indexOf(id);
       assert(selectedIndex != -1);
       var anchorIndex = displayedList.indexOf(anchor);
       if (anchorIndex == -1)
         anchorIndex = selectedIndex;
 
       var startIndex = Math.min(anchorIndex, selectedIndex);
       var endIndex = Math.max(anchorIndex, selectedIndex);
 
       for (var i = startIndex; i <= endIndex; i++)
         toSelect.push(displayedList[i]);
     } else {
       toSelect.push(id);
     }
 
     return {
       name: 'select-items',
-      add: add,
-      anchor: id,
+      add: config.add,
+      toggle: config.toggle,
+      anchor: config.updateAnchor ? id : anchor,
       items: toSelect,
     };
   }
 
   /**
    * @param {string} term
    * @return {!Action}
    */
   function setSearchTerm(term) {
     if (!term)
@@ skipping 25 matching lines @@
     moveBookmark: moveBookmark,
     refreshNodes: refreshNodes,
     removeBookmark: removeBookmark,
     reorderChildren: reorderChildren,
     selectFolder: selectFolder,
     selectItem: selectItem,
     setSearchResults: setSearchResults,
     setSearchTerm: setSearchTerm,
   };
 });