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

Call AllowSetForegroundWindow before sending IPC #13255

Merged
merged 2 commits into from Oct 7, 2016

Conversation

@the-ress
Copy link
Contributor

the-ress commented Oct 5, 2016

Call AllowSetForegroundWindow before sending IPC to the running instance on Windows (Fixes #929)

When a second instance of vscode is started on Windows, it now performs these actions:

  1. (New) Get PID of the original instance via IPC.
  2. (New) Call AllowSetForegroundWindow to allow the original instance to use SetForegroundWindow. (https://blogs.msdn.microsoft.com/oldnewthing/20090220-00/?p=19083)
  3. Send the start IPC to the original instance and exit.

I created a windows-foreground-love npm package to wrap the AllowSetForegroundWindow API.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Oct 5, 2016

@the-ress, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sandy081, @joaomoreno and @bpasero to be potential reviewers.

@msftclas

This comment has been minimized.

Copy link

msftclas commented Oct 5, 2016

Hi @the-ress, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Oct 5, 2016

@the-ress amazing stuff, I will review tomorrow and give some feedback. I encourage you to try to get this into Electron because this is imho the right place to fix it and you would basically fix it for tons of applications that all have this problem (WhatsApp, Atom, etc).

@bpasero bpasero self-assigned this Oct 5, 2016
src/vs/code/electron-main/main.ts Outdated
@@ -306,7 +305,12 @@ function setupIPC(accessor: ServicesAccessor): TPromise<Server> {
promise = service.getMainProcessId()
.then(processId => {
logService.log('Sending some foreground love to the running instance:', processId);
allowSetForegroundWindow(processId);
try {
const { allowSetForegroundWindow } = <any>require.__$__nodeRequire('windows-foreground-love');

This comment has been minimized.

Copy link
@bpasero

bpasero Oct 5, 2016

Member

@the-ress this is rather ugly, can we do what windows-mutex does? This native module is also only build for Windows and it still has a nice import 👍

This comment has been minimized.

Copy link
@the-ress

the-ress Oct 5, 2016

Author Contributor

I tried leaving the import there, but I got this error when committing:
Unused import: 'allowSetForegroundWindow'

This comment has been minimized.

Copy link
@bpasero

bpasero Oct 7, 2016

Member

Yup, its fine, we seem to do the same for windows-mutex

@the-ress

This comment has been minimized.

Copy link
Contributor Author

the-ress commented Oct 5, 2016

I don't think it's possible/practical to solve this in Electron.

VS Code has its own implementation of single-instance behavior (and Atom has another one). So on the Electron level, we don't know if the current instance is the second one, what is the PID of the first one or if the application is single-instance at all.

Only way I can think of would be a catch-all approach - call AllowSetForegroundWindow with ASFW_ANY as soon as the Electron application starts regardless of whether it's the first instance, or the other. (And I don't think this would be a good solution.)

@coveralls

This comment has been minimized.

Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage decreased (-0.0004%) to 61.234% when pulling fa494be48bb42c6c91795dc1133d2ac572e1c6bc on the-ress:send-foreground-love into 809cef1 on Microsoft:master.

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Oct 6, 2016

@the-ress I think that is fine. When someone calls app.focus() or window.focus() in Electron, AllowSetForegroundWindow should be set to ASFW_ANY for a short duration and could then unset. Only that ensures that the focus() call works as expected on Windows.

@the-ress

This comment has been minimized.

Copy link
Contributor Author

the-ress commented Oct 6, 2016

AllowSetForegroundWindow has to be called from the other process, not the one calling focus(). So we would have to know when the other process is calling focus().

Here's an article about how AllowSetForegroundWindow works: https://blogs.msdn.microsoft.com/oldnewthing/20090220-00/?p=19083

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Oct 6, 2016

@the-ress that is right and here is the problem:

  • the main process in Electron is the one that implements focus
  • the renderer process (this is the process that actually is the window) is treated as foreground process in Windows
  • the main process calls focus for the window of the renderer process
  • Windows blocks this because the main process is not the foreground process

Possible fix:

  • the main process sends a message to the renderer process AllowSetForegroundWindow ASFW_ANY
  • the renderer process acknowledges this and the main process calls focus

Alternatively I think it would be better to just implement the focus method in the renderer so that the correct foreground process is the one triggering the focus call.

In any case, this has nothing to do with how on startup a single instance is maintained because the second instance that is starting up is just telling the first (main) instance to focus a window. The second instance is imho not doing anything wrong that would prevent the first instance from focussing the window. It is due to the process model within Electron that this is problematic.

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Oct 7, 2016

@the-ress I apologise, I should have read the blog post earlier and now I better understand the problem. I will actually post it to the relevant Atom and Electron issues so that people become aware of it.

Nevertheless I still believe that a solution in Electron is possible. Electron provides API app.makeSingleInstance (https://github.com/electron/electron/blob/master/docs/api/app.md#appmakesingleinstancecallback) that covers the case of letting a second instance talk to a first instance. As far as I know this API is provided by Chromium. It would be nice if your AllowSetForeground trick could be added to that API so that consumers of it benefit.

Since VS Code currently does not plan to use app.makeSingleInstance we would still need (your) custom solution.

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Oct 7, 2016

@the-ress can you resolve the merge conflicts? I reviewed the change and it looks good 👍

@bpasero bpasero added this to the October 2016 milestone Oct 7, 2016
@the-ress the-ress force-pushed the the-ress:send-foreground-love branch Oct 7, 2016
@the-ress the-ress force-pushed the the-ress:send-foreground-love branch to 2f13f1d Oct 7, 2016
@the-ress

This comment has been minimized.

Copy link
Contributor Author

the-ress commented Oct 7, 2016

I resolved the conflicts.

The Chromium API already does this so app.makeSingleInstance should be probably fine.

@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Oct 7, 2016

Hm, ok, we should eventually think about moving to that API if it solves the issue.

@bpasero bpasero merged commit 774b83b into microsoft:master Oct 7, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@the-ress the-ress deleted the the-ress:send-foreground-love branch Oct 7, 2016
@the-ress

This comment has been minimized.

Copy link
Contributor Author

the-ress commented Oct 7, 2016

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.