[MD Bookmarks] Refine mouse selection

[MD Bookmarks] Refine mouse selection

This CL makes some changes to the way multiple selection works:
- single items can be selected/deselected via Ctrl+Click
- Shift-Clicking doesn't update the anchor

The changes made will also support keyboard navigation in a future patch.

BUG=692844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2888863002
Cr-Commit-Position: refs/heads/master@{#473508}
Committed: https://chromium.googlesource.com/chromium/src/+/8e3f9e3a9823a1814f0f1f4ff3c4594abbf8a7d5

[calamity, 2017-05-17 05:51:19]

Description was changed from

==========
[MD Bookmarks] Refine mouse selection

This CL makes some changes to the way multiple selection works:
- single items can be selected/deselected via Ctrl+Click
- Shift-Clicking doesn't update the anchor

The changes made will also support keyboard navigation in a future patch.

BUG=692844
==========

to

==========
[MD Bookmarks] Refine mouse selection

This CL makes some changes to the way multiple selection works:
- single items can be selected/deselected via Ctrl+Click
- Shift-Clicking doesn't update the anchor

The changes made will also support keyboard navigation in a future patch.

BUG=692844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[calamity, 2017-05-18 08:31:48]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[calamity, 2017-05-18 08:32:14]

calamity@chromium.org changed reviewers:
+ tsergeant@chromium.org

[commit-bot: I haz the power, 2017-05-18 08:32:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 08:42:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 08:42:55]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

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

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/actions.js (right):

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:145: * @param {{add: boolean,
`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?

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:148: updateAnchor: boolean}}
config
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?

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:152: assert(!(config.range &&
config.toggle));
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?

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.js (right):

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.js:78:
this.dispatch(bookmarks.actions.selectItem(this.itemId, this.getState(), {
I think it would be nice to pull out this and the identical block in the next
method out into a helper function named something like selectThisItem_()

[calamity, 2017-05-19 05:01:20]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 05:02:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 06:10:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 06:10:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[calamity, 2017-05-19 06:32:13]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[calamity, 2017-05-19 06:32:39]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-19 06:33:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-05-19 06:37:36]

Patchset #3 (id:60001) has been deleted

[calamity, 2017-05-19 06:37:42]

Patchset #3 (id:80001) has been deleted

[calamity, 2017-05-19 06:46:30]

The CQ bit was checked by calamity@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 06:46:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

add => clear is a doozy.

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/actions.js (right):

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:145: * @param {{add: boolean,
On 2017/05/18 23:13:37, tsergeant wrote:
> `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.
> 

#+1

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

Done.

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:148: updateAnchor: boolean}}
config
On 2017/05/18 23:13:37, tsergeant wrote:
> 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?

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

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:152: assert(!(config.range &&
config.toggle));
On 2017/05/18 23:13:37, tsergeant wrote:
> 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?

Done.

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/item.js (right):

https://codereview.chromium.org/2888863002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/item.js:78:
this.dispatch(bookmarks.actions.selectItem(this.itemId, this.getState(), {
On 2017/05/18 23:13:37, tsergeant wrote:
> I think it would be nice to pull out this and the identical block in the next
> method out into a helper function named something like selectThisItem_()

Done.

[commit-bot: I haz the power, 2017-05-19 08:54:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 08:54:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tsergeant, 2017-05-22 04:06:40]

lgtm

[calamity, 2017-05-22 04:20:57]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2017-05-22 04:21:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 05:42:45]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495426857791560,
"parent_rev": "38aad7912d2a3ec1ace8b4c0da38464fc957e042", "commit_rev":
"8e3f9e3a9823a1814f0f1f4ff3c4594abbf8a7d5"}

[commit-bot: I haz the power, 2017-05-22 05:43:02]

Description was changed from

==========
[MD Bookmarks] Refine mouse selection

This CL makes some changes to the way multiple selection works:
- single items can be selected/deselected via Ctrl+Click
- Shift-Clicking doesn't update the anchor

The changes made will also support keyboard navigation in a future patch.

BUG=692844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Bookmarks] Refine mouse selection

This CL makes some changes to the way multiple selection works:
- single items can be selected/deselected via Ctrl+Click
- Shift-Clicking doesn't update the anchor

The changes made will also support keyboard navigation in a future patch.

BUG=692844
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2888863002
Cr-Commit-Position: refs/heads/master@{#473508}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e3f9e3a9823a1814f0f1f4ff3c4...
==========

[commit-bot: I haz the power, 2017-05-22 05:43:04]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/8e3f9e3a9823a1814f0f1f4ff3c4...