Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New makeSingleInstance API #12782

Merged
merged 7 commits into from May 7, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 28 additions & 12 deletions atom/browser/api/atom_api_app.cc
Expand Up @@ -350,8 +350,8 @@ struct Converter<content::CertificateRequestResultType> {
namespace atom {

ProcessMetric::ProcessMetric(int type,
base::ProcessId pid,
std::unique_ptr<base::ProcessMetrics> metrics) {
base::ProcessId pid,
std::unique_ptr<base::ProcessMetrics> metrics) {
this->type = type;
this->pid = pid;
this->metrics = std::move(metrics);
Expand Down Expand Up @@ -422,7 +422,9 @@ int GetPathConstant(const std::string& name) {
}

bool NotificationCallbackWrapper(
const ProcessSingleton::NotificationCallback& callback,
const base::Callback<
void(const base::CommandLine::StringVector& command_line,
const base::FilePath& current_directory)> callback,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- current_directory)> callback,
+ current_directory)>& callback,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel bad for losing that... 😆

const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd) {
// Make sure the callback is called after app gets ready.
Expand Down Expand Up @@ -864,30 +866,43 @@ std::string App::GetLocale() {
return g_browser_process->GetApplicationLocale();
}

bool App::MakeSingleInstance(
const ProcessSingleton::NotificationCallback& callback) {
void App::OnSecondInstance(const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd) {
Emit("second-instance", cmd, cwd);
}

bool App::HasSingleInstanceLock() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not to beat a dead horse, but what is the use case for this method being public API? 😃

I'm not completely opposed to this; more like I need help understanding who would use this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example: A helper module for handling opening files in an Electron app, will need to know whether to expect second-instance events or whether to handle multiple instances 👍

Basically helpful for third party modules that devs might use, not first party code inside apps.

if (process_singleton_)
return false;
return true;
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did return process_singleton_; give warnings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

}

bool App::RequestSingleInstanceLock() {
if (HasSingleInstanceLock())
return true;

base::FilePath user_dir;
PathService::Get(brightray::DIR_USER_DATA, &user_dir);

auto cb = base::Bind(&App::OnSecondInstance, base::Unretained(this));

process_singleton_.reset(new ProcessSingleton(
user_dir, base::Bind(NotificationCallbackWrapper, callback)));
user_dir, base::Bind(NotificationCallbackWrapper, cb)));

switch (process_singleton_->NotifyOtherProcessOrCreate()) {
case ProcessSingleton::NotifyResult::LOCK_ERROR:
case ProcessSingleton::NotifyResult::PROFILE_IN_USE:
case ProcessSingleton::NotifyResult::PROCESS_NOTIFIED: {
process_singleton_.reset();
return true;
return false;
}
case ProcessSingleton::NotifyResult::PROCESS_NONE:
default: // Shouldn't be needed, but VS warns if it is not there.
return false;
return true;
}
}

