From f6a29707b64bc2f7364f89096d187246bfc53765 Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Mon, 15 Jul 2019 09:34:20 -0700 Subject: [PATCH] feat: app.moveToApplicationsFolder conflict handling (#18916) Resolves #18805. We want to keep default move conflict handling behavior in that it's still what most users would expect, but there exist edge cases in which users may not want to be forced into that behavior. This thus introduces an optional conflict handler that allows developers access to more granular move actions. They could now allow the user to choose whether to delete an existing app in favor of the current one being moved, or whether to quit the current app and focus on the existing one should it both exist and be running. I added a fair amount of new documentation outlining this behavior, but if there are things users may benefit from seeing examples of or nuances that should be added please leave feedback! --- docs/api/app.md | 29 +++++++++- shell/browser/api/atom_api_app.cc | 1 - shell/browser/ui/cocoa/atom_bundle_mover.h | 5 ++ shell/browser/ui/cocoa/atom_bundle_mover.mm | 60 ++++++++++++++++++--- 4 files changed, 86 insertions(+), 9 deletions(-) diff --git a/docs/api/app.md b/docs/api/app.md index e4c89982b587..86c42f417341 100644 --- a/docs/api/app.md +++ b/docs/api/app.md @@ -1223,7 +1223,11 @@ This method can only be called before app is ready. Returns `Boolean` - Whether the application is currently running from the systems Application folder. Use in combination with `app.moveToApplicationsFolder()` -### `app.moveToApplicationsFolder()` _macOS_ +### `app.moveToApplicationsFolder([options])` _macOS_ + +* `options` Object (optional) + * `conflictHandler` Function (optional) - A handler for potential conflict in move failure. + * `conflictType` String - the type of move conflict encountered by the handler; can be `exists` or `existsAndRunning`, where `exists` means that an app of the same name is present in the Applications directory and `existsAndRunning` means both that it exists and that it's presently running. Returns `Boolean` - Whether the move was successful. Please note that if the move is successful, your application will quit and relaunch. @@ -1236,7 +1240,28 @@ the user to confirm the operation, you may do so using the move to fail. For instance if the user cancels the authorization dialog, this method returns false. If we fail to perform the copy, then this method will throw an error. The message in the error should be informative and tell -you exactly what went wrong +you exactly what went wrong. + +By default, if an app of the same name as the one being moved exists in the Applications directory and is _not_ running, the existing app will be trashed and the active app moved into its place. If it _is_ running, the pre-existing running app will assume focus and the the previously active app will quit itself. This behavior can be changed by providing the optional conflict handler, where the boolean returned by the handler determines whether or not the move conflict is resolved with default behavior. i.e. returning `false` will ensure no further action is taken, returning `true` will result in the default behavior and the method continuing. + +For example: + +```js +app.moveToApplicationsFolder({ + conflictHandler: (conflictType) => { + if (conflictType === 'exists') { + return dialog.showMessageBoxSync({ + type: 'question', + buttons: ['Halt Move', 'Continue Move'], + defaultId: 0, + message: 'An app of this name already exists' + }) === 1 + } + } +}) +``` + +Would mean that if an app already exists in the user directory, if the user chooses to 'Continue Move' then the function would continue with its default behavior and the existing app will be trashed and the active app moved into its place. ## Properties diff --git a/shell/browser/api/atom_api_app.cc b/shell/browser/api/atom_api_app.cc index b4fcced61fd8..cf12f417ea3d 100644 --- a/shell/browser/api/atom_api_app.cc +++ b/shell/browser/api/atom_api_app.cc @@ -1430,7 +1430,6 @@ void App::BuildPrototype(v8::Isolate* isolate, base::BindRepeating(&Browser::ResignCurrentActivity, browser)) .SetMethod("updateCurrentActivity", base::BindRepeating(&Browser::UpdateCurrentActivity, browser)) - // TODO(juturu): Remove in 2.0, deprecate before then with warnings .SetMethod("moveToApplicationsFolder", &App::MoveToApplicationsFolder) .SetMethod("isInApplicationsFolder", &App::IsInApplicationsFolder) #endif diff --git a/shell/browser/ui/cocoa/atom_bundle_mover.h b/shell/browser/ui/cocoa/atom_bundle_mover.h index 74a98b6b3e93..2ac39d289b2e 100644 --- a/shell/browser/ui/cocoa/atom_bundle_mover.h +++ b/shell/browser/ui/cocoa/atom_bundle_mover.h @@ -11,6 +11,9 @@ namespace electron { +// Possible bundle movement conflicts +enum class BundlerMoverConflictType { EXISTS, EXISTS_AND_RUNNING }; + namespace ui { namespace cocoa { @@ -21,6 +24,8 @@ class AtomBundleMover { static bool IsCurrentAppInApplicationsFolder(); private: + static bool ShouldContinueMove(BundlerMoverConflictType type, + mate::Arguments* args); static bool IsInApplicationsFolder(NSString* bundlePath); static NSString* ContainingDiskImageDevice(NSString* bundlePath); static void Relaunch(NSString* destinationPath); diff --git a/shell/browser/ui/cocoa/atom_bundle_mover.mm b/shell/browser/ui/cocoa/atom_bundle_mover.mm index 9dba077da4c6..ae322fa0341d 100644 --- a/shell/browser/ui/cocoa/atom_bundle_mover.mm +++ b/shell/browser/ui/cocoa/atom_bundle_mover.mm @@ -14,6 +14,26 @@ #import #import "shell/browser/browser.h" +#include "shell/common/native_mate_converters/once_callback.h" + +namespace mate { + +template <> +struct Converter { + static v8::Local ToV8(v8::Isolate* isolate, + electron::BundlerMoverConflictType value) { + switch (value) { + case electron::BundlerMoverConflictType::EXISTS: + return mate::StringToV8(isolate, "exists"); + case electron::BundlerMoverConflictType::EXISTS_AND_RUNNING: + return mate::StringToV8(isolate, "existsAndRunning"); + default: + return mate::StringToV8(isolate, ""); + } + } +}; + +} // namespace mate namespace electron { @@ -21,6 +41,28 @@ namespace ui { namespace cocoa { +bool AtomBundleMover::ShouldContinueMove(BundlerMoverConflictType type, + mate::Arguments* args) { + mate::Dictionary options; + bool hasOptions = args->GetNext(&options); + base::OnceCallback(BundlerMoverConflictType)> + conflict_cb; + + if (hasOptions && options.Get("conflictHandler", &conflict_cb)) { + v8::Local value = std::move(conflict_cb).Run(type); + if (value->IsBoolean()) { + if (!value.As()->Value()) + return false; + } else if (!value->IsUndefined()) { + // we only want to throw an error if a user has returned a non-boolean + // value; this allows for client-side error handling should something in + // the handler throw + args->ThrowError("Invalid conflict handler return type."); + } + } + return true; +} + bool AtomBundleMover::Move(mate::Arguments* args) { // Path of the current bundle NSString* bundlePath = [[NSBundle mainBundle] bundlePath]; @@ -74,7 +116,12 @@ bool AtomBundleMover::Move(mate::Arguments* args) { if ([fileManager fileExistsAtPath:destinationPath]) { // But first, make sure that it's not running if (IsApplicationAtPathRunning(destinationPath)) { - // Give the running app focus and terminate myself + // Check for callback handler and get user choice for open/quit + if (!ShouldContinueMove(BundlerMoverConflictType::EXISTS_AND_RUNNING, + args)) + return false; + + // Unless explicitly denied, give running app focus and terminate self [[NSTask launchedTaskWithLaunchPath:@"/usr/bin/open" arguments:[NSArray @@ -83,6 +130,11 @@ bool AtomBundleMover::Move(mate::Arguments* args) { electron::Browser::Get()->Quit(); return true; } else { + // Check callback handler and get user choice for app trashing + if (!ShouldContinueMove(BundlerMoverConflictType::EXISTS, args)) + return false; + + // Unless explicitly denied, attempt to trash old app if (!Trash([applicationsDirectory stringByAppendingPathComponent:bundleName])) { args->ThrowError("Failed to delete existing application"); @@ -313,11 +365,7 @@ bool AtomBundleMover::CopyBundle(NSString* srcPath, NSString* dstPath) { NSFileManager* fileManager = [NSFileManager defaultManager]; NSError* error = nil; - if ([fileManager copyItemAtPath:srcPath toPath:dstPath error:&error]) { - return true; - } else { - return false; - } + return [fileManager copyItemAtPath:srcPath toPath:dstPath error:&error]; } NSString* AtomBundleMover::ShellQuotedString(NSString* string) {