New issue
Advanced search Search tips

Issue 738803 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Double-click opens either the selection or a single file indeterministically

Project Member Reported by yamaguchi@chromium.org, Jul 3 2017

Issue description

Chrome Version: 61.0.3148.0
Chrome OS Version: 9692.0.0

Steps To Reproduce:
(1) Open a folder containing 3+ image files Files app.
(2) Select 2 files A and B by clicking icons (check-select mode).
(3) Double-click the other file C (which is not selected).

Expected Result:
Either of these.
- C opens in Gallery. Since this is single selection, all the files in the directory appears as thumbnail in the ribbon.
- A and B opens in Gallery. Since this is multiple selection mode, only the 2 files appears in Gallery.
(I guess the former is the intended behavior, but not sure.)

Actual Result:
- If the double-click is slow enough, C opens.
- If the double-click is fast enough, A and B opens but not C.

How frequently does this problem reproduce? (Always, sometimes, hard to
reproduce?)
Depends on the device, but it happens relatively reliably.
On my Linux workstation, I could reproduce it about 90% of the time.

What is the impact to the user, and is there a workaround? If so, what is
it?
User may open some files unintentionally.
As a workaround, avoid double-clicking an unselected file in check-selection mode.

 
Owner: yamaguchi@chromium.org
Summary: Double-click opens either the selection or a single file indeterministically (was: Double-click after selection sometimes opens old selection)
I have found this is not limited to double clicking an unselected file.
If you double click B at step 3, it will also indeterministically open either {A,B} or {B} depending on the timing between the double click and a certain internal processing.
One of the cases where this happens is:

getFileTask invoked here returns existing task for the old seletions
https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/main_window_component.js?type=cs&q=file:main_window_component.js+getFileTasks&sq=package:chromium&l=204
when it runs
- AFTER FileSelectionHandler.onFileSelectionChanged issuing CHANGE event
- and BEFORE it's handled by TaskController.onSelectionChanged_ to update the tasks.

FileSelectionHandler.onFileSelectionChanged()
https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/file_selection.js?type=cs&q=FileSelectionHandler.prototype.onFileSelectionChanged+dispatchSimpleEvent&sq=package:chromium&l=241

TaskController.onFileSelectionChanged_()
https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/task_controller.js?type=cs&q=TaskController+Lsq%3Dpackage:chromium&l=269
Sorry, #2 is not quite correct.
TaskController.onSelectionChanged_ is run before MainWindowComponent.prototype.acceptSelection_. I am still not sure how this the regression is caused.

At least, this line at the beginning of TaskController.prototype.onSelectionChanged_
seems to be why the revert can fix the issue as it invalidates the cache currently.
>  this.tasks_ = null;

https://codereview.chromium.org/2966163005/diff/1/ui/file_manager/file_manager/foreground/js/task_controller.js

Cc: tetsui@chromium.org
Cc: -tetsui@chromium.org yamaguchi@chromium.org fukino@chromium.org
Owner: tetsui@chromium.org
Status: Started (was: Untriaged)
yamaguchi@: Thank you for investigating. Now I think I can fix the regression with a small CL, but feel free to revert it until then.

I added
> assert(util.isSameEntries(tasks.entries, selection.entries));
to
 https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/foreground/js/main_window_component.js?l=206&rcl=bdf4642db91687af930bd9c58dfee693350117fa

and it failed. Simply adding this.tasks_ = null; in onSelectionChanged_ temporarily fixes this, but it should break  crbug.com/712121  .
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa4c54cbd611f55c475eda159865c7134442f71c

commit aa4c54cbd611f55c475eda159865c7134442f71c
Author: yamaguchi <yamaguchi@chromium.org>
Date: Thu Jul 06 08:42:20 2017