void App::ReleaseSingleInstance() {
void App::ReleaseSingleInstanceLock() {
if (process_singleton_) {
process_singleton_->Cleanup();
process_singleton_.reset();
Expand Down Expand Up @@ -1252,8 +1267,9 @@ void App::BuildPrototype(v8::Isolate* isolate,
#if defined(USE_NSS_CERTS)
.SetMethod("importCertificate", &App::ImportCertificate)
#endif
.SetMethod("makeSingleInstance", &App::MakeSingleInstance)
.SetMethod("releaseSingleInstance", &App::ReleaseSingleInstance)
.SetMethod("hasSingleInstanceLock", &App::HasSingleInstanceLock)
.SetMethod("requestSingleInstanceLock", &App::RequestSingleInstanceLock)
.SetMethod("releaseSingleInstanceLock", &App::ReleaseSingleInstanceLock)
.SetMethod("relaunch", &App::Relaunch)
.SetMethod("isAccessibilitySupportEnabled",
&App::IsAccessibilitySupportEnabled)
Expand Down
8 changes: 5 additions & 3 deletions atom/browser/api/atom_api_app.h
Expand Up @@ -178,9 +178,11 @@ class App : public AtomBrowserClient::Delegate,

void SetDesktopName(const std::string& desktop_name);
std::string GetLocale();
bool MakeSingleInstance(
const ProcessSingleton::NotificationCallback& callback);
void ReleaseSingleInstance();
void OnSecondInstance(const base::CommandLine::StringVector& cmd,
const base::FilePath& cwd);
bool HasSingleInstanceLock() const;
bool RequestSingleInstanceLock();
void ReleaseSingleInstanceLock();
bool Relaunch(mate::Arguments* args);
void DisableHardwareAcceleration(mate::Arguments* args);
void DisableDomainBlockingFor3DAPIs(mate::Arguments* args);
Expand Down
73 changes: 46 additions & 27 deletions docs/api/app.md
Expand Up @@ -373,6 +373,22 @@ app.on('session-created', (event, session) => {
})
```

### Event: 'second-instance'

Returns:

* `argv` String[] - An array of the second instance's command line arguments
* `workingDirectory` String - The second instance's working directory

This event will be emitted inside the primary instance of your application
when a second instance has been executed. `argv` is an Array of the second instance's
command line arguments, and `workingDirectory` is its current working directory. Usually
applications respond to this by making their primary window focused and
non-minimized.

This event is guaranteed to be emitted after the `ready` event of `app`
gets emitted.

## Methods

The `app` object has the following methods:
Expand Down Expand Up @@ -742,32 +758,24 @@ app.setJumpList([
])
```

### `app.makeSingleInstance(callback)`
### `app.requestSingleInstanceLock()`

* `callback` Function
* `argv` String[] - An array of the second instance's command line arguments
* `workingDirectory` String - The second instance's working directory

Returns `Boolean`.
Returns `Boolean`

This method makes your application a Single Instance Application - instead of
allowing multiple instances of your app to run, this will ensure that only a
single instance of your app is running, and other instances signal this
instance and exit.

`callback` will be called by the first instance with `callback(argv, workingDirectory)`
when a second instance has been executed. `argv` is an Array of the second instance's
command line arguments, and `workingDirectory` is its current working directory. Usually
applications respond to this by making their primary window focused and
non-minimized.
The return value of this method indicates whether or not this instance of your
application successfully obtained the lock. If it failed to obtain the lock
you can assume that another instance of your application is already running with
the lock and exit immediately.

The `callback` is guaranteed to be executed after the `ready` event of `app`
gets emitted.

This method returns `false` if your process is the primary instance of the
application and your app should continue loading. And returns `true` if your
process has sent its parameters to another instance, and you should immediately
quit.
I.e. This methods returns `true` if your process is the primary instance of your
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/methods/method/

application and your app should continue loading. It returns `false` if your
process should immediately quit as it has sent it's parameters to another
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/it's/its/

instance that has already acquired the lock.

On macOS the system enforces single instance automatically when users try to open
a second instance of your app in Finder, and the `open-file` and `open-url`
Expand All @@ -782,27 +790,38 @@ starts:
const {app} = require('electron')
let myWindow = null

const isSecondInstance = app.makeSingleInstance((commandLine, workingDirectory) => {
const gotTheLock = app.requestSingleInstanceLock()

app.on('second-instance', (commandLine, workingDirectory) => {
// Someone tried to run a second instance, we should focus our window.
if (myWindow) {
if (myWindow.isMinimized()) myWindow.restore()
myWindow.focus()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, not a blocker to this review (esp since it's not new to this diff) but what is the difference between win.restore() and win.show()?

It looks like this block could be replaced with if (myWindow) myWindow.show() to restore + focus but it's not completely clear to me from the documentation, so I thought I'd ask...

}
})

if (isSecondInstance) {
if (!gotTheLock) {
app.quit()
} else {
// Create myWindow, load the rest of the app, etc...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-primary apps shouldn't register for 'second-instance' events, so the app.on('second-instance' should probably go here. Devs are going to copy-paste this, so let's make it 💯

app.on('ready', () => {
})
}

// Create myWindow, load the rest of the app, etc...
app.on('ready', () => {
})
```

### `app.releaseSingleInstance()`
### `app.hasSingleInstanceLock()`

Returns `Boolean`

This methods returns whether or not this instance of your app is currently
holding the single instance lock. You can request the lock with
`app.requestSingleInstanceLock()` and release with
`app.releaseSingleInstanceLock()`

### `app.releaseSingleInstanceLock()`

Releases all locks that were created by `makeSingleInstance`. This will allow
multiple instances of the application to once again run side by side.
Releases all locks that were created by `requestSingleInstanceLock`. This will
allow multiple instances of the application to once again run side by side.

### `app.setUserActivity(type, userInfo[, webpageURL])` _macOS_

Expand Down
9 changes: 9 additions & 0 deletions lib/browser/api/app.js
Expand Up @@ -109,6 +109,15 @@ for (let name of events) {
})
}

// TODO(MarshallOfSound): Remove in 4.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be added to docs/tutorial/planned-breaking-changes.md

app.makeSingleInstance = (oldStyleFn) => {
deprecate.warn('app.makeSingleInstance(cb)', 'app.requestSingleInstanceLock() and app.on(\'second-instance\', cb)')
if (oldStyleFn && typeof oldStyleFn === 'function') {
app.on('second-instance', (event, ...args) => oldStyleFn(...args))
}
return !app.requestSingleInstanceLock()
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a deprecate mesage for app.releaseSingleInstance() too

// Wrappers for native classes.
const {DownloadItem} = process.atomBinding('download_item')
Object.setPrototypeOf(DownloadItem.prototype, EventEmitter.prototype)
22 changes: 22 additions & 0 deletions spec/api-app-spec.js
Expand Up @@ -185,7 +185,29 @@ describe('app module', () => {
})
})

// TODO(MarshallOfSound) - Remove in 4.0.0
describe('app.makeSingleInstance', () => {
it('prevents the second launch of app', function (done) {
this.timeout(120000)
const appPath = path.join(__dirname, 'fixtures', 'api', 'singleton-old')
// First launch should exit with 0.
const first = ChildProcess.spawn(remote.process.execPath, [appPath])
first.once('exit', (code) => {
assert.equal(code, 0)
})
// Start second app when received output.
first.stdout.once('data', () => {
// Second launch should exit with 1.
const second = ChildProcess.spawn(remote.process.execPath, [appPath])
second.once('exit', (code) => {
assert.equal(code, 1)
done()
})
})
})
})

describe('app.requestSingleInstanceLock', () => {
it('prevents the second launch of app', function (done) {
this.timeout(120000)
const appPath = path.join(__dirname, 'fixtures', 'api', 'singleton')
Expand Down
13 changes: 13 additions & 0 deletions spec/fixtures/api/singleton-old/main.js
@@ -0,0 +1,13 @@
const {app} = require('electron')

app.once('ready', () => {
console.log('started') // ping parent
})

const shouldExit = app.makeSingleInstance(() => {
setImmediate(() => app.exit(0))
})

if (shouldExit) {
app.exit(1)
}
5 changes: 5 additions & 0 deletions spec/fixtures/api/singleton-old/package.json
@@ -0,0 +1,5 @@
{
"name": "electron-app-singleton",
"main": "main.js"
}

8 changes: 5 additions & 3 deletions spec/fixtures/api/singleton/main.js
Expand Up @@ -4,10 +4,12 @@ app.once('ready', () => {
console.log('started') // ping parent
})

const shouldExit = app.makeSingleInstance(() => {
process.nextTick(() => app.exit(0))
const gotTheLock = app.requestSingleInstanceLock()

app.on('second-instance', () => {
setImmediate(() => app.exit(0))
})

if (shouldExit) {
if (!gotTheLock) {
app.exit(1)
}