From f7c1a41ab8a29c3c5ae2d978cb5e2315df3100d9 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 9 Aug 2019 08:15:00 -0700 Subject: [PATCH 1/3] feat: macOS removal fallback when moveItemToTrash fails --- docs/api/shell.md | 5 +++-- shell/common/platform_util.h | 3 ++- shell/common/platform_util_linux.cc | 2 +- shell/common/platform_util_mac.mm | 34 +++++++++++++++++++++++------ shell/common/platform_util_win.cc | 2 +- 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/docs/api/shell.md b/docs/api/shell.md index 6f6b9306fe28f..e5bf7933809cc 100644 --- a/docs/api/shell.md +++ b/docs/api/shell.md @@ -43,11 +43,12 @@ Returns `Promise` Open the given external protocol URL in the desktop's default manner. (For example, mailto: URLs in the user's default mail agent). -### `shell.moveItemToTrash(fullPath)` +### `shell.moveItemToTrash(fullPath[, deleteOnFail])` * `fullPath` String +* `deleteOnFail` Boolean (optional) - Whether or not to unilaterally remove the item if the Trash is disabled or unsupported on the volume. _macOS_ -Returns `Boolean` - Whether the item was successfully moved to the trash. +Returns `Boolean` - Whether the item was successfully moved to the trash or otherwise deleted. Move the given file to trash and returns a boolean status for the operation. diff --git a/shell/common/platform_util.h b/shell/common/platform_util.h index 2d196e3271492..9e62a39f3b250 100644 --- a/shell/common/platform_util.h +++ b/shell/common/platform_util.h @@ -10,6 +10,7 @@ #include "base/callback_forward.h" #include "base/files/file_path.h" #include "build/build_config.h" +#include "native_mate/arguments.h" #if defined(OS_WIN) #include "base/strings/string16.h" @@ -41,7 +42,7 @@ void OpenExternal(const GURL& url, OpenExternalCallback callback); // Move a file to trash. -bool MoveItemToTrash(const base::FilePath& full_path); +bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args); void Beep(); diff --git a/shell/common/platform_util_linux.cc b/shell/common/platform_util_linux.cc index aaba6c8651a9a..87adc9f95a7e3 100644 --- a/shell/common/platform_util_linux.cc +++ b/shell/common/platform_util_linux.cc @@ -82,7 +82,7 @@ void OpenExternal(const GURL& url, std::move(callback).Run(XDGOpen(url.spec(), false) ? "" : "Failed to open"); } -bool MoveItemToTrash(const base::FilePath& full_path) { +bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { std::unique_ptr env(base::Environment::Create()); // find the trash method diff --git a/shell/common/platform_util_mac.mm b/shell/common/platform_util_mac.mm index cd11c050afeb2..079c79905e603 100644 --- a/shell/common/platform_util_mac.mm +++ b/shell/common/platform_util_mac.mm @@ -115,16 +115,36 @@ void OpenExternal(const GURL& url, }); } -bool MoveItemToTrash(const base::FilePath& full_path) { +bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { NSString* path_string = base::SysUTF8ToNSString(full_path.value()); - BOOL status = [[NSFileManager defaultManager] - trashItemAtURL:[NSURL fileURLWithPath:path_string] - resultingItemURL:nil - error:nil]; - if (!path_string || !status) + if (!path_string) { + args->ThrowError("moveItemToTrash(): Invalid path."); + return false; + } + + NSURL* url = [NSURL fileURLWithPath:path_string]; + NSError* err = nil; + BOOL did_trash = [[NSFileManager defaultManager] trashItemAtURL:url + resultingItemURL:nil + error:&err]; + + bool delete_on_fail = false; + if (args->GetNext(&delete_on_fail) && delete_on_fail) { + // Some volumes may not support a Trash folder or it may be disabled + // so these methods will report failure by returning NO or nil and + // an NSError with NSFeatureUnsupportedError. + // Handle this by deleting the item as a fallback. + if (!did_trash && [err code] == NSFeatureUnsupportedError) { + did_trash = [[NSFileManager defaultManager] removeItemAtURL:url + error:nil]; + } + } + + if (!did_trash) LOG(WARNING) << "NSWorkspace failed to move file " << full_path.value() << " to trash"; - return status; + + return did_trash; } void Beep() { diff --git a/shell/common/platform_util_win.cc b/shell/common/platform_util_win.cc index 3021c2e1fb367..fc66398b2a9d7 100644 --- a/shell/common/platform_util_win.cc +++ b/shell/common/platform_util_win.cc @@ -335,7 +335,7 @@ void OpenExternal(const GURL& url, std::move(callback)); } -bool MoveItemToTrash(const base::FilePath& path) { +bool MoveItemToTrash(const base::FilePath& path, mate::Arguments* args) { base::win::ScopedCOMInitializer com_initializer; if (!com_initializer.Succeeded()) return false; From 6117bfe737b488bc78392520792dd77fb908cbbe Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Fri, 9 Aug 2019 18:55:51 -0700 Subject: [PATCH 2/3] platform_util shouldn't know about mate::Arguments --- shell/common/api/atom_api_shell.cc | 9 ++++++++- shell/common/platform_util.h | 3 +-- shell/common/platform_util_linux.cc | 2 +- shell/common/platform_util_mac.mm | 7 +++---- shell/common/platform_util_win.cc | 2 +- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/shell/common/api/atom_api_shell.cc b/shell/common/api/atom_api_shell.cc index 6a226a888d5ca..93f673f0e329e 100644 --- a/shell/common/api/atom_api_shell.cc +++ b/shell/common/api/atom_api_shell.cc @@ -71,6 +71,13 @@ v8::Local OpenExternal(const GURL& url, mate::Arguments* args) { return handle; } +bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { + bool delete_on_fail = false; + args->GetNext(&delete_on_fail); + + return platform_util::MoveItemToTrash(full_path, delete_on_fail); +} + #if defined(OS_WIN) bool WriteShortcutLink(const base::FilePath& shortcut_path, mate::Arguments* args) { @@ -134,7 +141,7 @@ void Initialize(v8::Local exports, dict.SetMethod("showItemInFolder", &platform_util::ShowItemInFolder); dict.SetMethod("openItem", &platform_util::OpenItem); dict.SetMethod("openExternal", &OpenExternal); - dict.SetMethod("moveItemToTrash", &platform_util::MoveItemToTrash); + dict.SetMethod("moveItemToTrash", &MoveItemToTrash); dict.SetMethod("beep", &platform_util::Beep); #if defined(OS_WIN) dict.SetMethod("writeShortcutLink", &WriteShortcutLink); diff --git a/shell/common/platform_util.h b/shell/common/platform_util.h index 9e62a39f3b250..9224bfd51a070 100644 --- a/shell/common/platform_util.h +++ b/shell/common/platform_util.h @@ -10,7 +10,6 @@ #include "base/callback_forward.h" #include "base/files/file_path.h" #include "build/build_config.h" -#include "native_mate/arguments.h" #if defined(OS_WIN) #include "base/strings/string16.h" @@ -42,7 +41,7 @@ void OpenExternal(const GURL& url, OpenExternalCallback callback); // Move a file to trash. -bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args); +bool MoveItemToTrash(const base::FilePath& full_path, bool delete_on_fail); void Beep(); diff --git a/shell/common/platform_util_linux.cc b/shell/common/platform_util_linux.cc index 87adc9f95a7e3..900f90cae1a57 100644 --- a/shell/common/platform_util_linux.cc +++ b/shell/common/platform_util_linux.cc @@ -82,7 +82,7 @@ void OpenExternal(const GURL& url, std::move(callback).Run(XDGOpen(url.spec(), false) ? "" : "Failed to open"); } -bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { +bool MoveItemToTrash(const base::FilePath& full_path, bool delete_on_fail) { std::unique_ptr env(base::Environment::Create()); // find the trash method diff --git a/shell/common/platform_util_mac.mm b/shell/common/platform_util_mac.mm index 079c79905e603..f141c4063178c 100644 --- a/shell/common/platform_util_mac.mm +++ b/shell/common/platform_util_mac.mm @@ -115,10 +115,10 @@ void OpenExternal(const GURL& url, }); } -bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { +bool MoveItemToTrash(const base::FilePath& full_path, bool delete_on_fail) { NSString* path_string = base::SysUTF8ToNSString(full_path.value()); if (!path_string) { - args->ThrowError("moveItemToTrash(): Invalid path."); + LOG(WARNING) << "Invalid file path " << full_path.value(); return false; } @@ -128,8 +128,7 @@ bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { resultingItemURL:nil error:&err]; - bool delete_on_fail = false; - if (args->GetNext(&delete_on_fail) && delete_on_fail) { + if (delete_on_fail) { // Some volumes may not support a Trash folder or it may be disabled // so these methods will report failure by returning NO or nil and // an NSError with NSFeatureUnsupportedError. diff --git a/shell/common/platform_util_win.cc b/shell/common/platform_util_win.cc index fc66398b2a9d7..87e389ddd54ac 100644 --- a/shell/common/platform_util_win.cc +++ b/shell/common/platform_util_win.cc @@ -335,7 +335,7 @@ void OpenExternal(const GURL& url, std::move(callback)); } -bool MoveItemToTrash(const base::FilePath& path, mate::Arguments* args) { +bool MoveItemToTrash(const base::FilePath& path, bool delete_on_fail) { base::win::ScopedCOMInitializer com_initializer; if (!com_initializer.Succeeded()) return false; From 4e4e6139ae9b29563a746326a9c20101120da77e Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 12 Aug 2019 10:01:04 -0700 Subject: [PATCH 3/3] pull full_path from args as well --- shell/common/api/atom_api_shell.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/shell/common/api/atom_api_shell.cc b/shell/common/api/atom_api_shell.cc index 93f673f0e329e..14d9b98d5bba8 100644 --- a/shell/common/api/atom_api_shell.cc +++ b/shell/common/api/atom_api_shell.cc @@ -71,7 +71,10 @@ v8::Local OpenExternal(const GURL& url, mate::Arguments* args) { return handle; } -bool MoveItemToTrash(const base::FilePath& full_path, mate::Arguments* args) { +bool MoveItemToTrash(mate::Arguments* args) { + base::FilePath full_path; + args->GetNext(&full_path); + bool delete_on_fail = false; args->GetNext(&delete_on_fail);