Implement refreshing logic in Files app.

Implement refreshing logic in Files app.

This CL adds an implementation for the refresh button for volumes which do not
support file watching.

Since refreshing is often fast and seems like not doing anything, a blink method
on the spinner controller has been added, so the spinner always shows for at
least one second when refreshing, so users have feedback that the refreshing
is completed.

TEST=Tested manually.
BUG=501864
Committed: https://crrev.com/3e8fc7ac642049732a2beceb4b6b33b7db19272d
Cr-Commit-Position: refs/heads/master@{#337573}

[mtomasz, 2015-07-03 08:09:12]

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

[mtomasz, 2015-07-03 08:09:56]

@hirono: PTAL. This CL depends on two other CLs:
1. https://codereview.chromium.org/1221093002/
2. https://codereview.chromium.org/1215533003/

Thanks!

[hirono, 2015-07-06 04:52:38]

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/scan_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/scan_controller.js:151:
this.hideSpinner_();
I think this hideSpinner_() should not hide blinking spinner.

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/scan_controller.js:267:
ScanController.prototype.hideSpinner_ = function() {
Please add /** @private */

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/spinner_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:32:
SpinnerController.prototype.blink = function() {
Can we hide blinking spinner when the current directory is changed?

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:35:
this.blinkHideTimeoutId_ = 0;
Is this line needed?

[mtomasz, 2015-07-06 08:48:48]

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/scan_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/scan_controller.js:151:
this.hideSpinner_();
On 2015/07/06 04:52:38, hirono wrote:
> I think this hideSpinner_() should not hide blinking spinner.

Done (in spinner_controller.js).

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/scan_controller.js:267:
ScanController.prototype.hideSpinner_ = function() {
On 2015/07/06 04:52:38, hirono wrote:
> Please add /** @private */

Done.

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/spinner_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:32:
SpinnerController.prototype.blink = function() {
On 2015/07/06 04:52:38, hirono wrote:
> Can we hide blinking spinner when the current directory is changed?

Do we need to hide it? Blinking is only one second. The code would be more
complicated (dependency on DirectoryModel).

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:35:
this.blinkHideTimeoutId_ = 0;
On 2015/07/06 04:52:38, hirono wrote:
> Is this line needed?

Done.

[hirono, 2015-07-06 09:39:27]

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/spinner_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:32:
SpinnerController.prototype.blink = function() {
On 2015/07/06 08:48:48, mtomasz wrote:
> On 2015/07/06 04:52:38, hirono wrote:
> > Can we hide blinking spinner when the current directory is changed?
> 
> Do we need to hide it? Blinking is only one second. The code would be more
> complicated (dependency on DirectoryModel).

I think it is worth to handle it. I'm guessing the spinner is not designed for
laying on files. e.g. We hide the spinner at the first scan update event.
Let's ask UX.

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:67: if
(!this.element_.hidden)
I think we should clear blinkHideTimeout here.

blink() -> showLater(), means the client code expect the spinner eventually is
shown.

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/scan_controller.js (right):

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/scan_controller.js:270:
ScanController.prototype.hideSpinner_ = function() {
Is the method still needed? The hide method no longer hide the blinking spinner.

[mtomasz, 2015-07-07 03:42:11]

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/spinner_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:32:
SpinnerController.prototype.blink = function() {
On 2015/07/06 09:39:27, hirono wrote:
> On 2015/07/06 08:48:48, mtomasz wrote:
> > On 2015/07/06 04:52:38, hirono wrote:
> > > Can we hide blinking spinner when the current directory is changed?
> > 
> > Do we need to hide it? Blinking is only one second. The code would be more
> > complicated (dependency on DirectoryModel).
> 
> I think it is worth to handle it. I'm guessing the spinner is not designed for
> laying on files. e.g. We hide the spinner at the first scan update event.
> Let's ask UX.

Sebastien said he's fine with blinking the spinner for 1s on top, so I guess
it's OK to keep it. Also, I find it very unlikely that a user will click the
refresh button and within one second navigate to other directory. But I can add
the hiding on dir change logic if you feel strong about it. WDYT?

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:67: if
(!this.element_.hidden)
On 2015/07/06 09:39:27, hirono wrote:
> I think we should clear blinkHideTimeout here.
> 
> blink() -> showLater(), means the client code expect the spinner eventually is
> shown.

Good point. Done.

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/scan_controller.js (right):

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/scan_controller.js:270:
ScanController.prototype.hideSpinner_ = function() {
On 2015/07/06 09:39:27, hirono wrote:
> Is the method still needed? The hide method no longer hide the blinking
spinner.

It's used if there is no blinking spinner shown. The blinking spinner is used
only for refreshing via the refresh button.

[hirono, 2015-07-07 03:58:22]

lgtm!

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/spinner_controller.js (right):

https://codereview.chromium.org/1222563005/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/spinner_controller.js:32:
SpinnerController.prototype.blink = function() {
On 2015/07/07 03:42:10, mtomasz wrote:
> On 2015/07/06 09:39:27, hirono wrote:
> > On 2015/07/06 08:48:48, mtomasz wrote:
> > > On 2015/07/06 04:52:38, hirono wrote:
> > > > Can we hide blinking spinner when the current directory is changed?
> > > 
> > > Do we need to hide it? Blinking is only one second. The code would be more
> > > complicated (dependency on DirectoryModel).
> > 
> > I think it is worth to handle it. I'm guessing the spinner is not designed
for
> > laying on files. e.g. We hide the spinner at the first scan update event.
> > Let's ask UX.
> 
> Sebastien said he's fine with blinking the spinner for 1s on top, so I guess
> it's OK to keep it. Also, I find it very unlikely that a user will click the
> refresh button and within one second navigate to other directory. But I can
add
> the hiding on dir change logic if you feel strong about it. WDYT?

SGTM. Thank you for confirming!

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/scan_controller.js (right):

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/scan_controller.js:270:
ScanController.prototype.hideSpinner_ = function() {
On 2015/07/07 03:42:11, mtomasz wrote:
> On 2015/07/06 09:39:27, hirono wrote:
> > Is the method still needed? The hide method no longer hide the blinking
> spinner.
> 
> It's used if there is no blinking spinner shown. The blinking spinner is used
> only for refreshing via the refresh button.

I just noticed the method originally came from SpinnerController. But I still
think we can remove the method. If the method is called by onScanCompleted_ or
onScanCancelled_, directoryModel_.isScanning should be false. And if
onScanUpdated is dispatched, it means we got some entries.

[mtomasz, 2015-07-07 04:01:48]

Thanks!

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/scan_controller.js (right):

https://codereview.chromium.org/1222563005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/scan_controller.js:270:
ScanController.prototype.hideSpinner_ = function() {
On 2015/07/07 03:58:22, hirono wrote:
> On 2015/07/07 03:42:11, mtomasz wrote:
> > On 2015/07/06 09:39:27, hirono wrote:
> > > Is the method still needed? The hide method no longer hide the blinking
> > spinner.
> > 
> > It's used if there is no blinking spinner shown. The blinking spinner is
used
> > only for refreshing via the refresh button.
> 
> I just noticed the method originally came from SpinnerController. But I still
> think we can remove the method. If the method is called by onScanCompleted_ or
> onScanCancelled_, directoryModel_.isScanning should be false. And if
> onScanUpdated is dispatched, it means we got some entries.

Ah, got it. Yes, I moved the scanning logic from spinner controller to the
scanner controller. As a result we got rid of the DirectoryModel dependency from
the spinner controller.

It was just copy paste of the logic. Let me remove the conditions in a separate
CL, as I have to think carefully of all edge cases.

[mtomasz, 2015-07-07 08:46:46]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-07-07 08:46:47]

The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org
Link to the patchset: https://codereview.chromium.org/1222563005/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-07-07 08:46:59]

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

[commit-bot: I haz the power, 2015-07-07 09:22:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-07 09:23:17]

Patchset 4 (id:??) landed as
https://crrev.com/3e8fc7ac642049732a2beceb4b6b33b7db19272d
Cr-Commit-Position: refs/heads/master@{#337573}