Double-click opens either the selection or a single file indeterministically |
|||||
Issue descriptionChrome 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.
,
Jul 6 2017
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
,
Jul 6 2017
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
,
Jul 6 2017
,
Jul 6 2017
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 .
,
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
,
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
,
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
,
Jul 11 2017
,
Jan 22 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by yamaguchi@chromium.org
, Jul 3 2017Summary: 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.