[MD Bookmarks] Refine mouse selection

chrome/browser/resources/md_bookmarks/actions.js

Index: chrome/browser/resources/md_bookmarks/actions.js
diff --git a/chrome/browser/resources/md_bookmarks/actions.js b/chrome/browser/resources/md_bookmarks/actions.js
index 9f7800a123e1e77cc0ea7ac6664884858cda2030..95fe802e38a8f29f5c4e0ba587062f051e7b27bd 100644
--- a/chrome/browser/resources/md_bookmarks/actions.js
+++ b/chrome/browser/resources/md_bookmarks/actions.js
@@ -141,18 +141,20 @@ cr.define('bookmarks.actions', function() {
 
   /**
    * @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);
@@ -171,8 +173,9 @@ cr.define('bookmarks.actions', function() {
 
     return {
       name: 'select-items',
-      add: add,
-      anchor: id,
+      add: config.add,
+      toggle: config.toggle,
+      anchor: config.updateAnchor ? id : anchor,
       items: toSelect,
     };
   }