Revert of Stop updating context menu task items when not changed. (patchset #2 id:20001 of https://codereview.chromium.org/2932803002/ )

Reason for revert:
We'd like to revert 2833413003 because it caused regression  crbug.com/738803 .
This is change depends on 2833413003.

Original issue's description:
> Stop updating context menu task items when not changed.
>
> During FileSelectionHandler.EventType.CHANGE to CHANGE_THROTTLED,
> existing task items in context menu were overwritten by a temporary
> item. File copy operation fires CHANGE and CHANGE_THROTTLED repeatedly,
> which leads to flickering context menu during copy.
> If selection is not changed, we should not clear the task items in the
> context menu.
>
> BUG= 731483 
> TEST=manual
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
>
> Review-Url: https://codereview.chromium.org/2932803002
> Cr-Commit-Position: refs/heads/master@{#478239}
> Committed: https://chromium.googlesource.com/chromium/src/+/f8c224c31c133a07f58a8d922ec15aa730c06cde

TBR=oka@chromium.org,fukino@chromium.org,tetsui@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 731483 , 738803 

Review-Url: https://codereview.chromium.org/2973003002
Cr-Commit-Position: refs/heads/master@{#484500}

[modify] https://crrev.com/aa4c54cbd611f55c475eda159865c7134442f71c/ui/file_manager/file_manager/foreground/js/task_controller.js

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5474326ed3852a9e39a625af8c3b4f1b46ac7854

commit 5474326ed3852a9e39a625af8c3b4f1b46ac7854
Author: yamaguchi <yamaguchi@chromium.org>
Date: Thu Jul 06 09:24:15 2017

Revert of Reuse FileTasks when entries are not changed. (patchset #3 id:40001 of https://codereview.chromium.org/2833413003/ )

Reason for revert:
We'd like to revert this patch because this seem to have caused regression  crbug.com/738803 .
2973003002 needs to be merged before this CL in order to revert another dependent change first.

Original issue's description:
> Reuse FileTasks when entries are not changed.
>
> fileManagerPrivate.getFileTasks is slow.
> However, every time a file is copied during multi file copy, the method
> was called via CHANGE_THROTTLED event listener.
> This CL stops creating FileTasks again when file entries for the
> previous FileTasks are same as the current ones.
> Other possible solution to this problem is to change C++ interface of
> fileManagerPrivate.getFileTasks to support incremental update of
> FileTasks.
>
> BUG= 462989 , 712121 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
>
> Review-Url: https://codereview.chromium.org/2833413003
> Cr-Commit-Position: refs/heads/master@{#467264}
> Committed: https://chromium.googlesource.com/chromium/src/+/6e537c987f4d8cc6fcfda8f80b4c3216663371d4

TBR=fukino@chromium.org,tetsui@google.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 462989 , 712121 , 738803 

Review-Url: https://codereview.chromium.org/2966163005
Cr-Commit-Position: refs/heads/master@{#484512}

[modify] https://crrev.com/5474326ed3852a9e39a625af8c3b4f1b46ac7854/ui/file_manager/file_manager/common/js/util.js
[modify] https://crrev.com/5474326ed3852a9e39a625af8c3b4f1b46ac7854/ui/file_manager/file_manager/foreground/js/file_tasks.js
[modify] https://crrev.com/5474326ed3852a9e39a625af8c3b4f1b46ac7854/ui/file_manager/file_manager/foreground/js/task_controller.js

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de06552d022a46e64a63a0ce601cfc463710ec17

commit de06552d022a46e64a63a0ce601cfc463710ec17
Author: Tetsui Ohkubo <tetsui@chromium.org>
Date: Tue Jul 11 05:57:13 2017

Fix double click regression and restore MTP related fixes.

MTP related fixes caused regression on the behavior when multiple files
selected are double-clicked.
These MTP related fixes were reverted in:
* https://codereview.chromium.org/2973003002/
* https://codereview.chromium.org/2966163005/

This CL restores these reverted CLs while fixes double click regression.

BUG= 462989 , 712121 , 731483 , 738803 
TEST=
unit: out/Debug/browser_tests '--gtest_filter=FileManagerJsTest.TaskController'
* Verified it fails with the original patches.
manual:
* Transfer files on MTP while selecting many source files.
* Transfer files on MTP while opening context menu.
* Double click multiple selected files.

Change-Id: If61e5f306f821d0bf096c068b01866ce1cfd38c9
Reviewed-on: https://chromium-review.googlesource.com/562912
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Tetsui Ohkubo <tetsui@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485534}
[modify] https://crrev.com/de06552d022a46e64a63a0ce601cfc463710ec17/ui/file_manager/file_manager/common/js/util.js
[modify] https://crrev.com/de06552d022a46e64a63a0ce601cfc463710ec17/ui/file_manager/file_manager/foreground/js/file_tasks.js
[modify] https://crrev.com/de06552d022a46e64a63a0ce601cfc463710ec17/ui/file_manager/file_manager/foreground/js/task_controller.js
[modify] https://crrev.com/de06552d022a46e64a63a0ce601cfc463710ec17/ui/file_manager/file_manager/foreground/js/task_controller_unittest.js

Comment 9 by tetsui@chromium.org, Jul 11 2017

Status: Fixed (was: Started)

Comment 10 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment