Skip to content

Commit

Permalink
[M110] [FilesTrash] Only show permanent delete dialog on trashable fi…
Browse files Browse the repository at this point in the history
…lesystems

There are potentially other filesystems (other than Drive) that don't
perform a permanent delete of their files. Given this set is not easily
defined (e.g. FSPs can be created at any time with a backend that has
unknown behaviour) thus change the fallback logic of the delete dialog
to occur whenever the underlying filesystem is not enabled for trash.

(cherry picked from commit 36463ec)

Bug: b:265353795
Test: browser_tests --gtest_filter=*trash*:*tooltip*
Change-Id: I3f35fa764e45d601798c53045cb354485c41c0f2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4170029
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Ben Reich <benreich@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1093159}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4190697
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Auto-Submit: Ben Reich <benreich@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#703}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
ben-reich authored and Chromium LUCI CQ committed Jan 27, 2023
1 parent c85e7e2 commit cf43e73
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 40 deletions.
17 changes: 17 additions & 0 deletions ui/file_manager/file_manager/common/js/trash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,23 @@ export function isAllTrashEntries(
});
}

/**
* Returns true if all supplied entries are on a volume that has trash enabled.
*/
export function isAllEntriesOnTrashEnabledVolumes(
entries: FileSystemEntry[], volumeManager: VolumeManager): boolean {
const enabledTrashVolumeURLs =
getEnabledTrashVolumeURLs(volumeManager, /*includeTrashPath=*/ false);
return entries.every((e: FileSystemEntry) => {
for (const volumeURL of enabledTrashVolumeURLs) {
if (e.toURL().startsWith(volumeURL)) {
return true;
}
}
return false;
});
}

/**
* Returns true if all entries are on a trashable volume and they aren't already
* trashed.
Expand Down
41 changes: 21 additions & 20 deletions ui/file_manager/file_manager/foreground/js/file_manager_commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {DialogType, isModal} from '../../common/js/dialog_type.js';
import {FileType} from '../../common/js/file_type.js';
import {EntryList} from '../../common/js/files_app_entry_types.js';
import {metrics} from '../../common/js/metrics.js';
import {RestoreFailedType, RestoreFailedTypesUMA, RestoreFailedUMA, shouldMoveToTrash, TrashEntry} from '../../common/js/trash.js';
import {isAllEntriesOnTrashEnabledVolumes, RestoreFailedType, RestoreFailedTypesUMA, RestoreFailedUMA, shouldMoveToTrash, TrashEntry} from '../../common/js/trash.js';
import {str, strf, util} from '../../common/js/util.js';
import {VolumeManagerCommon} from '../../common/js/volume_manager_types.js';
import {NudgeType} from '../../containers/nudge_container.js';
Expand Down Expand Up @@ -1233,28 +1233,29 @@ CommandHandler.deleteCommand_ = new (class extends FilesCommand {
dialogDoneCallback();
};

// When a user deletes a file from Drive, it gets moved to the Google Drive
// trash. This means the file is not technically permanently removed.
// Fallback to the delete text that doesn't specify permanent delete in this
// case.
if (fileManager.directoryModel.isOnDrive()) {
const deleteMessage = entries.length === 1 ?
strf('CONFIRM_DELETE_ONE', entries[0].name) :
strf('CONFIRM_DELETE_SOME', entries.length);
dialog.setOkLabel(str('DELETE_BUTTON_LABEL'));
dialog.show(deleteMessage, deleteAction, cancelAction, null);
// Files that are deleted from locations that are trash enabled should
// instead show copy indicating the files will be permanently deleted. For
// all other filesystem the permanent deletion can't necessarily be verified
// (e.g. a copy may be moved to the underlying filesystems version of
// trash).
if (isAllEntriesOnTrashEnabledVolumes(entries, fileManager.volumeManager)) {
const title = entries.length === 1 ?
strf('CONFIRM_PERMANENTLY_DELETE_ONE_TITLE') :
strf('CONFIRM_PERMANENTLY_DELETE_SOME_TITLE');

const message = entries.length === 1 ?
strf('CONFIRM_PERMANENTLY_DELETE_ONE_DESC', entries[0].name) :
strf('CONFIRM_PERMANENTLY_DELETE_SOME_DESC', entries.length);

dialog.setOkLabel(str('PERMANENTLY_DELETE_FOREVER'));
dialog.showWithTitle(title, message, deleteAction, cancelAction, null);
return;
}

const title = entries.length === 1 ?
strf('CONFIRM_PERMANENTLY_DELETE_ONE_TITLE') :
strf('CONFIRM_PERMANENTLY_DELETE_SOME_TITLE');

const message = entries.length === 1 ?
strf('CONFIRM_PERMANENTLY_DELETE_ONE_DESC', entries[0].name) :
strf('CONFIRM_PERMANENTLY_DELETE_SOME_DESC', entries.length);

dialog.showWithTitle(title, message, deleteAction, cancelAction, null);
const deleteMessage = entries.length === 1 ?
strf('CONFIRM_DELETE_ONE', entries[0].name) :
strf('CONFIRM_DELETE_SOME', entries.length);
dialog.show(deleteMessage, deleteAction, cancelAction, null);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class FileManagerUI {
* @const
*/
this.deleteConfirmDialog = new FilesConfirmDialog(this.element);
this.deleteConfirmDialog.setOkLabel(str('PERMANENTLY_DELETE_FOREVER'));
this.deleteConfirmDialog.setOkLabel(str('DELETE_BUTTON_LABEL'));
this.deleteConfirmDialog.focusCancelButton = true;

/**
Expand Down
4 changes: 2 additions & 2 deletions ui/file_manager/integration_tests/file_manager/quick_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -3078,7 +3078,7 @@ testcase.openQuickViewAndDeleteSingleSelection = async () => {
let deleteDialogButton = ['#quick-view', '.cr-dialog-ok:not([hidden])'];
deleteDialogButton =
await remoteCall.waitAndClickElement(appId, deleteDialogButton);
chrome.test.assertEq('Delete', deleteDialogButton.text);
chrome.test.assertEq('Delete forever', deleteDialogButton.text);
}

// Check: |hello.txt| should have been deleted.
Expand Down Expand Up @@ -3137,7 +3137,7 @@ testcase.openQuickViewAndDeleteCheckSelection = async () => {
let deleteDialogButton = ['#quick-view', '.cr-dialog-ok:not([hidden])'];
deleteDialogButton =
await remoteCall.waitAndClickElement(appId, deleteDialogButton);
chrome.test.assertEq('Delete', deleteDialogButton.text);
chrome.test.assertEq('Delete forever', deleteDialogButton.text);
}

// Check: |hello.txt| should have been deleted.
Expand Down
30 changes: 13 additions & 17 deletions ui/file_manager/integration_tests/file_manager/trash.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,10 @@ async function clickDeleteButton(appId) {
}

/**
* Clicks the delete button and confirms the deletion.
* Confirm the deletion happens and assert the dialog has the correct text.
* @param {string} appId
*/
async function clickDeleteButtonAndConfirmDeletion(appId) {
await clickDeleteButton(appId);

async function confirmPermanentDeletion(appId) {
// Check: the delete confirm dialog should appear.
await remoteCall.waitForElement(appId, '.cr-dialog-container.shown');

Expand All @@ -57,6 +55,15 @@ async function clickDeleteButtonAndConfirmDeletion(appId) {
appId, '#file-list [file-name="hello.txt"]');
}

/**
* Clicks the delete button and confirms the deletion.
* @param {string} appId
*/
async function clickDeleteButtonAndConfirmDeletion(appId) {
await clickDeleteButton(appId);
await confirmPermanentDeletion(appId);
}

/**
* Shows hidden files to facilitate tests again the .Trash directory.
* @param {string} appId
Expand Down Expand Up @@ -163,12 +170,9 @@ testcase.trashPermanentlyDelete = async () => {
chrome.test.assertTrue(
await remoteCall.callRemoteTestUtil('fakeKeyDown', appId, shiftDeleteKey),
'Pressing Shift+Delete failed.');
await remoteCall.waitAndClickElement(
appId, '.files-confirm-dialog .cr-dialog-ok');

// Wait for completion of file deletion.
await remoteCall.waitForElementLost(
appId, '#file-list [file-name="hello.txt"]');
// Confirm the permanent deletion of the "hello.txt" file.
await confirmPermanentDeletion(appId);
};

/**
Expand Down Expand Up @@ -209,10 +213,6 @@ testcase.trashDeleteFromTrashOriginallyFromMyFiles = async () => {

// Delete selected item.
await clickDeleteButtonAndConfirmDeletion(appId);

// Wait for completion of file deletion.
await remoteCall.waitForElementLost(
appId, '#file-list [file-name="hello.txt"]');
};

/**
Expand Down Expand Up @@ -417,10 +417,6 @@ testcase.trashDeleteFromTrash = async () => {

// Delete selected item.
await clickDeleteButtonAndConfirmDeletion(appId);

// Wait for completion of file deletion.
await remoteCall.waitForElementLost(
appId, '#file-list [file-name="hello.txt"]');
};

/**
Expand Down

0 comments on commit cf43e73

Please sign in to comment.