New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 758677 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug


Participants' hotlists:
Hotlist-Recent


Sign in to add a comment

Recent folder gets crowded by Drive files modified by someone else

Project Member Reported by mcirimele@chromium.org, Aug 24 2017

Issue description

What steps will reproduce the problem?
(1) Have a Drive folder set up in Files app that contains shared documents
(2) Go to "recent" folder after not using your Chromebook for a while.

What is the expected result?
Files shown are ones I have recently modified.

What happens instead?
Recent folder is full of files that I haven't modified or opened (some I don't even recognize). 

Is there a way to modify the rules for what is shown in "recent" to only include files modified by me? 
 

Comment 1 by nya@chromium.org, Aug 25 2017

Thanks for the report, I think this is rather a high-priority item. However resolving this is tricky because we need to fix both backend and frontend.

Backend: Drive files should be retrieved by last-modified-by-me order. However seems like Drive Metadata in Chrome OS do not track last-modified-by-me at this moment.
https://cs.chromium.org/chromium/src/components/drive/chromeos/search_metadata.cc?rcl=1bdf82f83c51a1f42672bc8e59494d238269fc6f&l=45
https://cs.chromium.org/chromium/src/components/drive/drive.proto?rcl=1bdf82f83c51a1f42672bc8e59494d238269fc6f&l=15


Frontend: We need to sort files in last-modified-by-me order. Do we need to add a new column for it?

Comment 2 by nya@chromium.org, Aug 25 2017

Labels: -Pri-3 Pri-1
Increasing priority. I think we should seriously consider putting this in M61 as a bug-fix.

Comment 3 by nya@chromium.org, Aug 28 2017

Status: Started (was: Untriaged)
Regarding comment #1 I don't think we need add a new column for "last-modified-by-me". 

Right now I can think of two options:
1. We sort by "last-modified-by-me" but keep the columns as is. If the user clicks one of the column titles we would re-sort based on that column. In this option there would be no way to go back to the "last-modified-by-me" sort other than going to a different folder and returning to "Recent". 

2. Replace sorting of the "Date modified" column to actually be "last-modified-by-me" but keep the label as is. This is what "Date modified" means in Downloads already but will create inconsistency with Drive. 

Out of these two I propose we go with #2 but I don't think it's ideal and if we can think of better solutions that would be great. 

Comment 5 by nya@chromium.org, Aug 30 2017

Labels: -M-63 M-62
Discussed offline with PM/UX and decided to prioritize this issue.

#c4:

fukino@ and I was also discussing about the option 2 primarily because adding new columns to Files app is not easy for now. Let's go with that option.

Comment 6 by nya@chromium.org, Aug 30 2017

FYI: I'm sending out following patches for reviews which will resolve this issue.

drive: Increment the DB version to 14.
https://chromium-review.googlesource.com/c/chromium/src/+/640435

recent: Store modifiedByMeDate in local Drive database.
https://chromium-review.googlesource.com/c/chromium/src/+/641735

recent: Use last-modified-by-me date in Drive source.
https://chromium-review.googlesource.com/c/chromium/src/+/641173

recent: Return RecentFile from RecentModel.
https://chromium-review.googlesource.com/c/chromium/src/+/641693

recent: Use last-modified-by-me time in Recent root.
https://chromium-review.googlesource.com/c/chromium/src/+/642431

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 30 2017

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

commit 11eb9b4f1e95753d0f153b929b1f2e6888d3c712
Author: Shuhei Takahashi <nya@chromium.org>
Date: Wed Aug 30 07:09:21 2017

recent: Return RecentFile from RecentModel.

This patch makes RecentModel::GetRecentFiles() return RecentFile instead
of FileSystemURL to allow the caller to know the last modified dates.

Bug:  758677 
Test: unit_tests
Change-Id: Ide6b9e4315af2f75607eb9b42ad4b122315ec6c5
Reviewed-on: https://chromium-review.googlesource.com/641693
Reviewed-by: Tomasz Mikolajewski <mtomasz@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498385}
[modify] https://crrev.com/11eb9b4f1e95753d0f153b929b1f2e6888d3c712/chrome/browser/chromeos/extensions/file_manager/private_api_misc.cc
[modify] https://crrev.com/11eb9b4f1e95753d0f153b929b1f2e6888d3c712/chrome/browser/chromeos/extensions/file_manager/private_api_misc.h
[modify] https://crrev.com/11eb9b4f1e95753d0f153b929b1f2e6888d3c712/chrome/browser/chromeos/fileapi/recent_model.cc
[modify] https://crrev.com/11eb9b4f1e95753d0f153b929b1f2e6888d3c712/chrome/browser/chromeos/fileapi/recent_model.h
[modify] https://crrev.com/11eb9b4f1e95753d0f153b929b1f2e6888d3c712/chrome/browser/chromeos/fileapi/recent_model_unittest.cc

Comment 8 by nya@chromium.org, Aug 30 2017

Cc: weifangsun@chromium.org
Owner: nya@chromium.org

Comment 9 by nya@chromium.org, Sep 4 2017

Cc: hashimoto@chromium.org
Owner: weifangsun@chromium.org
+hashimoto, an expert on Drive backend in Chrome OS

During the review of https://chromium-review.googlesource.com/c/chromium/src/+/640435, hashimoto@ expressed a concern about cherry-picking fixes to this issue in M62.

The problem is that, though Google Drive API provides last-modified-by-me timestamp, currently Drive backend in Chrome OS does not save the timestamp in local database. Thus, if we want to use the timestamp to sort Drive files in Recent, we need to change the local database schema and trigger refresh of the local database.

However changing the database schema needs special care, especially on stable/beta channels where data corruption should never happen (the situation would be different in dev). In particular, database schema change must not be reverted on release branches because current database code does not support forward compatibility and all data will be reverted if the database version is unknown to the code.

So he suggested two alternative options:
 1. Use a different timestamp than last-modified-by-me, e.g. last-accessed-by-me.
 2. Punt this fix to M63 to have more testing on dev channel.

I'm hesitant with option 1 because Recent's logic will get far difficult to understand by users. I'm kind of okay with option 2, though I think it's really great to have this issue fixed in M62.

I want to hear opinion from weifangsun@.

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 5 2017

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

commit 87ce7f50c65807a549c0a207709c75cf7671780f
Author: Shuhei Takahashi <nya@chromium.org>
Date: Tue Sep 05 05:15:41 2017

drive: Increment the DB version to 14.

Since v13, two modifications to the database were done without bumping
the version, making it harder to add further migrations:

  1. Delete orphan entries ( crbug.com/374648 ).
  2. Trigger refresh to fill |starred| field ( crbug.com/646839 ).

This change bumps the DB version from 13 to 14 and performs two
migration above.

Bug:  758677 
Test: unit_tests
Change-Id: Iba3fa84bf187cd8f4865466a6f5f5397802f265e
Reviewed-on: https://chromium-review.googlesource.com/640435
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499561}
[modify] https://crrev.com/87ce7f50c65807a549c0a207709c75cf7671780f/components/drive/drive.proto
[modify] https://crrev.com/87ce7f50c65807a549c0a207709c75cf7671780f/components/drive/resource_metadata_storage.cc
[modify] https://crrev.com/87ce7f50c65807a549c0a207709c75cf7671780f/components/drive/resource_metadata_storage.h
[modify] https://crrev.com/87ce7f50c65807a549c0a207709c75cf7671780f/components/drive/resource_metadata_storage_unittest.cc

Cc: -nya@chromium.org
Owner: nya@chromium.org
nya@ - Thanks for the update and details. I agree with your thoughts on this. While it would be great to get a fix into M62, given the potential risk of the cherry pick, I'd rather punt the fix to M63 to ensure we keep M62 stable and are able to test the update in M63 more thoroughly.
Labels: OS-Chrome
Labels: -M-62 M-63
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 15 2017

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

commit a25ace5694e61cdf5403c20edbbc361897f582f2
Author: Shuhei Takahashi <nya@chromium.org>
Date: Fri Sep 15 09:22:17 2017

recent: Store modifiedByMeDate in local Drive database.

In order to trigger refresh of local Drive metadata, DB version is
incremented from 14 to 15.

New timestamps will be used in Recent root.

Bug:  758677 
Test: unit_tests
Change-Id: I51e85ce10d1777e1a0c0bb928c3da92faae376fb
Reviewed-on: https://chromium-review.googlesource.com/641735
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502215}
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/chrome/browser/ui/webui/chromeos/drive_internals_ui.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_cache.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_cache_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_system/copy_operation.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_system/copy_operation.h
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_system/create_directory_operation.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_system/create_file_operation.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/chromeos/file_system/touch_operation.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/drive.proto
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/file_system/copy_operation_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/file_system/create_directory_operation_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/file_system/create_file_operation_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/file_system/touch_operation_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/file_system_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/resource_entry_conversion.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/resource_entry_conversion_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/resource_metadata_storage.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/resource_metadata_storage.h
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/service/drive_api_service.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/service/fake_drive_service.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/service/fake_drive_service.h
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/components/drive/service/fake_drive_service_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/google_apis/drive/drive_api_parser.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/google_apis/drive/drive_api_parser.h
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/google_apis/drive/drive_api_parser_unittest.cc
[modify] https://crrev.com/a25ace5694e61cdf5403c20edbbc361897f582f2/google_apis/test/data/drive/filelist.json

Project Member

Comment 15 by bugdroid1@chromium.org, Sep 26 2017

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

commit 6a8b4f5f8632ca2af664245583f6c780e2d4d320
Author: Shuhei Takahashi <nya@chromium.org>
Date: Tue Sep 26 03:01:09 2017

recent: Use modificationByMeDate in Recent root.

This change adds MetadataModel.setUseModificationByMeTime() to ask it to
override modificationDate field in MetadataItem with
modificationByMeDate.

This function is called on changing the current directory, and set to
true when Recent root is being opened.

Bug:  758677 
Test: Last modified column in Recent is replaced
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I838e7ab5b46d4a4962becea60a2225531ba67363
Reviewed-on: https://chromium-review.googlesource.com/676506
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Tomasz Mikolajewski <mtomasz@chromium.org>
Commit-Queue: Shuhei Takahashi <nya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504269}
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/chrome/common/extensions/api/file_manager_private.idl
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/third_party/closure_compiler/externs/file_manager_private.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/compiled_resources2.gyp
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/constants.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/file_list_model.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/file_manager.js
[add] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/last_modified_controller.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/main_scripts.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/metadata/external_metadata_provider.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/metadata/metadata_item.js
[modify] https://crrev.com/6a8b4f5f8632ca2af664245583f6c780e2d4d320/ui/file_manager/file_manager/foreground/js/ui/file_table.js

Comment 16 by nya@chromium.org, Sep 26 2017

Status: Fixed (was: Started)

Sign in to add a comment