From d3622f9c37e8cd94e47206bb6211346011d5f76b Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Thu, 7 Nov 2019 23:08:43 -0800 Subject: [PATCH] feat: convert shell.openItem to async shell.openPath (#20682) --- docs/api/shell.md | 9 ++- .../ui/inspectable_web_contents_impl.cc | 4 +- shell/common/api/atom_api_shell.cc | 23 +++++-- shell/common/platform_util.h | 6 +- shell/common/platform_util_linux.cc | 62 +++++++++++++------ shell/common/platform_util_mac.mm | 40 ++++++------ shell/common/platform_util_win.cc | 30 +++++++-- spec/ts-smoke/electron/main.ts | 5 +- 8 files changed, 121 insertions(+), 58 deletions(-) diff --git a/docs/api/shell.md b/docs/api/shell.md index e5bf7933809c..07f4e85b20ae 100644 --- a/docs/api/shell.md +++ b/docs/api/shell.md @@ -24,11 +24,14 @@ The `shell` module has the following methods: Show the given file in a file manager. If possible, select the file. -### `shell.openItem(fullPath)` +### `shell.openPath(path)` -* `fullPath` String +* `path` String -Returns `Boolean` - Whether the item was successfully opened. +Returns `Promise` - Resolve with an object containing the following: + +* `success` Boolean - whether or not the path was successfully opened in the desktop's default manner. +* `errorMessage` String (optional) - The error message corresponding to the failure if a failure occurred, otherwise empty string. Open the given file in the desktop's default manner. diff --git a/shell/browser/ui/inspectable_web_contents_impl.cc b/shell/browser/ui/inspectable_web_contents_impl.cc index 9a32130d505f..683fc415d743 100644 --- a/shell/browser/ui/inspectable_web_contents_impl.cc +++ b/shell/browser/ui/inspectable_web_contents_impl.cc @@ -559,7 +559,9 @@ void InspectableWebContentsImpl::ShowItemInFolder( return; base::FilePath path = base::FilePath::FromUTF8Unsafe(file_system_path); - platform_util::OpenItem(path); + + // Pass empty callback here; we can ignore errors + platform_util::OpenPath(path, platform_util::OpenCallback()); } void InspectableWebContentsImpl::SaveToFile(const std::string& url, diff --git a/shell/common/api/atom_api_shell.cc b/shell/common/api/atom_api_shell.cc index 2ce7d370e72d..dbb5557e10d9 100644 --- a/shell/common/api/atom_api_shell.cc +++ b/shell/common/api/atom_api_shell.cc @@ -44,8 +44,8 @@ struct Converter { namespace { -void OnOpenExternalFinished(gin_helper::Promise promise, - const std::string& error) { +void OnOpenFinished(gin_helper::Promise promise, + const std::string& error) { if (error.empty()) promise.Resolve(); else @@ -66,8 +66,21 @@ v8::Local OpenExternal(const GURL& url, gin::Arguments* args) { } platform_util::OpenExternal( - url, options, - base::BindOnce(&OnOpenExternalFinished, std::move(promise))); + url, options, base::BindOnce(&OnOpenFinished, std::move(promise))); + return handle; +} + +v8::Local OpenPath(v8::Isolate* isolate, + const base::FilePath& full_path) { + gin_helper::Promise promise(isolate); + v8::Local handle = promise.GetHandle(); + + platform_util::OpenPath( + full_path, + base::BindOnce( + [](gin_helper::Promise promise, + const std::string& err_msg) { promise.Resolve(err_msg); }, + std::move(promise))); return handle; } @@ -142,7 +155,7 @@ void Initialize(v8::Local exports, void* priv) { gin_helper::Dictionary dict(context->GetIsolate(), exports); dict.SetMethod("showItemInFolder", &platform_util::ShowItemInFolder); - dict.SetMethod("openItem", &platform_util::OpenItem); + dict.SetMethod("openPath", &OpenPath); dict.SetMethod("openExternal", &OpenExternal); dict.SetMethod("moveItemToTrash", &MoveItemToTrash); dict.SetMethod("beep", &platform_util::Beep); diff --git a/shell/common/platform_util.h b/shell/common/platform_util.h index 9224bfd51a07..5af45651acd2 100644 --- a/shell/common/platform_util.h +++ b/shell/common/platform_util.h @@ -19,7 +19,7 @@ class GURL; namespace platform_util { -typedef base::OnceCallback OpenExternalCallback; +typedef base::OnceCallback OpenCallback; // Show the given file in a file manager. If possible, select the file. // Must be called from the UI thread. @@ -27,7 +27,7 @@ void ShowItemInFolder(const base::FilePath& full_path); // Open the given file in the desktop's default manner. // Must be called from the UI thread. -bool OpenItem(const base::FilePath& full_path); +void OpenPath(const base::FilePath& full_path, OpenCallback callback); struct OpenExternalOptions { bool activate = true; @@ -38,7 +38,7 @@ struct OpenExternalOptions { // (For example, mailto: URLs in the default mail user agent.) void OpenExternal(const GURL& url, const OpenExternalOptions& options, - OpenExternalCallback callback); + OpenCallback callback); // Move a file to trash. bool MoveItemToTrash(const base::FilePath& full_path, bool delete_on_fail); diff --git a/shell/common/platform_util_linux.cc b/shell/common/platform_util_linux.cc index 408319e23306..6af946699416 100644 --- a/shell/common/platform_util_linux.cc +++ b/shell/common/platform_util_linux.cc @@ -19,7 +19,25 @@ namespace { -bool XDGUtil(const std::vector& argv, const bool wait_for_exit) { +// Descriptions pulled from https://linux.die.net/man/1/xdg-open +std::string GetErrorDescription(int error_code) { + switch (error_code) { + case 1: + return "Error in command line syntax"; + case 2: + return "The item does not exist"; + case 3: + return "A required tool could not be found"; + case 4: + return "The action failed"; + default: + return ""; + } +} + +bool XDGUtil(const std::vector& argv, + const bool wait_for_exit, + platform_util::OpenCallback callback) { base::LaunchOptions options; options.allow_new_privs = true; // xdg-open can fall back on mailcap which eventually might plumb through @@ -34,52 +52,56 @@ bool XDGUtil(const std::vector& argv, const bool wait_for_exit) { if (wait_for_exit) { int exit_code = -1; - if (!process.WaitForExit(&exit_code)) - return false; - return (exit_code == 0); + bool success = process.WaitForExit(&exit_code); + if (!callback.is_null()) + std::move(callback).Run(GetErrorDescription(exit_code)); + return success ? (exit_code == 0) : false; } base::EnsureProcessGetsReaped(std::move(process)); return true; } -bool XDGOpen(const std::string& path, const bool wait_for_exit) { - return XDGUtil({"xdg-open", path}, wait_for_exit); +bool XDGOpen(const std::string& path, + const bool wait_for_exit, + platform_util::OpenCallback callback) { + return XDGUtil({"xdg-open", path}, wait_for_exit, std::move(callback)); } bool XDGEmail(const std::string& email, const bool wait_for_exit) { - return XDGUtil({"xdg-email", email}, wait_for_exit); + return XDGUtil({"xdg-email", email}, wait_for_exit, + platform_util::OpenCallback()); } } // namespace namespace platform_util { -// TODO(estade): It would be nice to be able to select the file in the file -// manager, but that probably requires extending xdg-open. For now just -// show the folder. void ShowItemInFolder(const base::FilePath& full_path) { base::FilePath dir = full_path.DirName(); if (!base::DirectoryExists(dir)) return; - XDGOpen(dir.value(), false); + XDGOpen(dir.value(), false, platform_util::OpenCallback()); } -bool OpenItem(const base::FilePath& full_path) { - return XDGOpen(full_path.value(), false); +void OpenPath(const base::FilePath& full_path, OpenCallback callback) { + // This is async, so we don't care about the return value. + XDGOpen(full_path.value(), true, std::move(callback)); } void OpenExternal(const GURL& url, const OpenExternalOptions& options, - OpenExternalCallback callback) { + OpenCallback callback) { // Don't wait for exit, since we don't want to wait for the browser/email // client window to close before returning - if (url.SchemeIs("mailto")) - std::move(callback).Run(XDGEmail(url.spec(), false) ? "" - : "Failed to open"); - else - std::move(callback).Run(XDGOpen(url.spec(), false) ? "" : "Failed to open"); + if (url.SchemeIs("mailto")) { + bool success = XDGEmail(url.spec(), false); + std::move(callback).Run(success ? "" : "Failed to open path"); + } else { + bool success = XDGOpen(url.spec(), false, platform_util::OpenCallback()); + std::move(callback).Run(success ? "" : "Failed to open path"); + } } bool MoveItemToTrash(const base::FilePath& full_path, bool delete_on_fail) { @@ -111,7 +133,7 @@ bool MoveItemToTrash(const base::FilePath& full_path, bool delete_on_fail) { argv = {"gio", "trash", filename}; } - return XDGUtil(argv, true); + return XDGUtil(argv, true, platform_util::OpenCallback()); } void Beep() { diff --git a/shell/common/platform_util_mac.mm b/shell/common/platform_util_mac.mm index f141c4063178..77d610da4f78 100644 --- a/shell/common/platform_util_mac.mm +++ b/shell/common/platform_util_mac.mm @@ -58,6 +58,23 @@ NSString* GetLoginHelperBundleIdentifier() { stringByAppendingString:@".loginhelper"]; } +std::string OpenPathOnThread(const base::FilePath& full_path) { + NSString* path_string = base::SysUTF8ToNSString(full_path.value()); + NSURL* url = [NSURL fileURLWithPath:path_string]; + if (!url) + return "Invalid path"; + + const NSWorkspaceLaunchOptions launch_options = + NSWorkspaceLaunchAsync | NSWorkspaceLaunchWithErrorPresentation; + BOOL success = [[NSWorkspace sharedWorkspace] openURLs:@[ url ] + withAppBundleIdentifier:nil + options:launch_options + additionalEventParamDescriptor:nil + launchIdentifiers:NULL]; + + return success ? "" : "Failed to open path"; +} + } // namespace namespace platform_util { @@ -75,28 +92,13 @@ void ShowItemInFolder(const base::FilePath& path) { } } -bool OpenItem(const base::FilePath& full_path) { - DCHECK([NSThread isMainThread]); - NSString* path_string = base::SysUTF8ToNSString(full_path.value()); - if (!path_string) - return false; - - NSURL* url = [NSURL fileURLWithPath:path_string]; - if (!url) - return false; - - const NSWorkspaceLaunchOptions launch_options = - NSWorkspaceLaunchAsync | NSWorkspaceLaunchWithErrorPresentation; - return [[NSWorkspace sharedWorkspace] openURLs:@[ url ] - withAppBundleIdentifier:nil - options:launch_options - additionalEventParamDescriptor:nil - launchIdentifiers:NULL]; +void OpenPath(const base::FilePath& full_path, OpenCallback callback) { + std::move(callback).Run(OpenPathOnThread(full_path)); } void OpenExternal(const GURL& url, const OpenExternalOptions& options, - OpenExternalCallback callback) { + OpenCallback callback) { DCHECK([NSThread isMainThread]); NSURL* ns_url = net::NSURLWithGURL(url); if (!ns_url) { @@ -105,7 +107,7 @@ void OpenExternal(const GURL& url, } bool activate = options.activate; - __block OpenExternalCallback c = std::move(callback); + __block OpenCallback c = std::move(callback); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ __block std::string error = OpenURL(ns_url, activate); diff --git a/shell/common/platform_util_win.cc b/shell/common/platform_util_win.cc index b82b3bca747d..5a606fbc4b4a 100644 --- a/shell/common/platform_util_win.cc +++ b/shell/common/platform_util_win.cc @@ -30,6 +30,7 @@ #include "base/win/scoped_com_initializer.h" #include "base/win/windows_version.h" #include "content/public/browser/browser_task_traits.h" +#include "content/public/browser/browser_thread.h" #include "ui/base/win/shell.h" #include "url/gurl.h" @@ -303,6 +304,19 @@ void ShowItemInFolderOnWorkerThread(const base::FilePath& full_path) { } } +std::string OpenPathOnThread(const base::FilePath& full_path) { + // May result in an interactive dialog. + base::ScopedBlockingCall scoped_blocking_call(FROM_HERE, + base::BlockingType::MAY_BLOCK); + bool success; + if (base::DirectoryExists(full_path)) + success = ui::win::OpenFolderViaShell(full_path); + else + success = ui::win::OpenFileViaShell(full_path); + + return success ? "" : "Failed to open path"; +} + } // namespace namespace platform_util { @@ -314,16 +328,20 @@ void ShowItemInFolder(const base::FilePath& full_path) { base::BindOnce(&ShowItemInFolderOnWorkerThread, full_path)); } -bool OpenItem(const base::FilePath& full_path) { - if (base::DirectoryExists(full_path)) - return ui::win::OpenFolderViaShell(full_path); - else - return ui::win::OpenFileViaShell(full_path); +void OpenPath(const base::FilePath& full_path, OpenCallback callback) { + DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + + base::PostTaskAndReplyWithResult( + base::CreateCOMSTATaskRunner({base::ThreadPool(), base::MayBlock(), + base::TaskPriority::USER_BLOCKING}) + .get(), + FROM_HERE, base::BindOnce(&OpenPathOnThread, full_path), + std::move(callback)); } void OpenExternal(const GURL& url, const OpenExternalOptions& options, - OpenExternalCallback callback) { + OpenCallback callback) { base::PostTaskAndReplyWithResult( base::CreateCOMSTATaskRunner({base::ThreadPool(), base::MayBlock(), base::TaskPriority::USER_BLOCKING}) diff --git a/spec/ts-smoke/electron/main.ts b/spec/ts-smoke/electron/main.ts index edbf40804d3e..4a3170a47b0c 100644 --- a/spec/ts-smoke/electron/main.ts +++ b/spec/ts-smoke/electron/main.ts @@ -1056,9 +1056,12 @@ app.on('ready', () => { // https://github.com/atom/electron/blob/master/docs/api/shell.md shell.showItemInFolder('/home/user/Desktop/test.txt') -shell.openItem('/home/user/Desktop/test.txt') shell.moveItemToTrash('/home/user/Desktop/test.txt') +shell.openPath('/home/user/Desktop/test.txt').then(err => { + if (err) console.log(err) +}) + shell.openExternal('https://github.com', { activate: false }).then(() => {})