-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add option to only allow a single instance of the Electron app #6280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way you can clean up the code?
There are a lot of changes and it is hard to follow if actual functionality was removed as part of the changes. I was able to extract only the part for singleInstance
in my local changes and came under 20 lines.
With the changes locally, I can see that if another electron instance is attempted to be started with the singleInstance
config set to true
, it doesn't open and the previous window is focused so it works correctly.
dev-packages/application-manager/src/generator/frontend-generator.ts
Outdated
Show resolved
Hide resolved
If a second instance is opened, it closes immediately and the original instance receives focus. Signed-off-by: Matthew Gordon <matthew.gordon@arm.com>
@vince-fugnitto Good point, the indent change made it very hard to read the diff. I've updated now. Thanks for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks much better now thank you! 👍
The code works and does what it intends. I tested the following cases:
- if no backend config with
singleInstance
is defined, the behavior remains the same as today. - if
singleInstance
isfalse
, it is possible to spawn multiple Electron instances. - if
singleInstance
istrue
, it is no longer possible to spawn multiple Electron instances.
(the sole instance is always revealed - ex: if it is hidden behind a window, if it is minimized)
I'll give others a chance to review as well :) |
@vince-fugnitto do you still think this is ready be merged? |
Yes, I was just hoping for others to get a chance to review as well but they might be busy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What it does
Added an option to the backend config:
singleInstance
. If true and in Electron mode, only one instance of the application is allowed to run at a time. If a second is started, it terminates itself and the original instance's window is focused.See: https://electronjs.org/docs/api/app#apprequestsingleinstancelock.
How to test
Add the following to the "theia" section of the electron example's package.json: https://github.com/eclipse-theia/theia/blob/master/examples/electron/package.json#L6.
Then build the example.
With
singleInstance
set to true, only one instance of the application can be started. A second instance will exit immediately and the first instance will receive focus.With
singleInstance
false or not set, any number of instances can be run (as before).Review checklist
Reminder for reviewers