Files.app: Introduce check-select mode.

Files.app: Introduce check-select mode.

Check-select mode spec is here: https://docs.google.com/document/d/1kCG0AFtZSlxlkuipNHlKLY2nvjPdDCViG6RKhMI5-dc/edit

Mode transition rules:
 + Entering check-select mode
   - When two or more items are selected.
   - When checkmark area is clicked.
 + Leaving check-select mode
   - When all items are unselected.
   - When non-checkmark area on an item is clicked.
   - When direction key(UP,LEFT,etc...) results in single selection.

BUG=460220
TEST=manually tested

Committed: https://crrev.com/e144d30232209034b82d544926070bb8dc62f6ec
Cr-Commit-Position: refs/heads/master@{#318658}

[fukino, 2015-02-27 04:26:25]

fukino@chromium.org changed reviewers:
+ hirono@chromium.org

[fukino, 2015-02-27 04:26:25]

Could you take a look? Thank you!

[fukino, 2015-02-27 04:28:26]

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/util.js (left):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/util.js:1046:
util.repeatMouseEventWithCtrlKey = function(target, event) {
Now that we can directly modify the selection controller's behavior, I removed
this hack.

[hirono, 2015-02-27 05:45:46]

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:122: }
nit: Please add ;

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:448: * @param
{cr.ui.ListSelectionModel} selectionModel The selection model to
nit: !cr.ui.ListSelectionModel

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:450: * @param
{cr.ui.Grid} grid The grid to interact with.
ditto.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:452: * @extends
{cr.ui.GridSelectionController}
nit: Can we mark this constructor as @struct?

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
(right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:8: *
@extends {cr.ui.ListSelectionModel}
Can we mark this constructor as @struct?

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:13:
/** @public {boolean} */
nit: @public or @private?

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:21: *
Update the check-select mode.
nit: Updates

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:24:
setCheckSelectMode: function(enabled) {
nit: FileListSelectionModel.prototype.setCheckSelectMode = ... style is better
for consistency.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:29: *
Get the check-select mode.
nit: Gets

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:37: *
Dispatches an event and calls all the listeners that are listening to
We don't need to add details if we mark a method as @override

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:53:
this.isCheckSelectMode_ = false;
Why do we update the mode in dispatchEvent method?

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:64: *
@extends {cr.ui.ListSingleSelectionModel}
@struct?

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:74: *
Update the check-select mode.
nit: Updates

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:82: *
Get the check-select mode.
nit: Gets

[fukino, 2015-02-27 06:43:52]

Thank you!

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_grid.js (right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:122: }
On 2015/02/27 05:45:46, hirono wrote:
> nit: Please add ;

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:448: * @param
{cr.ui.ListSelectionModel} selectionModel The selection model to
On 2015/02/27 05:45:46, hirono wrote:
> nit: !cr.ui.ListSelectionModel

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:450: * @param
{cr.ui.Grid} grid The grid to interact with.
On 2015/02/27 05:45:46, hirono wrote:
> ditto.

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_grid.js:452: * @extends
{cr.ui.GridSelectionController}
On 2015/02/27 05:45:46, hirono wrote:
> nit: Can we mark this constructor as @struct?

It seems this can't be @struct because cr.ui.GridSelectionController is not
@struct.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
(right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:8: *
@extends {cr.ui.ListSelectionModel}
On 2015/02/27 05:45:46, hirono wrote:
> Can we mark this constructor as @struct?

structs can only extends structs, but cr.ui.ListSelectionModel is not struct...

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:13:
/** @public {boolean} */
On 2015/02/27 05:45:46, hirono wrote:
> nit: @public or @private?

It should be @private, thanks! Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:21: *
Update the check-select mode.
On 2015/02/27 05:45:46, hirono wrote:
> nit: Updates

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:24:
setCheckSelectMode: function(enabled) {
On 2015/02/27 05:45:46, hirono wrote:
> nit: FileListSelectionModel.prototype.setCheckSelectMode = ... style is better
> for consistency.

Done for FileListSelectionMode, FileListSingleSelectionModel,
FileListSelectionController, and FileGridSelectionController.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:29: *
Get the check-select mode.
On 2015/02/27 05:45:46, hirono wrote:
> nit: Gets

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:37: *
Dispatches an event and calls all the listeners that are listening to
On 2015/02/27 05:45:46, hirono wrote:
> We don't need to add details if we mark a method as @override

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:53:
this.isCheckSelectMode_ = false;
On 2015/02/27 05:45:46, hirono wrote:
> Why do we update the mode in dispatchEvent method?

When redrawing file list, selectedIndexes can be 0 temporarily.
In such case, it can occur that check-select mode is disabled unintentionally.
To avoid updating mode between startChange() and endChange(), we wait
dispatchEvent() which is triggered by endChange().

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:64: *
@extends {cr.ui.ListSingleSelectionModel}
On 2015/02/27 05:45:46, hirono wrote:
> @struct?

Same situation as cr.ui.ListSelectionModel.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:74: *
Update the check-select mode.
On 2015/02/27 05:45:46, hirono wrote:
> nit: Updates

Done.

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:82: *
Get the check-select mode.
On 2015/02/27 05:45:46, hirono wrote:
> nit: Gets

Done.

https://codereview.chromium.org/962103002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/toolbar_controller.js (right):

https://codereview.chromium.org/962103002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/toolbar_controller.js:124: if
(this.directoryModel_.getFileListSelection().multiple) {
I added this condition not to show multi-selection UI when it is not allowed.
(e.g. Open-folder dialog)

[hirono, 2015-02-27 09:03:52]

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
(right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:8: *
@extends {cr.ui.ListSelectionModel}
On 2015/02/27 06:43:52, fukino wrote:
> On 2015/02/27 05:45:46, hirono wrote:
> > Can we mark this constructor as @struct?
> 
> structs can only extends structs, but cr.ui.ListSelectionModel is not
struct...

If possible, I prefer to use @suppress {checkStructDictInheritance} rather than
giving up all unknown property checks.

I wrote about it before at item #14.
https://docs.google.com/a/google.com/document/d/16VY9cRVNo4Hav2_ciIuRcdEZXfb7...

WDYT?

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:53:
this.isCheckSelectMode_ = false;
On 2015/02/27 06:43:51, fukino wrote:
> On 2015/02/27 05:45:46, hirono wrote:
> > Why do we update the mode in dispatchEvent method?
> 
> When redrawing file list, selectedIndexes can be 0 temporarily.
> In such case, it can occur that check-select mode is disabled unintentionally.
> To avoid updating mode between startChange() and endChange(), we wait
> dispatchEvent() which is triggered by endChange().

How about override endChange instead of dispatchEvent?
It sounds more suitable for the method names (endChange vs dispatchEvent).

[fukino, 2015-02-27 09:25:27]

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
(right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:8: *
@extends {cr.ui.ListSelectionModel}
On 2015/02/27 09:03:52, hirono wrote:
> On 2015/02/27 06:43:52, fukino wrote:
> > On 2015/02/27 05:45:46, hirono wrote:
> > > Can we mark this constructor as @struct?
> > 
> > structs can only extends structs, but cr.ui.ListSelectionModel is not
> struct...
> 
> If possible, I prefer to use @suppress {checkStructDictInheritance} rather
than
> giving up all unknown property checks.
> 
> I wrote about it before at item #14.
>
https://docs.google.com/a/google.com/document/d/16VY9cRVNo4Hav2_ciIuRcdEZXfb7...
> 
> WDYT?

I've just remembered!
Done for FileListSelectionModel, FileListSingleSelectionModel,
FileListSelectionController, and FileGridSelectionController.
Thank you!

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:53:
this.isCheckSelectMode_ = false;
On 2015/02/27 09:03:52, hirono wrote:
> On 2015/02/27 06:43:51, fukino wrote:
> > On 2015/02/27 05:45:46, hirono wrote:
> > > Why do we update the mode in dispatchEvent method?
> > 
> > When redrawing file list, selectedIndexes can be 0 temporarily.
> > In such case, it can occur that check-select mode is disabled
unintentionally.
> > To avoid updating mode between startChange() and endChange(), we wait
> > dispatchEvent() which is triggered by endChange().
> 
> How about override endChange instead of dispatchEvent?
> It sounds more suitable for the method names (endChange vs dispatchEvent).

To do it with endChange(), we need to access this.changeCount_. However, it is
private member and we shouldn't rely on it.
I don't think dispatchEvent is not suitable, because client-observable state
changes are strongly connected to the dispatchEvent. WDYT?

[hirono, 2015-02-27 09:49:59]

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
(right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:53:
this.isCheckSelectMode_ = false;
On 2015/02/27 09:25:26, fukino wrote:
> On 2015/02/27 09:03:52, hirono wrote:
> > On 2015/02/27 06:43:51, fukino wrote:
> > > On 2015/02/27 05:45:46, hirono wrote:
> > > > Why do we update the mode in dispatchEvent method?
> > > 
> > > When redrawing file list, selectedIndexes can be 0 temporarily.
> > > In such case, it can occur that check-select mode is disabled
> unintentionally.
> > > To avoid updating mode between startChange() and endChange(), we wait
> > > dispatchEvent() which is triggered by endChange().
> > 
> > How about override endChange instead of dispatchEvent?
> > It sounds more suitable for the method names (endChange vs dispatchEvent).
> 
> To do it with endChange(), we need to access this.changeCount_. However, it is
> private member and we shouldn't rely on it.
> I don't think dispatchEvent is not suitable, because client-observable state
> changes are strongly connected to the dispatchEvent. WDYT?

I think it causes a strange behavior like:

model.getCheckSelectMode(); // true
model.leadIndex = newValue;
model.getCheckSelectMode(); // false

If we cannot use endChange easily, how about filtering out the above case by
seeing event.type, or just adding event listener itself in the constructor to
update isCheckSelectMode_?

[fukino, 2015-03-02 03:51:07]

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js
(right):

https://codereview.chromium.org/962103002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/file_list_selection_model.js:53:
this.isCheckSelectMode_ = false;
On 2015/02/27 09:49:59, hirono wrote:
> I think it causes a strange behavior like:
> 
> model.getCheckSelectMode(); // true
> model.leadIndex = newValue;
> model.getCheckSelectMode(); // false
> 
> If we cannot use endChange easily, how about filtering out the above case by
> seeing event.type, or just adding event listener itself in the constructor to
> update isCheckSelectMode_?
> 

Ah, you're right! Thank you pointing this out.
What I wanted to do was "Update the mode based on selection count before
notifying changes of selected indexes". 'leadIndex' and 'anchorIndex' events
should have been filtered out.

Using event listeners to update isCheckSelectMode_ can make another complication
to deal with event dispatching order, so I've simply added filtering for event
type.

[hirono, 2015-03-02 04:03:52]

> Using event listeners to update isCheckSelectMode_ can make another
complication
> to deal with event dispatching order, so I've simply added filtering for event
> type.

EventTarget (or cr.EventTarget) ensures to call listeners in the same order that
the listeners were registered.
So if we add the listener in the constructor, we don't have to care about it.

[fukino, 2015-03-02 04:30:15]

On 2015/03/02 04:03:52, hirono wrote:
> > Using event listeners to update isCheckSelectMode_ can make another
> complication
> > to deal with event dispatching order, so I've simply added filtering for
event
> > type.
> 
> EventTarget (or cr.EventTarget) ensures to call listeners in the same order
that
> the listeners were registered.
> So if we add the listener in the constructor, we don't have to care about it.

I misunderstood the constructor you meant..
Adding listener to FileListSelectionModel makes sense. I modified it to use the
listener.

[hirono, 2015-03-02 05:15:01]

LGTM, thanks!

[fukino, 2015-03-02 06:12:09]

Thank you!

[fukino, 2015-03-02 06:12:18]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2015-03-02 06:12:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/962103002/100001

[commit-bot: I haz the power, 2015-03-02 07:03:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-02 07:04:15]

Patchset 6 (id:??) landed as
https://crrev.com/e144d30232209034b82d544926070bb8dc62f6ec
Cr-Commit-Position: refs/heads/master@{#318658}