Skip to content

Commit

Permalink
address feedback from review
Browse files Browse the repository at this point in the history
  • Loading branch information
codebytere committed Jun 27, 2019
1 parent 4e98940 commit 7515760
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 12 deletions.
8 changes: 4 additions & 4 deletions docs/api/app.md
Expand Up @@ -1226,7 +1226,7 @@ systems Application folder. Use in combination with `app.moveToApplicationsFolde
### `app.moveToApplicationsFolder([options])` _macOS_

* `options` Object (optional)
* `conflictHandler` Function (optional) - A handler for potential conflict in move failure.
* `conflictHandler` Function<Boolean> (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
Expand All @@ -1242,20 +1242,20 @@ 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.

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 invoking the optional callback handler, where the boolean returned by the handler determines whether or not the move proceeds with its default behavior.
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') {
dialog.showMessageBoxSync({
return dialog.showMessageBoxSync({
type: 'question',
buttons: ['Halt Move', 'Continue Move'],
defaultId: 0,
message: 'An app of this name already exists'
}, response => response)
}) === 1
}
})
})
Expand Down
5 changes: 3 additions & 2 deletions shell/browser/ui/cocoa/atom_bundle_mover.h
Expand Up @@ -12,7 +12,7 @@
namespace electron {

// Possible bundle movement conflicts
enum class ConflictType { EXISTS, EXISTS_AND_RUNNING };
enum class BundlerMoverConflictType { EXISTS, EXISTS_AND_RUNNING };

namespace ui {

Expand All @@ -24,7 +24,8 @@ class AtomBundleMover {
static bool IsCurrentAppInApplicationsFolder();

private:
static bool ShouldContinueMove(ConflictType type, mate::Arguments* args);
static bool ShouldContinueMove(BundlerMoverConflictType type,
mate::Arguments* args);
static bool IsInApplicationsFolder(NSString* bundlePath);
static NSString* ContainingDiskImageDevice(NSString* bundlePath);
static void Relaunch(NSString* destinationPath);
Expand Down
14 changes: 8 additions & 6 deletions shell/browser/ui/cocoa/atom_bundle_mover.mm
Expand Up @@ -21,11 +21,11 @@
template <>
struct Converter<electron::ConflictType> {
static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
electron::ConflictType value) {
electron::BundlerMoverConflictType value) {
switch (value) {
case electron::ConflictType::EXISTS:
case electron::BundlerMoverConflictType::EXISTS:
return mate::StringToV8(isolate, "exists");
case electron::ConflictType::EXISTS_AND_RUNNING:
case electron::BundlerMoverConflictType::EXISTS_AND_RUNNING:
return mate::StringToV8(isolate, "existsAndRunning");
default:
return mate::StringToV8(isolate, "");
Expand Down Expand Up @@ -55,7 +55,8 @@ static bool FromV8(v8::Isolate* isolate,
mate::Arguments* args) {
mate::Dictionary options;
bool hasOptions = args->GetNext(&options);
base::OnceCallback<v8::MaybeLocal<v8::Value>(ConflictType)> conflict_cb;
base::OnceCallback<v8::MaybeLocal<v8::Value>(BundlerMoverConflictType)>
conflict_cb;

if (hasOptions && options.Get("conflictHandler", &conflict_cb)) {
v8::MaybeLocal<v8::Value> maybe = std::move(conflict_cb).Run(type);
Expand Down Expand Up @@ -124,7 +125,8 @@ static bool FromV8(v8::Isolate* isolate,
// But first, make sure that it's not running
if (IsApplicationAtPathRunning(destinationPath)) {
// Check for callback handler and get user choice for open/quit
if (!ShouldContinueMove(ConflictType::EXISTS_AND_RUNNING, args))
if (!ShouldContinueMove(BundlerMoverConflictType::EXISTS_AND_RUNNING,
args))
return false;

// Unless explicitly denied, give running app focus and terminate self
Expand All @@ -137,7 +139,7 @@ static bool FromV8(v8::Isolate* isolate,
return true;
} else {
// Check callback handler and get user choice for app trashing
if (!ShouldContinueMove(ConflictType::EXISTS, args))
if (!ShouldContinueMove(BundlerMoverConflictType::EXISTS, args))
return false;

// Unless explicitly denied, attempt to trash old app
Expand Down

0 comments on commit 7515760

Please sign in to comment.